std: make `thread::current` available in all `thread_local!` destructors #127912

joboet · 2024-07-18T12:07:47Z

... and thereby allow the panic runtime to always print the right thread name.

This works by modifying the TLS destructor system to schedule a runtime cleanup function after all other TLS destructors registered by std have run. Unfortunately, this doesn't affect foreign TLS destructors, thread::current will still panic there.

Additionally, the thread ID returned by current_id will now always be available, even inside the global allocator, and will not change during the lifetime of one thread (this was previously the case with key-based TLS).

The mechanisms I added for this (local_pointer and thread_cleanup) will also allow finally fixing #111272 by moving the signal stack to a similar runtime-cleanup TLS variable.

rustbot · 2024-07-18T12:07:55Z

r? @cuviper

rustbot has assigned @cuviper.
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

library/std/src/thread/mod.rs

bors · 2024-07-18T21:18:40Z

☔ The latest upstream changes (presumably #127924) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot · 2024-07-20T12:19:36Z

The Miri subtree was changed

cc @rust-lang/miri

bors · 2024-07-26T03:36:06Z

☔ The latest upstream changes (presumably #128213) made this pull request unmergeable. Please resolve the merge conflicts.

cuviper · 2024-08-26T23:33:14Z

Needs a rebase, but I think it looks ok, so r=me if rebasing doesn't change anything significant.

@rustbot author

joboet · 2024-08-27T13:00:24Z

T'was just a few imports and an unsafe block that needed reordering.

@bors r=cupiver

bors · 2024-08-27T13:00:27Z

📌 Commit cfbd250 has been approved by cupiver

It is now in the queue for this repository.

…cupiver std: make `thread::current` available in all `thread_local!` destructors ... and thereby allow the panic runtime to always print the right thread name. This works by modifying the TLS destructor system to schedule a runtime cleanup function after all other TLS destructors registered by `std` have run. Unfortunately, this doesn't affect foreign TLS destructors, `thread::current` will still panic there. Additionally, the thread ID returned by `current_id` will now always be available, even inside the global allocator, and will not change during the lifetime of one thread (this was previously the case with key-based TLS). The mechanisms I added for this (`local_pointer` and `thread_cleanup`) will also allow finally fixing rust-lang#111272 by moving the signal stack to a similar runtime-cleanup TLS variable.

…piver std: make `thread::current` available in all `thread_local!` destructors ... and thereby allow the panic runtime to always print the right thread name. This works by modifying the TLS destructor system to schedule a runtime cleanup function after all other TLS destructors registered by `std` have run. Unfortunately, this doesn't affect foreign TLS destructors, `thread::current` will still panic there. Additionally, the thread ID returned by `current_id` will now always be available, even inside the global allocator, and will not change during the lifetime of one thread (this was previously the case with key-based TLS). The mechanisms I added for this (`local_pointer` and `thread_cleanup`) will also allow finally fixing rust-lang#111272 by moving the signal stack to a similar runtime-cleanup TLS variable.

bors · 2024-10-02T13:04:07Z

⌛ Testing commit b1e383b with merge 45366f5...

bors · 2024-10-02T13:55:49Z

💔 Test failed - checks-actions

joboet · 2024-10-02T16:05:08Z

Another import error...
@bors r=@cupiver

bors · 2024-10-02T16:05:11Z

📌 Commit a71ba0c has been approved by cupiver

It is now in the queue for this repository.

cuviper · 2024-10-02T16:14:04Z

Just fixing my name for the record...

@bors r-

cuviper · 2024-10-02T16:14:08Z

@bors r+

bors · 2024-10-02T16:14:13Z

📌 Commit a71ba0c has been approved by cuviper

It is now in the queue for this repository.

bors · 2024-10-03T03:31:51Z

⌛ Testing commit a71ba0c with merge fd1f8aa...

bors · 2024-10-03T06:01:13Z

☀️ Test successful - checks-actions
Approved by: cuviper
Pushing fd1f8aa to master...

rust-timer · 2024-10-03T07:19:18Z

Finished benchmarking commit (fd1f8aa): comparison URL.

Overall result: ❌✅ regressions and improvements - 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)	0.2%	[0.2%, 0.2%]	1
Improvements ✅ (primary)	-0.3%	[-0.3%, -0.3%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.3%	[-0.3%, -0.3%]	1

Max RSS (memory usage)

Results (primary -0.5%, secondary -0.9%)

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.6%	[1.1%, 4.1%]	4
Regressions ❌ (secondary)	2.2%	[2.2%, 2.2%]	1
Improvements ✅ (primary)	-4.6%	[-12.1%, -0.9%]	3
Improvements ✅ (secondary)	-2.4%	[-2.9%, -2.0%]	2
All ❌✅ (primary)	-0.5%	[-12.1%, 4.1%]	7

Cycles

Results (secondary 1.6%)

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)	-	-	0
Regressions ❌ (secondary)	3.4%	[2.3%, 4.4%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.1%	[-2.1%, -2.1%]	1
All ❌✅ (primary)	-	-	0

Binary size

Results (primary -0.2%, secondary -0.6%)

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)	0.0%	[0.0%, 0.1%]	4
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-0.6%, -0.1%]	13
Improvements ✅ (secondary)	-0.6%	[-0.6%, -0.2%]	38
All ❌✅ (primary)	-0.2%	[-0.6%, 0.1%]	17

Bootstrap: 772.767s -> 771.674s (-0.14%)
Artifact size: 341.94 MiB -> 341.97 MiB (0.01%)

Fixes rust-lang#111272. With rust-lang#127912 merged, we now have all the infrastructure in place to support stack overflow detection in TLS destructors. This was not possible before because the signal stack was freed in the thread main function, thus a SIGSEGV afterwards would immediately crash. And on platforms without native TLS, the guard page address was stored in an allocation freed in a TLS destructor, so would not be available. rust-lang#127912 introduced the `local_pointer` macro which allows storing a pointer-sized TLS variable without allocation and the `thread_cleanup` runtime function which is called after all other code managed by the Rust runtime. This PR simply moves the signal stack cleanup to the end of `thread_cleanup` and uses `local_pointer` to store every necessary variable. And so, everything run under the Rust runtime is now properly protected against stack overflows.

rustbot assigned cuviper Jul 18, 2024

rustbot added O-hermit Operating System: Hermit S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 18, 2024

joboet commented Jul 18, 2024

View reviewed changes

library/std/src/thread/mod.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

joboet force-pushed the tls_dtor_thread_current branch from 7dd342e to f4486ee Compare July 18, 2024 13:45

This comment has been minimized.

Sign in to view

joboet force-pushed the tls_dtor_thread_current branch from f4486ee to fa1cb86 Compare July 18, 2024 17:34

This comment has been minimized.

Sign in to view

joboet force-pushed the tls_dtor_thread_current branch from fa1cb86 to 4459a4f Compare July 20, 2024 12:19

joboet mentioned this pull request Jul 20, 2024

Use atomics instead of mutex in exit guard #127863

Open

joboet added the A-thread-locals Area: Thread local storage (TLS) label Jul 20, 2024

joboet mentioned this pull request Jul 20, 2024

Tracking issue for cleaning up std's thread_local implementation details #110897

Open

24 tasks

joboet force-pushed the tls_dtor_thread_current branch from 4459a4f to d6777a9 Compare July 20, 2024 12:53

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 26, 2024

joboet force-pushed the tls_dtor_thread_current branch from d6777a9 to cfbd250 Compare August 27, 2024 10:16

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 27, 2024

workingjubilee mentioned this pull request Aug 29, 2024

Rollup of 6 pull requests #129728

Closed

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 2, 2024

This comment has been minimized.

Sign in to view

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

joboet added 2 commits October 2, 2024 18:04

std: make thread::current available in all thread_local! destructors

d868fdc

bless miri tests

a71ba0c

joboet force-pushed the tls_dtor_thread_current branch from b1e383b to a71ba0c Compare October 2, 2024 16:04

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 Oct 2, 2024

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 2, 2024

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 2, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 3, 2024

bors merged commit fd1f8aa into rust-lang:master Oct 3, 2024
7 checks passed

rustbot added this to the 1.83.0 milestone Oct 3, 2024

joboet mentioned this pull request Oct 5, 2024

std: detect stack overflows in TLS destructors on UNIX #131282

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

std: make `thread::current` available in all `thread_local!` destructors #127912

std: make `thread::current` available in all `thread_local!` destructors #127912

joboet commented Jul 18, 2024 •

edited

Loading

rustbot commented Jul 18, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors commented Jul 18, 2024

rustbot commented Jul 20, 2024

bors commented Jul 26, 2024

cuviper commented Aug 26, 2024

joboet commented Aug 27, 2024

bors commented Aug 27, 2024

bors commented Oct 2, 2024

This comment has been minimized.

bors commented Oct 2, 2024

joboet commented Oct 2, 2024

bors commented Oct 2, 2024

cuviper commented Oct 2, 2024

cuviper commented Oct 2, 2024

bors commented Oct 2, 2024

bors commented Oct 3, 2024

bors commented Oct 3, 2024

rust-timer commented Oct 3, 2024

std: make thread::current available in all thread_local! destructors #127912

std: make thread::current available in all thread_local! destructors #127912

Conversation

joboet commented Jul 18, 2024 • edited Loading

rustbot commented Jul 18, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors commented Jul 18, 2024

rustbot commented Jul 20, 2024

bors commented Jul 26, 2024

cuviper commented Aug 26, 2024

joboet commented Aug 27, 2024

bors commented Aug 27, 2024

bors commented Oct 2, 2024

This comment has been minimized.

bors commented Oct 2, 2024

joboet commented Oct 2, 2024

bors commented Oct 2, 2024

cuviper commented Oct 2, 2024

cuviper commented Oct 2, 2024

bors commented Oct 2, 2024

bors commented Oct 3, 2024

bors commented Oct 3, 2024

rust-timer commented Oct 3, 2024

Overall result: ❌✅ regressions and improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

std: make `thread::current` available in all `thread_local!` destructors #127912

std: make `thread::current` available in all `thread_local!` destructors #127912

joboet commented Jul 18, 2024 •

edited

Loading