Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221114225719.1657174-1-nathan@kernel.org>
In-Reply-To: <20221114225719.1657174-1-nathan@kernel.org>
From:   Nick Desaulniers <ndesaulniers@google.com>
Date:   Thu, 17 Nov 2022 11:15:09 -0800
Message-ID: <CAKwvOdn67r3ZYb5XZkae3i5797GGV3=8=nLC7kT2d4On3OEm5A@mail.gmail.com>
Subject: Re: [PATCH] ARM: Drop '-mthumb' from AFLAGS_ISA
To:     Nathan Chancellor <nathan@kernel.org>,
        Sylvestre Ledru <sylvestre@debian.org>,
        Serge Guelton <sguelton@mozilla.com>
Cc:     Russell King <linux@armlinux.org.uk>,
        Arnd Bergmann <arnd@arndb.de>,
        Ard Biesheuvel <ardb@kernel.org>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        llvm@lists.linux.dev, patches@lists.linux.dev,
        "kernelci.org bot" <bot@kernelci.org>,
        Sylvestre Ledru <sylvestre@mozilla.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Nov 14, 2022 at 2:58 PM Nathan Chancellor <nathan@kernel.org> wrote:
>
> When building with CONFIG_THUMB2_KERNEL=y + a version of clang from
> Debian, the following warning occurs frequently:

I also needed to explicitly set
CROSS_COMPILE=arm-linux-gnueabihf-
to reproduce.  Please add that detail to the commit message.  Thanks
for helping spot that difference on IRC.

It sounds like tuxmake (which our CI is built on) and perhaps kernelCI
are both setting that variable, which is no longer necessary when
using LLVM=1 for ARCH=arm.

Not CROSS_COMPILE=arm-linux-gnueabi- like the triple we use by default
for ARCH=arm in scripts/Makefile.clang.  So this issue arises from:
1. using debian's clang, which is carrying an out of tree patch affecting this.
2. using `CROSS_COMPILE=arm-linux-gnueabihf-`.

The use of both of those in conjunction I'd like to think would be
relatively unlikely, but it seems that we have both CI systems doing
this (and the patch LGTM regardless of changing the CI).

>
>   <built-in>:383:9: warning: '__thumb2__' macro redefined [-Wmacro-redefined]
>   #define __thumb2__ 2
>           ^
>   <built-in>:353:9: note: previous definition is here
>   #define __thumb2__ 1
>           ^
>   1 warning generated.
>
> Debian carries a downstream patch that changes the default CPU of the
> arm-linux-gnueabihf target from 'arm1176jzf-s' (v6) to 'cortex-a7' (v7).
> As a result, '-mthumb' defines both '__thumb__' and '__thumb2__'. The
> define of '__thumb2__' via the command line was purposefully added to
> catch a situation like this.

And we caught something!  It's almost like Ard has sight-beyond-sight
or something when he made that suggestion. Coincidence? I think not...
:P

>
> In a similar vein as commit 26b12e084bce ("ARM: 9264/1: only use
> -mtp=cp15 for the compiler"), do not add '-mthumb' to AFLAGS_ISA, as it
> is already passed to the assembler via '-Wa,-mthumb' and '__thumb2__' is
> already defined for preprocessing.
>
> Fixes: 1d2e9b67b001 ("ARM: 9265/1: pass -march= only to compiler")
> Link: htps://salsa.debian.org/pkg-llvm-team/llvm-toolchain/-/blob/17354b030ac4252ff6c5e9d01f4eba28bd406b2d/debian/patches/930008-arm.diff

Would you mind using
https://salsa.debian.org/pkg-llvm-team/llvm-toolchain/-/blob/snapshot/debian/patches/930008-arm.diff
as the link instead? The link on this commit message is a diff against
llvm-14, not ToT which is currently llvm-16; the context is quite
different as the logic moved source files completely.  Though it does
look like Sylvestre has not yet cut a 16 branch for debian's patches.

If not, at least re-add the missing `t` from the protocol in the URL
(s/htps/https/).

> Reported-by: "kernelci.org bot" <bot@kernelci.org>
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>

I verified this locally with LLVM built from source, comparing no out
of tree patches vs just debian's 930008-arm.diff applied.

Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>

---

If memory serves, this is perhaps the third time downstream debian
patches to llvm have caused us initially-difficult-to-reproduce bugs.
Sylvestre, going forward, would you mind please giving your diff's
more descriptive file names, or making them actual commits with some
context in the commit message?  Time and resource permitting,
submitting them upstream, even if they're not accepted, but pointing
to the upstream discussion (if any) from commit messages would provide
us more context for these kind of things.  Maybe Serge could help you
burn down those out of tree patches? ;)

> ---
>  arch/arm/Makefile | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/Makefile b/arch/arm/Makefile
> index 357f0d9b8607..d1ebb746ff40 100644
> --- a/arch/arm/Makefile
> +++ b/arch/arm/Makefile
> @@ -131,8 +131,9 @@ endif
>  AFLAGS_NOWARN  :=$(call as-option,-Wa$(comma)-mno-warn-deprecated,-Wa$(comma)-W)
>
>  ifeq ($(CONFIG_THUMB2_KERNEL),y)
> -CFLAGS_ISA     :=-mthumb -Wa,-mimplicit-it=always $(AFLAGS_NOWARN)
> +CFLAGS_ISA     :=-Wa,-mimplicit-it=always $(AFLAGS_NOWARN)
>  AFLAGS_ISA     :=$(CFLAGS_ISA) -Wa$(comma)-mthumb -D__thumb2__=2
> +CFLAGS_ISA     +=-mthumb
>  else
>  CFLAGS_ISA     :=$(call cc-option,-marm,) $(AFLAGS_NOWARN)
>  AFLAGS_ISA     :=$(CFLAGS_ISA)
>
> base-commit: 0c52591d22e99759da3793f19249bbf45ad742bd
> --
> 2.38.1
>


-- 
Thanks,
~Nick Desaulniers