Catch panics/unwinding in destruction of TLS values #105426

flba-eb · 2022-12-07T14:56:00Z

destroy_value is/can be called from C code (libc). Unwinding from Rust to C code is undefined behavior, which is why unwinding is caught here.

This problem caused an infinite loop inside the unwinding code when running src/test/ui/threads-sendsync/issue-24313.rs on a tier 3 target (QNX/Neutrino) on aarch64.

See also https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/Infinite.20unwinding.20bug.

rustbot · 2022-12-07T14:56:10Z

r? @m-ou-se

(rustbot has picked a reviewer for you, use r? to override)

library/std/src/thread/local.rs

flba-eb · 2022-12-16T08:06:05Z

library/std/src/thread/local.rs

+        if let Err(_) = panic::catch_unwind(panic::AssertUnwindSafe(|| unsafe {
+            let ptr = ptr as *mut Key<T>;


Is panic::AssertUnwindSafe correct here? Is it allowed to move let ptr = down to limit its scope?

You only have to put the drop() line in the catch_unwind.

@m-ou-se Unfortunately the dereferencing of raw pointers needs to be done inside the unsafe block and it cannot be done in a separate unsafe block as we need the variable value (unless we have a quite long unsafe block). I've restored the position of let ptr = ... line as it neither requires catch_unwind nor unsafe and keeps the diff smaller. Is this what you had in mind?

rustbot · 2022-12-16T08:08:32Z

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

m-ou-se · 2022-12-28T16:12:51Z

library/std/src/thread/local.rs

            let value = (*ptr).inner.take();
            (*ptr).dtor_state.set(DtorState::RunningOrHasRun);
            drop(value);
+        })) {
+            rtabort!("destructor panicked");


Can you change the panic message to "thread local panicked on drop"? Then it matches the message of thread results:

rust/library/std/src/thread/mod.rs

Lines 1424 to 1428 in 6a20f7d

if let Err(_) = panic::catch_unwind(panic::AssertUnwindSafe(|| {

*self.result.get_mut() = None;

})) {

rtabort!("thread result panicked on drop");

}

Great hint, thank & done

`destroy_value` is/can be called from C code (libc). Unwinding from Rust to C code is undefined behavior, which is why unwinding is caught here.

flba-eb · 2022-12-28T18:35:53Z

@rustbot label -S-waiting-on-author +S-waiting-on-review

m-ou-se · 2022-12-30T12:45:01Z

@bors r+

bors · 2022-12-30T12:45:03Z

📌 Commit 04a6f22 has been approved by m-ou-se

It is now in the queue for this repository.

bors · 2022-12-30T12:58:53Z

⌛ Testing commit 04a6f22 with merge 7c99186...

bors · 2022-12-30T16:09:52Z

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing 7c99186 to master...

rust-timer · 2022-12-30T17:29:01Z

Finished benchmarking commit (7c99186): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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.7%	[0.3%, 1.9%]	11
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.3%	[-0.6%, -0.2%]	41
Improvements ✅ (secondary)	-1.8%	[-14.6%, -0.2%]	31
All ❌✅ (primary)	-0.1%	[-0.6%, 1.9%]	52

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.6%	[0.4%, 3.4%]	8
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-5.0%	[-8.1%, -1.2%]	4
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[-8.1%, 3.4%]	12

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)	2.4%	[2.2%, 2.7%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-5.5%	[-14.9%, -2.3%]	11
All ❌✅ (primary)	2.4%	[2.2%, 2.7%]	2

rylev · 2023-01-03T17:42:08Z

@m-ou-se @flba-eb I'm not sure I understand where this perf regression is coming from. This does not look like noise as the impact is pretty broad and impacts some of the more stable benchmarks. Some of the benchmarks are showing increased activity in LLVM which might be caused by extra code gen, but this is not the case in all regressions and it certainly doesn't explain the improvements.

Any thoughts?

flba-eb · 2023-01-03T18:04:46Z

@m-ou-se @flba-eb I'm not sure I understand where this perf regression is coming from. This does not look like noise as the impact is pretty broad and impacts some of the more stable benchmarks. Some of the benchmarks are showing increased activity in LLVM which might be caused by extra code gen, but this is not the case in all regressions and it certainly doesn't explain the improvements.

Any thoughts?

Good question, @rylev . I don't know how CatchUnwind is implemented but I guess it adds a special symbol on the stack which let's the unwinding stop early. On qnx it stops a loop which would run endlessly otherwise (when drop panics) -- I could imagine that on other systems unwinding have a similar issue but have some safety counter to also stop after a high number of e.g. checked frames. In such a case unwinding would stop earlier now (at CatchUnwind instead after reaching some number). But this is really some very wild guess... It also would only make sense if the improved test cases have drop calls that panic.

…, r=m-ou-se Catch panics/unwinding in destruction of TLS values `destroy_value` is/can be called from C code (libc). Unwinding from Rust to C code is undefined behavior, which is why unwinding is caught here. This problem caused an infinite loop inside the unwinding code when running `src/test/ui/threads-sendsync/issue-24313.rs` on a tier 3 target (QNX/Neutrino) on aarch64. See also https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/Infinite.20unwinding.20bug.

rustbot assigned m-ou-se Dec 7, 2022

rustbot added 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 Dec 7, 2022

flba-eb force-pushed the fix_tls_destructor_unwinding branch from e55bcb7 to b7691da Compare December 7, 2022 15:00

Noratrieb reviewed Dec 7, 2022

View reviewed changes

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

riking reviewed Dec 9, 2022

View reviewed changes

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

flba-eb force-pushed the fix_tls_destructor_unwinding branch from b7691da to f6a6177 Compare December 16, 2022 08:04

flba-eb commented Dec 16, 2022

View reviewed changes

flba-eb marked this pull request as ready for review December 16, 2022 08:08

m-ou-se reviewed Dec 28, 2022

View reviewed changes

m-ou-se 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 Dec 28, 2022

Catch panics in destruction of TLS values

04a6f22

`destroy_value` is/can be called from C code (libc). Unwinding from Rust to C code is undefined behavior, which is why unwinding is caught here.

flba-eb force-pushed the fix_tls_destructor_unwinding branch from f6a6177 to 04a6f22 Compare December 28, 2022 17:35

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

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 Dec 30, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 30, 2022

bors merged commit 7c99186 into rust-lang:master Dec 30, 2022

rustbot added this to the 1.68.0 milestone Dec 30, 2022

rustbot added the perf-regression Performance regression. label Dec 30, 2022

flba-eb deleted the fix_tls_destructor_unwinding branch January 11, 2023 07:15

workingjubilee added the O-neutrino OS: QNX Neutrino, a POSIX-compatible real-time operating system label Feb 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Catch panics/unwinding in destruction of TLS values #105426

Catch panics/unwinding in destruction of TLS values #105426

flba-eb commented Dec 7, 2022 •

edited

Loading

rustbot commented Dec 7, 2022

flba-eb Dec 16, 2022

m-ou-se Dec 28, 2022

flba-eb Dec 28, 2022 •

edited

Loading

rustbot commented Dec 16, 2022

m-ou-se Dec 28, 2022

flba-eb Dec 28, 2022

flba-eb commented Dec 28, 2022

m-ou-se commented Dec 30, 2022

bors commented Dec 30, 2022

bors commented Dec 30, 2022

bors commented Dec 30, 2022

rust-timer commented Dec 30, 2022

rylev commented Jan 3, 2023

flba-eb commented Jan 3, 2023

		if let Err(_) = panic::catch_unwind(panic::AssertUnwindSafe(\|\| unsafe {
		let ptr = ptr as *mut Key<T>;

	if let Err(_) = panic::catch_unwind(panic::AssertUnwindSafe(\|\| {
	*self.result.get_mut() = None;
	})) {
	rtabort!("thread result panicked on drop");
	}

Catch panics/unwinding in destruction of TLS values #105426

Catch panics/unwinding in destruction of TLS values #105426

Conversation

flba-eb commented Dec 7, 2022 • edited Loading

rustbot commented Dec 7, 2022

flba-eb Dec 16, 2022

Choose a reason for hiding this comment

m-ou-se Dec 28, 2022

Choose a reason for hiding this comment

flba-eb Dec 28, 2022 • edited Loading

Choose a reason for hiding this comment

rustbot commented Dec 16, 2022

m-ou-se Dec 28, 2022

Choose a reason for hiding this comment

flba-eb Dec 28, 2022

Choose a reason for hiding this comment

flba-eb commented Dec 28, 2022

m-ou-se commented Dec 30, 2022

bors commented Dec 30, 2022

bors commented Dec 30, 2022

bors commented Dec 30, 2022

rust-timer commented Dec 30, 2022

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

rylev commented Jan 3, 2023

flba-eb commented Jan 3, 2023

flba-eb commented Dec 7, 2022 •

edited

Loading

flba-eb Dec 28, 2022 •

edited

Loading