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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

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,

-- 
Anthony PERARD