bjoernager/rust
bjoernager/rust
1
Fork 0
Code Issues Pull requests Activity

Auto merge of #139453 - compiler-errors:incr, r=jieyouxu

Browse source 
Prepend temp files with per-invocation random string to avoid temp filename conflicts

https://github.com/rust-lang/rust/issues/139407 uncovered a very subtle unsoundness with incremental codegen, failing compilation sessions (due to assembler errors), and the "prefer hard linking over copying files" strategy we use in the compiler for file management.

Specifically, imagine we're building a single file 3 times, all with `-Csave-temps -Cincremental=...`. Let's call the object file we're building for the codegen unit for `main` "`XXX.o`" just for clarity since it's probably some gigantic hash name:

```
#[inline(never)]
#[cfg(any(rpass1, rpass3))]
fn a() -> i32 {
    0
}

#[cfg(any(cfail2))]
fn a() -> i32 {
    1
}

fn main() {
    evil::evil();
    assert_eq!(a(), 0);
}

mod evil {
    #[cfg(any(rpass1, rpass3))]
    pub fn evil() {
        unsafe {
            std::arch::asm!("/*  */");
        }
    }

    #[cfg(any(cfail2))]
    pub fn evil() {
        unsafe {
            std::arch::asm!("missing");
        }
    }
}
```

Session 1 (`rpass1`):
* Type-check, borrow-check, etc.
* Serialize the dep graph to the incremental working directory `.../s-...-working/`.
* Codegen object file to a temp file `XXX.rcgu.o` which is spit out in the cwd.
* Hard-link[^1] `XXX.rcgu.o` to the incremental working directory `.../s-...-working/XXX.o`.
* Save-temps option means we don't delete `XXX.rgcu.o`.
* Link the binary and stuff.
* Finalize[^2] the working incremental session by renaming `.../s-...-working` to ` s-...-asjkdhsjakd` (some other finalized incr comp session dir name).

Session 2 (`cfail2`):
* Load artifacts from the previous *finalized* incremental session, namely the dep graph.
* Type-check, borrow-check, etc. since the file has changed, so most dep graph nodes are red.
* Serialize the dep graph to the incremental working directory `.../s-...-working/`.
* Codegen object file to a temp file `XXX.rcgu.o`. **HERE IS THE PROBLEM**: The hard-link is still set up to point to the inode from `XXX.o` from the first session, so this also modifies the `XXX.o` in the previous finalized session directory.
* Codegen emits an error b/c `missing` is not an instruction, so we abort before finalizing the incremental session. Specifically, this means that the *previous* session is the last finalized session.

Session 3 (`rpass3`):
* Load artifacts from the previous *finalized* incremental session, namely the dep graph. NOTE that this is from session 1.
* All the dep graph nodes are green since we are basically replaying session 1.
* codegen object file `XXX.o`, which is detected as *reused* from session 1 since dep nodes were green. That means we **reuse** `XXX.o` which had been dirtied from session 2.
* Link the binary and stuff.

This results in a binary which reuses some of the build artifacts from session 2, but thinks it's from session 1.

At this point, I hope it's clear to see that the incremental results from session 1 were dirtied from session 2, but we reuse them as if session 1 was the previous (finalized) incremental session we ran. This is at best really buggy, and at worst **unsound**.

This isn't limited to `-C save-temps`, since there are other combinations of flags that may keep around temporary files (hard linked) in the working directory (like `-C debuginfo=1 -C split-debuginfo=unpacked` on darwin, for example).

---

This PR implements a fix which is to prepend temp filenames with a random string that is generated per invocation of rustc. This string is not *deterministic*, but temporary files are transient anyways, so I don't believe this is a problem.

That means that temp files are now something like... `{crate-name}.{cgu}.{invocation_temp}.rcgu.o`, where `{invocation_temp}` is the new temporary string we generate per invocation of rustc.

Fixes https://github.com/rust-lang/rust/issues/139407

[^1]: 175dcc7773/compiler/rustc_fs_util/src/lib.rs (L60)
[^2]: 175dcc7773/compiler/rustc_incremental/src/persist/fs.rs (L1-L40)
This commit is contained in:
bors 2025-04-11 13:59:33 +00:00
commit e1b06f7730
20 changed files with 285 additions and 97 deletions
Download patch file Download diff file Expand all files Collapse all files

8
compiler/rustc_codegen_ssa/src/back/link.rs
View file

@ -112,8 +112,12 @@ pub fn link_binary(
codegen_results.crate_info.local_crate_name,
);
let crate_name = format!("{}", codegen_results.crate_info.local_crate_name);
let out_filename =
output.file_for_writing(outputs, OutputType::Exe, Some(crate_name.as_str()));
let out_filename = output.file_for_writing(
outputs,
OutputType::Exe,
&crate_name,
sess.invocation_temp.as_deref(),
);
match crate_type {
CrateType::Rlib => {
let _timer = sess.timer("link_rlib");

45
compiler/rustc_codegen_ssa/src/back/write.rs
View file

@ -306,14 +306,18 @@ impl TargetMachineFactoryConfig {
cgcx.output_filenames.split_dwarf_path(
cgcx.split_debuginfo,
cgcx.split_dwarf_kind,
Some(module_name),
module_name,
cgcx.invocation_temp.as_deref(),
)
} else {
None
};
let output_obj_file =
Some(cgcx.output_filenames.temp_path(OutputType::Object, Some(module_name)));
let output_obj_file = Some(cgcx.output_filenames.temp_path_for_cgu(
OutputType::Object,
module_name,
cgcx.invocation_temp.as_deref(),
));
TargetMachineFactoryConfig { split_dwarf_file, output_obj_file }
}
}
@ -344,6 +348,7 @@ pub struct CodegenContext<B: WriteBackendMethods> {
pub crate_types: Vec<CrateType>,
pub each_linked_rlib_for_lto: Vec<(CrateNum, PathBuf)>,
pub output_filenames: Arc<OutputFilenames>,
pub invocation_temp: Option<String>,
pub regular_module_config: Arc<ModuleConfig>,
pub metadata_module_config: Arc<ModuleConfig>,
pub allocator_module_config: Arc<ModuleConfig>,
@ -582,8 +587,11 @@ fn produce_final_output_artifacts(
if let [module] = &compiled_modules.modules[..] {
// 1) Only one codegen unit. In this case it's no difficulty
// to copy `foo.0.x` to `foo.x`.
let module_name = Some(&module.name[..]);
let path = crate_output.temp_path(output_type, module_name);
let path = crate_output.temp_path_for_cgu(
output_type,
&module.name,
sess.invocation_temp.as_deref(),
);
let output = crate_output.path(output_type);
if !output_type.is_text_output() && output.is_tty() {
sess.dcx()
@ -596,22 +604,15 @@ fn produce_final_output_artifacts(
ensure_removed(sess.dcx(), &path);
}
} else {
let extension = crate_output
.temp_path(output_type, None)
.extension()
.unwrap()
.to_str()
.unwrap()
.to_owned();
if crate_output.outputs.contains_explicit_name(&output_type) {
// 2) Multiple codegen units, with `--emit foo=some_name`. We have
// no good solution for this case, so warn the user.
sess.dcx().emit_warn(errors::IgnoringEmitPath { extension });
sess.dcx()
.emit_warn(errors::IgnoringEmitPath { extension: output_type.extension() });
} else if crate_output.single_output_file.is_some() {
// 3) Multiple codegen units, with `-o some_name`. We have
// no good solution for this case, so warn the user.
sess.dcx().emit_warn(errors::IgnoringOutput { extension });
sess.dcx().emit_warn(errors::IgnoringOutput { extension: output_type.extension() });
} else {
// 4) Multiple codegen units, but no explicit name. We
// just leave the `foo.0.x` files in place.
@ -967,7 +968,12 @@ fn execute_copy_from_cache_work_item<B: ExtraBackendMethods>(
module.source.saved_files.get("dwo").as_ref().and_then(|saved_dwarf_object_file| {
let dwarf_obj_out = cgcx
.output_filenames
.split_dwarf_path(cgcx.split_debuginfo, cgcx.split_dwarf_kind, Some(&module.name))
.split_dwarf_path(
cgcx.split_debuginfo,
cgcx.split_dwarf_kind,
&module.name,
cgcx.invocation_temp.as_deref(),
)
.expect(
"saved dwarf object in work product but `split_dwarf_path` returned `None`",
);
@ -977,7 +983,11 @@ fn execute_copy_from_cache_work_item<B: ExtraBackendMethods>(
let mut load_from_incr_cache = |perform, output_type: OutputType| {
if perform {
let saved_file = module.source.saved_files.get(output_type.extension())?;
let output_path = cgcx.output_filenames.temp_path(output_type, Some(&module.name));
let output_path = cgcx.output_filenames.temp_path_for_cgu(
output_type,
&module.name,
cgcx.invocation_temp.as_deref(),
);
load_from_incr_comp_dir(output_path, &saved_file)
} else {
None
@ -1222,6 +1232,7 @@ fn start_executing_work<B: ExtraBackendMethods>(
split_dwarf_kind: tcx.sess.opts.unstable_opts.split_dwarf_kind,
parallel: backend.supports_parallel() && !sess.opts.unstable_opts.no_parallel_backend,
pointer_size: tcx.data_layout.pointer_size,
invocation_temp: sess.invocation_temp.clone(),
};
// This is the "main loop" of parallel work happening for parallel codegen.

7
compiler/rustc_codegen_ssa/src/base.rs
View file

@ -640,8 +640,11 @@ pub fn codegen_crate<B: ExtraBackendMethods>(
let metadata_cgu_name =
cgu_name_builder.build_cgu_name(LOCAL_CRATE, &["crate"], Some("metadata")).to_string();
tcx.sess.time("write_compressed_metadata", || {
let file_name =
tcx.output_filenames(()).temp_path(OutputType::Metadata, Some(&metadata_cgu_name));
let file_name = tcx.output_filenames(()).temp_path_for_cgu(
OutputType::Metadata,
&metadata_cgu_name,
tcx.sess.invocation_temp.as_deref(),
);
let data = create_compressed_metadata_file(
tcx.sess,
&metadata,

4
compiler/rustc_codegen_ssa/src/errors.rs
View file

@ -277,13 +277,13 @@ pub struct BinaryOutputToTty {
#[derive(Diagnostic)]
#[diag(codegen_ssa_ignoring_emit_path)]
pub struct IgnoringEmitPath {
pub extension: String,
pub extension: &'static str,
}
#[derive(Diagnostic)]
#[diag(codegen_ssa_ignoring_output)]
pub struct IgnoringOutput {
pub extension: String,
pub extension: &'static str,
}
#[derive(Diagnostic)]

18
compiler/rustc_codegen_ssa/src/lib.rs
View file

@ -105,13 +105,19 @@ impl<M> ModuleCodegen<M> {
emit_asm: bool,
emit_ir: bool,
outputs: &OutputFilenames,
invocation_temp: Option<&str>,
) -> CompiledModule {
let object = emit_obj.then(|| outputs.temp_path(OutputType::Object, Some(&self.name)));
let dwarf_object = emit_dwarf_obj.then(|| outputs.temp_path_dwo(Some(&self.name)));
let bytecode = emit_bc.then(|| outputs.temp_path(OutputType::Bitcode, Some(&self.name)));
let assembly = emit_asm.then(|| outputs.temp_path(OutputType::Assembly, Some(&self.name)));
let llvm_ir =
emit_ir.then(|| outputs.temp_path(OutputType::LlvmAssembly, Some(&self.name)));
let object = emit_obj
.then(|| outputs.temp_path_for_cgu(OutputType::Object, &self.name, invocation_temp));
let dwarf_object =
emit_dwarf_obj.then(|| outputs.temp_path_dwo_for_cgu(&self.name, invocation_temp));
let bytecode = emit_bc
.then(|| outputs.temp_path_for_cgu(OutputType::Bitcode, &self.name, invocation_temp));
let assembly = emit_asm
.then(|| outputs.temp_path_for_cgu(OutputType::Assembly, &self.name, invocation_temp));
let llvm_ir = emit_ir.then(|| {
outputs.temp_path_for_cgu(OutputType::LlvmAssembly, &self.name, invocation_temp)
});
CompiledModule {
name: self.name.clone(),