Date:   Mon, 18 Oct 2021 13:34:31 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     Masahiro Yamada <masahiroy@...nel.org>
Cc:     Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>, llvm@...ts.linux.dev,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        Vincenzo Frascino <vincenzo.frascino@....com>,
        Lucas Henneman <henneman@...gle.com>
Subject: Re: [PATCH 2/3] arm64: vdso32: lazily invoke COMPAT_CC

On Sat, Oct 16, 2021 at 7:20 AM Masahiro Yamada <masahiroy@...nel.org> wrote:
>
> On Fri, Oct 15, 2021 at 5:59 AM Nick Desaulniers
> <ndesaulniers@...gle.com> wrote:
> >
> > On Tue, Oct 12, 2021 at 8:03 PM Masahiro Yamada <masahiroy@...nel.org> wrote:
> > >
> > > On Wed, Oct 13, 2021 at 8:46 AM Nick Desaulniers
> > > <ndesaulniers@...gle.com> wrote:
> > > >
> > > > When running the following command without arm-linux-gnueabi-gcc in
> > > > one's $PATH, the following warning is observed:
> > > >
> > > > $ ARCH=arm64 CROSS_COMPILE_COMPAT=arm-linux-gnueabi- make -j72 LLVM=1 mrproper
> > > > make[1]: arm-linux-gnueabi-gcc: No such file or directory
> > > >
> > > > This is because KCONFIG is not run for mrproper, so CONFIG_CC_IS_CLANG
> > > > is not set, and we end up eagerly evaluating various variables that try
> > > > to invoke CC_COMPAT.
> > > >
> > > > This is a similar problem to what was observed in
> > > > commit 3ec8a5b33dea ("kbuild: do not export LDFLAGS_vmlinux")
> > > >
> > > > Cc: Masahiro Yamada <masahiroy@...nel.org>
> > > > Reported-by: Lucas Henneman <henneman@...gle.com>
> > > > Signed-off-by: Nick Desaulniers <ndesaulniers@...gle.com>
> > >
> > >
> > > There are two ways to fix it:
> > >
> > >   [1]: sink the error message to /dev/null
> > >         (as in commit dc960bfeedb01cf832c5632ed1f3daed4416b142)
> > >   [2]: use a recursively-expanded variable as you did.
> > >
> > >
> > > "Simple variable (:=) vs recursive variable (=)" is a trade-off.
> > >
> > > Please be careful about the cost when you try the [2] approach.
> > >
> > >
> > >
> > > Simple variables are immediately expanded while parsing Makefile.
> > > There are 7 call-sites for cc32-option, hence
> > > the compiler is invoked 7 times for building vdso32,
> > > 0 times for cleaning.
> > > (Since 57fd251c789647552d32d2fc51bedd4f90d70f9f,
> > > try-run is no-op for 'make clean').
> > >
> > >
> > >
> > >
> > > Recursive variables are expanded every time they are used.
> > >
> > > IIUC, if_changed expands the command line 3 times.
> > > There are 2 objects (note.o and vgettimeofday.o)
> > > There are 7 call-sites for cc32-option.
> > >
> > > So, the compiler is invoked 42 (3 * 2 * 7) times
> > > for building vdso32.
> >
> > With this patch applied:
> > $ ARCH=arm64 CROSS_COMPILE_COMPAT=arm-linux-gnueabi- make LLVM=1 -j72
> > clean defconfig
> > $ ARCH=arm64 CROSS_COMPILE_COMPAT=arm-linux-gnueabi- make LLVM=1 -j72
> > arch/arm64/kernel/vdso32/ V=1 | tr -s ' ' | cut -d ' ' -f 2 | grep
> > clang | wc -l
> > 55
> > $ find arch/arm64/kernel/vdso32/ -name \*.o | xargs rm
> > $ ARCH=arm64 CROSS_COMPILE_COMPAT=arm-linux-gnueabi- make LLVM=1 -j72
> > arch/arm64/kernel/vdso32/ V=1 | tr -s ' ' | cut -d ' ' -f 2 | grep
> > clang | wc -l
> > 2
> >
> > Prior to this series:
> > $ ARCH=arm64 CROSS_COMPILE_COMPAT=arm-linux-gnueabi- make LLVM=1 -j72
> > clean defconfig
> > $ ARCH=arm64 CROSS_COMPILE_COMPAT=arm-linux-gnueabi- make LLVM=1 -j72
> > arch/arm64/kernel/vdso32/ V=1 | tr -s ' ' | cut -d ' ' -f 2 | grep
> > clang | wc -l
> > 55
> > $ find arch/arm64/kernel/vdso32/ -name \*.o | xargs rm
> > $ ARCH=arm64 CROSS_COMPILE_COMPAT=arm-linux-gnueabi- make LLVM=1 -j72
> > arch/arm64/kernel/vdso32/ V=1 | tr -s ' ' | cut -d ' ' -f 2 | grep
> > clang | wc -l
> > 2
> >
> > With patch 3 applied, we can drop CROSS_COMPILE_COMPAT, and we now get:
> > $ ARCH=arm64 make LLVM=1 -j72 clean defconfig
> > ARCH=arm64 make LLVM=1 -j72 arch/arm64/kernel/vdso32/ V=1 | tr -s ' '
> > | cut -d ' ' -f 2 | grep clang | wc -l
> > 44
> > $ find arch/arm64/kernel/vdso32/ -name \*.o | xargs rm
> > $ ARCH=arm64 make LLVM=1 -j72 arch/arm64/kernel/vdso32/ V=1 | tr -s '
> > ' | cut -d ' ' -f 2 | grep clang | wc -l
> > 2
> >
> > Please confirm; perhaps my pipeline missed some invocations? Or was
> > there a different target you were referring to?
>
>
>
>
>
>
> It is pointless to check the build commands.
>
> I am talking about how many times $(call cc32-option, ) is evaluated.

Of course V=1 doesn't print the cc-option invocations! /s

>
>
> How about adding the following debug code?
>
> (Everytime cc32-option is evaluated, a file "dummy-cc32-option-<PID>"
> is created)
>
>
>
> diff --git a/arch/arm64/kernel/vdso32/Makefile
> b/arch/arm64/kernel/vdso32/Makefile
> index 89299a26638b..e40365f5bc38 100644
> --- a/arch/arm64/kernel/vdso32/Makefile
> +++ b/arch/arm64/kernel/vdso32/Makefile
> @@ -26,9 +26,9 @@ LD_COMPAT ?= $(CROSS_COMPILE_COMPAT)ld
>  endif
>
>  cc32-option = $(call try-run,\
> -        $(CC_COMPAT) $(1) -c -x c /dev/null -o "$$TMP",$(1),$(2))
> +        $(CC_COMPAT) $(1) -c -x c /dev/null -o "$$TMP"; touch
> dummy-cc32-option-$$$$,$(1),$(2))
>  cc32-disable-warning = $(call try-run,\
> -       $(CC_COMPAT) -W$(strip $(1)) -c -x c /dev/null -o
> "$$TMP",-Wno-$(strip $(1)))
> +       $(CC_COMPAT) -W$(strip $(1)) -c -x c /dev/null -o "$$TMP";
> touch dummy-cc32-option-$$$$,-Wno-$(strip $(1)))

heh, I usually add `($info $$VAR is [${VAR}]); \` debugging statements
to these. Touching files is another neat trick.

>
>  # We cannot use the global flags to compile the vDSO files, the main reason
>  # being that the 32-bit compiler may be older than the main (64-bit) compiler
>
>
>
>
>
> Without this patch:
>
> masahiro@...ver:~/ref/linux$ rm dummy-cc32-*
> masahiro@...ver:~/ref/linux$ make -s LLVM=1 ARCH=arm64
> CROSS_COMPILE_COMPAT=arm-linux-gnueabi- defconfig clean
> arch/arm64/kernel/vdso32/ -j8
> masahiro@...ver:~/ref/linux$ ls -1 dummy-cc32-*
> dummy-cc32-disable-warning-765530
> dummy-cc32-option-765495
> dummy-cc32-option-765500
> dummy-cc32-option-765505
> dummy-cc32-option-765510
> dummy-cc32-option-765515
> dummy-cc32-option-765520
> dummy-cc32-option-765525
>
>
>
>
> With this patch:
>
>
>
> masahiro@...ver:~/ref/linux$ rm dummy-cc32-*
> masahiro@...ver:~/ref/linux$ make -s LLVM=1 ARCH=arm64
> CROSS_COMPILE_COMPAT=arm-linux-gnueabi- defconfig clean
> arch/arm64/kernel/vdso32/ -j8
> masahiro@...ver:~/ref/linux$ ls -1 dummy-cc32-*
> dummy-cc32-disable-warning-768908
> dummy-cc32-disable-warning-768949
> dummy-cc32-disable-warning-768990
> dummy-cc32-disable-warning-769035
> dummy-cc32-disable-warning-769076
> dummy-cc32-disable-warning-769117
> dummy-cc32-option-768871
> dummy-cc32-option-768878
> dummy-cc32-option-768883
> dummy-cc32-option-768888
> dummy-cc32-option-768893
> dummy-cc32-option-768898
> dummy-cc32-option-768903
> dummy-cc32-option-768914
> dummy-cc32-option-768919
> dummy-cc32-option-768924
> dummy-cc32-option-768929
> dummy-cc32-option-768934
> dummy-cc32-option-768939
> dummy-cc32-option-768944
> dummy-cc32-option-768955
> dummy-cc32-option-768960
> dummy-cc32-option-768965
> dummy-cc32-option-768970
> dummy-cc32-option-768975
> dummy-cc32-option-768980
> dummy-cc32-option-768985
> dummy-cc32-option-768998
> dummy-cc32-option-769005
> dummy-cc32-option-769010
> dummy-cc32-option-769015
> dummy-cc32-option-769020
> dummy-cc32-option-769025
> dummy-cc32-option-769030
> dummy-cc32-option-769041
> dummy-cc32-option-769046
> dummy-cc32-option-769051
> dummy-cc32-option-769056
> dummy-cc32-option-769061
> dummy-cc32-option-769066
> dummy-cc32-option-769071
> dummy-cc32-option-769082
> dummy-cc32-option-769087
> dummy-cc32-option-769092
> dummy-cc32-option-769097
> dummy-cc32-option-769102
> dummy-cc32-option-769107
> dummy-cc32-option-769112
>
>
>
>
>
>
> The diff of  the number of expansions:
>
> cc32-option                        7    ->   42
> cc32-disable-warning         1   ->    6

Indeed, thanks for confirming.  An unfortunate dichotomy.  Surely
there's another way to avoid cc-option+cc-disable-warning calls for
the `clean` target?  I'll change to just redirecting errors to
/dev/null with your Suggested-by tag, but this seems a bit of an
unfortunate situation.
-- 
Thanks,
~Nick Desaulniers

Powered by blists - more mailing lists