Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Never inline Windows dtor access #100007

Merged
merged 1 commit into from
Aug 16, 2022
+4 −0
Merged

Never inline Windows dtor access #100007

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions library/std/src/sys/windows/thread_local_dtor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,14 @@
#[thread_local]
static mut DESTRUCTORS: Vec<(*mut u8, unsafe extern "C" fn(*mut u8))> = Vec::new();

// Ensure this can never be inlined because otherwise this may break in dylibs.
// See #44391.
#[inline(never)]
pub unsafe fn register_dtor(t: *mut u8, dtor: unsafe extern "C" fn(*mut u8)) {
DESTRUCTORS.push((t, dtor));
Comment on lines 8 to 15
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @michaelwoerister @bjorn3 Could we make it so that on Windows you cannot use #[thread_local] from a function that may end up unable to actually reference that thread-local? See also linked issue:

(Though I have no idea what's happening here, since it doesn't change when/where these functions are codegenned, they're not generic and didn't have #[inline(always)] on it).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Though I have no idea what's happening here, since it doesn't change when/where these functions are codegenned, they're not generic and didn't have #[inline(always)] on it).

Not sure either.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, the context for this PR is #94820 where I was seeing local test failures related to dtors (but this didn't seem to affect CI). However, I'm not able to reproduce it now and unfortunately back then I was rather lax in my documentation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any chance LTO or ThinLTO was enabled in a way that could result in some of these definitions crossing between LLVM modules?

Alternatively, if you move the functions into different modules than the #[thread_local] static, could you sometimes end up with them being in separate CGUs and somehow that being enough to cause an issue?

Sadly that's also still not enough because of the whole "per-DLL/EXE" aspect.

It might be something like ThinLTO'd librustc_driver-*.so against libstd-*.so but the libstd-*.sos I'm seeing in nightly don't seem to have any LLVM bitcode sections so it can't be that.

(A custom build of libstd-*.so might still have LLVM bitcode - also just noticed I'm looking at Linux .sos not Windows .dlls, but I doubt that would affect whether we put LLVM bitcode in them?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, in general, ThinLTO can cause small functions like this one to be inlined into other modules. I haven't looked at this specific case though.

}

#[inline(never)] // See comment above
/// Runs destructors. This should not be called until thread exit.
pub unsafe fn run_keyless_dtors() {
// Drop all the destructors.
Expand Down