Forbid inlining thread_local!'s __getit function on Windows

[thomcc]

Sadly, this will make things slower to avoid UB in an edge case, but it seems hard to avoid... and really whenever I look at this code I can't help but think we're asking for trouble.

It's pretty dodgy for us to leave this as a normal function rather than #[inline(never)], given that if it does get inlined into a dynamically linked component, it's extremely unsafe (you get some other thread local, or if you're lucky, crash). Given that it's pretty rare for people to use dylibs on Windows, the fact that we haven't gotten bug reports about it isn't really that convincing. Ideally we'd come up with some kind of compiler solution (that avoids paying for this cost when static linking, or at least for use within the same crate...), but it's not clear what that looks like.

Oh, and because all this is only needed when we're implementing thread_local! with #[thread_local], this patch adjusts the cfg_attr to be all(windows, target_thread_local) as well.

r? @ChrisDenton

See also #84933, which is about improving the situation.

[nikic]

Just to clarify my understanding of the issue here: #[inline] has two effects. One is that it makes rustc materialize the function when used in other crates. And the second one is to control whether inlining is legal/recommended on the LLVM side. Do I understand correctly that the issue here is purely with the first part? That is, it would be fine to make this always #[inline], so long as it does not get materialized across crate-boundaries? I assume this is also why this has worked fine with just the absence of #[inline] rather than #[inline(never)], as inlining itself is not the issue here.

[nikic]

Or in other words, if we make generates_cgu_internal_copy() return false for function that reference #[thread_local] on Windows, would that fix the issue even if #[inline] is present?

[ChrisDenton]

Just to be clear, it's not crate boundaries that are the issue. It's dll/exe boundaries. Ideally it'd be great if it could inline across crate boundaries if that doesn't also cause it to jump across dll boundaries.

[thomcc]

@nikic The issue is that on Windows, if the instantiation of the #[thread_local] ends up in a different DLL from a function that references it, then it's UB (and will crash or worse).

This happens in practice if we have #[inline], so we don't use #[inline] on that function on Windows. The issue is that the compiler is completely allowed to inline functions even if they don't have #[inline] on them.

My understanding is that it's completely possible for this to happen even across a crate_type = "dylib" boundary, and preventing such would be difficult to do in practice in a way that correctly handles transitive inlining and such (for example, even inlining __getit into functions in the same crate could cause problems if those functions are #[inline] or happen to get inlined anyway).

That said, I don't know that anybody has seen it happen. @ChrisDenton indicated that they may have seen it once, but weren't sure. I feel like there are enough configuration options here (including things like -Zshare-generics, which seem like they'd make it more likely that something ends up inlined across a crate boundary) that it feels like the only reason we might not have seen it is that crate_type = "dylib"s are rarely used on Window.

However, if you know a reason that I'm wrong and the inlining can't happen without #[inline], or a better way to prevent this, then I'd be thrilled!

[ChrisDenton]

I'm going to cc a few people just in case they want to comment on the compiler/dylibs situation with Windows #[thread_local]. Not totally sure who the most relevant people are here so sorry for the ping if it's not you.

cc @bjorn3 @eddyb @michaelwoerister

[thomcc]

Sounds good, there's no rush here, it's mostly a PR made out of caution, and it would be way better to solve this in a better way (it just seems like if we're going to rely on it not being inlined for correctness we shouldn't mark it as something which is allowed to be inlined).

[eddyb]

If the fix is from going between "no #[inline] of any kind" and #[inline(never)], I don't really understand how that happens.

Is it "per-CGU copies" while building a single crate (and was that what @nikic was referencing above)? Is it from LTO?

Normally a non-(type/const)-generic function doesn't get cross-crate codegen without an #[inline] attribute and it sounds like LTO wasn't involved, so it has to be the "per-CGU copy" thing?

If that is the case, it's probably safe to claim that people working on these parts of the stdlib weren't aware that such duplication existed without an #[inline] opt-in, right?

[Subsentient]

I just hit this issue on x86_64 Windows on nightly. Please merge this fix, it would be helpful. Thanks!

[thomcc]

@eddyb I think that's right, yes.

[ChrisDenton]

@Subsentient would you be able to give more details? It would be super helpful if you're able. Also did you test if this PR does address the problem you're seeing? No worries if not but it'd be good to confirm.

Btw, I am minded to merge this as an interim fix but I would really love for the underlying problem to be addressed somehow.

[Subsentient]

@Subsentient would you be able to give more details? It would be super helpful if you're able. Also did you test if this PR does address the problem you're seeing? No worries if not but it'd be good to confirm.

Btw, I am minded to merge this as an interim fix but I would really love for the underlying problem to be addressed somehow.

So I'm hitting this in a performance library specific to my company. It is happening in __getit().
It's compiled as both a static rlib for the main executable, and as a cdylib for use in C and C++-based dependency DLLs, as it exposes a C API. Both are frequently loaded at once in the same process.

I can't post source, it's legally not my code, it belongs to the company, but I can post a screenshot of a backtrace.

Yes, I would appreciate a merge. It's obviously not a great fix, but if it fixes the functionality, that will be very useful.
Thanks!

[ChrisDenton]

Thank you! As I say, I do think this needs investigating more but this PR should fix it while that happens.

@bors r+

[bors]

📌 Commit 4062925df2350e8c51eb59f5d6dd157d38f79274 has been approved by ChrisDenton

It is now in the queue for this repository.

[klensy]

Revert last commit?

[ChrisDenton]

Oh oops.

@bors r-

[thomcc]

Crap, totally forgot to revert that.

[rustbot]

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

[thomcc]

@bors r=ChrisDenton

[bors]

📌 Commit 3099dfd has been approved by ChrisDenton

It is now in the queue for this repository.

[ehuss]

@thomcc This PR seems to have caused a regression on x86_64-pc-windows-gnu (see #104852), which is affecting Cargo's testsuite.

[thomcc]

Wow, that's pretty spooky since it definitely shouldn't have an impact like that. That's definitely a compiler bug, but I'll put up a revert anyway...