Panic when the global allocator tries to register a TLS destructor

[joboet]

Using a RefCell avoids the undefined behaviour encountered in #116390 and reduces the amount of unsafe code in the codebase.

[rustbot]

r? @cuviper

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

[nvzqz]

I think an ideal long term solution would be to make register_dtor reentrant, so that global allocators can register TLS destructors.

That said, I think this is a good step in the right direction.

[thomcc]

Looks good to me. If someone wants to fix this to make it actually reentrant I think we'd take that, but no reason to block a soundness fix on that.

@bors r+

[bors]

📌 Commit b18990b has been approved by thomcc

It is now in the queue for this repository.

[matthiaskrgr]

@bors r-
panics during tests now 😅
#116441 (comment)

[joboet]

I don't have access to a Windows machine, but I'm pretty sure that the test failed because the RefMut lived too long. This should be fixed now.

[workingjubilee]

@bors r=thomcc rollup=iffy

[bors]

📌 Commit 7953b56 has been approved by thomcc

It is now in the queue for this repository.

[bors]

⌛ Testing commit 7953b56 with merge a64a576...

[workingjubilee]

???????????????????

[workingjubilee]

I saw this error in a seemingly unrelated PR so I'm going to see what happens with
@bors retry

[bors]

⌛ Testing commit 88efb1b with merge 982b101...

[rust-log-analyzer]

The job x86_64-msvc-ext failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

tests\ui-toml\undocumented_unsafe_blocks\undocumented_unsafe_blocks.rs (default) ... ok
tests\ui-toml\undocumented_unsafe_blocks\undocumented_unsafe_blocks.rs (disabled) ... ok

tests\ui-toml\doc_valid_idents_replace\doc_markdown.rs FAILED:
command: "C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage2-tools\\x86_64-pc-windows-msvc\\release\\clippy-driver.exe" "--error-format=json" "--emit=metadata" "-Aunused" "-Ainternal_features" "-Zui-testing" "-Dwarnings" "-Ldependency=C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage2-tools\\x86_64-pc-windows-msvc\\release\\deps" "--extern=clippy_lints=\\\\?\\C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage2-tools\\x86_64-pc-windows-msvc\\release\\deps\\libclippy_lints-5e8d785e054edb28.rlib" "--extern=clippy_utils=\\\\?\\C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage2-tools\\x86_64-pc-windows-msvc\\release\\deps\\libclippy_utils-0cf228b0c1780fd5.rlib" "--extern=futures=\\\\?\\C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage2-tools\\x86_64-pc-windows-msvc\\release\\deps\\libfutures-a074388a1bf2e009.rlib" "--extern=if_chain=\\\\?\\C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage2-tools\\x86_64-pc-windows-msvc\\release\\deps\\libif_chain-ba0253edeba7a8f0.rlib" "--extern=itertools=\\\\?\\C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage2-tools\\x86_64-pc-windows-msvc\\release\\deps\\libitertools-6894b4f3e8c5523f.rlib" "--extern=parking_lot=\\\\?\\C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage2-tools\\x86_64-pc-windows-msvc\\release\\deps\\libparking_lot-f87e1900e520ff9b.rlib" "--extern=quote=\\\\?\\C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage2-tools\\x86_64-pc-windows-msvc\\release\\deps\\libquote-3ebce4a1d5d43cf1.rlib" "--extern=regex=\\\\?\\C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage2-tools\\x86_64-pc-windows-msvc\\release\\deps\\libregex-3e46ad2dfdd0c657.rlib" "--extern=serde=\\\\?\\C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage2-tools\\x86_64-pc-windows-msvc\\release\\deps\\libserde-12500b7f802b2beb.rlib" "--extern=serde_derive=\\\\?\\C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage2-tools\\release\\deps\\serde_derive-67ed154847593e2b.dll" "--extern=syn=\\\\?\\C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage2-tools\\x86_64-pc-windows-msvc\\release\\deps\\libsyn-bc3ce53f417d339c.rlib" "--extern=tokio=\\\\?\\C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage2-tools\\x86_64-pc-windows-msvc\\release\\deps\\libtokio-159ee9fbda356223.rlib" "-Ldependency=C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage2-tools\\release\\deps" "--out-dir" "\\\\?\\C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage2-tools\\ui_test" "tests\\ui-toml\\doc_valid_idents_replace\\doc_markdown.fixed" "--edition" "2021" "--crate-name" "doc_markdown"
called `Result::unwrap()` on an `Err` value: tests failed

Location:
    C:\Users\runneradmin\.cargo\registry\src\index.crates.io-6f17d22bba15001f\ui_test-0.20.0\src\lib.rs:362:13
    C:\Users\runneradmin\.cargo\registry\src\index.crates.io-6f17d22bba15001f\ui_test-0.20.0\src\lib.rs:362:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

rustfix failed with exit code: 1

full stderr:
error: failed to write \\?\C:\a\rust\rust\build\x86_64-pc-windows-msvc\stage2-tools\ui_test\libdoc_markdown.rmeta: Access is denied. (os error 5)
error: aborting due to previous error


full stdout:

[bors]

💔 Test failed - checks-actions

[workingjubilee]

Different error so it might be real this time?

[joboet]

The first one was just another case of #116759, and the second one also seems to be spurious, I haven't touched anything that could lead to write failures. Sorry for taking up so much CI time, but could we try again?

[cuviper]

🍀
@bors retry

[bors]

⌛ Testing commit 88efb1b with merge 89432aa...

[bors]

☀️ Test successful - checks-actions
Approved by: thomcc,workingjubilee
Pushing 89432aa to master...

[rust-timer]

Finished benchmarking commit (89432aa): comparison URL.

Overall result: ✅ 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
Improvements ✅ (primary)	-0.7%	[-0.7%, -0.7%]	2
Improvements ✅ (secondary)	-0.4%	[-0.7%, -0.2%]	14
All ❌✅ (primary)	-0.7%	[-0.7%, -0.7%]	2

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 628.752s -> 628.777s (0.00%)
Artifact size: 304.01 MiB -> 304.01 MiB (-0.00%)

[RalfJung]

Is there any way to make a testcase for this? This changes a bunch of subtle code and most of it without comments, so I am concerned someone might in the future try to optimize this by removing the RefCell again.