Repository navigation
[BUG] Emulated TLS across shared libraries is (more) broken with r23 #1551
Description
Repro test case (here's a tarball for convenience):
CMakeLists.txt:
cmake_minimum_required(VERSION 3.13.4)
project(tlstest C)
add_library(tls SHARED tls.c)
target_link_options(tls PRIVATE -rtlib=compiler-rt)
add_executable(main main.c)
target_link_libraries(main PRIVATE tls)
target_link_options(main PRIVATE -rtlib=compiler-rt)
tls.h:
#pragma once
extern _Thread_local int tls_var;
int get(void);
void set(int val);
tls.c:
#include "tls.h"
_Thread_local int tls_var;
int get() { return tls_var; }
void set(int val) { tls_var = val; }
main.c:
#include "tls.h"
#include <stdio.h>
int main() {
set(42);
printf("get() is %d\n", get());
printf("tls_var is %d\n", tls_var);
}
Build and run with NDK r22:
$ cmake -G Ninja \
-DCMAKE_TOOLCHAIN_FILE=$HOME/local/android-ndk-r22/build/cmake/android.toolchain.cmake \
-DANDROID_ABI=armeabi-v7a -DANDROID_PLATFORM=16 ..
$ ninja
$ adb push main libtls.so /data/local/tmp
$ adb shell LD_LIBRARY_PATH=/data/local/tmp /data/local/tmp/main
get() is 42
tls_var is 42
Build and run with NDK r23 beta 6:
$ cmake -G Ninja \
-DCMAKE_TOOLCHAIN_FILE=$HOME/local/android-ndk-r23-beta6/build/cmake/android.toolchain.cmake \
-DANDROID_ABI=armeabi-v7a -DANDROID_PLATFORM=16 ..
$ ninja
$ adb push main libtls.so /data/local/tmp
$ adb shell LD_LIBRARY_PATH=/data/local/tmp /data/local/tmp/main
get() is 42
tls_var is 0
The culprit is https://reviews.llvm.org/D93431, which gave __emutls_get_address hidden visibility. Each .so now gets its own copy of that symbol, which in turn has its own copy of the emutls_pthread_key (and other emutls state), so the same emutls address will return different values across different shared objects.
The example is obviously contrived, but we've observed an actual app breakage from this. I say "more broken" instead of just "broken" in the title because it was kinda coincidental that this was working with the previous setup. The compiler driver inserts compiler-rt late in the link command, so if a shared object foo happens to reference __emutls_get_address (and so exports it with the old setup), other shared objects depending on foo will reference the symbol from that shared object instead of pulling in their own copy from compiler-rt. You could have a shared object not depending on foo that'd still get its own copy of __emutls_get_address though, and would be subject to the same breakage.
I'm not sure what the right fix is here ... maybe providing a shared library with the emutls bits to guarantee a single copy of emutls state across all shared objects? (For the time being, I'm just marking __emutls_get_address as default visibility and that works for us, but that could still bite us with the scenario described above.)
Activity
smeenai commentedon Jul 28, 2021
(Note that https://reviews.llvm.org/D93431 brings Android behavior in line with all other platforms, so this exact same issue would affect all other platforms as well; it's not Android specific. I'm reporting it here since it's a new issue with r23, but I can also file an upstream LLVM bug report instead if y'all would prefer.)
DanAlbert commentedon Jul 29, 2021
Thanks for the detailed report 👍
@rprichard I think making the symbol WEAK probably solves the problem, including the pre-existing one?
DanAlbert commentedon Jul 29, 2021
I think this counts as medium as a source compat issue which does qualify this for an RC respin, but those are certainly guidelines meant to be interpreted based on severity rather than strictly followed. I suspect that the use case needed to trigger the bug (direct access to a global thread local in another shared library, rather than through a function) is probably uncommon enough that we don't need to delay promotion of RC1 to stable for this, but we of course should get this fixed in a minor release soon after. @smeenai do you disagree with that?
@stephenhines assuming this is a trivial patch and update to clang-r416183, any estimate on how long it would take to deliver to prebuilts? I was actually just about to send the latest build to QA, but if this can be turned around quickly perhaps we should.
smeenai commentedon Jul 29, 2021
We build compiler-rt ourselves and don't use the one from the NDK, so you don't need to delay anything on our behalf :) I agree that this is a pretty big edge case and likely quite rare.
As in making it default visibility and weak, so that the dynamic linker coalesces multiple copies at runtime? That won't work for anyone who's built with
-Bsymbolicor-Bsymbolic-functions, but I don't know how common that is either. (Fangrui proposed-Bsymbolic-non-weak-functionsin https://reviews.llvm.org/D102570, but that hasn't landed yet.)DanAlbert commentedon Jul 29, 2021
Good point. I think your earlier suggestion of having an extra shared library to provide this function is the only fix there without fangrui's patch.
-Bsymbolic-non-weak-functionshonestly sounds like it should just be the default behavior. I'm checking with the rest of the team first but I think we'd probably make that be our default behavior if it lands, since it preserves the C++ specified behavior for new/delete but also does what everyone wants for everything else (no interposition by default, get interposition if you want it by making a weak symbol, smaller code size thanks to fewer relocations, etc). Filed #1552 for us to consider that. Thanks for the link!stephenhines commentedon Jul 30, 2021
We have (unfortunately not visible on public ci.android.com) https://android-build.googleplex.com/builds/branches/aosp-llvm-r416183/grid? so this should be pretty easy to cherry-pick/rebuild/test. https://android-review.googlesource.com/c/toolchain/llvm_android/+/1782411 is the cherry-pick.
smeenai commentedon Jul 30, 2021
I put up https://reviews.llvm.org/D107127 to add the weak default visibility to
__emutls_get_address.smeenai commentedon Jul 30, 2021
If I'm reading this right, it's cherry-picking https://reviews.llvm.org/D93431, which is the cause of this issue, not the solution :) https://reviews.llvm.org/D107127 is a possible fix.
Like I said earlier, we don't use compiler-rt from the NDK toolchain, so you don't have to worry about scrambling to update prebuilts or delay releases on our behalf. (I don't know how likely it is for other people to run into this scenario.)
stephenhines commentedon Jul 30, 2021
Ah, I misunderstood above. It's also way too easy for us these days to just cherry-pick things, so I figured it wouldn't hurt to upload it. Oh well. I abandoned it now.
I'll let @DanAlbert decide whether he wants to wait your other fix, especially if the bug isn't present unless you use a more recent compiler-rt than what is shipping. It seems like we couldn't reproduce this with stock NDK r23 (nor should anyone else unless they're also building their own newer compiler-rt).
smeenai commentedon Jul 30, 2021
The issue should be present in a stock NDK r23 ... my repro instructions are with a stock NDK r23 beta 6. From what I understand, r23 is using the clang-r416183b prebuilt, which contains https://reviews.llvm.org/D93431.
DanAlbert commentedon Jul 30, 2021
Discussed this a bit and decided to stick with my plan above: fix in r23b. @rprichard pointed out that for anyone using ndk-build or CMake this isn't a regression because they were getting
--exclude-libseven before r23, which means this hits an even smaller number of folks than my already small estimation.@smeenai I approved your change upstream (thanks!). Strongly considering making
-Bsymbolic-non-weak-functionsthe default for all of Android as well, which solves even that subset of the problem. Thanks again for all the help and feedback 👍[builtins] Try to ensure single copy of emulated TLS state
6 remaining items
backport https://reviews.llvm.org/D107127 to fix shared libraries usi…
juha-h commentedon Nov 13, 2021
I built ffmpeg libraries using 23.1.7779620 that according to this page https://developer.android.com/ndk/downloads is r23b, and still I get the crash, when my app starts:
There is no crash if I build ffmpeg libs using 23.0.7599858.
Should the fix mentioned in above be already included in 23.1.7779620?
backport https://reviews.llvm.org/D107127 to fix shared libraries usi…
backport https://reviews.llvm.org/D107127 to fix shared libraries usi…
backport https://reviews.llvm.org/D107127 to fix shared libraries usi…
DanAlbert commentedon Nov 15, 2021
Yes, but what you have is different than the issue reported here. It seems like the problem you're encountering was surfaced by the fix here, but with the information available I can't tell if the fix caused a bug or just made it visible. Can you file a new bug with all the information the template asks for so we can take a look?
juha-h commentedon Nov 15, 2021
Done #1606.
[builtins] Try to ensure single copy of emulated TLS state
[builtins] Try to ensure single copy of emulated TLS state
[builtins] Try to ensure single copy of emulated TLS state
[builtins] Try to ensure single copy of emulated TLS state
[builtins] Try to ensure single copy of emulated TLS state
[builtins] Try to ensure single copy of emulated TLS state
Update LLVM to r416183c1.
[builtins] Try to ensure single copy of emulated TLS state