Re: [PATCH] ARM: Emit __gnu_mcount_nc when using Clang 10.0.0 or newer

[Nathan Chancellor]

On Thu, Aug 29, 2019 at 01:57:35PM -0700, Nick Desaulniers wrote:
> On Thu, Aug 29, 2019 at 1:21 PM Stefan Agner <stefan@xxxxxxxx> wrote:
> >
> > On 2019-08-29 21:34, Nathan Chancellor wrote:
> > > On Thu, Aug 29, 2019 at 10:55:28AM -0700, Nick Desaulniers wrote:
> > >> On Wed, Aug 28, 2019 at 11:27 PM Nathan Chancellor
> > >> <natechancellor@xxxxxxxxx> wrote:
> > >> >
> > >> > Currently, multi_v7_defconfig + CONFIG_FUNCTION_TRACER fails to build
> > >> > with clang:
> > >> >
> > >> > arm-linux-gnueabi-ld: kernel/softirq.o: in function `_local_bh_enable':
> > >> > softirq.c:(.text+0x504): undefined reference to `mcount'
> > >> > arm-linux-gnueabi-ld: kernel/softirq.o: in function `__local_bh_enable_ip':
> > >> > softirq.c:(.text+0x58c): undefined reference to `mcount'
> > >> > arm-linux-gnueabi-ld: kernel/softirq.o: in function `do_softirq':
> > >> > softirq.c:(.text+0x6c8): undefined reference to `mcount'
> > >> > arm-linux-gnueabi-ld: kernel/softirq.o: in function `irq_enter':
> > >> > softirq.c:(.text+0x75c): undefined reference to `mcount'
> > >> > arm-linux-gnueabi-ld: kernel/softirq.o: in function `irq_exit':
> > >> > softirq.c:(.text+0x840): undefined reference to `mcount'
> > >> > arm-linux-gnueabi-ld: kernel/softirq.o:softirq.c:(.text+0xa50): more undefined references to `mcount' follow
> > >> >
> > >> > clang can emit a working mcount symbol, __gnu_mcount_nc, when
> > >> > '-meabi gnu' is passed to it. Until r369147 in LLVM, this was
> > >> > broken and caused the kernel not to boot because the calling
> > >> > convention was not correct. Now that it is fixed, add this to
> > >> > the command line when clang is 10.0.0 or newer so everything
> > >> > works properly.
> > >> >
> > >> > Link: https://github.com/ClangBuiltLinux/linux/issues/35
> > >> > Link: https://bugs.llvm.org/show_bug.cgi?id=33845
> > >> > Link: https://github.com/llvm/llvm-project/commit/16fa8b09702378bacfa3d07081afe6b353b99e60
> > >> > Signed-off-by: Nathan Chancellor <natechancellor@xxxxxxxxx>
> > >> > ---
> > >> >  arch/arm/Makefile | 6 ++++++
> > >> >  1 file changed, 6 insertions(+)
> > >> >
> > >> > diff --git a/arch/arm/Makefile b/arch/arm/Makefile
> > >> > index c3624ca6c0bc..7b5a26a866fc 100644
> > >> > --- a/arch/arm/Makefile
> > >> > +++ b/arch/arm/Makefile
> > >> > @@ -112,6 +112,12 @@ ifeq ($(CONFIG_ARM_UNWIND),y)
> > >> >  CFLAGS_ABI     +=-funwind-tables
> > >> >  endif
> > >> >
> > >> > +ifeq ($(CONFIG_CC_IS_CLANG),y)
> > >> > +ifeq ($(shell test $(CONFIG_CLANG_VERSION) -ge 100000; echo $$?),0)
> > >> > +CFLAGS_ABI     +=-meabi gnu
> > >> > +endif
> > >> > +endif
> > >> > +
> > >>
> > >> Thanks for the patch!  I think this is one of the final issues w/ 32b
> > >> ARM configs when building w/ Clang.
> > >>
> > >> I'm not super enthused about the version check.  The flag is indeed
> > >> not recognized by GCC, but I think it would actually be more concise
> > >> with $(cc-option) and no compiler or version check.
> > >>
> > >> Further, I think that the working __gnu_mcount_nc in Clang would
> > >> better be represented as marking the arch/arm/KConfig option for
> > >> CONFIG_FUNCTION_TRACER for dependent on a version of Clang greater
> > >> than or equal to Clang 10, not conditionally adding this flag. (We
> > >> should always add the flag when supported, IMO.  __gnu_mcount_nc's
> > >> calling convention being broken is orthogonal to the choice of
> > >> __gnu_mcount_nc vs mcount, and it's the former's that should be
> > >> checked, not the latter as in this patch.
> > >
> > > I will test with or without CONFIG_AEABI like Matthias asked and I will
> > > implement your Kconfig suggestion if it passes all of my tests. The
> > > reason that I did it this way is because I didn't want a user to end up
> > > with a non-booting kernel since -meabi gnu works with older versions of
> > > clang at build time, the issue happens at boot time but the Kconfig
> > > suggestion + cc-option should fix that.
> >
> > I agree with Nathan here, I'd rather prefer the build system to fail
> > building rather than runtime error.
> >
> > If we decide we want to have it building despite it not building a
> > functional kernel, we should at least add a #warning...
> 
> Just to be clear...I was suggesting a build failure, but for
> __gnu_mcount_nc not having the correct calling convention in older
> clang releases, which is orthogonal to passing `-meabi gnu`.  This
> patch uses the __gnu_mcount_nc calling convention problem to justify a
> version check for a flag that while closely related, is not actually
> the problem, IMO.

I am not entirely sure what you mean. -meabi gnu has always produced a
buildable kernel (see Stefan's original report [1]), it just doesn't
boot and that all happens silently.

[1]: https://lore.kernel.org/linux-arm-kernel/35ae3d464c7b77a5fe86a732079e32bc@xxxxxxxx/

Okay, after doing some further testing...

Disabling CONFIG_AEABI does not work with clang, I get a ton of failures
around undefined references to __aeabi_idivmod and __aeabi_uidivmod. I
don't think this is worth looking into given that CONFIG_AEABI is not
selectable when CONFIG_CPU_V6 or CONFIG_CPU_V7 is selected, which is
what we primarily care about.. We currently build and boot
multi_v5_defconfig but it has CONFIG_AEABI set in it. As a result, I
don't think we need to care about it with this patch.

This diff would also solve the issue without the need for the version
check in the Makefile, as it is the combination of -meabi gnu and -pg
that causes the boot issue and -pg gets added when
CONFIG_FUNCTION_TRACER is enabled. Would that be preferred? I do not
think adding cc-option is necessary given that we know GCC universally
does not support this flag; there is no point in adding a small penalty
with cc-option to either compiler.

Cheers,
Nathan

================================================

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index a98c7af50bf0..440ad41e77e4 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -83,7 +83,7 @@ config ARM
 	select HAVE_FAST_GUP if ARM_LPAE
 	select HAVE_FTRACE_MCOUNT_RECORD if !XIP_KERNEL
 	select HAVE_FUNCTION_GRAPH_TRACER if !THUMB2_KERNEL && !CC_IS_CLANG
-	select HAVE_FUNCTION_TRACER if !XIP_KERNEL
+	select HAVE_FUNCTION_TRACER if !XIP_KERNEL && (CC_IS_GCC || CLANG_VERSION >= 100000)
 	select HAVE_GCC_PLUGINS
 	select HAVE_HW_BREAKPOINT if PERF_EVENTS && (CPU_V6 || CPU_V6K || CPU_V7)
 	select HAVE_IDE if PCI || ISA || PCMCIA
diff --git a/arch/arm/Makefile b/arch/arm/Makefile
index a43fc753aa53..23c2bf0fbd30 100644
--- a/arch/arm/Makefile
+++ b/arch/arm/Makefile
@@ -115,6 +115,10 @@ ifeq ($(CONFIG_ARM_UNWIND),y)
 CFLAGS_ABI	+=-funwind-tables
 endif
 
+ifeq ($(CONFIG_CC_IS_CLANG),y)
+CFLAGS_ABI	+=-meabi gnu
+endif
+
 # Accept old syntax despite ".syntax unified"
 AFLAGS_NOWARN	:=$(call as-option,-Wa$(comma)-mno-warn-deprecated,-Wa$(comma)-W)