| Message ID | 20230208172416.725028-1-ross.lagerwall@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | build: Make FILE symbol paths consistent | expand |
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
> 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
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
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
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(-)