android / ndk

The Android Native Development Kit
1.95k stars 254 forks source link

clang compilation error with unknown argument, works with gcc 4.9 #111

Closed Brahmasmi closed 7 years ago

Brahmasmi commented 8 years ago

I am not an expert at compilers, so please bear with me.

I am trying to compile https://github.com/projectNe10/Ne10/ using clang (NDK version r11c), since ndk has deprecated gcc. Using gcc 4.9 works fine. However, using clang, the compilation barfs with:

Running cmake...
-- The C compiler identification is Clang 3.8.243773
-- The CXX compiler identification is Clang 3.8.243773
-- The ASM compiler identification is unknown
-- Found assembler: /android/ndk/toolchains/llvm/prebuilt/linux-x86_64/bin/llvm-as
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Warning: Did not find file Compiler/-ASM
-- Target architecture : armv7
-- Building type: RELEASE
-- Loaded toolchain:
    /android/ndk/toolchains/llvm/prebuilt/linux-x86_64/bin/clang
    /android/ndk/toolchains/llvm/prebuilt/linux-x86_64/bin/clang++
    /android/ndk/toolchains/llvm/prebuilt/linux-x86_64/bin/llvm-as
-- CMAKE_C_FLAGS:
     -O2 -DNDEBUG --sysroot=/android/ndk/platforms/android-9/arch-arm/ -pie -mthumb-interwork -mthumb -march=armv7-a -mfloat-abi=softfp -mfpu=vfp3
-- Configuring done
-- Generating done
-- Build files have been written to: Ne10/build

Running make...
Scanning dependencies of target NE10
[  0%] Building C object modules/CMakeFiles/NE10.dir/math/NE10_abs.c.o
clang: error: unknown argument: '-mthumb-interwork'
make[2]: *** [modules/CMakeFiles/NE10.dir/math/NE10_abs.c.o] Error 1
make[1]: *** [modules/CMakeFiles/NE10.dir/all] Error 2
make: *** [all] Error 2

Changing the llvm-as to arm-linux-androideabi-as does not resolve the compilation error, although it does resolve the warning

-- Warning: Did not find file Compiler/-ASM

The relevant files are: https://github.com/projectNe10/Ne10/blob/master/android/android_config.cmake and https://github.com/projectNe10/Ne10/blob/master/CMakeLists.txt.

Based on my limited understanding, -mthumb-interwork is required to be passed for the code to work on android. However, a cursory glance at http://clang.llvm.org/docs/UsersManual.html#arm does not seem to reveal any useful information, nor does searching the bugzilla database at llvm.org. So, either clang does not support this compile time option or I am missing something really obvious here.

DanAlbert commented 8 years ago

https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html

-mthumb-interwork Generate code that supports calling between the ARM and Thumb instruction sets. Without this option, on pre-v5 architectures, the two instruction sets cannot be reliably used inside one program. The default is -mno-thumb-interwork, since slightly larger code is generated when -mthumb-interwork is specified. In AAPCS configurations this option is meaningless.

Android is only arm5+ and aapcs. This option is meaningless for Android. Those flags should just be removed from the CMakeLists.txt.

For the sake of compatibility though, @stephenhines, thoughts? All I could really find was https://lists.gnu.org/archive/html/grub-devel/2013-12/msg00094.html, which says the thumb interwork is the default for Clang. Assuming that's true, does Clang have precedent for adding no-op flags for GCC command line compatibility?

jmgao commented 8 years ago

Assuming that's true, does Clang have precedent for adding no-op flags for GCC command line compatibility?

Alternatively, we could do #92.

DanAlbert commented 8 years ago

That only helps the build systems that we fix though. I'm not even sure if there is a (correct) way to do this for cmake.

jmgao commented 8 years ago

Oh, that's for ndk-build? I thought that was for replacing the standalone toolchain clang with a wrapper that strips out gcc-only flags.

DanAlbert commented 8 years ago

Good point. Once we have the toolchain wrappers we can fix this there. I don't think we'll have those for some time though, so a clang fix might be nice to have now if it's reasonable.

stephenhines commented 8 years ago

Any clang fix would probably not land until r13 at the earliest, and I would hope that we can have some simple wrappers at that point too. I don't think that the upstream community is too fond of flags that are no-ops either, although they will match gcc flags for actual features. In this case, since the feature is going to be always-on, I can't see struggling to add this for very little value.

Brahmasmi commented 8 years ago

@DanAlbert I am not sure whether -mthumb-interwork is meaningless for android.

Before submitting this issue, I did go through the gcc compiler options and the minimum android CTS requirement of ARMv5TE or better. Logically, I inferred that the flag could be done away with. However, I chanced upon this thread on the android-ndk google group - https://groups.google.com/forum/#!topic/android-ndk/AVz2FsOoMdI.

Anyone know if there is an option I can add to an NDK project's
Application.mk or Android.mk so that it compiles with:
-mno-thumb-interwork
instead of:
-mthumb-interwork
...
Why would you want to do that ? The resulting binary would
not be compatible with the Android ABI anyway.

Since the reply seems to be from an engineer on the android NDK team, I presume that removing the flag may not be prudent. This is further supported empirically, although unscientifically, by the presence of the flag across many results - https://groups.google.com/forum/#!searchin/android-ndk/mthumb-interwork .

May be someone could reach out to the gentleman within Google to better understand whether the flag can actually be removed while still maintaining ABI compatibility (is this his handle? - https://github.com/digit-android). In case that flag is required and is relevant for clang, then may be we need a different solution than a no-op. In case that flag is not required, its great (and may I add, logical). In either case, it would be interesting to understand the technical reason behind that thread exchange.

On a related note, you referenced https://lists.gnu.org/archive/html/grub-devel/2013-12/msg00094.html (emphasis added) -

My experiments with clang have shown that it doesn't accept
-mthumb-interwork and generates code that isn't interworkable
on armv5t even with explicit -march=armv5t.
Moreover the thumb code it generates even with -march=armv5t
contains Thumb-2 instructions which qemu rightfully interprets as
bl to random address corrupting lr in process and producing
crash dumps with senseless $pc and $lr.
So any kind of thumb1 support with clang is in any case out of
the question.
Then the question if it makes sense to allow (but mention that it's
not supported) compilations with clang and old GCC as long as they
use only arm and thumb2.
Based on my very limited read, clang does not generate code that is interworkable since Thumb-1 seems to be supported on armeabi (https://developer.android.com/ndk/guides/abis.html#sa). So I am curious to understand your deduction that thumb-interwork is default for clang. Again, I am not an expert on compilers/instruction sets/archs.
Brahmasmi commented 7 years ago

Bumping this.

Would appreciate guidance with respect to resolving this bug. I may be wrong here but from what I understand, gcc is no longer supported (as of r13) and clang is not there yet.

DanAlbert commented 7 years ago

@stephenhines @pirama-arumuga-nainar: Could either of you confirm Clang's behavior here? If Clang does the right thing here by default we can close, otherwise we'll need to get a fix for this.

Brahmasmi commented 7 years ago

Could someone from within Google connect internally with David 'Digit' Turner (@digit-android) to better understand this? I request this because in that NDK google group thread (https://groups.google.com/forum/#!topic/android-ndk/AVz2FsOoMdI), he indicated the need for -mthumb-interwork for ABI compatibility. As such, I presume he would be in a better, possibly definitive, position to help us out.

digit-android commented 7 years ago

Hello,

Dan kindly contacted me regarding this issue. For some reason I was not receiving any github email :-/

The short answer is that Android uses the AAPCS ARM ABI specification, which requires interworking (see section 5.6).

Whether -mthumb-interwork / -mno-thumb-interwork has any effect when used in combination with -mabi=aapcs (which is the default) depends on the compiler. The recent GCC ARM options documentation mentions that it is ignored in AAPCs configurations (which makes sense), but this was not always true, as far as I remember (the NDK has always used pretty dated versions of GCC, especially in 2010 / 2011 when my first email was written).

Technically, interworking means that function pointer calls should always use the BLX [reg] instruction (instead of BL [reg]), and that final binaries (executables and shared libraries) contain small code stubs (called veneers) that are generated at link time to perform explicit CPU state changes (they appear under the names $a and $t when using readelf to look inside said binaries).

ARMv7-A introduced changes to the way instructions are decoded by the CPU, e.g. mov PC, [reg] will actually use the lower-bit of the target address to determine the new CPU thumb state, this didn't happen before. This makes most of the interworking machine code requirements obsolete.

So in a nutshell:

If you want to run your code on ARMv5TE or ARMv6 CPUs, you must implement interworking or you won't be able to run properly on Android systems.

If your code only runs on ARMv7-A or higher, you should not care about interworking at all.

What happens with -mthumb-interwork / -mno-thumb-interwork is compiler-specific, e.g. recent versions of GCC will ignore it since -mabi=aapc is the default (or is it aapc-linux, I haven't followed through since a long time).

digit-android commented 7 years ago

I just performed some tests and it looks like the prebuilt GCC 4.9 that comes with AOSP ignores -mthumb-interwork -mno-thumb-interwork completely for -march=armv5te, it does not for -march=armv4t though.

In other words, just omitting the flags completely from the build scripts should be enough for GCC. Don't know about CLang though.

Brahmasmi commented 7 years ago

Dan - Thanks for connecting with David. David - Thank you for that elaborate explanation with the historical context. It helps clarify the entire thing immensely, especially for an end user who has little to no knowledge of compilers/archs/instruction sets.

From what I understand - if (arm_arch < 7) -mthumb-interwork is required, else not. Since we use mabi=aapcs, GCC always generates code that interworks between thumb1 and arm instructions. This is further confirmed from your tests with AOSP (==NDK?) GCC 4.9.

The armv7a arch is a requirement for API level >= 19 (https://source.android.com/compatibility/4.4/android-4.4-cdd.pdf#section-3.3) but there are 18.6% users with API level < 19 as of September 2016 (https://developer.android.com/about/dashboards/index.html). There is no public dashboard of arch with API levels, so one has to support arm_arch < 7. Assuming there are at least a billion android devices, one can hardly afford to disregard 186 million of them - unless someone here knows more about this.

1) I understand that this is obvious, but could you please execute your interwork tests with clang? Alternatively could you please share your steps for testing interworking, in case they could be executed by an end user in clang? This could help to verify. 2) What would be the impact (if any) of mabi=aapcs-linux instead of mabi=aapcs? I ask this because of https://android.googlesource.com/toolchain/clang/+/master/lib/Basic/Targets.cpp#4874 , although I am not sure whether that code gets executed based on the comment at the start of the function.

As for this bug, the focus here shifts to clang/llvm. Based on David's explanation, I went spelunking into toolchain/clang code with a rudimentary search for the string "interwork". I will share my understanding here (caveat - no expertise/knowledge apart from basic ability to read and search): Clang does not seem to support interworking. Amongst the search results for "interwork" (https://android.googlesource.com/toolchain/clang/+/master/lib/Basic/Targets.cpp#5169) - "// FIXME: It's more complicated than this and we don't really support // interworking. // Windows on ARM does not "support" interworking if (5 <= ArchVersion && ArchVersion <= 8 && !getTriple().isOSWindows()) Builder.defineMacro("THUMB_INTERWORK");"

The relevant part in clang code seems to start from https://android.googlesource.com/toolchain/clang/+/master/lib/Basic/Targets.cpp#4553 where the ARMTargetInfo class is defined and ends at https://android.googlesource.com/toolchain/clang/+/master/lib/Basic/Targets.cpp#5493. I would suggest reading it to understand what clang seems to be doing with the ARM Target.

Could I request the clang experts here to suggest a way forward?

digit-android commented 7 years ago

First, there are very probably far less than 18.6% of devices running on ARMv6 or lower, we don't have the real numbers but a lot of the devices supporting API level < 19 are already running on ARMv7-A. The exceptions are really old phones, and, surprisingly the 1st generation Rasperry Pi (The Pi 2 and Pi 3 are on ARMv7-A).

As an Android developer, I would probably not bother too much about ARMv6 support, just put a runtime check for ARMv7-A at the start of your application, and mention it on your Store page that this doesn't support older phones.

I don't have much time to perform the checks myself, but essentially you can do that by compiling a small object file containing a single function that performs a function-pointer-call as in:

typedef int (*callback_t)(int);

int foo(int x, callback_t cb) {
   return cb(x + 32) - 32;
}

And look at the disassembly, if a "blx" instruction is used to implement the call, then interwork was enabled by the compiler, otherwise, it will be a "bl". For example on how to look at the results (assuming the source file is named "foo.c"):

/path/to/compiler -O2 -march=armv4t -mno-thumb-interwork foo.c cat foo.s

/path/to/compiler -O2 -march=armv4t -mthumb-interwork foo.c cat foo.s

try with different versions for -march= (e.g. armv5te, armv6. armv7-a) and see the results.

Regarding aapcs versus aapcs-linux, I believe the latter specifies a few things that are left unresolved in aapcs, see http://www.boost.org/doc/libs/1_51_0_beta1/libs/context/doc/pdf/arm-linux-aapcs.pdf

The biggest change is that enum are 32-bit ints by default (while they have variable size depending on the enum values in aapcs). IIRC, Android uses the aapcs-linux variant, but I'm fairly rusty on this one :-/ There is nothing here related to interworking though.

fornwall commented 7 years ago

As an Android developer, I would probably not bother too much about ARMv6 support, just put a runtime check for ARMv7-A at the start of your application, and mention it on your Store page that this doesn't support older phones.

Isn't it enough to avoid building armeabi versions of native code (instead building for armeabi-v7a and other desired arches) - then the play store will mark ARMv6 devices as unsupported and prevent installation on them, and no runtime check would be necessary?

@DanAlbert Should perhaps dropping armeabi support be on the NDK roadmap?

enh commented 7 years ago

@fornwall we're actually working on a roadmap to dropping armeabi support, perhaps even starting gently in r15 by no longer building it by default.

stephenhines commented 7 years ago

We don't plan to implement a -mthumb-interwork no-op flag in Clang, so I am closing this issue out. The proper fix is to not pass this flag when compiling with Clang.