ARM: Drop '-mthumb' from AFLAGS_ISA

  When building with CONFIG_THUMB2_KERNEL=y + a version of clang from
Debian, the following warning occurs frequently:

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

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
Reported-by: "kernelci.org bot" <bot@kernelci.org>
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
 arch/arm/Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


base-commit: 0c52591d22e99759da3793f19249bbf45ad742bd
  

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
>
  

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
  

On Thu, Nov 17, 2022 at 11:34 AM Nathan Chancellor <nathan@kernel.org> wrote:
>
> 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:
> > >
> > > 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?

It doesn't matter much to me; I trust your judgement; you pick.
Perhaps that depends if the snapshot branch has stable SHAs or whether
they change over time? Maybe Sylvestre can comment on that.
  

Le 17/11/2022 à 23:51, Nick Desaulniers a écrit :
> On Thu, Nov 17, 2022 at 11:34 AM Nathan Chancellor <nathan@kernel.org> wrote:
>>
>> 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:
>>>>
>>>> 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?
> 
> It doesn't matter much to me; I trust your judgement; you pick.
> Perhaps that depends if the snapshot branch has stable SHAs or whether
> they change over time? Maybe Sylvestre can comment on that.
Yeah, it can change overtime. I have to rebase them from time to time.

Cheers,
Sylvestre