| Message ID | 20190829062635.45609-1-natechancellor@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: Emit __gnu_mcount_nc when using Clang 10.0.0 or newer | expand |
On 2019-08-29 08:26, Nathan Chancellor 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. Cool, finally function tracing with Clang :-) > > 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@gmail.com> Reviewed-by: Stefan Agner <stefan@agner.ch> -- Stefan > --- > 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 > + > # Accept old syntax despite ".syntax unified" > AFLAGS_NOWARN :=$(call as-option,-Wa$(comma)-mno-warn-deprecated,-Wa$(comma)-W)
On Wed, Aug 28, 2019 at 11:26:35PM -0700, Nathan Chancellor 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@gmail.com> > --- > 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 Is this also correct/needed when CONFIG_AEABI is not set?
On Wed, Aug 28, 2019 at 11:27 PM Nathan Chancellor <natechancellor@gmail.com> 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@gmail.com> > --- > 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. > # Accept old syntax despite ".syntax unified" > AFLAGS_NOWARN :=$(call as-option,-Wa$(comma)-mno-warn-deprecated,-Wa$(comma)-W) > > -- > 2.23.0 >
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@gmail.com> 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@gmail.com> > > --- > > 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 should have a v2 out this evening. Cheers, Nathan
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@gmail.com> 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@gmail.com> >> > --- >> > 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... -- Stefan > > I should have a v2 out this evening. > > Cheers, > Nathan
On Thu, Aug 29, 2019 at 1:21 PM Stefan Agner <stefan@agner.ch> 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@gmail.com> 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@gmail.com> > >> > --- > >> > 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.
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@agner.ch> 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@gmail.com> 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@gmail.com> > > >> > --- > > >> > 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@agner.ch/ 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)
On Fri, Aug 30, 2019 at 10:28 AM Nathan Chancellor <natechancellor@gmail.com> wrote: > > 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@agner.ch> 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 > > > > 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. > 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. The plan of record is to never support !CONFIG_AEBI (ie OABI) w/ Clang. See also my commit currently in linux-next: ARM: 8875/1: Kconfig: default to AEABI w/ Clang https://github.com/ClangBuiltLinux/linux/issues/482 so !AEABI is a moot point. If we ever changed our minds, then yes we should additionally guard on !CONFIG_AEABI, but I feel like that's a highly unlikely scenario at this point. > > 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 Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > + > # Accept old syntax despite ".syntax unified" > AFLAGS_NOWARN :=$(call as-option,-Wa$(comma)-mno-warn-deprecated,-Wa$(comma)-W) >
On Fri, Aug 30, 2019 at 2:13 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Fri, Aug 30, 2019 at 10:28 AM Nathan Chancellor > <natechancellor@gmail.com> wrote: > > 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 Needs a space. `+=-`.
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 + # Accept old syntax despite ".syntax unified" AFLAGS_NOWARN :=$(call as-option,-Wa$(comma)-mno-warn-deprecated,-Wa$(comma)-W)
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@gmail.com> --- arch/arm/Makefile | 6 ++++++ 1 file changed, 6 insertions(+)