Merge b8f04a670f27a84412099dd025fa762ee58f4c1a into 13.0.1

[llvmbot]

Bugzilla Link	52497
Version	unspecified
OS	OpenBSD
Blocks	#51489
Reporter	LLVM Bugzilla Contributor
CC	@smeenai,@tstellar
Fixed by commit(s)	b8f04a6

Extended Description

Please merge b8f04a6.

[PATCH] [builtins] Try to ensure single copy of emulated TLS state

Multiple copies of emulated TLS state means inconsistent results when
accessing the same thread-local variable from different shared objects
(android/ndk#1551). Making __emutls_get_address
be a weak default visibility symbol should make the dynamic linker
ensure only a single copy gets used at runtime. This is best-effort, but
the more robust approach of putting emulated TLS into its own shared
object would (a) be a much bigger change, and (b) shared objects are
pretty heavyweight, and adding a new one to a space-constrained
environment isn't an easy sell. Given the expected rarity of direct
accesses to emulated TLS variables across different shared objects, the
best-effort approach should suffice.

[llvmbot]

assigned to @rprichard

[tstellar]

Hi Unassigned,

What is your opinion on backporting this?

https://reviews.llvm.org/rGb8f04a670f27a84412099dd025fa762ee58f4c1a

[tstellar]

Hi Ryan,

What is your opinion on backporting this?

https://reviews.llvm.org/rGb8f04a670f27a84412099dd025fa762ee58f4c1a

[rprichard]

There was a bug reported against the NDK complaining about __emutls_get_address, and I'm wondering if we need to revert that LLVM change. (android/ndk#1606)

Exporting the symbol from the DSO as weak is fine, but I'm not sure there's a reason to make it weak. The effect it has on the dynamic loader is the same if it's weak or non-weak.

Exporting the symbol from the DSO changes how the static linker links things against the DSO.

e.g. They start importing the now-exported symbol rather than embedding their own copy of the symbol from the builtins archive.

This breaks if the DSO at build-time has __emutls_get_address, but the DSO at load-time doesn't. I suspect that's what happened with the NDK bug. The bug's libavutil.so depends only on platform DSOs
and the NDK's libc++_shared.so. The new NDK libc++_shared.so exports __emutls_get_address. Maybe the app is bundling a different libc++_shared.so than the one used to build libavutil.so.

The NDK has had this same problem with the unwinder in the past, but now the unwinder symbols are hidden.

[tstellar]

@rprichard So are you saying we shouldn't backport this?

[rprichard]

@DanAlbert @smeenai

I wondered if this commit needed to be reverted, but OTOH, maybe the NDK bug is user error for mixing incompatible versions of libc++_shared.so.

[wang-bin]

@DanAlbert @smeenai

I wondered if this commit needed to be reverted, but OTOH, maybe the NDK bug is user error for mixing incompatible versions of libc++_shared.so.

3rd party libraries may build with different version of libc++, and even worse, for example qt5 does not support ndk 22+. i think the commit fixes a rare case but breaks binary compatibility. an easy solution for that case is not using tls var across dso, instead we can set/get the value via a function, just like errno.

[DanAlbert]

Reverting the change would also be a break in compatibility. As I said on the NDK bug for this, yes, we'd have been better off not taking this change in r23b, but it's too late for that now.

Aiui the problem isn't mismatched versions of libc++, the problem is apps shipping an older libc++ than they built against. If the newer library is shipped there's no incompatibility. It's not at all unexpected that compiling against a newer version of a library than you'll run with can cause problems.