| Message ID | 20230215152111.51218-1-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [XEN] libs: Fix unstable libs build on FreeBSD, auto-generate version-script | expand |
On 15.02.2023 16:21, Anthony PERARD wrote: > @@ -13,6 +14,10 @@ MAJOR := $(shell $(XEN_ROOT)/version.sh $(XEN_ROOT)/xen/Makefile) > endif > MINOR ?= 0 > > +ifeq ($(origin version-script), undefined) > +version-script := libxen$(LIBNAME).map.tmp > +endif Such a use of $(origin ...) is pretty fragile. Maybe better use ?= ? > @@ -72,6 +77,10 @@ headers.lst: FORCE > @{ set -e; $(foreach h,$(LIBHEADERS),echo $(h);) } > $@.tmp > @$(call move-if-changed,$@.tmp,$@) > > +libxen$(LIBNAME).map.tmp: FORCE > + echo 'VERS_$(MAJOR).$(MINOR) { global: *; };' >.$@.tmp > + $(call move-if-changed,.$@.tmp,$@) Isn't this going to get in the way of your "build everything from root" effort, where $@ will include a path? Also do we really need .tmp.tmp files? > lib$(LIB_FILE_NAME).a: $(OBJS-y) Seeing this right adjacent in context - any reason you use libxen$(LIBNAME) and not the same lib$(LIB_FILE_NAME) for the base file name? > @@ -120,7 +129,7 @@ TAGS: > clean:: > rm -rf $(TARGETS) *~ $(DEPS_RM) $(OBJS-y) $(PIC_OBJS) > rm -f lib$(LIB_FILE_NAME).so.$(MAJOR).$(MINOR) lib$(LIB_FILE_NAME).so.$(MAJOR) > - rm -f headers.chk headers.lst > + rm -f headers.chk headers.lst libxen*.map.tmp If I hadn't checked, I'd have assumed that *.tmp are removed without being named explicitly. So yes, I see the need for the addition, but then I wonder why you don't also remove the .*.tmp.tmp file, which may be left around if the build is interrupted at exactly the "right" time. Jan
On 15/02/2023 3:21 pm, Anthony PERARD wrote: > Unfortunatly, --default-symver doesn't work on FreeBSD 12, with LLVM's > LD. Instead, we will generate a temporary version-script. It was all builds, not just FreeBSD 12, and not really FreeBSD either. LLD simply doesn't understand the --default-symver. It's just that the FreeBSD builds are the only ones where we're using LLD. All the gitlab clang tests are clang+binutils, not clang+llvm. We ought to change this irrespective. > diff --git a/tools/libs/libs.mk b/tools/libs/libs.mk > index 0e4b5e0bd0..cab8e9704a 100644 > --- a/tools/libs/libs.mk > +++ b/tools/libs/libs.mk > @@ -72,6 +77,10 @@ headers.lst: FORCE > @{ set -e; $(foreach h,$(LIBHEADERS),echo $(h);) } > $@.tmp > @$(call move-if-changed,$@.tmp,$@) > > +libxen$(LIBNAME).map.tmp: FORCE > + echo 'VERS_$(MAJOR).$(MINOR) { global: *; };' >.$@.tmp > + $(call move-if-changed,.$@.tmp,$@) It has come up in the past that using literally VERS_ is buggy, because anyone who copy&pastes too much of the canonical reference will end up making a compatible binary. Xen's stable libraries are buggy, and at some point we really do need to bump them all to 2.0 to fix this. VERS should be a library reference, so libxen$(LIBNAME) in our case. I suggest we take this opportunity to fix the unstable libraries. ~Andrew
On Wed, Feb 15, 2023 at 04:30:43PM +0100, Jan Beulich wrote: > On 15.02.2023 16:21, Anthony PERARD wrote: > > @@ -13,6 +14,10 @@ MAJOR := $(shell $(XEN_ROOT)/version.sh $(XEN_ROOT)/xen/Makefile) > > endif > > MINOR ?= 0 > > > > +ifeq ($(origin version-script), undefined) > > +version-script := libxen$(LIBNAME).map.tmp > > +endif > > Such a use of $(origin ...) is pretty fragile. Maybe better use ?= ? I'm not sure why I used $(origin), I've written that more than 6 month ago, but this way of using it is actually described in the manual, when documenting ?= but with a = instead of := . So, maybe I wanted to have ?= but with immediate evaluation rather than deferred. Maybe I had issue with $(version-script) evaluating to different values. If that ok, I'd like to keep using these runes, since ?:= doesn't exist. > > @@ -72,6 +77,10 @@ headers.lst: FORCE > > @{ set -e; $(foreach h,$(LIBHEADERS),echo $(h);) } > $@.tmp > > @$(call move-if-changed,$@.tmp,$@) > > > > +libxen$(LIBNAME).map.tmp: FORCE > > + echo 'VERS_$(MAJOR).$(MINOR) { global: *; };' >.$@.tmp > > + $(call move-if-changed,.$@.tmp,$@) > > Isn't this going to get in the way of your "build everything from root" > effort, where $@ will include a path? Also do we really need .tmp.tmp > files? Good call, I'll replace that with $(@D)/.$(@F), that will be one less thing to deal with, Also, I guess just adding a dot to the filename would be enough, and we would have `mv .libxen.map.tmp libxen.map.tmp`, and no more .tmp.tmp files. > > lib$(LIB_FILE_NAME).a: $(OBJS-y) > > Seeing this right adjacent in context - any reason you use libxen$(LIBNAME) > and not the same lib$(LIB_FILE_NAME) for the base file name? That was the name used before, I guess it would be fine to rename. I just need to set $(version-script) later as $(LIB_FILE_NAME) would not be defined yet. > > @@ -120,7 +129,7 @@ TAGS: > > clean:: > > rm -rf $(TARGETS) *~ $(DEPS_RM) $(OBJS-y) $(PIC_OBJS) > > rm -f lib$(LIB_FILE_NAME).so.$(MAJOR).$(MINOR) lib$(LIB_FILE_NAME).so.$(MAJOR) > > - rm -f headers.chk headers.lst > > + rm -f headers.chk headers.lst libxen*.map.tmp > > If I hadn't checked, I'd have assumed that *.tmp are removed without > being named explicitly. So yes, I see the need for the addition, but then > I wonder why you don't also remove the .*.tmp.tmp file, which may be left > around if the build is interrupted at exactly the "right" time. I forgot because $(move-if-changed) remove the temporary file. I'll clean the .*.tmp file. Thanks,
On 15.02.2023 18:41, Anthony PERARD wrote: > On Wed, Feb 15, 2023 at 04:30:43PM +0100, Jan Beulich wrote: >> On 15.02.2023 16:21, Anthony PERARD wrote: >>> @@ -13,6 +14,10 @@ MAJOR := $(shell $(XEN_ROOT)/version.sh $(XEN_ROOT)/xen/Makefile) >>> endif >>> MINOR ?= 0 >>> >>> +ifeq ($(origin version-script), undefined) >>> +version-script := libxen$(LIBNAME).map.tmp >>> +endif >> >> Such a use of $(origin ...) is pretty fragile. Maybe better use ?= ? > > I'm not sure why I used $(origin), I've written that more than 6 month > ago, but this way of using it is actually described in the manual, when > documenting ?= but with a = instead of := . > So, maybe I wanted to have ?= but with immediate evaluation rather than > deferred. Maybe I had issue with $(version-script) evaluating to > different values. What's wrong with using deferred evaluation for this macro? $(LIBNAME) (and $(LIB_FILE_NAME)) don't vary over time, do they? Plus ... >>> lib$(LIB_FILE_NAME).a: $(OBJS-y) >> >> Seeing this right adjacent in context - any reason you use libxen$(LIBNAME) >> and not the same lib$(LIB_FILE_NAME) for the base file name? > > That was the name used before, I guess it would be fine to rename. I > just need to set $(version-script) later as $(LIB_FILE_NAME) would not > be defined yet. ... you'd likely deal with that issue as well, unless there's a "too early" use of $(version-script) somewhere. Jan
diff --git a/tools/libs/libs.mk b/tools/libs/libs.mk index 0e4b5e0bd0..cab8e9704a 100644 --- a/tools/libs/libs.mk +++ b/tools/libs/libs.mk @@ -5,6 +5,7 @@ # MAJOR: major version of lib (Xen version if empty) # MINOR: minor version of lib (0 if empty) # version-script: Specify the name of a version script to the linker. +# (If empty, a temporary one for unstable library is created) LIBNAME := $(notdir $(CURDIR)) @@ -13,6 +14,10 @@ MAJOR := $(shell $(XEN_ROOT)/version.sh $(XEN_ROOT)/xen/Makefile) endif MINOR ?= 0 +ifeq ($(origin version-script), undefined) +version-script := libxen$(LIBNAME).map.tmp +endif + CFLAGS += -Wmissing-prototypes CFLAGS += $(CFLAGS_xeninclude) CFLAGS += $(foreach lib, $(USELIBS_$(LIBNAME)), $(CFLAGS_libxen$(lib))) @@ -72,6 +77,10 @@ headers.lst: FORCE @{ set -e; $(foreach h,$(LIBHEADERS),echo $(h);) } > $@.tmp @$(call move-if-changed,$@.tmp,$@) +libxen$(LIBNAME).map.tmp: FORCE + echo 'VERS_$(MAJOR).$(MINOR) { global: *; };' >.$@.tmp + $(call move-if-changed,.$@.tmp,$@) + lib$(LIB_FILE_NAME).a: $(OBJS-y) $(AR) rc $@ $^ @@ -81,7 +90,7 @@ lib$(LIB_FILE_NAME).so.$(MAJOR): lib$(LIB_FILE_NAME).so.$(MAJOR).$(MINOR) $(SYMLINK_SHLIB) $< $@ lib$(LIB_FILE_NAME).so.$(MAJOR).$(MINOR): $(PIC_OBJS) $(version-script) - $(CC) $(LDFLAGS) $(PTHREAD_LDFLAGS) -Wl,$(SONAME_LDFLAG) -Wl,lib$(LIB_FILE_NAME).so.$(MAJOR) -Wl,$(if $(version-script),--version-script=$(version-script),--default-symver) $(SHLIB_LDFLAGS) -o $@ $(PIC_OBJS) $(LDLIBS) $(APPEND_LDFLAGS) + $(CC) $(LDFLAGS) $(PTHREAD_LDFLAGS) -Wl,$(SONAME_LDFLAG) -Wl,lib$(LIB_FILE_NAME).so.$(MAJOR) -Wl,--version-script=$(version-script) $(SHLIB_LDFLAGS) -o $@ $(PIC_OBJS) $(LDLIBS) $(APPEND_LDFLAGS) # If abi-dumper is available, write out the ABI analysis ifneq ($(ABI_DUMPER),) @@ -120,7 +129,7 @@ TAGS: clean:: rm -rf $(TARGETS) *~ $(DEPS_RM) $(OBJS-y) $(PIC_OBJS) rm -f lib$(LIB_FILE_NAME).so.$(MAJOR).$(MINOR) lib$(LIB_FILE_NAME).so.$(MAJOR) - rm -f headers.chk headers.lst + rm -f headers.chk headers.lst libxen*.map.tmp .PHONY: distclean distclean: clean
Unfortunatly, --default-symver doesn't work on FreeBSD 12, with LLVM's LD. Instead, we will generate a temporary version-script. In order to allow regenerating the script, we'll have a different filename. In order to check if the content is up-to-date, we'll always generated it and compare. Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> Fixes: 98d95437edb6 ("libs: Fix auto-generation of version-script for unstable libs") Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- tools/libs/libs.mk | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)