Ensure TLS destructors run before thread joins in SGX #84409

mzohreva · 2021-04-21T20:49:02Z

The excellent test is from @jethrogb

For context see: #83416 (comment)

rust-highfive · 2021-04-21T20:49:05Z

r? @dtolnay

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

library/std/src/sys/sgx/thread.rs

jethrogb

LGTM

dtolnay

Thanks -- accepting without particular scrutiny on the basis of @jethrogb's review.

dtolnay · 2021-04-22T16:41:49Z

@bors r+

bors · 2021-04-22T16:41:50Z

📌 Commit 5d9eeff has been approved by dtolnay

…r=dtolnay Ensure TLS destructors run before thread joins in SGX The excellent test is from `@jethrogb` For context see: rust-lang#83416 (comment)

…r=dtolnay Ensure TLS destructors run before thread joins in SGX The excellent test is from ``@jethrogb`` For context see: rust-lang#83416 (comment)

…r=dtolnay Ensure TLS destructors run before thread joins in SGX The excellent test is from ```@jethrogb``` For context see: rust-lang#83416 (comment)

Dylan-DPC-zz · 2021-04-24T11:17:02Z

fails in rollup

@bors r-

std::sync::mpsc uses thread locals and depending on the order TLS dtors are run `rx.recv()` can panic when used in a TLS dtor.

mzohreva · 2021-04-30T04:27:09Z

@dtolnay this is ready for another look.

@rustbot modify labels to +S-waiting-on-review, -S-waiting-on-author

jethrogb · 2021-05-04T08:22:54Z

Ping for review

mzohreva · 2021-05-06T17:25:48Z

@jethrogb I think you need @ with your bors command.

jethrogb · 2021-05-06T19:50:49Z

@bors r+

bors · 2021-05-06T19:50:50Z

📌 Commit 2acd62d has been approved by jethrogb

…r=jethrogb Ensure TLS destructors run before thread joins in SGX The excellent test is from `@jethrogb` For context see: rust-lang#83416 (comment)

…r=jethrogb Ensure TLS destructors run before thread joins in SGX The excellent test is from ``@jethrogb`` For context see: rust-lang#83416 (comment)

Rollup of 11 pull requests Successful merges: - rust-lang#84409 (Ensure TLS destructors run before thread joins in SGX) - rust-lang#84500 (Add --run flag to compiletest) - rust-lang#84728 (Add test for suggestion to borrow unsized function parameters) - rust-lang#84734 (Add `needs-unwind` and beginning of support for testing `panic=abort` std to compiletest) - rust-lang#84755 (Allow using `core::` in intra-doc links within core itself) - rust-lang#84871 (Disallows `#![feature(no_coverage)]` on stable and beta (using standard crate-level gating)) - rust-lang#84872 (Wire up tidy dependency checks for cg_clif) - rust-lang#84896 (Handle incorrect placement of parentheses in trait bounds more gracefully) - rust-lang#84905 (CTFE engine: rename copy → copy_intrinsic, move to intrinsics.rs) - rust-lang#84953 (Remove unneeded call to with_default_session_globals in rustdoc highlight) - rust-lang#84987 (small nits) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

jethrogb · 2021-05-07T08:09:49Z

@rustbot modify labels: +beta-nominated

rustbot · 2021-05-07T08:09:51Z

Error: Label beta-nominated can only be set by Rust team members

Please let @rust-lang/release know if you're having trouble with this bot.

apiraino · 2021-05-07T09:24:28Z

@rustbot labels +beta-nominated

apiraino · 2021-05-20T15:30:25Z

Beta backport approved as per compiler team on Zulip

@rustbot label +beta-accepted

…ulacrum [beta] backports * Backport 1.52.1 release notes rust-lang#85404 * remove InPlaceIterable marker from Peekable due to unsoundness rust-lang#85340 * rustdoc: Call initSidebarItems in root module of crate rust-lang#85304 * Update LLVM submodule rust-lang#85236 * Do not ICE on invalid const param rust-lang#84913 * Disallows #![feature(no_coverage)] on stable and beta (using standard crate-level gating) rust-lang#84871 * Ensure TLS destructors run before thread joins in SGX rust-lang#84409

tmiasko · 2021-06-18T15:20:23Z

library/std/src/thread/local/tests.rs

+            match SYNC_STATE.compare_exchange_weak(
+                THREAD1_WAITING,
+                MAIN_THREAD_RENDEZVOUS,
+                Ordering::SeqCst,
+                Ordering::SeqCst,
+            ) {
+                Ok(_) => break,
+                Err(FRESH) => thread::yield_now(),
+                Err(THREAD2_LAUNCHED) => thread::yield_now(),
+                Err(THREAD2_JOINED) => {
+                    panic!("Main thread rendezvous after thread 2 joined thread 1")
+                }
+                v => unreachable!("sync state: {:?}", v),
+            }


compare_exchange_weak allows spurious failures. If the current value is THREAD1_WAITING but operation fails, the code will enter last unreachable branch and panic.

We observed this failure in #89584 on armhf Debian, I have started a re-try of the build to see if the error goes away. But yes it should be fixed so it never occurs.

This was already fixed in #86383 so you must've observed a different failure.

rust-highfive assigned dtolnay Apr 21, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 21, 2021

jethrogb reviewed Apr 21, 2021

View reviewed changes

library/std/src/sys/sgx/thread.rs Outdated Show resolved Hide resolved

Ensure TLS destructors run before thread joins in SGX

5d9eeff

mzohreva force-pushed the mz/tls-dtors-before-join branch from 950ea0d to 5d9eeff Compare April 21, 2021 21:47

jethrogb approved these changes Apr 22, 2021

View reviewed changes

dtolnay approved these changes Apr 22, 2021

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 Apr 22, 2021

jethrogb mentioned this pull request Apr 23, 2021

std: Add a variant of thread locals with const init #83416

Merged

Dylan-DPC-zz mentioned this pull request Apr 23, 2021

Rollup of 13 pull requests #84477

Closed

JohnTitor mentioned this pull request Apr 23, 2021

Rollup of 11 pull requests #84483

Closed

Dylan-DPC-zz mentioned this pull request Apr 24, 2021

Rollup of 7 pull requests #84509

Closed

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 Apr 24, 2021

Use atomics in join_orders_after_tls_destructors test

8a0a4b1

std::sync::mpsc uses thread locals and depending on the order TLS dtors are run `rx.recv()` can panic when used in a TLS dtor.

mzohreva force-pushed the mz/tls-dtors-before-join branch from 6af18d7 to 8a0a4b1 Compare April 29, 2021 15:51

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 Apr 30, 2021

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 May 6, 2021

This was referenced May 6, 2021

Rollup of 11 pull requests #85005

Closed

Rollup of 11 pull requests #85006

Closed

Dylan-DPC-zz mentioned this pull request May 6, 2021

Rollup of 11 pull requests #85014

Merged

bors merged commit b30e428 into rust-lang:master May 7, 2021

rustbot added this to the 1.54.0 milestone May 7, 2021

mzohreva deleted the mz/tls-dtors-before-join branch May 7, 2021 04:00

apiraino added beta-nominated Nominated for backporting to the compiler in the beta channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 7, 2021

rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label May 20, 2021

Mark-Simulacrum modified the milestones: 1.54.0, 1.53.0 May 22, 2021

Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label May 22, 2021

Mark-Simulacrum mentioned this pull request May 22, 2021

[beta] backports #85588

Merged

tmiasko reviewed Jun 18, 2021

View reviewed changes

infinity0 mentioned this pull request Oct 6, 2021

1.53.0 test join_orders_after_tls_destructors fails on armhf (armv7-unknown-linux-gnueabihf) Debian #89584

Closed

jethrogb mentioned this pull request Jun 7, 2022

Miri hangs in spin loops forever rust-lang/miri#1388

Closed

Ensure TLS destructors run before thread joins in SGX #84409

Ensure TLS destructors run before thread joins in SGX #84409

Uh oh!

Conversation

mzohreva commented Apr 21, 2021

Uh oh!

rust-highfive commented Apr 21, 2021

Uh oh!

Uh oh!

jethrogb left a comment

Choose a reason for hiding this comment

Uh oh!

dtolnay left a comment

Choose a reason for hiding this comment

Uh oh!

dtolnay commented Apr 22, 2021

Uh oh!

bors commented Apr 22, 2021

Uh oh!

Dylan-DPC-zz commented Apr 24, 2021

Uh oh!

mzohreva commented Apr 30, 2021

Uh oh!

jethrogb commented May 4, 2021

Uh oh!

mzohreva commented May 6, 2021

Uh oh!

jethrogb commented May 6, 2021

Uh oh!

bors commented May 6, 2021

Uh oh!

jethrogb commented May 7, 2021

Uh oh!

rustbot commented May 7, 2021

Uh oh!

apiraino commented May 7, 2021

Uh oh!

apiraino commented May 20, 2021

Uh oh!

tmiasko Jun 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

infinity0 Oct 6, 2021

Choose a reason for hiding this comment

Uh oh!

jethrogb Oct 6, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tmiasko Jun 18, 2021 •

edited

Loading