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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Wed, Jun 26, 2024 at 12:31:42PM +0200, Jan Beulich wrote:
> 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.

None. The name is missleading anyway, it would suggest to me that it
contain a directory, but that's wrong.

Another thing that suboptimal, use make to call a shell to generate a
string that is going to be later use in shell context. How about just
doing the work in that later shell context?

Something like:

 define filechk_asm-macros.h
+    guard=$$(echo ASM_${SRCARCH}_ASM_MACROS_H | tr a-z A-Z); \
+    echo "#ifndef $$guard"; \
+    echo "#define $$guard"; \
     echo '#if 0'; \
     echo '.if 0'; \

Or, instead of having to write the name of the file down, we could
use a name that is already registered in a variable:

 define filechk_asm-macros.h
+    guard=$$(echo $@ | tr a-z/.- A-Z_); \
+    echo "#ifndef $$guard"; \
+    echo "#define $$guard"; \
     echo '#if 0'; \
     echo '.if 0'; \

This produces:
    #ifndef ARCH_X86_INCLUDE_ASM_ASM_MACROS_H
    #define ARCH_X86_INCLUDE_ASM_ASM_MACROS_H
    #if 0
    .if 0

Cheers,

-- 

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech