Re: [PATCH] ARM: Drop '-mthumb' from AFLAGS_ISA

[Nathan Chancellor <nathan@kernel.org>]

On Thu, Nov 17, 2022 at 11:15:09AM -0800, Nick Desaulniers wrote:
> 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.

Ack.

> 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.

Right; I suspect that it is unlikely that either of those entities will
drop CROSS_COMPILE because they aim to work with multiple trees, which
may still require CROSS_COMPILE. Maybe in five years when 5.15 is the
oldest stable release that we support ;)

> 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

Or perhaps a deep familiarity with the potential pitfalls of all this ;)

> > 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.

I would rather use an actual hash to reduce the risk of the link going
stale from either a branch rename or file rename/removal. I can use a
hash from the snapshot branch instead, if that would work for you?

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

Oh whoops, good catch!

> > 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>

Thanks for the testing and review! I will send a v2 later today and
submit it to Russell's patch tracker on Monday so that it can be picked
up for -next.

> ---
> 
> 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? ;)

At the very least, it is the second time; the first was
https://github.com/ClangBuiltLinux/linux/issues/1355.

> > ---
> >  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
> >

Cheers,
Nathan