The instructions use NDK version r11c, but the latest version r12/r13beta also reproduce this issue. STEPS TO REPRODUCE: 1. make standalone toolchain using the following command ./build/tools/make-standalone-toolchain.sh \ --use-llvm \ --arch=arm64 \ --platform=android-24 \ --install_dir=$TOOLCHAIN_DIR 2. compile the Skia test executables using the instructions found at https://skia.org/user/quick/android 3. push the executable to the device and run. EXPECTED RESULTS: executable runs to completion OBSERVED RESULTS: crashes on launch with the following error message dlopen failed: cannot locate symbol "__mulodi4" referenced by “libskia_android.so" ADDITIONAL INFORMATION: In step 2, if we use the gcc binaries included with the toolchain then the executable runs with no issues. We have a quick hack at https://codereview.chromium.org/2011073002/ that attempts to work around the issue but is not desireable. https://llvm.org/bugs/show_bug.cgi?id=17693 seems similar, but this is not for sanitizer code, so we should be able to use a different rtlib than compiler-rt. Is it expected for clang to lower sequences of __builtin_smull_overflow (https://android.googlesource.com/platform/external/dng_sdk.git/+/master/source/dng_safe_arithmetic.h#112) to __mulodi4 in all cases? It is known that libgcc won't provide this symbol, so at the very least, clang should warn/error that the rtlib selected won't be able to resolve this symbol at runtime.
I don't understand. The code specifically asks if the compiler supports that function, and assuming you're using the compiler's own run-time library, it's safe to say "yes". Then Clang lowers that function (because you specifically asked for it) and now you complain it's not there because you haven't used Compiler-RT? Where's the logic in that? Of course GCC doesn't break, it probably returns false to the has_builtin and does whatever else. I don't think that's a compiler (or library) bug, but an assumption in user code and a failure to link the appropriate library. Or am I missing something obvious?
The default for Clang is to not use compiler-rt as its rtlib though. It also should be aware of what rtlib is requested at the point where it decides that these functions are available. Essentially this makes using libgcc with Clang increasingly difficult and error-prone. Workarounds are possible (Android regularly bundles the compiler-rt extras beyond libgcc in their own library), but they are ugly. These users don't want to use compiler-rt, mostly because we haven't finished validating/establishing it for the Android platform/NDK yet.
(In reply to comment #2) > The default for Clang is to not use compiler-rt as its rtlib though. This is not true in all platforms. > It also should be aware of what rtlib is requested at the point where it decides > that these functions are available. This is an interesting proposition, but not always possible. The compiler doesn't have to know how the linker will behave and which libraries will be linked. If you compile to object than link it, but only use the -lgcc or -lcompiler-rt at link time, the compiler has *no way* to know which. Plus, the __has_builtin is really asking the *compiler* for support, not the linker nor the libraries. The user code is *clearly* abusing of compiler specific behaviour and not getting what they "wanted", but getting what they should have gotten. > Essentially this makes using libgcc with Clang increasingly difficult and error-prone. I agree with you 100%. But how many compilers out there are trying to use two run-time libraries? > Workarounds are possible > (Android regularly bundles the compiler-rt extras beyond libgcc in their own > library), but they are ugly. I'd say that code in Skia is ugly. I wouldn't work around in Android at all, just ask the developer to fix the code. > These users don't want to use compiler-rt, > mostly because we haven't finished validating/establishing it for the > Android platform/NDK yet. Well, that's a hard sell. They ask for a compiler-rt specific function but don't want to link compiler-rt. I really don't know what to say... If this was Clang generating the builtin from C code, I'd agree with you that it was a bug and needed to be fixed. But the code is specifically asking for a compiler-rt builtin, via a compiler support flag. I can't see how this is *not* user error.
(In reply to comment #3) > (In reply to comment #2) > > The default for Clang is to not use compiler-rt as its rtlib though. > > This is not true in all platforms. > > > > It also should be aware of what rtlib is requested at the point where it decides > > that these functions are available. > > This is an interesting proposition, but not always possible. The compiler > doesn't have to know how the linker will behave and which libraries will be > linked. If you compile to object than link it, but only use the -lgcc or > -lcompiler-rt at link time, the compiler has *no way* to know which. If the default for the platform is libgcc (i.e. !compiler-rt), and no -rtlib option was passed, it seems like it would be nicer for Clang to not use functions that might not be implemented. At the very least it could warn about this problematic case. > > Plus, the __has_builtin is really asking the *compiler* for support, not the > linker nor the libraries. > > The user code is *clearly* abusing of compiler specific behaviour and not > getting what they "wanted", but getting what they should have gotten. I guess they misunderstood the use of __has_builtin, but I can't blame them. I clearly didn't think that this was a user bug, and fully believed this was entirely a compiler issue. > > > > Essentially this makes using libgcc with Clang increasingly difficult and error-prone. > > I agree with you 100%. But how many compilers out there are trying to use > two run-time libraries? I think Clang is in a unique situation here. How many of Clang's targets actually use compiler-rt? How often is compiler-rt used in production software? I don't have a great answer to those questions. I certainly would hope things are heading towards using compiler-rt more often, but I don't have evidence of that. > > > > Workarounds are possible > > (Android regularly bundles the compiler-rt extras beyond libgcc in their own > > library), but they are ugly. > > I'd say that code in Skia is ugly. I wouldn't work around in Android at all, > just ask the developer to fix the code. It is actually in yet another library that Skia is pulling in, so the path to fix gets longer. > > > > These users don't want to use compiler-rt, > > mostly because we haven't finished validating/establishing it for the > > Android platform/NDK yet. > > Well, that's a hard sell. They ask for a compiler-rt specific function but > don't want to link compiler-rt. I really don't know what to say... They want to link compiler-rt. We just can't provide it in a working form today, because it isn't ready yet for Android. > > If this was Clang generating the builtin from C code, I'd agree with you > that it was a bug and needed to be fixed. But the code is specifically > asking for a compiler-rt builtin, via a compiler support flag. > > I can't see how this is *not* user error. I will communicate this to them, and hopefully get this fixed a different way.
(In reply to comment #4) > If the default for the platform is libgcc (i.e. !compiler-rt), and no -rtlib > option was passed, it seems like it would be nicer for Clang to not use > functions that might not be implemented. At the very least it could warn > about this problematic case. This is complicated... Linux defaults to libgcc, but if I use "clang -c" + "ld -lcompiler-rt", I don't want to get *any* warnings, and they can even fail my build (-Werror) for doing something completely correct. Passing -lcompiler-rt to "clang -c" to remove the warning would be the wrong "fix". > I guess they misunderstood the use of __has_builtin, but I can't blame them. > I clearly didn't think that this was a user bug, and fully believed this was > entirely a compiler issue. I understand it completely. We got a lot of "user errors" from the kernel sources that took months to convince them that it was, indeed, user error. :) Some of them I even implemented a fix myself, and only then I realised it was user error. > I think Clang is in a unique situation here. How many of Clang's targets > actually use compiler-rt? How often is compiler-rt used in production > software? I don't have a great answer to those questions. I certainly would > hope things are heading towards using compiler-rt more often, but I don't > have evidence of that. AFAIK, FreeBSD and Darwin do. I would like to default to RT on Linux, but that means validating the whole set (libc++, RT, libunwind), which I haven't got around yet. My target was to make it the default and *only* use libgcc & friends if you reverted back to freestanding and added everything manually. And I still want to have buildbots on both configurations, just in case. Integration has proven a much bigger challenge than fixing bugs or implementing new features. I'm sure you know what I'm talking about. :) > They want to link compiler-rt. We just can't provide it in a working form > today, because it isn't ready yet for Android. Have you spoken to Chris Matthews about cross-builds for RT? You guys may get a pre-release sync going, so that you can speed up both processes and come up with a similar solution in the end. > I will communicate this to them, and hopefully get this fixed a different > way. Thanks! Let me know if you need my help explaining things on the original reports.
Hi Stephen, I now saw that you closed it. I don't think this is strictly a won't fix. We *could* implement a custom lowering of the builtin in the ARM/AArch64 back-end. I mean, users shouldn't *rely* on it, but that doesn't mean we can't help them. I'm re-opening as an enhancement, and will add to our local list of interesting things to do, but at a lower priority. cheers, --renato
Hi Stephen, It appears my interpretation of __has_builtin was wrong all along. It seems that the builtin means "not only I know about it, but my compiler knows how to custom-lower it without resorting to library calls". The __mulodi4 implementation was so perfectly hand-crafted that it tricked me into trusting it. I apologise. Now, I'm moving this back to a "normal" task, but this is one that needs to be corrected in Clang first (to know when it really *knows* how to lower it, and that can take a while). Meanwhile, implementing it locally would work. Custom lowering could work at a medium term fix, but a proper call back mechanism in Clang should be the way to go. cheers, --renato
I still see this with clang-70 when compiling ruby see http://errors.yoctoproject.org/Errors/Details/188372/