std: use realstd fast key when building tests #100201

RalfJung · 2022-08-06T17:14:43Z

Under cfg(test), the std crate is not the actual standard library, just any old crate we are testing. It imports the real standard library as realstd, and then does some careful cfg magic so that the crate built for testing uses the realstd global state rather than having its own copy of that.

However, this was not done for all global state hidden in std: the 'fast' version of thread-local keys, at least on some platforms, also involves some global state. Specifically its macOS version has this static REGISTERED that would get duplicated. So this PR imports the 'fast' key type from realstd rather than using the local copy, to ensure its internal state (and that of the functions it calls) does not get duplicated.

I also noticed that the __OsLocalKeyInner is unused under cfg(target_thread_local), so I removed it for that configuration. There was a comment saying macOS picks between __OsLocalKeyInner and __FastLocalKeyInner at runtime, but I think that comment is outdated -- I found no trace of such a runtime switching mechanism, and the library still check-builds on apple targets with this PR. (I don't have a Mac so I cannot actually run it.)

rust-highfive · 2022-08-06T17:14:46Z

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

rustbot · 2022-08-06T17:14:46Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

RalfJung · 2022-08-06T17:18:24Z

library/std/src/thread/mod.rs

 #[unstable(feature = "libstd_thread_internals", issue = "none")]
 #[doc(hidden)]
+#[cfg(not(target_thread_local))]
 pub use self::local::os::Key as __OsLocalKeyInner;


It turns out that only one of these three key types is ever actually used now. So I considered refactoring this to avoid giving them different names, instead making __LocalKeyInner always be the key type for the current configuration. Do you think that would be a good idea?

The only issue I can see is that the types have to be used in slightly different ways: __FastLocalKeyInner wants a #[thread_local] static, the others a regular static. Using the same name for both usages slightly increases the risk that this would be mixed up.

Also I wasn't sure whether all(target_family = "wasm", not(target_feature = "atomics")) implied not(target_thread_local), if that is the case some of these cfg could be simplified and we could potentially at least merge __OsLocalKeyInner and __StaticLocalKeyInner into one type (i.e., move the logic for which implementation to use for that into the local module, rather than having it spread between that module and this file).

I wasn't sure whether all(target_family = "wasm", not(target_feature = "atomics")) implied not(target_thread_local)

Turns out the answer is no: #84224

So I considered refactoring this to avoid giving them different names, instead making __LocalKeyInner always be the key type for the current configuration. Do you think that would be a good idea?

Hm, you mean just always using the same name? If you want to do this, please submit it as a different PR. I think it might be cleaner, but also might end up ending up making mistakes more likely (as you mention). Either way, it feels like an unrelated change to this. Feel free to r?me if you decide to do this.

Yeah, basically have a single name for "the TLS key type we are using".

Not sure if it's worth it though.

RalfJung · 2022-08-11T12:21:40Z

r? libs

joshtriplett · 2022-08-21T13:03:25Z

r? libs

thomcc

LGTM. In particular, thanks for updating the stale comments.

thomcc · 2022-08-24T01:58:41Z

library/std/src/thread/mod.rs

 #[unstable(feature = "libstd_thread_internals", issue = "none")]
 #[doc(hidden)]
+#[cfg(not(target_thread_local))]
 pub use self::local::os::Key as __OsLocalKeyInner;


So I considered refactoring this to avoid giving them different names, instead making __LocalKeyInner always be the key type for the current configuration. Do you think that would be a good idea?

Hm, you mean just always using the same name? If you want to do this, please submit it as a different PR. I think it might be cleaner, but also might end up ending up making mistakes more likely (as you mention). Either way, it feels like an unrelated change to this. Feel free to r?me if you decide to do this.

thomcc · 2022-08-24T02:01:38Z

@bors r+

bors · 2022-08-24T02:01:40Z

📌 Commit d13699d has been approved by thomcc

It is now in the queue for this repository.

RalfJung · 2022-08-28T15:00:31Z

@bors rollup=iffy

bors · 2022-08-28T15:12:33Z

⌛ Testing commit d13699d with merge 223d16e...

bors · 2022-08-28T17:56:33Z

☀️ Test successful - checks-actions
Approved by: thomcc
Pushing 223d16e to master...

rust-timer · 2022-08-29T02:45:39Z

Finished benchmarking commit (223d16e): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.2%	[1.2%, 1.2%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	2.2%	[2.2%, 2.2%]	1
Regressions ❌ (secondary)	3.0%	[3.0%, 3.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.9%	[-5.3%, -2.5%]	2
All ❌✅ (primary)	2.2%	[2.2%, 2.2%]	1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	3.3%	[2.9%, 3.7%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.1%	[-3.1%, -3.1%]	1
All ❌✅ (primary)	3.3%	[2.9%, 3.7%]	2

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

std tests: use __OsLocalKeyInner from realstd This is basically the same as rust-lang#100201, but for __OsLocalKeyInner: Some std tests are failing in Miri on Windows because [this static](https://github.com/rust-lang/rust/blob/a377893da2cd7124e5a18c7116cbb70e16dd5541/library/std/src/sys/windows/thread_local_key.rs#L234-L239) is getting duplicated, and Miri does not handle that properly -- Miri does not support this magic `.CRT$XLB` linker section, but instead just looks up this particular hard-coded static in the standard library. This PR lets the test suite use the std static instead of having its own copy. Fixes rust-lang/miri#2754 r? `@thomcc`

std tests: use __OsLocalKeyInner from realstd This is basically the same as rust-lang#100201, but for __OsLocalKeyInner: Some std tests are failing in Miri on Windows because [this static](https://github.com/rust-lang/rust/blob/a377893da2cd7124e5a18c7116cbb70e16dd5541/library/std/src/sys/windows/thread_local_key.rs#L234-L239) is getting duplicated, and Miri does not handle that properly -- Miri does not support this magic `.CRT$XLB` linker section, but instead just looks up this particular hard-coded static in the standard library. This PR lets the test suite use the std static instead of having its own copy. Fixes rust-lang/miri#2754 r? ``@thomcc``

avoid duplicating TLS state between test std and realstd This basically re-lands rust-lang#100201 and rust-lang#106638, which got reverted by rust-lang#110861. This works around 2 Miri limitations: - Miri doesn't support the magic linker section that our Windows TLS support relies on, and instead knows where in std to find the symbol that stores the thread callback. - For macOS, Miri only supports at most one destructor to be registered per thread. The 2nd would not be very hard to fix (though the intended destructor order is unclear); the first would be a lot of work to fix. Neither of these is a problem for regular Rust code, but in the std test suite we have essentially 2 copies of the std code and then these both become issues. To avoid that we have the std test crate import the TLS code from the real std instead of having its own copy. r? `@m-ou-se`

avoid duplicating TLS state between test std and realstd This basically re-lands rust-lang#100201 and rust-lang#106638, which got reverted by rust-lang#110861. This works around 2 Miri limitations: - Miri doesn't support the magic linker section that our Windows TLS support relies on, and instead knows where in std to find the symbol that stores the thread callback. - For macOS, Miri only supports at most one destructor to be registered per thread. The 2nd would not be very hard to fix (though the intended destructor order is unclear); the first would be a lot of work to fix. Neither of these is a problem for regular Rust code, but in the std test suite we have essentially 2 copies of the std code and then these both become issues. To avoid that we have the std test crate import the TLS code from the real std instead of having its own copy. r? ``@m-ou-se``

avoid duplicating TLS state between test std and realstd This basically re-lands rust-lang#100201 and rust-lang#106638, which got reverted by rust-lang#110861. This works around 2 Miri limitations: - Miri doesn't support the magic linker section that our Windows TLS support relies on, and instead knows where in std to find the symbol that stores the thread callback. - For macOS, Miri only supports at most one destructor to be registered per thread. The 2nd would not be very hard to fix (though the intended destructor order is unclear); the first would be a lot of work to fix. Neither of these is a problem for regular Rust code, but in the std test suite we have essentially 2 copies of the std code and then these both become issues. To avoid that we have the std test crate import the TLS code from the real std instead of having its own copy. r? ```@m-ou-se```

avoid duplicating TLS state between test std and realstd This basically re-lands rust-lang#100201 and rust-lang#106638, which got reverted by rust-lang#110861. This works around 2 Miri limitations: - Miri doesn't support the magic linker section that our Windows TLS support relies on, and instead knows where in std to find the symbol that stores the thread callback. - For macOS, Miri only supports at most one destructor to be registered per thread. The 2nd would not be very hard to fix (though the intended destructor order is unclear); the first would be a lot of work to fix. Neither of these is a problem for regular Rust code, but in the std test suite we have essentially 2 copies of the std code and then these both become issues. To avoid that we have the std test crate import the TLS code from the real std instead of having its own copy. r? ````@m-ou-se````

avoid duplicating TLS state between test std and realstd This basically re-lands rust-lang#100201 and rust-lang#106638, which got reverted by rust-lang#110861. This works around 2 Miri limitations: - Miri doesn't support the magic linker section that our Windows TLS support relies on, and instead knows where in std to find the symbol that stores the thread callback. - For macOS, Miri only supports at most one destructor to be registered per thread. The 2nd would not be very hard to fix (though the intended destructor order is unclear); the first would be a lot of work to fix. Neither of these is a problem for regular Rust code, but in the std test suite we have essentially 2 copies of the std code and then these both become issues. To avoid that we have the std test crate import the TLS code from the real std instead of having its own copy. r? `````@m-ou-se`````

avoid duplicating TLS state between test std and realstd This basically re-lands rust-lang#100201 and rust-lang#106638, which got reverted by rust-lang#110861. This works around 2 Miri limitations: - Miri doesn't support the magic linker section that our Windows TLS support relies on, and instead knows where in std to find the symbol that stores the thread callback. - For macOS, Miri only supports at most one destructor to be registered per thread. The 2nd would not be very hard to fix (though the intended destructor order is unclear); the first would be a lot of work to fix. Neither of these is a problem for regular Rust code, but in the std test suite we have essentially 2 copies of the std code and then these both become issues. To avoid that we have the std test crate import the TLS code from the real std instead of having its own copy. r? ``````@m-ou-se``````

std tests: use __OsLocalKeyInner from realstd This is basically the same as rust-lang/rust#100201, but for __OsLocalKeyInner: Some std tests are failing in Miri on Windows because [this static](https://github.com/rust-lang/rust/blob/a377893da2cd7124e5a18c7116cbb70e16dd5541/library/std/src/sys/windows/thread_local_key.rs#L234-L239) is getting duplicated, and Miri does not handle that properly -- Miri does not support this magic `.CRT$XLB` linker section, but instead just looks up this particular hard-coded static in the standard library. This PR lets the test suite use the std static instead of having its own copy. Fixes rust-lang/miri#2754 r? `@thomcc`

avoid duplicating TLS state between test std and realstd This basically re-lands rust-lang/rust#100201 and rust-lang/rust#106638, which got reverted by rust-lang/rust#110861. This works around 2 Miri limitations: - Miri doesn't support the magic linker section that our Windows TLS support relies on, and instead knows where in std to find the symbol that stores the thread callback. - For macOS, Miri only supports at most one destructor to be registered per thread. The 2nd would not be very hard to fix (though the intended destructor order is unclear); the first would be a lot of work to fix. Neither of these is a problem for regular Rust code, but in the std test suite we have essentially 2 copies of the std code and then these both become issues. To avoid that we have the std test crate import the TLS code from the real std instead of having its own copy. r? ``````@m-ou-se``````

rust-highfive assigned m-ou-se Aug 6, 2022

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Aug 6, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 6, 2022

RalfJung commented Aug 6, 2022

View reviewed changes

RalfJung force-pushed the thread-local-key branch from 8cd175d to d4e6b5b Compare August 6, 2022 17:20

rust-highfive assigned joshtriplett and unassigned m-ou-se Aug 11, 2022

rust-highfive assigned thomcc and unassigned joshtriplett Aug 21, 2022

RalfJung force-pushed the thread-local-key branch from d4e6b5b to 81ebf9d Compare August 21, 2022 23:31

RalfJung added 2 commits August 22, 2022 09:14

std: use realstd fast key when building tests

138375a

update and extend some comments, and cfg-out some unused code

d13699d

RalfJung force-pushed the thread-local-key branch from 81ebf9d to d13699d Compare August 22, 2022 13:15

thomcc approved these changes Aug 24, 2022

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 24, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 28, 2022

bors merged commit 223d16e into rust-lang:master Aug 28, 2022

rustbot added this to the 1.65.0 milestone Aug 28, 2022

RalfJung deleted the thread-local-key branch August 29, 2022 11:55

saethlin mentioned this pull request Sep 8, 2022

Empty the patch rust-lang/miri-test-libstd#9

Closed

RalfJung mentioned this pull request Oct 6, 2022

Could not find __OsLocalKeyInner in thread when running cargo install #102549

Closed

RalfJung mentioned this pull request Jan 9, 2023

std tests: use __OsLocalKeyInner from realstd #106638

Merged

joboet mentioned this pull request Apr 26, 2023

Restructure and rename std thread_local internals to make it less of a maze #110861

Merged

RalfJung mentioned this pull request Apr 28, 2023

avoid duplicating TLS state between test std and realstd #110946

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

std: use realstd fast key when building tests #100201

std: use realstd fast key when building tests #100201

RalfJung commented Aug 6, 2022

rust-highfive commented Aug 6, 2022

rustbot commented Aug 6, 2022

RalfJung Aug 6, 2022

RalfJung Aug 22, 2022

thomcc Aug 24, 2022 •

edited

Loading

RalfJung Aug 24, 2022

RalfJung commented Aug 11, 2022

joshtriplett commented Aug 21, 2022

thomcc left a comment

thomcc Aug 24, 2022 •

edited

Loading

thomcc commented Aug 24, 2022

bors commented Aug 24, 2022

RalfJung commented Aug 28, 2022

bors commented Aug 28, 2022

bors commented Aug 28, 2022

rust-timer commented Aug 29, 2022

std: use realstd fast key when building tests #100201

std: use realstd fast key when building tests #100201

Conversation

RalfJung commented Aug 6, 2022

rust-highfive commented Aug 6, 2022

rustbot commented Aug 6, 2022

RalfJung Aug 6, 2022

Choose a reason for hiding this comment

RalfJung Aug 22, 2022

Choose a reason for hiding this comment

thomcc Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

RalfJung Aug 24, 2022

Choose a reason for hiding this comment

RalfJung commented Aug 11, 2022

joshtriplett commented Aug 21, 2022

thomcc left a comment

Choose a reason for hiding this comment

thomcc Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

thomcc commented Aug 24, 2022

bors commented Aug 24, 2022

RalfJung commented Aug 28, 2022

bors commented Aug 28, 2022

bors commented Aug 28, 2022

rust-timer commented Aug 29, 2022

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

thomcc Aug 24, 2022 •

edited

Loading

thomcc Aug 24, 2022 •

edited

Loading