diff mbox series

[07/11] package/busybox: fix applets runtime issue when building with clang cross-compiler

Message ID 20190906090947.5476-8-romain.naour@smile.fr
State Superseded
Headers show
Series Add the support for Clang cross-compiler | expand

Commit Message

Romain Naour Sept. 6, 2019, 9:09 a.m. UTC
Apply a patch contributed by Luis Marques on the Busybox mailing list [1]
fixing a runtime issue (segfault) when busybox is compiled by Clang.

The patch disable the compiler optimizations for Clang/LLVM only.

Without this patch, busybox segfault with several applets
(login on aarch64 using Clang 8.0.1, init on x86_64 using Clang 9.0.0rc3)

[1] http://lists.busybox.net/pipermail/busybox/2019-June/087337.html

Signed-off-by: Romain Naour <romain.naour@smile.fr>
Cc: Luis Marques <luismarques@lowrisc.org>
Cc: Matt Weber <matthew.weber@rockwellcollins.com>
Cc: Valentin Korenblit <valentinkorenblit@gmail.com>
---
Note:
 The Luis Marques's SoB line in the Busybox patch is missing.
 The patch is still under review to avoid disabling optimizations.
---
 ...use-BB_GLOBAL_CONST-where-applicable.patch | 173 ++++++++++++++++++
 1 file changed, 173 insertions(+)
 create mode 100644 package/busybox/0003-use-BB_GLOBAL_CONST-where-applicable.patch

Comments

Matt Weber Sept. 7, 2019, 3:11 a.m. UTC | #1
Romain,

On Fri, Sep 6, 2019 at 4:10 AM Romain Naour <romain.naour@smile.fr> wrote:
>
> Apply a patch contributed by Luis Marques on the Busybox mailing list [1]
> fixing a runtime issue (segfault) when busybox is compiled by Clang.
>
> The patch disable the compiler optimizations for Clang/LLVM only.
>
> Without this patch, busybox segfault with several applets
> (login on aarch64 using Clang 8.0.1, init on x86_64 using Clang 9.0.0rc3)
>
> [1] http://lists.busybox.net/pipermail/busybox/2019-June/087337.html
>

Got a "crtbegin.o no such file or directory" error when I setup a
build using master with qemu_aarch64_virt_defconfig (updated for
prebuilt external toolchain and enabling clang as cross-compiler).
https://paste.ubuntu.com/p/wxrmVTGVvp/

From your comment earlier on irc that you tested with 9.0.1 (x86_64
and aarch64), I wonder if this error is a difference between building
with clang 8.0.1 and 9.

I was able to build compiler-rt & libfuzzer with your series applied
and pass a runtime test (system still cross compiled with gcc but
using "[01/11] package/clang: help host-clang to find our external
toolchain" to find the sysroot).  This was instead of using the -B
option when building libfuzzer as part of the runtime test.

Matt
Matt Weber Sept. 7, 2019, 3:17 a.m. UTC | #2
Romain,

On Fri, Sep 6, 2019 at 10:11 PM Matthew Weber
<matthew.weber@rockwellcollins.com> wrote:
>
> Romain,
>
> On Fri, Sep 6, 2019 at 4:10 AM Romain Naour <romain.naour@smile.fr> wrote:
> >
> > Apply a patch contributed by Luis Marques on the Busybox mailing list [1]
> > fixing a runtime issue (segfault) when busybox is compiled by Clang.
> >
> > The patch disable the compiler optimizations for Clang/LLVM only.
> >
> > Without this patch, busybox segfault with several applets
> > (login on aarch64 using Clang 8.0.1, init on x86_64 using Clang 9.0.0rc3)
> >
> > [1] http://lists.busybox.net/pipermail/busybox/2019-June/087337.html
> >
>
> Got a "crtbegin.o no such file or directory" error when I setup a
> build using master with qemu_aarch64_virt_defconfig (updated for
> prebuilt external toolchain and enabling clang as cross-compiler).
> https://paste.ubuntu.com/p/wxrmVTGVvp/
>

# find output/host/ -name  crtbegin.o
output/host/opt/ext-toolchain/lib/gcc/aarch64-linux-gnu/8.3.0/crtbegin.o
# output/host/bin/clang -print-search-dirs
programs: =/accts/mlweber1/tmp.M0HlOc7IKF-buildroot/output/host/bin:/..//bin
libraries: =/accts/mlweber1/tmp.M0HlOc7IKF-buildroot/output/host/lib/clang/8.0.1:/accts/mlweber1/tmp.M0HlOc7IKF-buildroot/output/host/aarch64-buildroot-linux-gnu/sysroot/lib/../lib64:/accts/mlweber1/tmp.M0HlOc7IKF-buildroot/output/host/aarch64-buildroot-linux-gnu/sysroot/usr/lib/../lib64:/accts/mlweber1/tmp.M0HlOc7IKF-buildroot/output/host/aarch64-buildroot-linux-gnu/sysroot/lib:/accts/mlweber1/tmp.M0HlOc7IKF-buildroot/output/host/aarch64-buildroot-linux-gnu/sysroot/usr/lib


> From your comment earlier on irc that you tested with 9.0.1 (x86_64
> and aarch64), I wonder if this error is a difference between building
> with clang 8.0.1 and 9.
>
> I was able to build compiler-rt & libfuzzer with your series applied
> and pass a runtime test (system still cross compiled with gcc but
> using "[01/11] package/clang: help host-clang to find our external
> toolchain" to find the sysroot).  This was instead of using the -B
> option when building libfuzzer as part of the runtime test.
>
> Matt
Romain Naour Sept. 7, 2019, 8:02 a.m. UTC | #3
Hi Matt,

Le 07/09/2019 à 05:11, Matthew Weber a écrit :
> Romain,
> 
> On Fri, Sep 6, 2019 at 4:10 AM Romain Naour <romain.naour@smile.fr> wrote:
>>
>> Apply a patch contributed by Luis Marques on the Busybox mailing list [1]
>> fixing a runtime issue (segfault) when busybox is compiled by Clang.
>>
>> The patch disable the compiler optimizations for Clang/LLVM only.
>>
>> Without this patch, busybox segfault with several applets
>> (login on aarch64 using Clang 8.0.1, init on x86_64 using Clang 9.0.0rc3)
>>
>> [1] http://lists.busybox.net/pipermail/busybox/2019-June/087337.html
>>
> 
> Got a "crtbegin.o no such file or directory" error when I setup a
> build using master with qemu_aarch64_virt_defconfig (updated for
> prebuilt external toolchain and enabling clang as cross-compiler).
> https://paste.ubuntu.com/p/wxrmVTGVvp/

Thanks for testing the series :)

Ok, the "crtbegin.o no such file or directory" error mean that realpath command
failed. It failed because the GCC external toolchain was not installed when the
realpath command is executed, this is due to :

# Allow host-clang to be build as part of the toolchain
ifeq ($(BR2_USER_HOST_CLANG_AS_CROSS_COMPILER),y)
HOST_CLANG_ADD_TOOLCHAIN_DEPENDENCY = NO
endif

I fixed this issue by adding an explicit dependency on toolchain-external:

# Help host-clang to find our external toolchain, use a relative path from the clang
# installation directory to the external toolchain installation directory in
order to
# not hardcode the toolchain absolute path.
# Install the GCC external toolchain before executing realpath command.
ifeq ($(BR2_TOOLCHAIN_EXTERNAL),y)
HOST_CLANG_DEPENDENCIES += toolchain-external
HOST_CLANG_CONF_OPTS += -DGCC_INSTALL_PREFIX:PATH=`realpath
--relative-to=$(HOST_DIR)/bin/ $(TOOLCHAIN_EXTERNAL_INSTALL_DIR)`
endif

I'll send a v2 shortly.

> 
> From your comment earlier on irc that you tested with 9.0.1 (x86_64
> and aarch64), I wonder if this error is a difference between building
> with clang 8.0.1 and 9.

No no, that's me breaking my own patch :p

> 
> I was able to build compiler-rt & libfuzzer with your series applied
> and pass a runtime test (system still cross compiled with gcc but
> using "[01/11] package/clang: help host-clang to find our external
> toolchain" to find the sysroot).  This was instead of using the -B
> option when building libfuzzer as part of the runtime test.

That's great :)

I'll try to upstream the patch then. I think Clang doesn't like when the
toolchain sysroot is relocated because it doesn't seem to take into account the
GCC search patch.

Maybe your libfuzzer may be added at the end of the series (with a dependency on
Clang cross-compiler option)?

Best regards,
Romain

> 
> Matt
>
Matthew Weber Sept. 9, 2019, 1:49 p.m. UTC | #4
Romain,

On Sat, Sep 7, 2019 at 3:02 AM Romain Naour <romain.naour@smile.fr> wrote:
>
> Hi Matt,
>
> Le 07/09/2019 à 05:11, Matthew Weber a écrit :
> > Romain,
> >
> > On Fri, Sep 6, 2019 at 4:10 AM Romain Naour <romain.naour@smile.fr> wrote:
> >>
> >> Apply a patch contributed by Luis Marques on the Busybox mailing list [1]
> >> fixing a runtime issue (segfault) when busybox is compiled by Clang.
> >>
> >> The patch disable the compiler optimizations for Clang/LLVM only.
> >>
> >> Without this patch, busybox segfault with several applets
> >> (login on aarch64 using Clang 8.0.1, init on x86_64 using Clang 9.0.0rc3)
> >>
> >> [1] https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.busybox.net_pipermail_busybox_2019-2DJune_087337.html&d=DwIDaQ&c=ilBQI1lupc9Y65XwNblLtw&r=y1sOV0GV8NZUkffv7oCRxs2Sd3nOBS-NxDM3NY8lOgs&m=gE7oHor6AQZYHYD00ApKk2X5MH2LVRR_EyXIt5jiU4A&s=VKnAVOuzWWTy3aWJijmMwPLpl9NkHk2zZ5nx7u0A-mA&e=
> >>
> >
> > Got a "crtbegin.o no such file or directory" error when I setup a
> > build using master with qemu_aarch64_virt_defconfig (updated for
> > prebuilt external toolchain and enabling clang as cross-compiler).
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__paste.ubuntu.com_p_wxrmVTGVvp_&d=DwIDaQ&c=ilBQI1lupc9Y65XwNblLtw&r=y1sOV0GV8NZUkffv7oCRxs2Sd3nOBS-NxDM3NY8lOgs&m=gE7oHor6AQZYHYD00ApKk2X5MH2LVRR_EyXIt5jiU4A&s=8olVqcVp_os9SDrgZ_W2pw6WsPZiOXiJqkBAI8aqnAI&e=
>
> Thanks for testing the series :)
>
> Ok, the "crtbegin.o no such file or directory" error mean that realpath command
> failed. It failed because the GCC external toolchain was not installed when the
> realpath command is executed, this is due to :
>
> # Allow host-clang to be build as part of the toolchain
> ifeq ($(BR2_USER_HOST_CLANG_AS_CROSS_COMPILER),y)
> HOST_CLANG_ADD_TOOLCHAIN_DEPENDENCY = NO
> endif
>
> I fixed this issue by adding an explicit dependency on toolchain-external:
>
> # Help host-clang to find our external toolchain, use a relative path from the clang
> # installation directory to the external toolchain installation directory in
> order to
> # not hardcode the toolchain absolute path.
> # Install the GCC external toolchain before executing realpath command.
> ifeq ($(BR2_TOOLCHAIN_EXTERNAL),y)
> HOST_CLANG_DEPENDENCIES += toolchain-external
> HOST_CLANG_CONF_OPTS += -DGCC_INSTALL_PREFIX:PATH=`realpath
> --relative-to=$(HOST_DIR)/bin/ $(TOOLCHAIN_EXTERNAL_INSTALL_DIR)`
> endif
>
> I'll send a v2 shortly.
>
> >
> > From your comment earlier on irc that you tested with 9.0.1 (x86_64
> > and aarch64), I wonder if this error is a difference between building
> > with clang 8.0.1 and 9.
>
> No no, that's me breaking my own patch :p
>
> >
> > I was able to build compiler-rt & libfuzzer with your series applied
> > and pass a runtime test (system still cross compiled with gcc but
> > using "[01/11] package/clang: help host-clang to find our external
> > toolchain" to find the sysroot).  This was instead of using the -B
> > option when building libfuzzer as part of the runtime test.
>
> That's great :)
>
> I'll try to upstream the patch then. I think Clang doesn't like when the
> toolchain sysroot is relocated because it doesn't seem to take into account the
> GCC search patch.
>
> Maybe your libfuzzer may be added at the end of the series (with a dependency on
> Clang cross-compiler option)?

I think the current compiler RT run-time test case is still valid as
you could use the libfuzzer as a way to test the compiler-rt is
working when your primary system is built with GCC.  However you do
have a point that libfuzzer could be added as a package when
compiler-rt is enabled and Clang is the cross-compiler.  I think I'd
still have to keep a copy in the run-time test br2-external for the
test case?

Matt
diff mbox series

Patch

diff --git a/package/busybox/0003-use-BB_GLOBAL_CONST-where-applicable.patch b/package/busybox/0003-use-BB_GLOBAL_CONST-where-applicable.patch
new file mode 100644
index 0000000000..738a15b0de
--- /dev/null
+++ b/package/busybox/0003-use-BB_GLOBAL_CONST-where-applicable.patch
@@ -0,0 +1,173 @@ 
+From 6f53fa8303edff685aee3cc16a6c8967fae869db Mon Sep 17 00:00:00 2001
+From: Luis Marques <luismarques@lowrisc.org>
+Date: Wed, 4 Sep 2019 17:48:39 +0200
+Subject: [PATCH] use BB_GLOBAL_CONST where applicable
+
+Signed-off-by: Romain Naour <romain.naour@smile.fr>
+---
+ coreutils/test.c          |  3 +--
+ coreutils/test_ptr_hack.c |  2 +-
+ include/libbb.h           | 27 +++++++++++++++++++++++++--
+ libbb/lineedit.c          |  3 +--
+ libbb/lineedit_ptr_hack.c |  2 +-
+ libbb/ptr_to_globals.c    |  2 +-
+ shell/ash.c               | 13 -------------
+ shell/ash_ptr_hack.c      |  6 +++---
+ 8 files changed, 33 insertions(+), 25 deletions(-)
+
+diff --git a/coreutils/test.c b/coreutils/test.c
+index 8d7dac025..e1d440106 100644
+--- a/coreutils/test.c
++++ b/coreutils/test.c
+@@ -400,8 +400,7 @@ struct test_statics {
+ 	jmp_buf leaving;
+ };
+ 
+-/* See test_ptr_hack.c */
+-extern struct test_statics *const test_ptr_to_statics;
++extern struct test_statics *BB_GLOBAL_CONST test_ptr_to_statics;
+ 
+ #define S (*test_ptr_to_statics)
+ #define args            (S.args         )
+diff --git a/coreutils/test_ptr_hack.c b/coreutils/test_ptr_hack.c
+index 5ba9dcc68..6759b2144 100644
+--- a/coreutils/test_ptr_hack.c
++++ b/coreutils/test_ptr_hack.c
+@@ -18,6 +18,6 @@ struct test_statics *test_ptr_to_statics;
+ /* gcc -combine will see through and complain */
+ /* Using alternative method which is more likely to break
+  * on weird architectures, compilers, linkers and so on */
+-struct test_statics *const test_ptr_to_statics __attribute__ ((section (".data")));
++struct test_statics *BB_GLOBAL_CONST test_ptr_to_statics __attribute__ ((section (".data")));
+ 
+ #endif
+diff --git a/include/libbb.h b/include/libbb.h
+index 021100db1..2523fd89c 100644
+--- a/include/libbb.h
++++ b/include/libbb.h
+@@ -338,10 +338,33 @@ struct BUG_off_t_size_is_misdetected {
+ #endif
+ #endif
+ 
++/* We use a trick to have more optimized code (fewer pointer reloads). E.g.:
++ *  ash.c:   extern struct globals *const ash_ptr_to_globals;
++ *  ash_ptr_hack.c: struct globals *ash_ptr_to_globals;
++ * This way, compiler in ash.c knows the pointer cannot change.
++ *
++ * However, this relies on C undefined behavior, so we whitelist compilers
++ * where we know this isn't problematic, by using the the BB_GLOBAL_CONST
++ * preprocessor definition.
++ * If you are sure this trick also works with your toolchain you can add
++ * "-DBB_GLOBAL_CONST='const'" to CONFIG_EXTRA_CFLAGS or add your compiler to
++ * the whitelist below.
++ */
++
++#ifndef BB_GLOBAL_CONST
++# if defined(__clang__)
++#  define BB_GLOBAL_CONST
++# elif defined(__GNUC__)
++#  define BB_GLOBAL_CONST const
++# else
++#  define BB_GLOBAL_CONST
++# endif
++#endif
++
+ #if defined(__GLIBC__)
+ /* glibc uses __errno_location() to get a ptr to errno */
+ /* We can just memorize it once - no multithreading in busybox :) */
+-extern int *const bb_errno;
++extern int *BB_GLOBAL_CONST bb_errno;
+ #undef errno
+ #define errno (*bb_errno)
+ #endif
+@@ -2109,7 +2132,7 @@ struct globals;
+ /* '*const' ptr makes gcc optimize code much better.
+  * Magic prevents ptr_to_globals from going into rodata.
+  * If you want to assign a value, use SET_PTR_TO_GLOBALS(x) */
+-extern struct globals *const ptr_to_globals;
++extern struct globals *BB_GLOBAL_CONST ptr_to_globals;
+ /* At least gcc 3.4.6 on mipsel system needs optimization barrier */
+ #define barrier() __asm__ __volatile__("":::"memory")
+ #define SET_PTR_TO_GLOBALS(x) do { \
+diff --git a/libbb/lineedit.c b/libbb/lineedit.c
+index fbabc6c12..b9e9719c5 100644
+--- a/libbb/lineedit.c
++++ b/libbb/lineedit.c
+@@ -180,8 +180,7 @@ struct lineedit_statics {
+ #endif
+ };
+ 
+-/* See lineedit_ptr_hack.c */
+-extern struct lineedit_statics *const lineedit_ptr_to_statics;
++extern struct lineedit_statics *BB_GLOBAL_CONST lineedit_ptr_to_statics;
+ 
+ #define S (*lineedit_ptr_to_statics)
+ #define state            (S.state           )
+diff --git a/libbb/lineedit_ptr_hack.c b/libbb/lineedit_ptr_hack.c
+index dc45855d5..ac33bd409 100644
+--- a/libbb/lineedit_ptr_hack.c
++++ b/libbb/lineedit_ptr_hack.c
+@@ -18,6 +18,6 @@ struct lineedit_statics *lineedit_ptr_to_statics;
+ /* gcc -combine will see through and complain */
+ /* Using alternative method which is more likely to break
+  * on weird architectures, compilers, linkers and so on */
+-struct lineedit_statics *const lineedit_ptr_to_statics __attribute__ ((section (".data")));
++struct lineedit_statics *BB_GLOBAL_CONST lineedit_ptr_to_statics __attribute__ ((section (".data")));
+ 
+ #endif
+diff --git a/libbb/ptr_to_globals.c b/libbb/ptr_to_globals.c
+index 8ba9cd154..26d7b2042 100644
+--- a/libbb/ptr_to_globals.c
++++ b/libbb/ptr_to_globals.c
+@@ -25,7 +25,7 @@ int *bb_errno;
+ /* gcc -combine will see through and complain */
+ /* Using alternative method which is more likely to break
+  * on weird architectures, compilers, linkers and so on */
+-struct globals *const ptr_to_globals __attribute__ ((section (".data")));
++struct globals *BB_GLOBAL_CONST ptr_to_globals __attribute__ ((section (".data")));
+ 
+ #ifdef __GLIBC__
+ int *const bb_errno __attribute__ ((section (".data")));
+diff --git a/shell/ash.c b/shell/ash.c
+index e3bbac9a0..3141f3812 100644
+--- a/shell/ash.c
++++ b/shell/ash.c
+@@ -288,19 +288,6 @@ typedef long arith_t;
+ # error "Do not even bother, ash will not run on NOMMU machine"
+ #endif
+ 
+-/* We use a trick to have more optimized code (fewer pointer reloads):
+- *  ash.c:   extern struct globals *const ash_ptr_to_globals;
+- *  ash_ptr_hack.c: struct globals *ash_ptr_to_globals;
+- * This way, compiler in ash.c knows the pointer can not change.
+- *
+- * However, this may break on weird arches or toolchains. In this case,
+- * set "-DBB_GLOBAL_CONST=''" in CONFIG_EXTRA_CFLAGS to disable
+- * this optimization.
+- */
+-#ifndef BB_GLOBAL_CONST
+-# define BB_GLOBAL_CONST const
+-#endif
+-
+ 
+ /* ============ Hash table sizes. Configurable. */
+ 
+diff --git a/shell/ash_ptr_hack.c b/shell/ash_ptr_hack.c
+index f69840825..af16cca27 100644
+--- a/shell/ash_ptr_hack.c
++++ b/shell/ash_ptr_hack.c
+@@ -22,8 +22,8 @@ struct globals_var      *ash_ptr_to_globals_var;
+ /* gcc -combine will see through and complain */
+ /* Using alternative method which is more likely to break
+  * on weird architectures, compilers, linkers and so on */
+-struct globals_misc     *const ash_ptr_to_globals_misc __attribute__ ((section (".data")));
+-struct globals_memstack *const ash_ptr_to_globals_memstack __attribute__ ((section (".data")));
+-struct globals_var      *const ash_ptr_to_globals_var __attribute__ ((section (".data")));
++struct globals_misc     *BB_GLOBAL_CONST ash_ptr_to_globals_misc __attribute__ ((section (".data")));
++struct globals_memstack *BB_GLOBAL_CONST ash_ptr_to_globals_memstack __attribute__ ((section (".data")));
++struct globals_var      *BB_GLOBAL_CONST ash_ptr_to_globals_var __attribute__ ((section (".data")));
+ 
+ #endif
+-- 
+2.21.0
+