Ensure TLS accesses don't call the global allocator through panic (part 2)

[maxdexh]

View all comments

In #160934, I missed some calls that are reachable from LocalKey::with on platforms that use the racy LazyKey implementation.

How it didn't occur to me that the other functions (besides get and set) are reachable is beyond me ^^
I guess this is what I get for going through call graphs of disabled code by hand.

Yet another argument for function coloring.

r? nia-e

[maxdexh]

There are still some code paths that can panic...

[maxdexh]

It wasn't that bad, most of the panicking code in the modules (which are way too spread apart) was in the destructor code, which can panic anyway and isn't called on access

[nia-e]

grepping for unwrap and assert in sys/thread_local also brought up:

// unwrap
os.rs106:let layout = Layout::new::<Value<T>>().align_to(ALIGN).unwrap();142:let layout = Layout::new::<Value<T>>().align_to(ALIGN).unwrap();
key/xous.rs191:.unwrap()
key/windows.rs191:let dtor = unsafe{(*cur).dtor.unwrap()};// assert
guard/key.rs53: debug_assert_eq!(state,RUN);
key/xous.rs111: assert!((*val).is_null());

are these not also possible violations of our guarantee? ^^

[maxdexh]

As long as they can't happen, no. But I'll take a look at them

[maxdexh]

Note that there are also uses of indexing and addition, but to me they didn't look like they can go wrong. Maybe we should replace them anyway?

[maxdexh]

Thinking about it, we probably should. They are essentially unchecked operations without safety comments. I'll get back to it...

[maxdexh]

@nia-e is there a way to test the build against all targets? I would like to slap all the panic-related deny attributes we have on the code

[nia-e]

i think the only way unless you have a lot of cross-compilation set up locally is to do a bors try job with a bunch of architectures ^^ you should(?) have perms for that

[maxdexh]

Uh I'll try (haha get it)

[maxdexh]

I think this is it?
I'm just gonna try a bors command from one of the rollups, pretty sure I don't have the perms though.
I also couldn't find a list of jobs anywhere so I'll just try a random one I saw on a rollup
@bors try jobs=test-various

Edit: Welp

[rust-bors]

@maxdexh: 🔑 Insufficient privileges: not in try users

[maxdexh]

Okay at least some targets are in the CI ^^

[maxdexh]

test-various was just a random job i picked up from a rollup PR, idk what that job even does

[maxdexh]

Yeah test-various doesn't test any exotic platforms from what i can see in the job details

[maxdexh]

For some reason ./x clippy library/std doesn't limit clippy to just std for me.
And when I pass --target, it doesn't change the target...
That makes it impossible for me to lint on all platforms I changed here...

[rust-bors]

☀️ Try build successful (CI)
Build commit: 995aa02 (995aa02cd857529ed7195ac264ddc51c01c6afd3)
Base parent: c98d0cb (c98d0cb27cc63afdd62602a52eb4feb8a1c682dd)

[nia-e]

this looks a lot better ^^ even if it's not everything, it's def meaningfully reducing the potential unwind surface. ty!

@bors r+ rollup=iffy

[rust-bors]

📌 Commit 5b71324 has been approved by nia-e

It is now in the queue for this repository.

[maxdexh]

Yeah I don't feel comfortable closing the issue until we have a way to auto detect such code

[rust-timer]

Finished benchmarking commit (004192f): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never rustc-perf
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.1%, 0.8%]	6
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-5.5%	[-8.9%, -1.5%]	4
All ❌✅ (primary)	0.4%	[0.1%, 0.8%]	6

Max RSS (memory usage)

Results (primary -1.6%)

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

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.6%	[-1.6%, -1.6%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.6%	[-1.6%, -1.6%]	1

Cycles

Results (primary -2.0%, secondary -3.7%)

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

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.5%	[2.5%, 2.5%]	1
Improvements ✅ (primary)	-2.0%	[-2.1%, -1.8%]	2
Improvements ✅ (secondary)	-5.2%	[-8.4%, -2.5%]	4
All ❌✅ (primary)	-2.0%	[-2.1%, -1.8%]	2

Binary size

Results (primary 0.2%, secondary -7.4%)

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

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.1%, 0.7%]	16
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-0.2%, -0.0%]	5
Improvements ✅ (secondary)	-7.4%	[-8.8%, -6.9%]	4
All ❌✅ (primary)	0.2%	[-0.2%, 0.7%]	21

Bootstrap: 455.642s -> 457.256s (0.35%)
Artifact size: 396.40 MiB -> 396.37 MiB (-0.01%)

[Kobzol]

This caused a small regression, but also a large win on one stress test. Overall, it is mostly neutral.

@rustbot label: +perf-regression-triaged