tools/libs/libs.mk | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
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(-)
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
--
Anthony PERARD
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 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 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, -- Anthony PERARD
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
© 2016 - 2024 Red Hat, Inc.