Re: [XEN PATCH v5 08/16] build: Introduce $(cpp_flags)

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

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,

-- 
Anthony PERARD