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

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Thursday, February 9, 2023 8:44 AM
> To: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>; Anthony Perard 
> <anthony.perard@xxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap 
> <George.Dunlap@xxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; Stefano Stabellini 
> <sstabellini@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; 
> xen-devel@xxxxxxxxxxxxxxxxxxxx <xen-devel@xxxxxxxxxxxxxxxxxxxx>
> 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