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
