Re: [PATCH v4] ARM: decompressor: simplify libfdt builds

[Geert Uytterhoeven]

Hi Yamada-san, Kees, Russell,

-CC RMK's patch system
+CC lakml

On Sun, Apr 19, 2020 at 9:26 PM Masahiro Yamada <masahiroy@xxxxxxxxxx> wrote:
> Copying source files during the build time may not end up with
> as clean code as expected.
>
> lib/fdt*.c simply wrap scripts/dtc/libfdt/fdt*.c, and it works
> nicely. Let's follow this approach for the arm decompressor, too.
>
> Add four wrappers, arch/arm/boot/compressed/fdt*.c and remove
> the Makefile messes. Another nice thing is we no longer need to
> maintain the own libfdt_env.h because the decompressor can include
> <linux/libfdt_env.h>.
>
> There is a subtle problem when generated files are turned into
> check-in files.
>
> When you are doing a rebuild of an existing object tree with O=
> option, there exists stale "shipped" copies that the old Makefile
> implementation created. The build system ends up with compiling the
> stale generated files because Make searches for prerequisites in the
> current directory, i.e. $(objtree) first, and then the directory
> listed in VPATH, i.e. $(srctree).
>
> To mend this issue, I added the following code:
>
>   ifdef building_out_of_srctree
>   $(shell rm -f $(addprefix $(obj)/, fdt_rw.c fdt_ro.c fdt_wip.c fdt.c))
>   endif
>
> This will need to stay for a while because "git bisect" crossing this
> commit, otherwise, would result in a build error.
>
> Signed-off-by: Masahiro Yamada <masahiroy@xxxxxxxxxx>

This is now commit 365a6327cd643eed ("ARM: 8968/1: decompressor:
simplify libfdt builds") in arm/for-next.

In light of reworking "[PATCH v5] ARM: boot: Obtain start of physical
memory from DTB"[1] on top of this, which would conditionally add
another source file to libfdt_objs, I have a few questions.

> --- a/arch/arm/boot/compressed/Makefile
> +++ b/arch/arm/boot/compressed/Makefile
> @@ -76,29 +76,31 @@ compress-$(CONFIG_KERNEL_LZMA) = lzma
>  compress-$(CONFIG_KERNEL_XZ)   = xzkern
>  compress-$(CONFIG_KERNEL_LZ4)  = lz4
>
> -# Borrowed libfdt files for the ATAG compatibility mode
> -
> -libfdt         := fdt_rw.c fdt_ro.c fdt_wip.c fdt.c
> -libfdt_hdrs    := fdt.h libfdt.h libfdt_internal.h
> -
> -libfdt_objs    := $(addsuffix .o, $(basename $(libfdt)))
> +ifeq ($(CONFIG_ARM_ATAG_DTB_COMPAT),y)
> +libfdt_objs = fdt_rw.o fdt_ro.o fdt_wip.o fdt.o atags_to_fdt.o
>

I guess the code below can be moved out of the ifeq block, as it doesn't
really do anything if CONFIG_ARM_ATAG_DTB_COMPAT=n, and $(libfdt_objs)
becomes empty?
If not, I think I'll have to add a new Kconfig symbol ARM_BOOT_LIBFDT,
to be selected by ARM_ATAG_DTB_COMPAT and USE_OF.

> -$(addprefix $(obj)/,$(libfdt) $(libfdt_hdrs)): $(obj)/%: $(srctree)/scripts/dtc/libfdt/%
> -       $(call cmd,shipped)
> +OBJS   += $(libfdt_objs)
>
> -$(addprefix $(obj)/,$(libfdt_objs) atags_to_fdt.o): \
> -       $(addprefix $(obj)/,$(libfdt_hdrs))
> +# -fstack-protector-strong triggers protection checks in this code,
> +# but it is being used too early to link to meaningful stack_chk logic.
> +nossp-flags-$(CONFIG_CC_HAS_STACKPROTECTOR_NONE) := -fno-stack-protector
> +$(foreach o, $(libfdt_objs), \
> +       $(eval CFLAGS_$(o) := -I $(srctree)/scripts/dtc/libfdt $(nossp-flags-y)))

Is there a real reason this is only applied to a subset of the C object
files, and not to all of them? Or have we been lucky so far, by not
triggering the issue in decompressed.c, misc.c, and string.c (yet)?

Thanks!

> +
> +# These were previously generated C files. When you are building the kernel
> +# with O=, make sure to remove the stale files in the output tree. Otherwise,
> +# the build system wrongly compiles the stale ones.
> +ifdef building_out_of_srctree
> +$(shell rm -f $(addprefix $(obj)/, fdt_rw.c fdt_ro.c fdt_wip.c fdt.c))
> +endif
>
> -ifeq ($(CONFIG_ARM_ATAG_DTB_COMPAT),y)
> -OBJS   += $(libfdt_objs) atags_to_fdt.o
>  endif
>
>  targets       := vmlinux vmlinux.lds piggy_data piggy.o \
>                  lib1funcs.o ashldi3.o bswapsdi2.o \
>                  head.o $(OBJS)
>
> -clean-files += piggy_data lib1funcs.S ashldi3.S bswapsdi2.S \
> -               $(libfdt) $(libfdt_hdrs) hyp-stub.S
> +clean-files += piggy_data lib1funcs.S ashldi3.S bswapsdi2.S hyp-stub.S
>
>  KBUILD_CFLAGS += -DDISABLE_BRANCH_PROFILING
>
> @@ -107,15 +109,6 @@ ORIG_CFLAGS := $(KBUILD_CFLAGS)
>  KBUILD_CFLAGS = $(subst -pg, , $(ORIG_CFLAGS))
>  endif
>
> -# -fstack-protector-strong triggers protection checks in this code,
> -# but it is being used too early to link to meaningful stack_chk logic.
> -nossp-flags-$(CONFIG_CC_HAS_STACKPROTECTOR_NONE) := -fno-stack-protector
> -CFLAGS_atags_to_fdt.o := $(nossp-flags-y)
> -CFLAGS_fdt.o := $(nossp-flags-y)
> -CFLAGS_fdt_ro.o := $(nossp-flags-y)
> -CFLAGS_fdt_rw.o := $(nossp-flags-y)
> -CFLAGS_fdt_wip.o := $(nossp-flags-y)
> -
>  ccflags-y := -fpic $(call cc-option,-mno-single-pic-base,) -fno-builtin \
>              -I$(obj) $(DISABLE_ARM_SSP_PER_TASK_PLUGIN)
>  asflags-y := -DZIMAGE

[1] https://lore.kernel.org/r/20200415153409.30112-1-geert+renesas@xxxxxxxxx

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds