[v1] libs: Fix unstable libs build on FreeBSD, auto-generate version-script

[XEN PATCH] libs: Fix unstable libs build on FreeBSD, auto-generate version-script

Posted by Anthony PERARD 2 years, 11 months ago

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

Re: [XEN PATCH] libs: Fix unstable libs build on FreeBSD, auto-generate version-script

Posted by Andrew Cooper 2 years, 11 months ago

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

Re: [XEN PATCH] libs: Fix unstable libs build on FreeBSD, auto-generate version-script

Posted by Jan Beulich 2 years, 11 months ago

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

Re: [XEN PATCH] libs: Fix unstable libs build on FreeBSD, auto-generate version-script

Posted by Anthony PERARD 2 years, 11 months ago

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

Re: [XEN PATCH] libs: Fix unstable libs build on FreeBSD, auto-generate version-script

Posted by Jan Beulich 2 years, 11 months ago

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