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.

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

Remove cgu_reuse_tracker from Session #115964

Merged
merged 5 commits into from
Oct 13, 2023

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Sep 19, 2023

This removes a bit of global mutable state.

It will now miss post-lto cgu reuse when ThinLTO determines that a cgu doesn't get changed, but there weren't any tests for this anyway and a test for it would be fragile to the exact implementation of ThinLTO in LLVM.

@rustbot
Copy link
Collaborator

rustbot commented Sep 19, 2023

r? @compiler-errors

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 19, 2023
@rustbot
Copy link
Collaborator

rustbot commented Sep 19, 2023

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@bors
Copy link
Contributor

bors commented Sep 29, 2023

☔ The latest upstream changes (presumably #113301) made this pull request unmergeable. Please resolve the merge conflicts.

@bjorn3 bjorn3 force-pushed the cgu_reuse_tracker_global_state branch from 6a2eb57 to 43ac2cf Compare September 30, 2023 13:10
@bjorn3
Copy link
Member Author

bjorn3 commented Sep 30, 2023

Rebased

compiler/rustc_codegen_ssa/src/assert_module_sources.rs Outdated Show resolved Hide resolved
)
.0
}
CguReuse::PreLto => unreachable!(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a message to the unreachable? That's not immediate that it comes from determine_cgu_reuse never emitting that variant.

Copy link
Member Author

Choose a reason for hiding this comment

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

determine_cgu_reuse can emit PreLto, but cg_clif currently doesn't support LTO and without LTO only No and PostLto are emitted by determine_cgu_reuse.

@@ -727,7 +734,6 @@ pub fn codegen_crate<B: ExtraBackendMethods>(
ongoing_codegen.check_for_errors(tcx.sess);

let cgu_reuse = cgu_reuse[i];
tcx.sess.cgu_reuse_tracker.set_actual_reuse(cgu.name().as_str(), cgu_reuse);

match cgu_reuse {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is PreLto reachable in this match? cgu_reuse comes from determine_cgu_reuse too, doesn't it?

@rustbot
Copy link
Collaborator

rustbot commented Oct 5, 2023

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@bors
Copy link
Contributor

bors commented Oct 9, 2023

☔ The latest upstream changes (presumably #116569) made this pull request unmergeable. Please resolve the merge conflicts.

@cjgillot
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Oct 12, 2023

📌 Commit f0b5820 has been approved by cjgillot

It is now in the queue for this repository.

@bors 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 Oct 12, 2023
@bors
Copy link
Contributor

bors commented Oct 13, 2023

⌛ Testing commit f0b5820 with merge 130ff8c...

@bors
Copy link
Contributor

bors commented Oct 13, 2023

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 130ff8c to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 13, 2023
@bors bors merged commit 130ff8c into rust-lang:master Oct 13, 2023
11 checks passed
@rustbot rustbot added this to the 1.75.0 milestone Oct 13, 2023
This was referenced Oct 13, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (130ff8c): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

Binary size

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

Bootstrap: 628.455s -> 628.344s (-0.02%)
Artifact size: 271.30 MiB -> 271.27 MiB (-0.01%)

bors-ferrocene bot added a commit to ferrocene/ferrocene that referenced this pull request Oct 13, 2023
50: Automated pull from upstream `master` r=Dajamante a=github-actions[bot]


This PR pulls the following changes from the upstream repository:

* rust-lang/rust#116619
* rust-lang/rust#115964
* rust-lang/rust#116391
* rust-lang/rust#116510
* rust-lang/rust#116671
  * rust-lang/rust#116669
  * rust-lang/rust#116654
  * rust-lang/rust#116642
  * rust-lang/rust#116625
  * rust-lang/rust#116593
* rust-lang/rust#116649
* rust-lang/rust#116600
* rust-lang/rust#116628



Co-authored-by: Nadrieril <nadrieril+git@gmail.com>
Co-authored-by: Scott McMurray <scottmcm@users.noreply.github.com>
Co-authored-by: bjorn3 <17426603+bjorn3@users.noreply.github.com>
Co-authored-by: Nicholas Nethercote <n.nethercote@gmail.com>
Co-authored-by: Trevor Gross <tmgross@umich.edu>
Co-authored-by: Georg Semmler <github@weiznich.de>
Co-authored-by: Guillaume Gomez <guillaume.gomez@huawei.com>
Co-authored-by: Gurinder Singh <frederick.the.fool@gmail.com>
Co-authored-by: bors <bors@rust-lang.org>
@matthiaskrgr
Copy link
Member

fwiw this causes an ice now when manually compiling some of the incremental tests without passing -Zquery-dep-graph.
The test have this set as compiler flag which is why this did not fail in ci, but previously, there would be n crash when not passing -Zquery-dep-graph

~/.rustup/toolchains/master/bin/rustc -Zincremental-verify-ich=yes -Cincremental=/tmp -Cdebuginfo=2 /home/gh-matthiaskrgr/vcs/github/rust_icemaker/tests/incremental/split_debuginfo_mode.rs --crate-type lib

warning: the feature `rustc_attrs` is internal to the compiler or standard library
  --> /home/gh-matthiaskrgr/vcs/github/rust_icemaker/tests/incremental/split_debuginfo_mode.rs:14:12
   |
14 | #![feature(rustc_attrs)]
   |            ^^^^^^^^^^^
   |
   = note: using it is strongly discouraged
   = note: `#[warn(internal_features)]` on by default

error: found CGU-reuse attribute but `-Zquery-dep-graph` was not specified
  --> /home/gh-matthiaskrgr/vcs/github/rust_icemaker/tests/incremental/split_debuginfo_mode.rs:16:1
   |
16 | #![rustc_partition_codegened(module = "split_debuginfo_mode", cfg = "rpass2")]
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

thread 'coordinator' panicked at /rustc/34bc5716b539272b193fdd2a549a28c60783cf5d/compiler/rustc_codegen_ssa/src/back/write.rs:1353:29:
Could not send CguMessage to main thread
stack backtrace:
   0:     0x7f8b26a388fc - std::backtrace_rs::backtrace::libunwind::trace::h873293d44a0f49a3
                               at /rustc/34bc5716b539272b193fdd2a549a28c60783cf5d/library/std/src/../../backtrace/src/backtrace/libunwind.rs:93:5
   1:     0x7f8b26a388fc - std::backtrace_rs::backtrace::trace_unsynchronized::hf9007960812a0c1b
                               at /rustc/34bc5716b539272b193fdd2a549a28c60783cf5d/library/std/src/../../backtrace/src/backtrace/mod.rs:66:5
   2:     0x7f8b26a388fc - std::sys_common::backtrace::_print_fmt::h8b81515c90e97120
                               at /rustc/34bc5716b539272b193fdd2a549a28c60783cf5d/library/std/src/sys_common/backtrace.rs:67:5
   3:     0x7f8b26a388fc - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::h6778754e680c2464
                               at /rustc/34bc5716b539272b193fdd2a549a28c60783cf5d/library/std/src/sys_common/backtrace.rs:44:22
   4:     0x7f8b26aa0350 - core::fmt::rt::Argument::fmt::hc7aef32ad4bd631c
                               at /rustc/34bc5716b539272b193fdd2a549a28c60783cf5d/library/core/src/fmt/rt.rs:142:9
   5:     0x7f8b26aa0350 - core::fmt::write::ha11263d44bdb427f
                               at /rustc/34bc5716b539272b193fdd2a549a28c60783cf5d/library/core/src/fmt/mod.rs:1117:17
   6:     0x7f8b26a2bc4f - std::io::Write::write_fmt::he1435c00ac758d60
                               at /rustc/34bc5716b539272b193fdd2a549a28c60783cf5d/library/std/src/io/mod.rs:1762:15
   7:     0x7f8b26a386e4 - std::sys_common::backtrace::_print::h0e4edb3230340495
                               at /rustc/34bc5716b539272b193fdd2a549a28c60783cf5d/library/std/src/sys_common/backtrace.rs:47:5
   8:     0x7f8b26a386e4 - std::sys_common::backtrace::print::h6ca906ffd474b926
                               at /rustc/34bc5716b539272b193fdd2a549a28c60783cf5d/library/std/src/sys_common/backtrace.rs:34:9
   9:     0x7f8b26a3b4f7 - std::panicking::default_hook::{{closure}}::h12912d17e1aa4dda
  10:     0x7f8b26a3b23a - std::panicking::default_hook::hc8f6d1561e436dcc
                               at /rustc/34bc5716b539272b193fdd2a549a28c60783cf5d/library/std/src/panicking.rs:292:9
  11:     0x7f8b295eceba - std[b03cbf7d5f7aa02e]::panicking::update_hook::<alloc[29de4bc0869589ad]::boxed::Box<rustc_driver_impl[6b35cd9df5076c5b]::install_ice_hook::{closure#0}>>::{closure#0}
  12:     0x7f8b26a3bc98 - <alloc::boxed::Box<F,A> as core::ops::function::Fn<Args>>::call::hdf04a5eb748e7999
                               at /rustc/34bc5716b539272b193fdd2a549a28c60783cf5d/library/alloc/src/boxed.rs:2021:9
  13:     0x7f8b26a3bc98 - std::panicking::rust_panic_with_hook::h348fd0948ee269e5
                               at /rustc/34bc5716b539272b193fdd2a549a28c60783cf5d/library/std/src/panicking.rs:735:13
  14:     0x7f8b26a3b9a6 - std::panicking::begin_panic_handler::{{closure}}::hc7895c95e3cbd65f
                               at /rustc/34bc5716b539272b193fdd2a549a28c60783cf5d/library/std/src/panicking.rs:601:13
  15:     0x7f8b26a38e16 - std::sys_common::backtrace::__rust_end_short_backtrace::h275cc4647f2930f9
                               at /rustc/34bc5716b539272b193fdd2a549a28c60783cf5d/library/std/src/sys_common/backtrace.rs:170:18
  16:     0x7f8b26a3b722 - rust_begin_unwind
                               at /rustc/34bc5716b539272b193fdd2a549a28c60783cf5d/library/std/src/panicking.rs:597:5
  17:     0x7f8b26a9c975 - core::panicking::panic_fmt::hc8e7081edeea1c25
                               at /rustc/34bc5716b539272b193fdd2a549a28c60783cf5d/library/core/src/panicking.rs:72:14
  18:     0x7f8b28ef82fd - rustc_codegen_ssa[123f0f5ddc78276b]::back::write::start_executing_work::<rustc_codegen_llvm[49631b38d636bbd1]::LlvmCodegenBackend>::{closure#5}
  19:     0x7f8b28ef5200 - std[b03cbf7d5f7aa02e]::sys_common::backtrace::__rust_begin_short_backtrace::<<rustc_codegen_llvm[49631b38d636bbd1]::LlvmCodegenBackend as rustc_codegen_ssa[123f0f5ddc78276b]::traits::backend::ExtraBackendMethods>::spawn_named_thread<rustc_codegen_ssa[123f0f5ddc78276b]::back::write::start_executing_work<rustc_codegen_llvm[49631b38d636bbd1]::LlvmCodegenBackend>::{closure#5}, core[9ce9627cc2ac9ccf]::result::Result<rustc_codegen_ssa[123f0f5ddc78276b]::back::write::CompiledModules, ()>>::{closure#0}, core[9ce9627cc2ac9ccf]::result::Result<rustc_codegen_ssa[123f0f5ddc78276b]::back::write::CompiledModules, ()>>
  20:     0x7f8b28ef4fd0 - <<std[b03cbf7d5f7aa02e]::thread::Builder>::spawn_unchecked_<<rustc_codegen_llvm[49631b38d636bbd1]::LlvmCodegenBackend as rustc_codegen_ssa[123f0f5ddc78276b]::traits::backend::ExtraBackendMethods>::spawn_named_thread<rustc_codegen_ssa[123f0f5ddc78276b]::back::write::start_executing_work<rustc_codegen_llvm[49631b38d636bbd1]::LlvmCodegenBackend>::{closure#5}, core[9ce9627cc2ac9ccf]::result::Result<rustc_codegen_ssa[123f0f5ddc78276b]::back::write::CompiledModules, ()>>::{closure#0}, core[9ce9627cc2ac9ccf]::result::Result<rustc_codegen_ssa[123f0f5ddc78276b]::back::write::CompiledModules, ()>>::{closure#1} as core[9ce9627cc2ac9ccf]::ops::function::FnOnce<()>>::call_once::{shim:vtable#0}
  21:     0x7f8b26a46c05 - <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once::hbadcdf302d508572
                               at /rustc/34bc5716b539272b193fdd2a549a28c60783cf5d/library/alloc/src/boxed.rs:2007:9
  22:     0x7f8b26a46c05 - <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once::h6e5cf8eb148692cc
                               at /rustc/34bc5716b539272b193fdd2a549a28c60783cf5d/library/alloc/src/boxed.rs:2007:9
  23:     0x7f8b26a46c05 - std::sys::unix::thread::Thread::new::thread_start::h9b29230db5d14baa
                               at /rustc/34bc5716b539272b193fdd2a549a28c60783cf5d/library/std/src/sys/unix/thread.rs:108:17
  24:     0x7f8b26791ac3 - start_thread
                               at ./nptl/pthread_create.c:442:8
  25:     0x7f8b26823a40 - clone3
                               at ./misc/../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
  26:                0x0 - <unknown>

error: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/issues/new?labels=C-bug%2C+I-ICE%2C+T-compiler&template=ice.md

note: please attach the file at `/home/gh-matthiaskrgr/vcs/github/rust_icemaker/rustc-ice-2023-10-13T14:31:41.34381041Z-1068218.txt` to your bug report

note: compiler flags: -Z incremental-verify-ich=yes -C incremental=[REDACTED] -C debuginfo=2 --crate-type lib

query stack during panic:
end of query stack
error: aborting due to previous error; 1 warning emitted

@bjorn3 bjorn3 deleted the cgu_reuse_tracker_global_state branch October 14, 2023 12:08
@bjorn3
Copy link
Member Author

bjorn3 commented Oct 14, 2023

I think this is because assert_module_sources emits a fatal error, causing the main thread to exit, but there is still a codegen thread running in the background which then fails to notify that it finished. I don't understand how this is not a problem for other fatal errors during codegen though. There is a comment about AbortCodegenOnDrop, but this type doesn't seem to exist anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants