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

[Jan Beulich <jbeulich@xxxxxxxx>]

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