| Message ID | 20200421161208.2429539-9-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: Build system improvements | expand |
On 21.04.2020 18:12, Anthony PERARD wrote: > --- a/xen/Rules.mk > +++ b/xen/Rules.mk > @@ -123,6 +123,7 @@ $(obj-bin-y): XEN_CFLAGS := $(filter-out -flto,$(XEN_CFLAGS)) > > c_flags = -MMD -MP -MF $(@D)/.$(@F).d $(XEN_CFLAGS) '-D__OBJECT_FILE__="$@"' > a_flags = -MMD -MP -MF $(@D)/.$(@F).d $(XEN_AFLAGS) > +cpp_flags = $(filter-out -Wa$(comma)%,$(a_flags)) I can see this happening to be this way right now, but in principle I could see a_flags to hold items applicable to assembly files only, but not to (the preprocessing of) C files. Hence while this is fine for now, ... > @@ -207,7 +208,7 @@ quiet_cmd_cc_s_c = CC $@ > cmd_cc_s_c = $(CC) $(filter-out -Wa$(comma)%,$(c_flags)) -S $< -o $@ > > quiet_cmd_s_S = CPP $@ > -cmd_s_S = $(CPP) $(filter-out -Wa$(comma)%,$(a_flags)) $< -o $@ > +cmd_s_S = $(CPP) $(cpp_flags) $< -o $@ ... this one is a trap waiting for someone to fall in imo. Instead where I'd expect this patch to use $(cpp_flags) is e.g. in xen/arch/x86/mm/Makefile: guest_walk_%.i: guest_walk.c Makefile $(CPP) $(cpp_flags) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ And note how this currently uses $(c_flags), not $(a_flags), which suggests that your deriving from $(a_flags) isn't correct either. Jan
On Thu, Apr 23, 2020 at 06:48:51PM +0200, Jan Beulich wrote: > On 21.04.2020 18:12, Anthony PERARD wrote: > > --- a/xen/Rules.mk > > +++ b/xen/Rules.mk > > @@ -123,6 +123,7 @@ $(obj-bin-y): XEN_CFLAGS := $(filter-out -flto,$(XEN_CFLAGS)) > > > > c_flags = -MMD -MP -MF $(@D)/.$(@F).d $(XEN_CFLAGS) '-D__OBJECT_FILE__="$@"' > > a_flags = -MMD -MP -MF $(@D)/.$(@F).d $(XEN_AFLAGS) > > +cpp_flags = $(filter-out -Wa$(comma)%,$(a_flags)) > > I can see this happening to be this way right now, but in principle > I could see a_flags to hold items applicable to assembly files only, > but not to (the preprocessing of) C files. Hence while this is fine > for now, ... > > > @@ -207,7 +208,7 @@ quiet_cmd_cc_s_c = CC $@ > > cmd_cc_s_c = $(CC) $(filter-out -Wa$(comma)%,$(c_flags)) -S $< -o $@ > > > > quiet_cmd_s_S = CPP $@ > > -cmd_s_S = $(CPP) $(filter-out -Wa$(comma)%,$(a_flags)) $< -o $@ > > +cmd_s_S = $(CPP) $(cpp_flags) $< -o $@ > > ... this one is a trap waiting for someone to fall in imo. Instead > where I'd expect this patch to use $(cpp_flags) is e.g. in > xen/arch/x86/mm/Makefile: > > guest_walk_%.i: guest_walk.c Makefile > $(CPP) $(cpp_flags) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ > > And note how this currently uses $(c_flags), not $(a_flags), which > suggests that your deriving from $(a_flags) isn't correct either. I think we can drop this patch for now, and change patch "xen/build: factorise generation of the linker scripts" to not use $(cpp_flags). If we derive $(cpp_flags) from $(c_flags) instead, we would need to find out if CPP commands using a_flags can use c_flags instead. On the other hand, I've looked at Linux source code, and they use $(cpp_flags) for only a few targets, only to generate the .lds scripts. For other rules, they use either a_flags or c_flags, for example: %.i: %.c ; uses $(c_flags) %.i: %.S ; uses $(a_flags) %.s: %.S ; uses $(a_flags) (Also, they use -Qunused-arguments clang's options, so they don't need to filter out -Wa,* arguments, I think.) So, maybe having a single $(cpp_flags) when running the CPP command isn't such a good idea. So, would dropping $(cpp_flags) for now, and rework the *FLAGS later, be good enough? Thanks,
On 28.04.2020 16:01, Anthony PERARD wrote: > On Thu, Apr 23, 2020 at 06:48:51PM +0200, Jan Beulich wrote: >> On 21.04.2020 18:12, Anthony PERARD wrote: >>> --- a/xen/Rules.mk >>> +++ b/xen/Rules.mk >>> @@ -123,6 +123,7 @@ $(obj-bin-y): XEN_CFLAGS := $(filter-out -flto,$(XEN_CFLAGS)) >>> >>> c_flags = -MMD -MP -MF $(@D)/.$(@F).d $(XEN_CFLAGS) '-D__OBJECT_FILE__="$@"' >>> a_flags = -MMD -MP -MF $(@D)/.$(@F).d $(XEN_AFLAGS) >>> +cpp_flags = $(filter-out -Wa$(comma)%,$(a_flags)) >> >> I can see this happening to be this way right now, but in principle >> I could see a_flags to hold items applicable to assembly files only, >> but not to (the preprocessing of) C files. Hence while this is fine >> for now, ... >> >>> @@ -207,7 +208,7 @@ quiet_cmd_cc_s_c = CC $@ >>> cmd_cc_s_c = $(CC) $(filter-out -Wa$(comma)%,$(c_flags)) -S $< -o $@ >>> >>> quiet_cmd_s_S = CPP $@ >>> -cmd_s_S = $(CPP) $(filter-out -Wa$(comma)%,$(a_flags)) $< -o $@ >>> +cmd_s_S = $(CPP) $(cpp_flags) $< -o $@ >> >> ... this one is a trap waiting for someone to fall in imo. Instead >> where I'd expect this patch to use $(cpp_flags) is e.g. in >> xen/arch/x86/mm/Makefile: >> >> guest_walk_%.i: guest_walk.c Makefile >> $(CPP) $(cpp_flags) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ >> >> And note how this currently uses $(c_flags), not $(a_flags), which >> suggests that your deriving from $(a_flags) isn't correct either. > > I think we can drop this patch for now, and change patch "xen/build: > factorise generation of the linker scripts" to not use $(cpp_flags). > > If we derive $(cpp_flags) from $(c_flags) instead, we would need to > find out if CPP commands using a_flags can use c_flags instead. > > On the other hand, I've looked at Linux source code, and they use > $(cpp_flags) for only a few targets, only to generate the .lds scripts. > For other rules, they use either a_flags or c_flags, for example: > %.i: %.c ; uses $(c_flags) > %.i: %.S ; uses $(a_flags) > %.s: %.S ; uses $(a_flags) The first on really ought to be use cpp_flags. I couldn't find the middle one. The last one clearly has to do something about -Wa, options, but apart from this I'd consider a_flags appropriate to use there. > (Also, they use -Qunused-arguments clang's options, so they don't need > to filter out -Wa,* arguments, I think.) Maybe we should do so too then? > So, maybe having a single $(cpp_flags) when running the CPP command > isn't such a good idea. Right - after all in particular the use of CPP to produce .lds is an abuse, as the source file (named .lds.S) isn't really what its name says. > So, would dropping $(cpp_flags) for now, and rework the *FLAGS later, be > good enough? I don't think so, no, I'm sorry. cpp_flags should be there for its real purpose. Whether the .lds.S -> .lds rule can use it, or should use a_flags, or yet something else is a different thing. Jan
On Tue, Apr 28, 2020 at 04:20:57PM +0200, Jan Beulich wrote: > On 28.04.2020 16:01, Anthony PERARD wrote: > > On Thu, Apr 23, 2020 at 06:48:51PM +0200, Jan Beulich wrote: > >> On 21.04.2020 18:12, Anthony PERARD wrote: > >>> --- a/xen/Rules.mk > >>> +++ b/xen/Rules.mk > >>> @@ -123,6 +123,7 @@ $(obj-bin-y): XEN_CFLAGS := $(filter-out -flto,$(XEN_CFLAGS)) > >>> > >>> c_flags = -MMD -MP -MF $(@D)/.$(@F).d $(XEN_CFLAGS) '-D__OBJECT_FILE__="$@"' > >>> a_flags = -MMD -MP -MF $(@D)/.$(@F).d $(XEN_AFLAGS) > >>> +cpp_flags = $(filter-out -Wa$(comma)%,$(a_flags)) > >> > >> I can see this happening to be this way right now, but in principle > >> I could see a_flags to hold items applicable to assembly files only, > >> but not to (the preprocessing of) C files. Hence while this is fine > >> for now, ... > >> > >>> @@ -207,7 +208,7 @@ quiet_cmd_cc_s_c = CC $@ > >>> cmd_cc_s_c = $(CC) $(filter-out -Wa$(comma)%,$(c_flags)) -S $< -o $@ > >>> > >>> quiet_cmd_s_S = CPP $@ > >>> -cmd_s_S = $(CPP) $(filter-out -Wa$(comma)%,$(a_flags)) $< -o $@ > >>> +cmd_s_S = $(CPP) $(cpp_flags) $< -o $@ > >> > >> ... this one is a trap waiting for someone to fall in imo. Instead > >> where I'd expect this patch to use $(cpp_flags) is e.g. in > >> xen/arch/x86/mm/Makefile: > >> > >> guest_walk_%.i: guest_walk.c Makefile > >> $(CPP) $(cpp_flags) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ > >> > >> And note how this currently uses $(c_flags), not $(a_flags), which > >> suggests that your deriving from $(a_flags) isn't correct either. > > > > I think we can drop this patch for now, and change patch "xen/build: > > factorise generation of the linker scripts" to not use $(cpp_flags). > > > > If we derive $(cpp_flags) from $(c_flags) instead, we would need to > > find out if CPP commands using a_flags can use c_flags instead. > > > > On the other hand, I've looked at Linux source code, and they use > > $(cpp_flags) for only a few targets, only to generate the .lds scripts. > > For other rules, they use either a_flags or c_flags, for example: > > %.i: %.c ; uses $(c_flags) > > %.i: %.S ; uses $(a_flags) > > %.s: %.S ; uses $(a_flags) > > The first on really ought to be use cpp_flags. I couldn't find the > middle one. The last one clearly has to do something about -Wa, > options, but apart from this I'd consider a_flags appropriate to > use there. > > > (Also, they use -Qunused-arguments clang's options, so they don't need > > to filter out -Wa,* arguments, I think.) > > Maybe we should do so too then? > > > So, maybe having a single $(cpp_flags) when running the CPP command > > isn't such a good idea. > > Right - after all in particular the use of CPP to produce .lds is > an abuse, as the source file (named .lds.S) isn't really what its > name says. > > > So, would dropping $(cpp_flags) for now, and rework the *FLAGS later, be > > good enough? > > I don't think so, no, I'm sorry. cpp_flags should be there for its > real purpose. Whether the .lds.S -> .lds rule can use it, or should > use a_flags, or yet something else is a different thing. OK. I think we can rework the patch to derive cpp_flags from c_flags, use this new cpp_flags for %.i:%.c; but keep using a_flags for %.s:%.S. As for the .lds, we could use this new cpp_flags, the only think I saw missing was -D__ASSEMBLY__, which can be added to the command line. (There would also be an extra -std=gnu99, but I don't think it matters.) Does that sounds good? (Just two patch to change, this one and the one reworking .lds generation.) As for using -Qunused-arguments with clang, I didn't managed to find the documentation of clang's command line argument for clang 3.5 on llvm website, but I found it for clang 5.0 and the option is listed there. I've tested building Xen on our gitlab CI, which as debian jessie which seems to have clang 3.5, and Xen built just fine. So that might be an option we can use later, but probably only for CPP flags. Thanks,
On 01.05.2020 16:32, Anthony PERARD wrote: > On Tue, Apr 28, 2020 at 04:20:57PM +0200, Jan Beulich wrote: >> On 28.04.2020 16:01, Anthony PERARD wrote: >>> On Thu, Apr 23, 2020 at 06:48:51PM +0200, Jan Beulich wrote: >>>> On 21.04.2020 18:12, Anthony PERARD wrote: >>>>> --- a/xen/Rules.mk >>>>> +++ b/xen/Rules.mk >>>>> @@ -123,6 +123,7 @@ $(obj-bin-y): XEN_CFLAGS := $(filter-out -flto,$(XEN_CFLAGS)) >>>>> >>>>> c_flags = -MMD -MP -MF $(@D)/.$(@F).d $(XEN_CFLAGS) '-D__OBJECT_FILE__="$@"' >>>>> a_flags = -MMD -MP -MF $(@D)/.$(@F).d $(XEN_AFLAGS) >>>>> +cpp_flags = $(filter-out -Wa$(comma)%,$(a_flags)) >>>> >>>> I can see this happening to be this way right now, but in principle >>>> I could see a_flags to hold items applicable to assembly files only, >>>> but not to (the preprocessing of) C files. Hence while this is fine >>>> for now, ... >>>> >>>>> @@ -207,7 +208,7 @@ quiet_cmd_cc_s_c = CC $@ >>>>> cmd_cc_s_c = $(CC) $(filter-out -Wa$(comma)%,$(c_flags)) -S $< -o $@ >>>>> >>>>> quiet_cmd_s_S = CPP $@ >>>>> -cmd_s_S = $(CPP) $(filter-out -Wa$(comma)%,$(a_flags)) $< -o $@ >>>>> +cmd_s_S = $(CPP) $(cpp_flags) $< -o $@ >>>> >>>> ... this one is a trap waiting for someone to fall in imo. Instead >>>> where I'd expect this patch to use $(cpp_flags) is e.g. in >>>> xen/arch/x86/mm/Makefile: >>>> >>>> guest_walk_%.i: guest_walk.c Makefile >>>> $(CPP) $(cpp_flags) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ >>>> >>>> And note how this currently uses $(c_flags), not $(a_flags), which >>>> suggests that your deriving from $(a_flags) isn't correct either. >>> >>> I think we can drop this patch for now, and change patch "xen/build: >>> factorise generation of the linker scripts" to not use $(cpp_flags). >>> >>> If we derive $(cpp_flags) from $(c_flags) instead, we would need to >>> find out if CPP commands using a_flags can use c_flags instead. >>> >>> On the other hand, I've looked at Linux source code, and they use >>> $(cpp_flags) for only a few targets, only to generate the .lds scripts. >>> For other rules, they use either a_flags or c_flags, for example: >>> %.i: %.c ; uses $(c_flags) >>> %.i: %.S ; uses $(a_flags) >>> %.s: %.S ; uses $(a_flags) >> >> The first on really ought to be use cpp_flags. I couldn't find the >> middle one. The last one clearly has to do something about -Wa, >> options, but apart from this I'd consider a_flags appropriate to >> use there. >> >>> (Also, they use -Qunused-arguments clang's options, so they don't need >>> to filter out -Wa,* arguments, I think.) >> >> Maybe we should do so too then? >> >>> So, maybe having a single $(cpp_flags) when running the CPP command >>> isn't such a good idea. >> >> Right - after all in particular the use of CPP to produce .lds is >> an abuse, as the source file (named .lds.S) isn't really what its >> name says. >> >>> So, would dropping $(cpp_flags) for now, and rework the *FLAGS later, be >>> good enough? >> >> I don't think so, no, I'm sorry. cpp_flags should be there for its >> real purpose. Whether the .lds.S -> .lds rule can use it, or should >> use a_flags, or yet something else is a different thing. > > > OK. I think we can rework the patch to derive cpp_flags from c_flags, > use this new cpp_flags for %.i:%.c; but keep using a_flags for %.s:%.S. > > As for the .lds, we could use this new cpp_flags, the only think I saw > missing was -D__ASSEMBLY__, which can be added to the command line. > (There would also be an extra -std=gnu99, but I don't think it matters.) > > Does that sounds good? Yes. I had another thought though in the meantime: What if cpp_flags became a macro to be used with $(call ), with c_flags or a_flags (or whatever else) passed in by the use sites? > As for using -Qunused-arguments with clang, I didn't managed to find the > documentation of clang's command line argument for clang 3.5 on llvm > website, but I found it for clang 5.0 and the option is listed there. > I've tested building Xen on our gitlab CI, which as debian jessie which > seems to have clang 3.5, and Xen built just fine. So that might be an > option we can use later, but probably only for CPP flags. Okay, thanks for checking. Jan
diff --git a/xen/Rules.mk b/xen/Rules.mk index 2e28c572305a..25bcf45612bd 100644 --- a/xen/Rules.mk +++ b/xen/Rules.mk @@ -123,6 +123,7 @@ $(obj-bin-y): XEN_CFLAGS := $(filter-out -flto,$(XEN_CFLAGS)) c_flags = -MMD -MP -MF $(@D)/.$(@F).d $(XEN_CFLAGS) '-D__OBJECT_FILE__="$@"' a_flags = -MMD -MP -MF $(@D)/.$(@F).d $(XEN_AFLAGS) +cpp_flags = $(filter-out -Wa$(comma)%,$(a_flags)) include $(BASEDIR)/arch/$(TARGET_ARCH)/Rules.mk @@ -207,7 +208,7 @@ quiet_cmd_cc_s_c = CC $@ cmd_cc_s_c = $(CC) $(filter-out -Wa$(comma)%,$(c_flags)) -S $< -o $@ quiet_cmd_s_S = CPP $@ -cmd_s_S = $(CPP) $(filter-out -Wa$(comma)%,$(a_flags)) $< -o $@ +cmd_s_S = $(CPP) $(cpp_flags) $< -o $@ %.i: %.c FORCE $(call if_changed,cpp_i_c)
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- Notes: v5: - new patch xen/Rules.mk | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)