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. |
|
|
||
| let cfg = parse_cfg(sess.dcx(), config.crate_cfg); | ||
| let mut cfg = config::build_configuration(&sess, cfg); | ||
| util::add_configuration(&mut cfg, &mut sess, &*codegen_backend); |
There was a problem hiding this comment.
FWIW there is some more codegen-backend-related session initialization happening inside add_configuration. And especially the handing of target features is a complete mess (not as bad as it used to be, but still bad). We're calling llvm_util::global_llvm_features like half a dozen times because we need it in various places and we don't have a tcx yet so it can't be a query...
Anyway, not really something for this PR. I just wondered what this PR does with the messy part of codegen backend initialization that I regularly run into, and the answer is "nothing". Which is fine, the cleanup here seems reasonable on its own. Maybe inspiration for a future cleanup PR. :)
| use rustc_span::{DUMMY_SP, Symbol}; | ||
| use rustc_symbol_mangling::mangle_internal_symbol; | ||
| use rustc_target::spec::{Arch, Os}; | ||
| use rustc_target::spec::{Arch, Os, Target as SpecTarget}; |
There was a problem hiding this comment.
Could you call it TargetSpec instead? I think that's closer to how we usually refer to these.
| // `type_id_eq` is a safe choice since *all* backends use the fallback body for that. When | ||
| // adding more intrinsics, keep in mind that the distributed standard library is compiled | ||
| // with the LLVM backend but might later be included in a project built with cranelift or | ||
| // GCC. |
There was a problem hiding this comment.
Reading this comment I realize it's not as clear as I'd like it to be...
| // GCC. | |
| // GCC. Adding an intrinsic here can therefore mean the fallback body | |
| // is used with cranelift/GCC even if they have dedicated implementations. |
| pub fallback_intrinsics: Vec<Symbol>, | ||
|
|
||
| /// Is ThinLTO supported by this backend? | ||
| pub thin_lto_supported: bool = true, |
There was a problem hiding this comment.
Maybe target_config and supported_crate_types should also be included here? Though on the other hand they are only queried after the Session is fully built.
| thin_lto_supported: true, // filled by `run_compiler` | ||
| replaced_intrinsics: FxHashSet::from_iter(codegen_backend_init.replaced_intrinsics), | ||
| fallback_intrinsics: FxHashSet::from_iter(codegen_backend_init.fallback_intrinsics), | ||
| thin_lto_supported: codegen_backend_init.thin_lto_supported, |
There was a problem hiding this comment.
Would it make sense to do let CodegenBackendInit { a, b, c } = codegen_backend_init;?
|
|
||
| pub struct CraneliftCodegenBackend { | ||
| pub config: OnceCell<BackendConfig>, | ||
| // `None` before `init`, `Some` after. |
There was a problem hiding this comment.
| // `None` before `init`, `Some` after. | |
| // Set by `init` if not yet set. |
and appropriate changes to init. This is intended to allow setting the config when building CraneliftCodegenBackend already.
5 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