Bug 13716 - Broken Android NDK x86 builds with TLS due to triggered ASM code
Summary: Broken Android NDK x86 builds with TLS due to triggered ASM code
Status: RESOLVED FIXED
Alias: None
Product: Busybox
Classification: Unclassified
Component: Networking (show other bugs)
Version: unspecified
Hardware: Other Other
: P5 major
Target Milestone: ---
Assignee: unassigned
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-04-01 00:25 UTC by Chris Renshaw
Modified: 2023-05-23 23:21 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:


Attachments
tls.h patch for the reported issue (672 bytes, patch)
2021-04-01 00:25 UTC, Chris Renshaw
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Renshaw 2021-04-01 00:25:35 UTC
Created attachment 8881 [details]
tls.h patch for the reported issue

This error only shows up on x86 Android compiles:

  CC      networking/tls_pstm_montgomery_reduce.o
networking/tls_pstm_montgomery_reduce.c: In function 'pstm_montgomery_reduce':
networking/tls_pstm_montgomery_reduce.c:66:1: error: 'asm' operand has impossible constraints
 asm(                                                      \
 ^
networking/tls_pstm_montgomery_reduce.c:385:4: note: in expansion of macro 'INNERMUL'
    INNERMUL;
    ^
make[1]: *** [scripts/Makefile.build:198: networking/tls_pstm_montgomery_reduce.o] Error 1
make: *** [Makefile:744: networking] Error 2

But can be resolved by patching around the ASM code in question, which was intended for Linux i386, not Android x86. Patch attached.
Comment 1 Denys Vlasenko 2021-04-14 16:37:04 UTC
THe asm code should be valid for Android i386 as well. Investigate what does it dislike about

#define INNERMUL                                          \
asm(                                                      \
   "movl %5,%%eax \n\t"                                   \
   "mull %4       \n\t"                                   \
   "addl %1,%%eax \n\t"                                   \
   "adcl $0,%%edx \n\t"                                   \
   "addl %%eax,%0 \n\t"                                   \
   "adcl $0,%%edx \n\t"                                   \
   "movl %%edx,%1 \n\t"                                   \
:"=g"(_c[LO]), "=r"(cy)                                   \
:"0"(_c[LO]), "1"(cy), "g"(mu), "g"(*tmpm++)              \
: "%eax", "%edx", "cc")
Comment 2 Chris Renshaw 2021-04-14 19:27:21 UTC
Sorry, I have no idea how to investigate that.. I do know it appears to build and work fine if we just patch around it as proposed.

I'll be happy to try any commands or anything you can supply to dig into it further.
Comment 3 Denys Vlasenko 2021-04-15 07:18:28 UTC
Replace INNERMUL define with this one:

#define INNERMUL                                          \
asm(                                                      \
   "mull %4       \n\t"                                   \
   "addl %3,%%eax \n\t"                                   \
   "adcl $0,%%edx \n\t"                                   \
   "addl %%eax,%0 \n\t"                                   \
   "adcl $0,%%edx \n\t"                                   \
:"=g"(_c[LO]), "=&d"(cy)                                  \
:"0"(_c[LO]), "g"(cy), "g"(mu), "a"(*tmpm++)              \
:"cc")

and see whether it compiles.
Comment 4 Chris Renshaw 2021-04-16 15:18:11 UTC
That compiled with no errors!
Comment 5 Denys Vlasenko 2021-04-20 17:08:10 UTC
Fixed in git, please confirm.
Comment 6 Chris Renshaw 2021-04-21 20:44:32 UTC
Cherry-picked your 2 TLS commits into 1.32.1 and it build with Android x86 NDK perfectly! Thanks for your effort figuring this one out Denys!
Comment 7 Chris Renshaw 2021-05-07 14:32:29 UTC
While your fix worked great on NDK gcc to compile for Android x86, I've just discovered it unfortunately still breaks compiles with NDK clang which has completely replaced gcc (i.e. gcc is removed) since NDK r18b (Sept 2018).


[...] ./obj/local/x86/objs/busybox/util-linux/volume_id/volume_id.o ./obj/local/x86/objs/busybox/util-linux/volume_id/xfs.o ./obj/local/x86/libselinux.a ./obj/local/x86/libpcre2.a --build-id --no-undefined -z noexecstack --warn-shared-textrel --fatal-warnings -lc -lm -lstdc++ -lm --start-group -lgcc -lc --end-group "G:/android/android-ndk-r20b/build//../toolchains/llvm/prebuilt/windows/bin/../sysroot/usr/lib/i686-linux-android/21\\crtend_android.o"
G:/android/android-ndk-r20b/build//../toolchains/llvm/prebuilt/windows/bin/../lib/gcc/i686-linux-android/4.9.x/../../../../i686-linux-android/bin\ld: fatal error: LLVM gold plugin: inline assembly requires more registers than available at line 2148562311
clang++: error: linker command failed with exit code 1

Avoiding the problematic ASM code as I had previously done still does successfully allow it to compile, so despite the vague linker error I have confirmed it's the same issue.

Any way to reduce register pressure further so clang-built busybox can have this corrected as well?

Thanks again for your time looking into this and everything!
Comment 8 Denys Vlasenko 2021-05-11 10:15:14 UTC
(In reply to Chris Renshaw from comment #7)
> While your fix worked great on NDK gcc to compile for Android x86, I've just discovered it unfortunately still breaks compiles with NDK clang which has completely replaced gcc (i.e. gcc is removed) since NDK r18b (Sept 2018).

> [...] ./obj/local/x86/objs/busybox/util-linux/volume_id/volume_id.o ./obj/local/x86/objs/busybox/util-linux/volume_id/xfs.o ./obj/local/x86/libselinux.a ./obj/local/x86/libpcre2.a --build-id --no-undefined -z noexecstack --warn-shared-textrel --fatal-warnings -lc -lm -lstdc++ -lm --start-group -lgcc -lc --end-group "G:/android/android-ndk-r20b/build//../toolchains/llvm/prebuilt/windows/bin/../sysroot/usr/lib/i686-linux-android/21\\crtend_android.o"
G:/android/android-ndk-r20b/build//../toolchains/llvm/prebuilt/windows/bin/../lib/gcc/i686-linux-android/4.9.x/../../../../i686-linux-android/bin\ld: fatal error: LLVM gold plugin: inline assembly requires more registers than available at line 2148562311

Which exactly asm statement fails to compile?
Comment 9 Chris Renshaw 2021-05-11 14:48:47 UTC
I was avoiding all the ASM with the following:

diff --git a/networking/tls.h b/networking/tls.h
index d4ac1bef8..07032477e 100644
--- a/networking/tls.h
+++ b/networking/tls.h
@@ -26,7 +26,7 @@
 #undef  USE_SEED
 /* pstm: multiprecision numbers */
 #undef  DISABLE_PSTM
-#if defined(__GNUC__) && defined(__i386__)
+#if defined(__GNUC__) && defined(__i386__) && !defined(__ANDROID__)
   /* PSTM_X86 works correctly. +25 bytes. */
 # define PSTM_32BIT
 # define PSTM_X86

Not sure how to avoid specific ASM statements and still ensure it'll overall compile. Do I simply comment out one define at a time in the montgomery and comba files?
Comment 10 Denys Vlasenko 2021-06-05 16:44:13 UTC
(In reply to Chris Renshaw from comment #7)
> While your fix worked great on NDK gcc to compile for Android x86, I've just discovered it unfortunately still breaks compiles with NDK clang which has completely replaced gcc (i.e. gcc is removed) since NDK r18b (Sept 2018).

I suggest opening a bug against clang. They have to fix it anyway, this should be affecting many other valid programs.
Comment 11 Chris Renshaw 2023-05-23 23:21:09 UTC
Resolved on latest NDK!