[PATCH] kbuild: Avoid including architecture-specific Makefile twice

[Ilias Apalodimas]

Hi Yao,

On Fri, 11 Jul 2025 at 10:28, Yao Zi <ziyao at disroot.org> wrote:
>
> On Fri, Jul 11, 2025 at 08:11:03AM +0300, Ilias Apalodimas wrote:
> > Hi Yao,
> >
> >
> > On Wed, 9 Jul 2025 at 19:15, Yao Zi <ziyao at disroot.org> wrote:
> > >
> > > Stranges errors are observed when building U-Boot master for almost any
> > > RISC-V board, the messages are in two types, one is about duplicated
> > > symbols,
> > >
> > >         u-boot/arch/riscv/cpu//mtrap.S:32: multiple definition of `trap_entry';
> > >         arch/riscv/cpu/mtrap.o: u-boot/arch/riscv/cpu//mtrap.S:32: first defined here
> > >
> > > and the other is fixdep's complaint about missing dependency files,
> > >
> > >         fixdep: error opening file: arch/riscv/cpu/.mtrap.o.d: No such file or directory
> > >         fixdep: error opening file: arch/riscv/cpu//.start.o.d: No such file or directory
> > >
> > > where the latter could only be reproduced when building parallelly.
> > >
> > > Both the two types of errors are about files in arch/riscv/cpu, and
> > > there's a suspicious slash character in the reported path. Looking
> > > through RISC-V-specific Makefiles, there's only one place that may
> > > expand to such a path,
> > >
> > >         libs-y += arch/riscv/cpu/$(CPU)/
> > >
> > > The right hand expands to "arch/riscv/cpu//" if $(CPU) isn't defined at
> > > the time of including. With some debug statement added to
> > > arch/riscv/Makefile, the output proves that arch/riscv/Makefile is
> > > included twice, once with $(CPU) undefined and once defined correctly
> > > according to CONFIG_SYS_CPU.
> > >
> > > Futher bisecting shows an extra include statement against
> > > arch/$(SRCARCH)/Makefile is added in earlier bump of Kbuild system. But
> > > the statement is evaluated before config.mk is included and definition
> > > of $(CPU), causing objects in arch/riscv/cpu/ are built and linked twice
> > > (once as "arch/riscv/cpu/*", and once as "arch/riscv/cpu//*"), resulting
> > > in the error.
> > >
> > > Let's simply remove the extra include to fix these nasty errors. Note
> > > though this therotically affects all architectures, ones except RISC-V
> > > may not reproduce similar errors since there are no source files
> > > directly under arch/*/cpu, thus the extra include doesn't hurt.
> >
> > There's another unconditional include under ifeq ($(config-targets),1).
> > Should we remove that as well?
>
> I don't think it really matters.

It doesn't indeed. It's just for consistency

> This include is only for config targets
> that don't actually run the build, but only descent into scripts/kconfig
> and generate a configuration. The include of arch/$(SRCARCH)/Makefile
> seems only for getting an appropriate name of defconfig
> (KBUILD_DEFCONFIG), as explained by the comment. It seems even okay to
> remove the include, as U-Boot has no architecture defining its own
> KBUILD_DEFCONFIG.
>
> > The patch it self seems correct, but can you try removing the
> > pre-existing include instead and add the config.mk inclusion?
> > Something like
> > diff --git a/Makefile b/Makefile
> > index db8d89587290..e6ad9be262ef 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -636,6 +636,7 @@ ifeq ($(config-targets),1)
> >  # KBUILD_DEFCONFIG may point out an alternative default configuration
> >  # used for 'make defconfig'
> >  # Modified for U-Boot
> > +include config.mk
> >  -include arch/$(SRCARCH)/Makefile
> >  export KBUILD_DEFCONFIG KBUILD_KCONFIG CC_VERSION_TEXT
> >
> > @@ -725,6 +726,7 @@ ARCH_CPPFLAGS :=
> >  ARCH_AFLAGS :=
> >  ARCH_CFLAGS :=
> >  # Modified for U-Boot
> > +include config.mk
>
> I don't think config.mk could be included without making sure
> include/autoconf.mk is update to date, just as pointed out by the
> comment at line 768
>

Ok thanks for testing.
Can you send a v2 removing both of the erroneous -includes and add a
comment, that we define it elsewhere for all the reasons you
mentioned?

With that feel free to add
Reviewed-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>

Thanks!
>         > We want to include arch/$(ARCH)/config.mk only when
>         > include/config/auto.conf is up-to-date. When we switch to a
>         > different board configuration, old CONFIG macros are still
>         > remaining in include/config/auto.conf. Without the following
>         > gimmick, wrong config.mk would be included leading nasty
>         > warnings/errors.
>
> And this change does cause errors if I use a defconfig, build U-Boot,
> then switch to another defconfig and build again, for example,
>
>         $ make ARCH=arm CROSS_COMPILE=arm-none-eabi- mx6sabresd_defconfig
>         $ make ARCH=arm CROSS_COMPILE=arm-none-eabi-
>         $ make ARCH=riscv CROSS_COMPILE=riscv64-unknown-linux-musl- \
>                 sifive_unleashed_defconfig
>         $ make ARCH=riscv CROSS_COMPILE=riscv64-unknown-linux-musl-
>
> results in errors like
>
>         drivers/gpio/sifive-gpio.c:9:10: fatal error: asm/arch/gpio.h: No such file or directory
>             9 | #include <asm/arch/gpio.h>
>               |          ^~~~~~~~~~~~~~~~~
>         compilation terminated.
>
> and ls -l arch/riscv/include/asm/arch shows the symlink points to a
> non-existing directory,
>
>         $ ls -l arch/riscv/include/asm/arch
>         lrwxrwxrwx    1 ziyao    ziyao            8 Jul 11 07:11 arch/riscv/include/asm/arch -> arch-mx6
>
> it's really likely that the old auto.conf is evaluated, since arch-mx6
> isn't a RISC-V thing.
>
> >  -include arch/$(SRCARCH)/Makefile
> >
> >  ifeq ($(dot-config),1)
> > @@ -773,10 +775,6 @@ ifneq ($(wildcard $(KCONFIG_CONFIG)),)
> >  ifneq ($(wildcard include/config/auto.conf),)
> >  autoconf_is_old := $(shell find . -path ./$(KCONFIG_CONFIG) -newer \
> >                                                 include/config/auto.conf)
> > -ifeq ($(autoconf_is_old),)
> > -include config.mk
> > -include arch/$(ARCH)/Makefile
> > -endif
> >  endif
> >  endif
> >
> > I prefer keeping the makefile closer to the kernel so we can sync it
> > up easier in the future
> >
> > Thanks for looking at this
> > /Ilias
> > >
>
> Regards,
> Yao Zi