fix: Fix overly strict volatile reads in Windows TLS implementation

[thomasdangl]

The current TLS implementation introduced with f3facf1 is overly strict:
It assumes the TLS callback and _tls_used to be aligned while not mandating this alignment with #[repr(align(...))].

Two solutions to this problem are conceivable:

1. Keep using read_volatile and mandate a usize-alignment.
2. Replace read_volatile with read_unaligned.

This PR chooses to implement the latter, as there is no general requirement for keeping these pointers aligned.
Replacing read_volatile with read_unaligned should be fine in this case, as we do not care about the reads being reordered (they only exist to prevent the variables from being optimized away).

[rustbot]

r? @ibraheemdev

rustbot has assigned @ibraheemdev.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[joboet]

It assumes the TLS callback and _tls_used to be aligned while not mandating this alignment with #[repr(align(...))].

That's not true. Since CALLBACK is defined by Rust, it will always be well-aligned for its type (a function pointer), and since _tls_used is defined as a byte, that access is trivially well-aligned too. Moving to read_unaligned doesn't work anyhow – we need to use volatile reads to ensure that the symbol references are not optimised away by the compiler, but read_unaligned is considered side-effect-free and thus removable.

[thomasdangl]

That's not true. Since CALLBACK is defined by Rust, it will always be well-aligned for its type (a function pointer), and since _tls_used is defined as a byte, that access is trivially well-aligned too.

No, it is not. Compile it on any Windows version and look at the resulting COFF file.
The alignment of the CALLBACK symbol is 1.
It only works by implicitly relying on the alignment of the .CRT$XLB section.

[ChrisDenton]

read_volatile is necessary to avoid the unused read being optimized out. It's essentially a hack.

Ideally the best fix here is to have the compiler pass /INCLUDE:_tls_used to the linker when TLS or thread destructors are used and to include CALLBACK when thread destructors are used. The former is done by #[thread_local] on supported platforms but not if the TlsAlloc fallback is used and the compiler is completely unaware of thread destructors.

Implementing this would however be a bigger job and would require some kind of compiler-supported attribute the standard library could use to annotate the callback function.

[thomasdangl]

read_volatile is necessary to avoid the unused read being optimized out. It's essentially a hack.

I see. What about wrapping the read_unaligned in a std::hint::black_box?
From my understanding, this would achieve the same result without the alignment issue.

Implementing this would however be a bigger job and would require some kind of compiler-supported attribute the standard library could use to annotate the callback function.

I get that the current implementation is far from optimal and I see the flaw in my current PR.
In any case I don't think the standard library should make assumptions about linker settings.

[joboet]

In any case I don't think the standard library should make assumptions about linker settings.

The standard library doesn't do that. A static item must always be aligned to the alignment of its type, #[link_section] doesn't change that. If you have found an example of that not being true, please do file an issue, as that would be a soundness bug in the compiler.