| Message ID | 20230523163811.30792-3-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | build: cleanup build log, avoid user's CFLAGS, avoid too many include of Config.mk | expand |
> On 23 May 2023, at 17:37, Anthony PERARD <anthony.perard@citrix.com> wrote: > > Use $(if_changed_dep, ) macro to generate "asm-offsets.s" and remove > the use of $(move-if-changes,). That mean that "asm-offset.s" will be > changed even when the output doesn't change. > > But "asm-offsets.s" is only used to generated "asm-offsets.h". So > instead of regenerating "asm-offsets.h" every time "asm-offsets.s" > change, we will use "$(filechk, )" to only update the ".h" when the > output change. Also, with "$(filechk, )", the file does get > regenerated when the rule change in the makefile. > > This changes also result in a cleaner build log. > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > --- > > Instead of having a special $(cmd_asm-offsets.s) command, we could > probably reuse $(cmd_cc_s_c) from Rules.mk, but that would mean that > an hypothetical additional flags "-flto" in CFLAGS would not be > removed anymore, not sure if that matter here. > > But then we could write this: > > targets += arch/$(TARGET_ARCH)/$(TARGET_SUBARCH)/asm-offsets.s > arch/$(TARGET_ARCH)/$(TARGET_SUBARCH)/asm-offsets.s: CFLAGS-y += -g0 > arch/$(TARGET_ARCH)/include/asm/asm-offsets.h: arch/$(TARGET_ARCH)/$(TARGET_SUBARCH)/asm-offsets.s FORCE > > instead of having to write a rule for asm-offsets.s The solution above seems clean, maybe I am wrong but -flto should not matter here as we are not building objects to include in the final build, isn’t it? And gcc documentation states just: “It is recommended that you compile all the files participating in the same link with the same options and also specify those options at link time." I’ve also tested this patch and it works fine, I have to say however that I preferred a more verbose output, so that people can check how we are invoking the compiler, but I guess now it’s more consistent with the other invocations that doesn’t print the compiler invocation. So if you want to proceed with this one, for me looks fine: Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> Tested-by: Luca Fancellu <luca.fancellu@arm.com>
On 24.05.2023 09:39, Luca Fancellu wrote: >> On 23 May 2023, at 17:37, Anthony PERARD <anthony.perard@citrix.com> wrote: >> Instead of having a special $(cmd_asm-offsets.s) command, we could >> probably reuse $(cmd_cc_s_c) from Rules.mk, but that would mean that >> an hypothetical additional flags "-flto" in CFLAGS would not be >> removed anymore, not sure if that matter here. >> >> But then we could write this: >> >> targets += arch/$(TARGET_ARCH)/$(TARGET_SUBARCH)/asm-offsets.s >> arch/$(TARGET_ARCH)/$(TARGET_SUBARCH)/asm-offsets.s: CFLAGS-y += -g0 >> arch/$(TARGET_ARCH)/include/asm/asm-offsets.h: arch/$(TARGET_ARCH)/$(TARGET_SUBARCH)/asm-offsets.s FORCE >> >> instead of having to write a rule for asm-offsets.s > > The solution above seems clean, maybe I am wrong but -flto should not matter here as we are > not building objects to include in the final build, isn’t it? And gcc documentation states just: > > “It is recommended that you compile all the files participating in the same link with the same > options and also specify those options at link time." > > I’ve also tested this patch and it works fine, I have to say however that I preferred > a more verbose output, so that people can check how we are invoking the compiler, > but I guess now it’s more consistent with the other invocations that doesn’t print > the compiler invocation. If you want it more verbose, you can pass V=1 on the make command line. (Of course that'll affect all commands' output.) Jan
> On 24 May 2023, at 09:01, Jan Beulich <jbeulich@suse.com> wrote: > > On 24.05.2023 09:39, Luca Fancellu wrote: >>> On 23 May 2023, at 17:37, Anthony PERARD <anthony.perard@citrix.com> wrote: >>> Instead of having a special $(cmd_asm-offsets.s) command, we could >>> probably reuse $(cmd_cc_s_c) from Rules.mk, but that would mean that >>> an hypothetical additional flags "-flto" in CFLAGS would not be >>> removed anymore, not sure if that matter here. >>> >>> But then we could write this: >>> >>> targets += arch/$(TARGET_ARCH)/$(TARGET_SUBARCH)/asm-offsets.s >>> arch/$(TARGET_ARCH)/$(TARGET_SUBARCH)/asm-offsets.s: CFLAGS-y += -g0 >>> arch/$(TARGET_ARCH)/include/asm/asm-offsets.h: arch/$(TARGET_ARCH)/$(TARGET_SUBARCH)/asm-offsets.s FORCE >>> >>> instead of having to write a rule for asm-offsets.s >> >> The solution above seems clean, maybe I am wrong but -flto should not matter here as we are >> not building objects to include in the final build, isn’t it? And gcc documentation states just: >> >> “It is recommended that you compile all the files participating in the same link with the same >> options and also specify those options at link time." >> >> I’ve also tested this patch and it works fine, I have to say however that I preferred >> a more verbose output, so that people can check how we are invoking the compiler, >> but I guess now it’s more consistent with the other invocations that doesn’t print >> the compiler invocation. > > If you want it more verbose, you can pass V=1 on the make command line. > (Of course that'll affect all commands' output.) Yes I have to say that after sending the mail, I’ve checked the Makefile and I’ve found the comment # To put more focus on warnings, be less verbose as default # Use 'make V=1' to see the full commands Thank you for pointing that out > > Jan
On 23.05.2023 18:37, Anthony PERARD wrote: > Use $(if_changed_dep, ) macro to generate "asm-offsets.s" and remove > the use of $(move-if-changes,). That mean that "asm-offset.s" will be > changed even when the output doesn't change. > > But "asm-offsets.s" is only used to generated "asm-offsets.h". So > instead of regenerating "asm-offsets.h" every time "asm-offsets.s" > change, we will use "$(filechk, )" to only update the ".h" when the > output change. Also, with "$(filechk, )", the file does get > regenerated when the rule change in the makefile. > > This changes also result in a cleaner build log. > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > --- > > Instead of having a special $(cmd_asm-offsets.s) command, we could > probably reuse $(cmd_cc_s_c) from Rules.mk, but that would mean that > an hypothetical additional flags "-flto" in CFLAGS would not be > removed anymore, not sure if that matter here. There's no real code being generated there, and what we're after are merely the special .ascii directives. So the presence of -flto should have no effect there, and hence it would even look more consistent to me if we didn't use different options (and even rules) for .c -> .s transformations. > But then we could write this: > > targets += arch/$(TARGET_ARCH)/$(TARGET_SUBARCH)/asm-offsets.s > arch/$(TARGET_ARCH)/$(TARGET_SUBARCH)/asm-offsets.s: CFLAGS-y += -g0 > arch/$(TARGET_ARCH)/include/asm/asm-offsets.h: arch/$(TARGET_ARCH)/$(TARGET_SUBARCH)/asm-offsets.s FORCE > > instead of having to write a rule for asm-offsets.s Ftaod, I'd be happy to ack the patch as it is, but I would favor if you could do the rework / simplification as outlined. Jan
On Wed, May 24, 2023 at 04:09:39PM +0200, Jan Beulich wrote: > On 23.05.2023 18:37, Anthony PERARD wrote: > > Use $(if_changed_dep, ) macro to generate "asm-offsets.s" and remove > > the use of $(move-if-changes,). That mean that "asm-offset.s" will be > > changed even when the output doesn't change. > > > > But "asm-offsets.s" is only used to generated "asm-offsets.h". So > > instead of regenerating "asm-offsets.h" every time "asm-offsets.s" > > change, we will use "$(filechk, )" to only update the ".h" when the > > output change. Also, with "$(filechk, )", the file does get > > regenerated when the rule change in the makefile. > > > > This changes also result in a cleaner build log. > > > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > > --- > > > > Instead of having a special $(cmd_asm-offsets.s) command, we could > > probably reuse $(cmd_cc_s_c) from Rules.mk, but that would mean that > > an hypothetical additional flags "-flto" in CFLAGS would not be > > removed anymore, not sure if that matter here. > > There's no real code being generated there, and what we're after are > merely the special .ascii directives. So the presence of -flto should > have no effect there, and hence it would even look more consistent to > me if we didn't use different options (and even rules) for .c -> .s > transformations. > > > But then we could write this: > > > > targets += arch/$(TARGET_ARCH)/$(TARGET_SUBARCH)/asm-offsets.s > > arch/$(TARGET_ARCH)/$(TARGET_SUBARCH)/asm-offsets.s: CFLAGS-y += -g0 > > arch/$(TARGET_ARCH)/include/asm/asm-offsets.h: arch/$(TARGET_ARCH)/$(TARGET_SUBARCH)/asm-offsets.s FORCE > > > > instead of having to write a rule for asm-offsets.s > > Ftaod, I'd be happy to ack the patch as it is, but I would favor if > you could do the rework / simplification as outlined. Thanks, I'll do this rework.
diff --git a/xen/build.mk b/xen/build.mk index 758590c68e..e2a78aa806 100644 --- a/xen/build.mk +++ b/xen/build.mk @@ -40,13 +40,15 @@ include/xen/compile.h: include/xen/compile.h.in .banner FORCE targets += include/xen/compile.h --include $(wildcard .asm-offsets.s.d) -asm-offsets.s: arch/$(TARGET_ARCH)/$(TARGET_SUBARCH)/asm-offsets.c - $(CC) $(call cpp_flags,$(c_flags)) -S -g0 -o $@.new -MQ $@ $< - $(call move-if-changed,$@.new,$@) +quiet_cmd_asm-offsets.s = CC $@ +cmd_asm-offsets.s = $(CC) $(call cpp_flags,$(c_flags)) -S -g0 $< -o $@ -arch/$(TARGET_ARCH)/include/asm/asm-offsets.h: asm-offsets.s - @(set -e; \ +asm-offsets.s: arch/$(TARGET_ARCH)/$(TARGET_SUBARCH)/asm-offsets.c FORCE + $(call if_changed_dep,asm-offsets.s) + +targets += asm-offsets.s + +define filechk_asm-offsets.h echo "/*"; \ echo " * DO NOT MODIFY."; \ echo " *"; \ @@ -57,9 +59,13 @@ arch/$(TARGET_ARCH)/include/asm/asm-offsets.h: asm-offsets.s echo "#ifndef __ASM_OFFSETS_H__"; \ echo "#define __ASM_OFFSETS_H__"; \ echo ""; \ - sed -rne "/^[^#].*==>/{s:.*==>(.*)<==.*:\1:; s: [\$$#]: :; p;}"; \ + sed -rne "/^[^#].*==>/{s:.*==>(.*)<==.*:\1:; s: [\$$#]: :; p;}" $<; \ echo ""; \ - echo "#endif") <$< >$@ + echo "#endif" +endef + +arch/$(TARGET_ARCH)/include/asm/asm-offsets.h: asm-offsets.s FORCE + $(call filechk,asm-offsets.h) build-dirs := $(patsubst %/built_in.o,%,$(filter %/built_in.o,$(ALL_OBJS) $(ALL_LIBS)))
Use $(if_changed_dep, ) macro to generate "asm-offsets.s" and remove the use of $(move-if-changes,). That mean that "asm-offset.s" will be changed even when the output doesn't change. But "asm-offsets.s" is only used to generated "asm-offsets.h". So instead of regenerating "asm-offsets.h" every time "asm-offsets.s" change, we will use "$(filechk, )" to only update the ".h" when the output change. Also, with "$(filechk, )", the file does get regenerated when the rule change in the makefile. This changes also result in a cleaner build log. Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- Instead of having a special $(cmd_asm-offsets.s) command, we could probably reuse $(cmd_cc_s_c) from Rules.mk, but that would mean that an hypothetical additional flags "-flto" in CFLAGS would not be removed anymore, not sure if that matter here. But then we could write this: targets += arch/$(TARGET_ARCH)/$(TARGET_SUBARCH)/asm-offsets.s arch/$(TARGET_ARCH)/$(TARGET_SUBARCH)/asm-offsets.s: CFLAGS-y += -g0 arch/$(TARGET_ARCH)/include/asm/asm-offsets.h: arch/$(TARGET_ARCH)/$(TARGET_SUBARCH)/asm-offsets.s FORCE instead of having to write a rule for asm-offsets.s --- xen/build.mk | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)