[dianne]

following up on #160902 (review), this removes a fast path that was found in #163150 to have a negligible performance impact.

[dianne]

let's double-check that perf's still neutral, especially since this isn't exactly the same as what I'd tested prior. @bors try

[dianne]

I don't love this being duplicated from ignore_borrow, but it seemed kind of excessive to factor it out. either way, I feel it's more appropriate to allow ref-to-thread-locals here than to require callers to avoid them (as we do currently)

View changes since the review

[oli-obk]

yea I dug into this a bit and this seems fine

[rust-bors]

☀️ Try build successful (CI)
Build commit: 93a41fb (93a41fbae453648fec40acd090a70c4ff23ef7f1)
Base parent: ce37c4f (ce37c4ff837c5c5dc45cddf9e1e3704e42ca67ea)

[rust-timer]

Finished benchmarking commit (93a41fb): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (primary 0.0%, secondary 2.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	1.9%	[0.5%, 2.9%]	4
Regressions ❌ (secondary)	3.0%	[2.3%, 3.6%]	5
Improvements ✅ (primary)	-0.6%	[-1.1%, -0.4%]	12
Improvements ✅ (secondary)	-1.0%	[-1.0%, -1.0%]	1
All ❌✅ (primary)	0.0%	[-1.1%, 2.9%]	16

Cycles

Results (primary 0.1%, secondary -0.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.8%	[0.5%, 1.4%]	9
Regressions ❌ (secondary)	2.4%	[2.4%, 2.4%]	1
Improvements ✅ (primary)	-1.1%	[-2.2%, -0.6%]	5
Improvements ✅ (secondary)	-3.1%	[-3.1%, -3.1%]	1
All ❌✅ (primary)	0.1%	[-2.2%, 1.4%]	14

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 490.291s -> 490.201s (-0.02%)
Artifact size: 406.63 MiB -> 406.71 MiB (0.02%)

[dianne]

looks like perf's still clean! r? @oli-obk or anyone

[oli-obk]

thanks for following up on that.

@bors r+ rollup

View changes since this review

[rust-bors]

📌 Commit 558c1ec has been approved by oli-obk

It is now in the queue for this repository.

[veluca93]

FYI, this change seems to introduce a soundness regression: the following code compiles, but did not before this PR, and segfaults without any unsafe.

#![feature(thread_local)]

#[thread_local]
static TLS: [u8; 64 << 20] = [0; 64 << 20];

fn leak_tls() -> &'static [u8; 64 << 20] {
    &TLS
}

fn main() {
    let dangling = std::thread::spawn(leak_tls).join().unwrap();
    std::hint::black_box(dangling[0]);
}

This was found by a LLM scanner that @cramertj is prototyping -- unfortunately I only managed to confirm the regression just after bors merged the patch :(

[dianne]

@veluca93 oops, yep! thanks for the catch. now that I see it, it feels obvious. it's definitely something we should have a test for, so thanks for that as well ^^

on the plus side, I think this should only affect direct uses of #![feature(thread_local)], so as long as the standard library keeps using it soundly, it shouldn't affect stable releases.

still not great! I'll try and get a fix ready soon