x86: add support for TLSDESC relocations

[kib]

Fix dta/dtv tls_gen check consistency.

[kib]

I looked at the implementation for aarch64 and I see some issues that I do not understand.

First, the problem seems to be that allocated tls_data AKA rtld_tlsdesc_dynamic argument structures are never freed. This is probably not too serious because typical program does not contain too many thread-local data and does not load/unload modules containing such data.

Second, is that tlsdesc->dtv_gen is initialized with the current tls_dtv_generation. Then, if any module is loaded afterward, the check in the inlined path for dtv->dtv_gen == tlsdata->dtv_gen always fail, so the code falls to the half of the slow path. I mean 'half' because it saves/restores all registers, and calls tls_get_addr_common(), which normally would observe dtv->dtv_gen == tls_dtv_generation and do not need to call _slow() resolver. But still, it seems unoptimal after the efforts to write the inline asm version.

Do I mis-read the code?

[kib]

Remove dtv_gen from tlsdesc_dynarg. There is no point of storing the generation there for our dtv update structure.
Add comment explaining the use of rtld_bind_lock in rtld_get_addr_slow().

[kib]

amd64: provide the fast path for dynamic TLSDESC resolver

[jrtc27]

I believe both to be true, yes (and have previously deduced the latter myself, too...)

[jrtc27]

libexec/rtld-elf/rtld.c

5468 ↗

(On Diff #185598)

This is specifically about why it needs to be exclusive not shared, right? I don't think it's the most clear it's talking about that, rather than taking the lock in general (which you need to synchronise with map_object bumping tls_max_index/tls_dtv_generation).

[kib]

Reword the comment about rtld_bind_lock.

[jrtc27]

Looking at the glibc source, they do have an equivalent to the dtv_gen field, but the fast path check is whether the DTV's generation is >= that field, not ==. I think this is a time optimisation that avoids the need to access the global generation in order to do the check, instead just using a copy that's more to hand, and also lets you avoid taking the slow path for DTV entries that don't actually need it, even though the generation counter is ahead (e.g. if you dlopen A, use TLS for A, dlopen B, then use TLS for A again, you can still use the fast path, but ye olde __tls_get_addr doesn't know that, so will pessimistically update the DTV for B's load).

[jrtc27]

Regarding leaking, glibc does seem to store them in a hash table tied to link_map (i.e. Obj_Entry) so will free on dlclose, unlike our arm64 implementation. Linked list seems a bit unnecessarily slow, but at the same time extensive dynamic TLSDESC + dlclose is probably not common enough to actually benefit from something like a reallocarray-based storage.

[jrtc27]

libexec/rtld-elf/amd64/reloc.c

159 ↗

(On Diff #185599)

I do fear there's going to be a lot of copy/pasting of all this code for every architecture, when it's 99% the same (I also started a RISC-V implementation a while back but didn't get to the point of writing the tests for the dynamic slow path). I don't know how enthusiastic you would be about trying to unify the amd64, aarch64 and i386 implementations?

[kib]

In D59269#1360938, @jrtc27 wrote:

Looking at the glibc source, they do have an equivalent to the dtv_gen field, but the fast path check is whether the DTV's generation is >= that field, not ==. I think this is a time optimisation that avoids the need to access the global generation in order to do the check, instead just using a copy that's more to hand, and also lets you avoid taking the slow path for DTV entries that don't actually need it, even though the generation counter is ahead (e.g. if you dlopen A, use TLS for A, dlopen B, then use TLS for A again, you can still use the fast path, but ye olde __tls_get_addr doesn't know that, so will pessimistically update the DTV for B's load).

Yes, something like that is described in the TSLDESC RFC. But you have to update the dtv gen in the TLSDESC arg structure at least once anyway. I think it is fine to skip this optimization.

[kib]

In D59269#1360939, @jrtc27 wrote:

Regarding leaking, glibc does seem to store them in a hash table tied to link_map (i.e. Obj_Entry) so will free on dlclose, unlike our arm64 implementation. Linked list seems a bit unnecessarily slow, but at the same time extensive dynamic TLSDESC + dlclose is probably not common enough to actually benefit from something like a reallocarray-based storage.

The list is only traversed on dlclose() to free items, so I do not see it as causing any slowdown. In fact, where the hashed lookup would be useful?

[jrtc27]

In D59269#1360942, @kib wrote:

In D59269#1360939, @jrtc27 wrote:

Regarding leaking, glibc does seem to store them in a hash table tied to link_map (i.e. Obj_Entry) so will free on dlclose, unlike our arm64 implementation. Linked list seems a bit unnecessarily slow, but at the same time extensive dynamic TLSDESC + dlclose is probably not common enough to actually benefit from something like a reallocarray-based storage.

The list is only traversed on dlclose() to free items, so I do not see it as causing any slowdown. In fact, where the hashed lookup would be useful?

I don't understand why they use a hash table to uniquify their dynamic tlsdesc structs. I only note that it's a less cache-hating data structure, but for our implementation if we wanted that we'd just use an array of pointers. But I suppose you still have the dynamic tlsdesc structs scattered throughout memory so pointer chasing the linked list doesn't really make it much worse, beyond a bit of extra data dependency. So I think the linked list is fine.

[kib]

In D59269#1360943, @jrtc27 wrote:

In D59269#1360942, @kib wrote:

In D59269#1360939, @jrtc27 wrote:

Regarding leaking, glibc does seem to store them in a hash table tied to link_map (i.e. Obj_Entry) so will free on dlclose, unlike our arm64 implementation. Linked list seems a bit unnecessarily slow, but at the same time extensive dynamic TLSDESC + dlclose is probably not common enough to actually benefit from something like a reallocarray-based storage.

The list is only traversed on dlclose() to free items, so I do not see it as causing any slowdown. In fact, where the hashed lookup would be useful?

I don't understand why they use a hash table to uniquify their dynamic tlsdesc structs. I only note that it's a less cache-hating data structure, but for our implementation if we wanted that we'd just use an array of pointers. But I suppose you still have the dynamic tlsdesc structs scattered throughout memory so pointer chasing the linked list doesn't really make it much worse, beyond a bit of extra data dependency. So I think the linked list is fine.

I believe that tlsdesc_args structures should be effectively allocated linearly. They are allocated from rtld malloc during the module load (not lazily), and the rtld malloc is relatively simple to be equiv to bump allocator while there is a space on the current page.

[kib]

Any more notes or opinions? I plan to commit this shortly.