[v1] build: Make FILE symbol paths consistent

[PATCH] build: Make FILE symbol paths consistent

Posted by Ross Lagerwall 1 year, 2 months ago

The FILE symbols in out-of-tree builds may be either a relative path to
the object dir or an absolute path depending on how the build is
invoked. Fix the paths for C files so that they are consistent with
in-tree builds - the path is relative to the "xen" directory (e.g.
common/irq.c).

This fixes livepatch builds when the original Xen build was out-of-tree
since livepatch-build always does in-tree builds.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 xen/Rules.mk | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/Rules.mk b/xen/Rules.mk
index 70b7489ea8..6b9269a70c 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -228,8 +228,9 @@ quiet_cmd_cc_o_c = CC      $@
 ifeq ($(CONFIG_ENFORCE_UNIQUE_SYMBOLS),y)
     cmd_cc_o_c = $(CC) $(c_flags) -c $< -o $(dot-target).tmp -MQ $@
     ifneq ($(CONFIG_CC_IS_CLANG)$(call clang-ifversion,-lt,600,y),yy)
+        rel_path = $(patsubst $(abs_srctree)/%,%,$(call realpath,$(1)))
         cmd_objcopy_fix_sym = \
-	    $(OBJCOPY) --redefine-sym $(<F)=$< $(dot-target).tmp $@ && rm -f $(dot-target).tmp
+           $(OBJCOPY) --redefine-sym $(<F)=$(call rel_path,$<) $(dot-target).tmp $@ && rm -f $(dot-target).tmp
     else
         cmd_objcopy_fix_sym = mv -f $(dot-target).tmp $@
     endif
-- 
2.31.1

Re: [PATCH] build: Make FILE symbol paths consistent

Posted by Jan Beulich 1 year, 2 months ago

On 08.02.2023 18:24, Ross Lagerwall wrote:
> The FILE symbols in out-of-tree builds may be either a relative path to
> the object dir or an absolute path depending on how the build is
> invoked. Fix the paths for C files so that they are consistent with
> in-tree builds - the path is relative to the "xen" directory (e.g.
> common/irq.c).
> 
> This fixes livepatch builds when the original Xen build was out-of-tree
> since livepatch-build always does in-tree builds.

Sounds all plausible, except that I was under the impression that as of
035ab75d8e37 ("build: fix enforce unique symbols for recent clang version")
things were consistent already. To clarify - Anthony, was this aspect
simply not considered at the time? What would help here is a Fixes: tag,
both for the purpose of review and for the purpose of deciding whether to
backport. It might be that 7a3bcd2babcc ("build: build everything from the
root dir, use obj=$subdir") is to blame, but I'm not really sure.

> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -228,8 +228,9 @@ quiet_cmd_cc_o_c = CC      $@
>  ifeq ($(CONFIG_ENFORCE_UNIQUE_SYMBOLS),y)
>      cmd_cc_o_c = $(CC) $(c_flags) -c $< -o $(dot-target).tmp -MQ $@
>      ifneq ($(CONFIG_CC_IS_CLANG)$(call clang-ifversion,-lt,600,y),yy)

You're altering only logic for pre-6.0.0 Clang, aiui. This is something
that's absolutely necessary to state the latest in the description, but
perhaps already in the subject. Among other things that's also an aspect
when it comes to considering whether to backport.

> +        rel_path = $(patsubst $(abs_srctree)/%,%,$(call realpath,$(1)))

Personally I'm against use of underscores when dashes would do, and using
a dash here would also be consistent with e.g. ...

>          cmd_objcopy_fix_sym = \
> -	    $(OBJCOPY) --redefine-sym $(<F)=$< $(dot-target).tmp $@ && rm -f $(dot-target).tmp
> +           $(OBJCOPY) --redefine-sym $(<F)=$(call rel_path,$<) $(dot-target).tmp $@ && rm -f $(dot-target).tmp
>      else
>          cmd_objcopy_fix_sym = mv -f $(dot-target).tmp $@
>      endif

... the several visible uses of $(dot-target) here.

Jan

Re: [PATCH] build: Make FILE symbol paths consistent

Posted by Ross Lagerwall 1 year, 2 months ago

> From: Jan Beulich <jbeulich@suse.com>
> Sent: Thursday, February 9, 2023 8:44 AM
> To: Ross Lagerwall <ross.lagerwall@citrix.com>; Anthony Perard <anthony.perard@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; George Dunlap <George.Dunlap@citrix.com>; Julien Grall <julien@xen.org>; Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; xen-devel@lists.xenproject.org <xen-devel@lists.xenproject.org>
> Subject: Re: [PATCH] build: Make FILE symbol paths consistent 
>  
> On 08.02.2023 18:24, Ross Lagerwall wrote:
> > The FILE symbols in out-of-tree builds may be either a relative path to
> > the object dir or an absolute path depending on how the build is
> > invoked. Fix the paths for C files so that they are consistent with
> > in-tree builds - the path is relative to the "xen" directory (e.g.
> > common/irq.c).
> > 
> > This fixes livepatch builds when the original Xen build was out-of-tree
> > since livepatch-build always does in-tree builds.
> 
> Sounds all plausible, except that I was under the impression that as of
> 035ab75d8e37 ("build: fix enforce unique symbols for recent clang version")
> things were consistent already. To clarify - Anthony, was this aspect
> simply not considered at the time? What would help here is a Fixes: tag,
> both for the purpose of review and for the purpose of deciding whether to
> backport. It might be that 7a3bcd2babcc ("build: build everything from the
> root dir, use obj=$subdir") is to blame, but I'm not really sure.

035ab75d8e37 ensures uniqueness for symbols but it doesn't ensure they
are consistent when using out of tree builds, i.e. the FILE symbols are
the same regardless of the value of "O=". Since this inconsistency
could only happen after out-of-tree builds were introduced, I guess it
should be:
Fixes: 7115fa562fe7 ("build: adding out-of-tree support to the xen build")

> 
> > --- a/xen/Rules.mk
> > +++ b/xen/Rules.mk
> > @@ -228,8 +228,9 @@ quiet_cmd_cc_o_c = CC      $@
> >  ifeq ($(CONFIG_ENFORCE_UNIQUE_SYMBOLS),y)
> >      cmd_cc_o_c = $(CC) $(c_flags) -c $< -o $(dot-target).tmp -MQ $@
> >      ifneq ($(CONFIG_CC_IS_CLANG)$(call clang-ifversion,-lt,600,y),yy)
> 
> You're altering only logic for pre-6.0.0 Clang, aiui. This is something
> that's absolutely necessary to state the latest in the description, but
> perhaps already in the subject. Among other things that's also an aspect
> when it comes to considering whether to backport.

I think that's backwards. The check is: if !(cc_is_clang && clang-version < 6)

I can mention in the description that this change doesn't cover older
versions of clang (which presumably have never worked with
livepatch-build-tools since it embeds full paths but that is a separate
issue).

> 
> > +        rel_path = $(patsubst $(abs_srctree)/%,%,$(call realpath,$(1)))
> 
> Personally I'm against use of underscores when dashes would do, and using
> a dash here would also be consistent with e.g. ...
> 
> >          cmd_objcopy_fix_sym = \
> > -         $(OBJCOPY) --redefine-sym $(<F)=$< $(dot-target).tmp $@ && rm -f $(dot-target).tmp
> > +           $(OBJCOPY) --redefine-sym $(<F)=$(call rel_path,$<) $(dot-target).tmp $@ && rm -f $(dot-target).tmp
> >      else
> >          cmd_objcopy_fix_sym = mv -f $(dot-target).tmp $@
> >      endif
> 
> ... the several visible uses of $(dot-target) here.

I'm not sure if there is much consistency since other variables like
abs_srctree and abs_objtree also use underscores but I can change it
if you prefer.

Ross

Re: [PATCH] build: Make FILE symbol paths consistent

Posted by Jan Beulich 1 year, 2 months ago

On 09.02.2023 12:40, Ross Lagerwall wrote:
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Thursday, February 9, 2023 8:44 AM
>>  
>> On 08.02.2023 18:24, Ross Lagerwall wrote:
>>> +        rel_path = $(patsubst $(abs_srctree)/%,%,$(call realpath,$(1)))
>>
>> Personally I'm against use of underscores when dashes would do, and using
>> a dash here would also be consistent with e.g. ...
>>
>>>           cmd_objcopy_fix_sym = \
>>> -         $(OBJCOPY) --redefine-sym $(<F)=$< $(dot-target).tmp $@ && rm -f $(dot-target).tmp
>>> +           $(OBJCOPY) --redefine-sym $(<F)=$(call rel_path,$<) $(dot-target).tmp $@ && rm -f $(dot-target).tmp
>>>       else
>>>           cmd_objcopy_fix_sym = mv -f $(dot-target).tmp $@
>>>       endif
>>
>> ... the several visible uses of $(dot-target) here.
> 
> I'm not sure if there is much consistency since other variables like
> abs_srctree and abs_objtree also use underscores but I can change it
> if you prefer.

These two variables had their names kept from their Linux origin, iirc,
so they aren't an overly good target for comparison. Nor are, for the
same reason, the various cmd_*. Linux, sadly, isn't consistent in that
regard ...

Jan