Re: [XEN PATCH v3 05/16] xen/x86: address violations of MISRA C:2012 Directive 4.10

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.06.2024 12:25, Nicola Vetrini wrote:
> On 2024-06-26 11:26, Jan Beulich wrote:
>> On 26.06.2024 11:20, Nicola Vetrini wrote:
>>> On 2024-06-26 11:06, Jan Beulich wrote:
>>>> On 25.06.2024 21:31, Nicola Vetrini wrote:
>>>>> On 2024-03-12 09:16, Jan Beulich wrote:
>>>>>> On 11.03.2024 09:59, Simone Ballarin wrote:
>>>>>>> --- a/xen/arch/x86/Makefile
>>>>>>> +++ b/xen/arch/x86/Makefile
>>>>>>> @@ -258,18 +258,20 @@ $(obj)/asm-macros.i: CFLAGS-y += -P
>>>>>>>  $(objtree)/arch/x86/include/asm/asm-macros.h: $(obj)/asm-macros.i
>>>>>>> $(src)/Makefile
>>>>>>>         $(call filechk,asm-macros.h)
>>>>>>>
>>>>>>> +ARCHDIR = $(shell echo $(SRCARCH) | tr a-z A-Z)
>>>>>>
>>>>>> This wants to use :=, I think - there's no reason to invoke the 
>>>>>> shell
>>>>>> ...
>>>>>
>>>>> I agree on this
>>>>>
>>>>>>
>>>>>>>  define filechk_asm-macros.h
>>>>>>> +    echo '#ifndef ASM_$(ARCHDIR)_ASM_MACROS_H'; \
>>>>>>> +    echo '#define ASM_$(ARCHDIR)_ASM_MACROS_H'; \
>>>>>>>      echo '#if 0'; \
>>>>>>>      echo '.if 0'; \
>>>>>>>      echo '#endif'; \
>>>>>>> -    echo '#ifndef __ASM_MACROS_H__'; \
>>>>>>> -    echo '#define __ASM_MACROS_H__'; \
>>>>>>>      echo 'asm ( ".include \"$@\"" );'; \
>>>>>>> -    echo '#endif /* __ASM_MACROS_H__ */'; \
>>>>>>>      echo '#if 0'; \
>>>>>>>      echo '.endif'; \
>>>>>>>      cat $<; \
>>>>>>> -    echo '#endif'
>>>>>>> +    echo '#endif'; \
>>>>>>> +    echo '#endif /* ASM_$(ARCHDIR)_ASM_MACROS_H */'
>>>>>>>  endef
>>>>>>
>>>>>> ... three times while expanding this macro. Alternatively (to avoid
>>>>>> an unnecessary shell invocation when this macro is never expanded 
>>>>>> at
>>>>>> all) a shell variable inside the "define" above would want
>>>>>> introducing.
>>>>>> Whether this 2nd approach is better depends on whether we 
>>>>>> anticipate
>>>>>> further uses of ARCHDIR.
>>>>>
>>>>> However here I'm not entirely sure about the meaning of this latter
>>>>> proposal.
>>>>> My proposal is the following:
>>>>>
>>>>> ARCHDIR := $(shell echo $(SRCARCH) | tr a-z A-Z)
>>>>>
>>>>> in a suitably generic place (such as Kbuild.include or maybe
>>>>> xen/Makefile) as you suggested in subsequent patches that reused 
>>>>> this
>>>>> pattern.
>>>>
>>>> If $(ARCHDIR) is going to be used elsewhere, then what you suggest is
>>>> fine.
>>>> My "whether" in the earlier reply specifically left open for
>>>> clarification
>>>> what the intentions with the variable are. The alternative I had
>>>> described
>>>> makes sense only when $(ARCHDIR) would only ever be used inside the
>>>> filechk_asm-macros.h macro.
>>>
>>> Yes, the intention is to reuse $(ARCHDIR) in the formation of other
>>> places, as you can tell from the fact that subsequent patches 
>>> replicate
>>> the same pattern. This is going to save some duplication.
>>> The only matter left then is whether xen/Makefile (around line 250, 
>>> just
>>> after setting SRCARCH) would be better, or Kbuild.include. To me the
>>> former place seems more natural, but I'm not totally sure.
>>
>> Depends on where all the intended uses are. If they're all in 
>> xen/Makefile,
>> then having the macro just there is of course sufficient. Whereas when 
>> it's
>> needed elsewhere, instead of exporting putting it in Kbuild.include 
>> would
>> seem more natural / desirable to me.
>>
> 
> The places where this would be used are these:
> file: target (or define)
> xen/build.mk: arch/$(SRCARCH)/include/asm/asm-offsets.h: asm-offsets.s
> xen/include/Makefile: define cmd_xlat_h
> xen/arch/x86/Makefile: define filechk_asm-macros.h
> 
> The only issue that comes to my mind (it may not be one at all) is that 
> SRCARCH is defined and exported in xen/Makefile after including 
> Kbuild.include, so it would need to be defined after SRCARCH is 
> assigned:
> 
> include scripts/Kbuild.include
> 
> # Don't break if the build process wasn't called from the top level
> # we need XEN_TARGET_ARCH to generate the proper config
> include $(XEN_ROOT)/Config.mk
> 
> # Set ARCH/SRCARCH appropriately.
> 
> ARCH := $(XEN_TARGET_ARCH)
> SRCARCH := $(shell echo $(ARCH) | \
>      sed -e 's/x86.*/x86/' -e 's/arm\(32\|64\)/arm/g' \
>          -e 's/riscv.*/riscv/g' -e 's/ppc.*/ppc/g')
> export ARCH SRCARCH
> 
> Am I missing something?

In that case the alternatives are exporting or using = rather than := in
Kbuild.include, i.e. other than initially requested. Personally I dislike
exporting to a fair degree, but I'm not sure which one's better in this
case. Cc-ing Anthony for possible input.

Jan