Improve Session/CodegenBackend construction - #161432
Conversation
Session creation is currently awkward: we build a mostly-initialized session, then use it to initialize a codegen backend, and then use the codegen backend to finish initializing the session. And it's not just awkward: within the Cranelift backend's `init` method `sess.lto()` is called, which consults `sess.thin_lto_supported`, *before* that field has been properly set! This commit cleans up this mess. It introduces `EarlySession`, which contains just four `Session` fields, the ones that are needed for codegen backend initialization. This is passed to `init`. `init` then returns a `CodegenBackendInit` which contains the backend-specific information needed to build a `Session`. (It replaces the `replaced_intrinsics`, `fallback_intrinsics`, and `thin_lto_supported` methods.) The `Session` can then be built in a single step. No more partial initialization problems. A few functions that previously took a `Session` now take something else, e.g. a `Target`, because they are used from some places where an `EarlySession` is available and other places where a `Session` is available. And a new `early_lto` method is used for Cranelift's LTO check.
It currently takes `&self`, which is a bit strange for an `init` method. As a result, the Cranelift and GCC backends have to use types with interior mutability. This commit changes it to `&mut self`. Benefits: - The Cranelift backend can use `Option` instead of `OnceCell` to indicate uninit vs. init. - The GCC backend can avoid `Mutex`, and use `bool` instead of `AtomicBool`, which makes things much simpler. The commit also restructures `GccCodegenBackend` to mirror `CraneliftCodegenBackend`: just contain an `Option<BackendConfig>`, which makes the uninit vs. init distinction foolproof. (E.g. no need to set `lto_supported` to false and then later overwrite it with the real value.) As part of this the `LockedTargetInfo` type is renamed `SharedTargetInfo` because that better matches its new internals. (All this compiles both with and without the "master" feature set.)
|
cc @rust-lang/miri
cc @bjorn3
|
rustbot
added
A-LLVM
|
This is an opinionated change, see what you all think. LLM disclosure: some of the ideas came from an analysis done by an LLM. I wrote all the code and text myself. |
| fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| self.info.lock().expect("lock").fmt(formatter) | ||
| } | ||
| #[derive(Clone)] |
There was a problem hiding this comment.
Do the cg_gcc changes need to be done in this PR?
I would be more confortable landing this directly in the cg_gcc repo so that the whole test suite can run (some cg_gcc tests do not run here in the Rust repo).
There was a problem hiding this comment.
I think they do, because both commits change the signature of CodegenBackend::init. Doing a local test run in cg_gcc is probably the way forward, if/when there's agreement that this PR is worth merging.
|
☔ The latest upstream changes (presumably #161043) made this pull request unmergeable. Please resolve the merge conflicts by rebasing. |
4 participants
The creation and initialization of sessions and codegen backends is intertwined, which is confusing and error prone. This commit detangles things, and also simplifies the types used for the state within the backends. Details in individual commits.
r? @bjorn3