Never inline Windows dtor access #100007

ChrisDenton · 2022-08-01T00:51:26Z

Inlining can cause problem If used in a Rust dylib. See #44391.

rustbot · 2022-08-01T00:51:29Z

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

ChrisDenton · 2022-08-01T00:55:10Z

Note that if I instead use inline(always) on register_dtor then the compiler reliably crashes:

Building stage1 std artifacts (x86_64-pc-windows-msvc -> x86_64-pc-windows-msvc)
   Compiling cc v1.0.69
   Compiling core v0.0.0 (R:\Rust\rust\library\core)
   Compiling libc v0.2.126
   Compiling memchr v2.5.0
   Compiling std v0.0.0 (R:\Rust\rust\library\std)
error: could not compile `core`

Caused by:
  process didn't exit successfully: `R:\Rust\rust\build\bootstrap\debug\rustc --crate-name core --edition=2021 library\core\src\lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --crate-type lib --emit=dep-info,metadata,link -C opt-level=3 -C embed-bitcode=no -C debuginfo=0 -Zunstable-options --check-cfg names() --check-cfg values() -C metadata=7a8ae34f67ccc69f -C extra-filename=-7a8ae34f67ccc69f --out-dir R:\Rust\rust\build\x86_64-pc-windows-msvc\stage1-std\x86_64-pc-windows-msvc\release\deps --target x86_64-pc-windows-msvc -C incremental=R:\Rust\rust\build\x86_64-pc-windows-msvc\stage1-std\x86_64-pc-windows-msvc\release\incremental -L dependency=R:\Rust\rust\build\x86_64-pc-windows-msvc\stage1-std\x86_64-pc-windows-msvc\release\deps -L dependency=R:\Rust\rust\build\x86_64-pc-windows-msvc\stage1-std\release\deps -Csymbol-mangling-version=legacy -Zunstable-options -Zunstable-options --check-cfg=values(bootstrap) --check-cfg=values(stdarch_intel_sde) --check-cfg=values(no_fp_fmt_parse) --check-cfg=values(no_global_oom_handling) --check-cfg=values(freebsd12) --check-cfg=values(backtrace_in_libstd) "--check-cfg=values(target_env,\"libnx\")" "--check-cfg=values(target_os,\"watchos\")" "--check-cfg=values(target_arch,\"asmjs\",\"spirv\",\"nvptx\",\"nvptx64\",\"le32\",\"xtensa\")" --check-cfg=values(dont_compile_me) -Zmacro-backtrace -Ctarget-feature=+crt-static -Cprefer-dynamic -Cllvm-args=-import-instr-limit=10 -Cembed-bitcode=yes "-Zcrate-attr=doc(html_root_url=\"https://doc.rust-lang.org/nightly/\")" -Z binary-dep-depinfo` (exit code: 0xc0000005, STATUS_ACCESS_VIOLATION)
warning: build failed, waiting for other jobs to finish...
Build completed unsuccessfully in 0:05:38

eddyb · 2022-08-02T08:20:01Z

library/std/src/sys/windows/thread_local_dtor.rs

 #[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));


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:

#[thread_local] from windows DLLs gets duplicated by inlining. #44391

(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).

(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.

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.

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?)

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.

Mark-Simulacrum · 2022-08-06T17:49:15Z

r? @michaelwoerister

I'm basically happy to just r+ this, but it seems like it might be worth doing a little bit more digging here around potential causes. Maybe not, though, since the change is trivial.

michaelwoerister · 2022-08-09T08:47:54Z

OK, let's get it merged.

@bors r+

I won't have time to investigate further. Do we have enough info for opening an issue?

bors · 2022-08-09T08:47:56Z

📌 Commit 847f461 has been approved by michaelwoerister

It is now in the queue for this repository.

ChrisDenton · 2022-08-09T10:52:40Z

Does this not essentially have the same root cause as the linked issue (#44391)? Windows TLS access should not be jumping exe/dll boundaries under any circumstances.

See also our TLS macro which cannot be marked inline on Windows due to the behavior of dylibs.

bjorn3 · 2022-08-09T10:59:37Z

Does this not essentially have the same root cause as the linked issue (#44391)?

Sure, but the question is why it jumped a dll boundary in the first place. register_dtor shouldn't get inlined across a dll boundary.

ChrisDenton · 2022-08-09T11:11:30Z

Right but neither should any TLS access is what I'm saying. Solving the underlying problem would also solve this and all related issues.

I'm not at all against opening an issue for this specific instance of the problem, it's just that
I can no longer reproduce what prompted my original PR so I'm uncertain if it would be useful now.

…ichaelwoerister Never inline Windows dtor access Inlining can cause problem If used in a Rust dylib. See rust-lang#44391. r? `@Mark-Simulacrum`

bors · 2022-08-15T23:57:46Z

⌛ Testing commit 847f461 with merge 3694b7d...

bors · 2022-08-16T02:32:42Z

☀️ Test successful - checks-actions
Approved by: michaelwoerister
Pushing 3694b7d to master...

rust-timer · 2022-08-16T03:46:53Z

Finished benchmarking commit (3694b7d): comparison url.

Instruction count

Primary benchmarks: ❌ relevant regression found
Secondary benchmarks: no relevant changes found

( )	mean¹	max	count²
Regressions ❌ (primary)	0.8%	0.8%	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.8%	0.8%	1

Max RSS (memory usage)

Results

Primary benchmarks: ✅ relevant improvement found
Secondary benchmarks: mixed results

( )	mean¹	max	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.1%	4.1%	1
Improvements ✅ (primary)	-2.0%	-2.0%	1
Improvements ✅ (secondary)	-3.0%	-3.0%	1
All ❌✅ (primary)	-2.0%	-2.0%	1

Cycles

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

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

the arithmetic mean of the percent change ↩ ↩²
number of relevant changes ↩ ↩²

Rollup of 7 pull requests Successful merges: - rust-lang#92744 (Check if enum from foreign crate has any non exhaustive variants when attempting a cast) - rust-lang#99337 (rustdoc: simplify highlight.rs) - rust-lang#100007 (Never inline Windows dtor access) - rust-lang#100030 (cleanup code w/ pointers in std a little) - rust-lang#100192 ( Remove duplicated temporaries creating during box derefs elaboration) - rust-lang#100247 (Generalize trait object generic param check to aliases.) - rust-lang#100374 (Improve crate selection on rustdoc search results page) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

kohanis · 2022-09-19T21:19:04Z

This change increased minimal size of msvc empty cdylib dll from 10KB to 120KB, bringing in panicking, fmt and demangle with no user code at all. Because now Vec<>.push() call actually always exists, I suppose

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Aug 1, 2022

rust-highfive assigned Mark-Simulacrum Aug 1, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 1, 2022

ChrisDenton mentioned this pull request Aug 1, 2022

Move Windows keyless TLS dtor into TLS callback module #94820

Closed

Never inline Windows dtor access

847f461

ChrisDenton force-pushed the dtor-inline-never branch from 88d6658 to 847f461 Compare August 1, 2022 02:53

eddyb reviewed Aug 2, 2022

View reviewed changes

rust-highfive assigned michaelwoerister and unassigned Mark-Simulacrum Aug 6, 2022

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 9, 2022

Dylan-DPC mentioned this pull request Aug 11, 2022

Rollup of 7 pull requests #100395

Merged

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 16, 2022

bors merged commit 3694b7d into rust-lang:master Aug 16, 2022

rustbot added this to the 1.65.0 milestone Aug 16, 2022

ChrisDenton deleted the dtor-inline-never branch August 16, 2022 03:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Never inline Windows dtor access #100007

Never inline Windows dtor access #100007

ChrisDenton commented Aug 1, 2022

rustbot commented Aug 1, 2022

ChrisDenton commented Aug 1, 2022

eddyb Aug 2, 2022

bjorn3 Aug 2, 2022

ChrisDenton Aug 2, 2022

eddyb Aug 3, 2022

michaelwoerister Aug 3, 2022

Mark-Simulacrum commented Aug 6, 2022

michaelwoerister commented Aug 9, 2022

bors commented Aug 9, 2022

ChrisDenton commented Aug 9, 2022

bjorn3 commented Aug 9, 2022

ChrisDenton commented Aug 9, 2022

bors commented Aug 15, 2022

bors commented Aug 16, 2022

rust-timer commented Aug 16, 2022

kohanis commented Sep 19, 2022 •

edited

Loading

Never inline Windows dtor access #100007

Never inline Windows dtor access #100007

Conversation

ChrisDenton commented Aug 1, 2022

rustbot commented Aug 1, 2022

ChrisDenton commented Aug 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mark-Simulacrum commented Aug 6, 2022

michaelwoerister commented Aug 9, 2022

bors commented Aug 9, 2022

ChrisDenton commented Aug 9, 2022

bjorn3 commented Aug 9, 2022

ChrisDenton commented Aug 9, 2022

bors commented Aug 15, 2022

bors commented Aug 16, 2022

rust-timer commented Aug 16, 2022

Footnotes

kohanis commented Sep 19, 2022 • edited Loading

kohanis commented Sep 19, 2022 •

edited

Loading