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

[smeenai]

Summary

Multiple copies of emulated TLS state means inconsistent results when
accessing the same thread-local variable from different shared objects
(https://github.com/android/ndk/issues/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.

[danalbert]

LGTM, but @rprichard knows this area better than I do so he should probably weigh in before submitting :)

[rprichard]

Yeah, this looks fine to me.

[eugenis]

Weak symbol is a purely link-time concept, AFAIK. It is ignored in the dynamic loader, so, unless we link multiple copies of this file into the same DSO, "weak" attribute here should not have any effect.

[rprichard]

In D107127#2917555, @eugenis wrote:

Weak symbol is a purely link-time concept, AFAIK. It is ignored in the dynamic loader, so, unless we link multiple copies of this file into the same DSO, "weak" attribute here should not have any effect.

There's a new -Bsymbolic-non-weak-functions flag (https://reviews.llvm.org/D102570), where marking something weak has an important effect on the linker.

[eugenis]

Oh, that's a cool feature.

LGTM then.

[smeenai]

In D107127#2917555, @eugenis wrote:

Weak symbol is a purely link-time concept, AFAIK. It is ignored in the dynamic loader, so, unless we link multiple copies of this file into the same DSO, "weak" attribute here should not have any effect.

Doesn't the dynamic linker coalesce multiple copies of weak symbols at runtime, to make things like vague linkage for typeinfos work? Or does that coalescing happen regardless of the symbol being strong or weak?

[eugenis]

In D107127#2917626, @smeenai wrote:

In D107127#2917555, @eugenis wrote:

Weak symbol is a purely link-time concept, AFAIK. It is ignored in the dynamic loader, so, unless we link multiple copies of this file into the same DSO, "weak" attribute here should not have any effect.

Doesn't the dynamic linker coalesce multiple copies of weak symbols at runtime, to make things like vague linkage for typeinfos work? Or does that coalescing happen regardless of the symbol being strong or weak?

It resolves everything to the first definition in the lookup order. The only special handling of weak symbols that I've found is that missing definition is not an error.

[rprichard]

In D107127#2917667, @eugenis wrote:

It resolves everything to the first definition in the lookup order. The only special handling of weak symbols that I've found is that missing definition is not an error.

FWIW: I think FreeBSD's loader keeps going after finding a weak definition, looking for a non-weak definition. This is probably where it's implemented.

[smeenai]

In D107127#2917769, @rprichard wrote:

In D107127#2917667, @eugenis wrote:

It resolves everything to the first definition in the lookup order. The only special handling of weak symbols that I've found is that missing definition is not an error.

FWIW: I think FreeBSD's loader keeps going after finding a weak definition, looking for a non-weak definition. This is probably where it's implemented.

glibc does this as well if you specify the LD_DYNAMIC_WEAK environment variable.

[MaskRay]

In D107127#2917769, @rprichard wrote:

In D107127#2917667, @eugenis wrote:

It resolves everything to the first definition in the lookup order. The only special handling of weak symbols that I've found is that missing definition is not an error.

Yes.

FWIW: I think FreeBSD's loader keeps going after finding a weak definition, looking for a non-weak definition. This is probably where it's implemented.

FreeBSD's behavior (glibc pre-2.2 behavior or LD_DYNAMIC_WEAK=1) does not conform to the ELF specification. https://reviews.freebsd.org/D26352

Mach-O's weak definition coalesce behavior is indeed similar to FreeBSD/glibc pre-2.2's.

The patch does make emutls work with the new ld.lld -Bsymbolic-non-weak-functions.

ELF doesn't use the term "coalescing". ELF uses symbol preemption or symbol interposition.

compiler-rt/lib/builtins/emutls.c

if (array == NULL) {
     if (array)
       memset(array->data + orig_size, 0,
              (new_size - orig_size) * sizeof(void *));
     emutls_check_array_set_size(array, new_size);
   }
   return array;
 }
 
+#ifndef _WIN32
+// Our emulated TLS implementation relies on local state (e.g. for the pthread
+// key), and if we duplicate this state across different shared libraries,
+// accesses to the same TLS variable from different shared libraries will yield
+// different results (see https://github.com/android/ndk/issues/1551 for an
+// example). __emutls_get_address is the only external entry point for emulated
+// TLS, and by making it default visibility and weak, we can rely on the dynamic
+// linker to coalesce multiple copies at runtime and ensure a single unique copy
+// of TLS state. This is a best effort; it won't work if the user is linking
+// with -Bsymbolic or -Bsymbolic-functions, and it also won't work on Windows,
+// where the dynamic linker has no notion of coalescing weak symbols at runtime.
+// A more robust solution would be to create a separate shared library for
+// emulated TLS, to ensure a single copy of its state.
+__attribute__((visibility("default"), weak))
+#endif
 void *__emutls_get_address(__emutls_control *control) {
   uintptr_t index = emutls_get_index(control);
   emutls_address_array *array = emutls_get_address_array(index--);
   if (array->data[index] == NULL)
     array->data[index] = emutls_allocate_object(control);
   return array->data[index];
 }