Auto merge of rust-lang#135395 - saethlin:compiler-builtins-cgus, r=b…

…jorn3

Enforce the compiler-builtins partitioning scheme

compiler-builtins needs every intrinsic in its own CGU. Currently, the compiler-builtins crate puts every intrinsic in its own inline module then `library/Cargo.toml` uses a profile override so that when we build the sysroot, compiler-builtins is built with more `codegen-units` than we have intrinsics, and partitioning never merges two intrinsics together. This approach does not work with `-Zbuild-std` because the profile override gets ignored. And it's kludgey anyway, our own standard library should not be fighting with our own compiler in an attempt to override its behavior. We should change the compiler's behavior to do the right thing in the first place.

So that's what this PR does. There's some light refactoring of the CGU partitioning code, then in 3 places I've added a check for `is_compiler_builtins`:
* There's a special case now in `cross_crate_inlinable`; every function in compiler-builtins that is not `#[no_mangle]` is made cross-crate-inlinable, which ensures we do not run into problems inlining helpers into intrinsics such as rust-lang#73135
* When building compiler-builtins, the name of the CGU that a MonoItem is given is just the MonoItem's symbol name. This puts every GloballyShared item in its own CGU.
* Then when building compiler-builtins, we skip CGU merging.

That should ensure that we have one object file per intrinsic, and if optimizations are enabled, there should be no extra extra CGUs full of helper functions (which is what currently happens in the precompiled standard library we distribute, my nightly libcompiler_builtins.rlib for x86_64-unknown-linux-gnu has 174 CGUs and with this PR we have 150).

compiler/rustc_mir_transform/src/cross_crate_inline.rs

@@ -34,6 +34,15 @@ fn cross_crate_inlinable(tcx: TyCtxt<'_>, def_id: LocalDefId) -> bool {
         return true;
     }
 
+    // compiler-builtins only defines intrinsics (which are handled above by checking
+    // contains_extern_indicator) and helper functions used by those intrinsics. The helper
+    // functions should always be inlined into intrinsics that use them. This check does not
+    // guarantee that we get the optimizations we want, but it makes them *much* easier.
+    // See https://github.com/rust-lang/rust/issues/73135
+    if tcx.is_compiler_builtins(rustc_span::def_id::LOCAL_CRATE) {
+        return true;
+    }
+
     if tcx.has_attr(def_id, sym::rustc_intrinsic) {
         // Intrinsic fallback bodies are always cross-crate inlineable.
         // To ensure that the MIR inliner doesn't cluelessly try to inline fallback

compiler/rustc_monomorphize/src/partitioning.rs

@@ -165,7 +165,8 @@ where
     // estimates.
     {
         let _prof_timer = tcx.prof.generic_activity("cgu_partitioning_merge_cgus");
-        merge_codegen_units(cx, &mut codegen_units);
+        let cgu_contents = merge_codegen_units(cx, &mut codegen_units);
+        rename_codegen_units(cx, &mut codegen_units, cgu_contents);
         debug_dump(tcx, "MERGE", &codegen_units);
     }
 
@@ -200,7 +201,6 @@ where
     I: Iterator<Item = MonoItem<'tcx>>,
 {
     let mut codegen_units = UnordMap::default();
-    let is_incremental_build = cx.tcx.sess.opts.incremental.is_some();
     let mut internalization_candidates = UnordSet::default();
 
     // Determine if monomorphizations instantiated in this crate will be made
@@ -227,20 +227,8 @@ where
             }
         }
 
-        let characteristic_def_id = characteristic_def_id_of_mono_item(cx.tcx, mono_item);
-        let is_volatile = is_incremental_build && mono_item.is_generic_fn();
-
-        let cgu_name = match characteristic_def_id {
-            Some(def_id) => compute_codegen_unit_name(
-                cx.tcx,
-                cgu_name_builder,
-                def_id,
-                is_volatile,
-                cgu_name_cache,
-            ),
-            None => fallback_cgu_name(cgu_name_builder),
-        };
-
+        let cgu_name =
+            compute_codegen_unit_name(cx.tcx, cgu_name_builder, mono_item, cgu_name_cache);
         let cgu = codegen_units.entry(cgu_name).or_insert_with(|| CodegenUnit::new(cgu_name));
 
         let mut can_be_internalized = true;
@@ -321,7 +309,7 @@ where
 fn merge_codegen_units<'tcx>(
     cx: &PartitioningCx<'_, 'tcx>,
     codegen_units: &mut Vec<CodegenUnit<'tcx>>,
-) {
+) -> UnordMap<Symbol, Vec<Symbol>> {
     assert!(cx.tcx.sess.codegen_units().as_usize() >= 1);
 
     // A sorted order here ensures merging is deterministic.
@@ -331,6 +319,13 @@ fn merge_codegen_units<'tcx>(
     let mut cgu_contents: UnordMap<Symbol, Vec<Symbol>> =
         codegen_units.iter().map(|cgu| (cgu.name(), vec![cgu.name()])).collect();
 
+    // When compiling compiler_builtins, we do not want to put multiple intrinsics in a CGU.
+    // There may be mergeable CGUs under this constraint, but just skipping over merging is much
+    // simpler.
+    if cx.tcx.is_compiler_builtins(LOCAL_CRATE) {
+        return cgu_contents;
+    }
+
     // If N is the maximum number of CGUs, and the CGUs are sorted from largest
     // to smallest, we repeatedly find which CGU in codegen_units[N..] has the
     // greatest overlap of inlined items with codegen_units[N-1], merge that
@@ -421,6 +416,14 @@ fn merge_codegen_units<'tcx>(
         // Don't update `cgu_contents`, that's only for incremental builds.
     }
 
+    cgu_contents
+}
+
+fn rename_codegen_units<'tcx>(
+    cx: &PartitioningCx<'_, 'tcx>,
+    codegen_units: &mut Vec<CodegenUnit<'tcx>>,
+    cgu_contents: UnordMap<Symbol, Vec<Symbol>>,
+) {
     let cgu_name_builder = &mut CodegenUnitNameBuilder::new(cx.tcx);
 
     // Rename the newly merged CGUs.
@@ -678,13 +681,26 @@ fn characteristic_def_id_of_mono_item<'tcx>(
     }
 }
 
-fn compute_codegen_unit_name(
-    tcx: TyCtxt<'_>,
+fn compute_codegen_unit_name<'tcx>(
+    tcx: TyCtxt<'tcx>,
     name_builder: &mut CodegenUnitNameBuilder<'_>,
-    def_id: DefId,
-    volatile: bool,
+    mono_item: MonoItem<'tcx>,
     cache: &mut CguNameCache,
 ) -> Symbol {
+    // When compiling compiler_builtins, we do not want to put multiple intrinsics in a CGU.
+    // Using the symbol name as the CGU name puts every GloballyShared item in its own CGU, but in
+    // an optimized build we actually want every item in the crate that isn't an intrinsic to get
+    // LocalCopy so that it is easy to inline away. In an unoptimized build, this CGU naming
+    // strategy probably generates more CGUs than we strictly need. But it is simple.
+    if tcx.is_compiler_builtins(LOCAL_CRATE) {
+        let name = mono_item.symbol_name(tcx);
+        return Symbol::intern(name.name);
+    }
+
+    let Some(def_id) = characteristic_def_id_of_mono_item(tcx, mono_item) else {
+        return fallback_cgu_name(name_builder);
+    };
+
     // Find the innermost module that is not nested within a function.
     let mut current_def_id = def_id;
     let mut cgu_def_id = None;
@@ -712,6 +728,9 @@ fn compute_codegen_unit_name(
 
     let cgu_def_id = cgu_def_id.unwrap();
 
+    let is_incremental_build = tcx.sess.opts.incremental.is_some();
+    let volatile = is_incremental_build && mono_item.is_generic_fn();
+
     *cache.entry((cgu_def_id, volatile)).or_insert_with(|| {
         let def_path = tcx.def_path(cgu_def_id);
 

library/Cargo.toml

@@ -11,19 +11,6 @@ exclude = [
   "windows_targets"
 ]
 
-[profile.release.package.compiler_builtins]
-# For compiler-builtins we always use a high number of codegen units.
-# The goal here is to place every single intrinsic into its own object
-# file to avoid symbol clashes with the system libgcc if possible. Note
-# that this number doesn't actually produce this many object files, we
-# just don't create more than this number of object files.
-#
-# It's a bit of a bummer that we have to pass this here, unfortunately.
-# Ideally this would be specified through an env var to Cargo so Cargo
-# knows how many CGUs are for this specific crate, but for now
-# per-crate configuration isn't specifiable in the environment.
-codegen-units = 10000
-
 # These dependencies of the standard library implement symbolication for
 # backtraces on most platforms. Their debuginfo causes both linking to be slower
 # (more data to chew through) and binaries to be larger without really all that

tests/run-make/compiler-builtins/rmake.rs → tests/run-make/compiler-builtins-linkage/rmake.rs

@@ -1,4 +1,7 @@
-//! The compiler_builtins library is special. It can call functions in core, but it must not
+//! The compiler_builtins library is special. When linkers are passed multiple libraries, they
+//! expect undefined symbols mentioned by libraries on the left to be defined in libraries to the
+//! right. Since calls to compiler_builtins may be inserted during codegen, it is placed all the way
+//! to the right. Therefore, compiler_builtins can call functions in core but it must not
 //! require linkage against a build of core. If it ever does, building the standard library *may*
 //! result in linker errors, depending on whether the linker in use applies optimizations first or
 //! resolves symbols first. So the portable and safe approach is to forbid such a linkage

tests/run-make/compiler-builtins-partitioning/Cargo.toml

@@ -0,0 +1,7 @@
+[package]
+name = "scratch"
+version = "0.1.0"
+edition = "2021"
+
+[lib]
+path = "lib.rs"

tests/run-make/compiler-builtins-partitioning/lib.rs

@@ -0,0 +1 @@
+#![no_std]

tests/run-make/compiler-builtins-partitioning/rmake.rs

@@ -0,0 +1,105 @@
+//! The compiler_builtins library is special. It exists to export a number of intrinsics which may
+//! also be provided by libgcc or compiler-rt, and when an intrinsic is provided by another
+//! library, we want that definition to override the one in compiler_builtins because we expect
+//! that those implementations are more optimized than compiler_builtins. To make sure that an
+//! attempt to override a compiler_builtins intrinsic does not result in a multiple definitions
+//! linker error, the compiler has special CGU partitioning logic for compiler_builtins that
+//! ensures every intrinsic gets its own CGU.
+//!
+//! This test is slightly overfit to the current compiler_builtins CGU naming strategy; it doesn't
+//! distinguish between "multiple intrinsics are in one object file!" which would be very bad, and
+//! "This object file has an intrinsic and also some of its helper functions!" which would be okay.
+//!
+//! This test ensures that the compiler_builtins rlib has only one intrinsic in each object file.
+
+// wasm and nvptx targets don't produce rlib files that object can parse.
+//@ ignore-wasm
+//@ ignore-nvptx64
+
+#![deny(warnings)]
+
+use std::str;
+
+use run_make_support::object::read::Object;
+use run_make_support::object::read::archive::ArchiveFile;
+use run_make_support::object::{ObjectSymbol, SymbolKind};
+use run_make_support::rfs::{read, read_dir};
+use run_make_support::{cargo, object, path, target};
+
+fn main() {
+    println!("Testing compiler_builtins CGU partitioning for {}", target());
+
+    // CGU partitioning has some special cases for codegen-units=1, so we also test 2 CGUs.
+    for cgus in [1, 2] {
+        for profile in ["debug", "release"] {
+            run_test(profile, cgus);
+        }
+    }
+}
+
+fn run_test(profile: &str, cgus: usize) {
+    println!("Testing with profile {profile} and -Ccodegen-units={cgus}");
+
+    let target_dir = path("target");
+
+    let mut cmd = cargo();
+    cmd.args(&[
+        "build",
+        "--manifest-path",
+        "Cargo.toml",
+        "-Zbuild-std=core",
+        "--target",
+        &target(),
+    ])
+    .env("RUSTFLAGS", &format!("-Ccodegen-units={cgus}"))
+    .env("CARGO_TARGET_DIR", &target_dir)
+    .env("RUSTC_BOOTSTRAP", "1")
+    // Visual Studio 2022 requires that the LIB env var be set so it can
+    // find the Windows SDK.
+    .env("LIB", std::env::var("LIB").unwrap_or_default());
+    if profile == "release" {
+        cmd.arg("--release");
+    }
+    cmd.run();
+
+    let rlibs_path = target_dir.join(target()).join(profile).join("deps");
+    let compiler_builtins_rlib = read_dir(rlibs_path)
+        .find_map(|e| {
+            let path = e.unwrap().path();
+            let file_name = path.file_name().unwrap().to_str().unwrap();
+            if file_name.starts_with("libcompiler_builtins") && file_name.ends_with(".rlib") {
+                Some(path)
+            } else {
+                None
+            }
+        })
+        .unwrap();
+
+    // rlib files are archives, where the archive members are our CGUs, and we also have one called
+    // lib.rmeta which is the encoded metadata. Each of the CGUs is an object file.
+    let data = read(compiler_builtins_rlib);
+
+    let archive = ArchiveFile::parse(&*data).unwrap();
+    for member in archive.members() {
+        let member = member.unwrap();
+        if member.name() == b"lib.rmeta" {
+            continue;
+        }
+        let data = member.data(&*data).unwrap();
+        let object = object::File::parse(&*data).unwrap();
+
+        let mut global_text_symbols = 0;
+        println!("Inspecting object {}", str::from_utf8(&member.name()).unwrap());
+        for symbol in object
+            .symbols()
+            .filter(|symbol| matches!(symbol.kind(), SymbolKind::Text))
+            .filter(|symbol| symbol.is_definition() && symbol.is_global())
+        {
+            println!("symbol: {:?}", symbol.name().unwrap());
+            global_text_symbols += 1;
+        }
+        // Assert that this object/CGU does not define multiple global text symbols.
+        // We permit the 0 case because some CGUs may only be assigned a static.
+        assert!(global_text_symbols <= 1);
+    }
+}