compiler-builtins: pass -fdebug-compilation-dir=. to fix S_OBJNAME reproducibility on Windows - #157914
Conversation
|
cc @tgross35 |
|
r? @jieyouxu rustbot has assigned @jieyouxu. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@bors try job=dist-x86_64-msvc |
This comment has been minimized.
This comment has been minimized.
…oducibility, r=<try> compiler-builtins: pass -fdebug-compilation-dir=. to fix S_OBJNAME reproducibility on Windows try-job: dist-x86_64-msvc
|
where can i download the |
|
You can use rustup-toolchain-install-master I think you can do something like |
IIRC you should be able to use something like |
|
thanks to both of you, i checked in downloaded artifact, sadly patch didn't work, |
|
Is this something that you can figure out by inspecting the object file? If so, you could create a PR to the compiler-builtins repo adding a failing test to https://github.com/rust-lang/compiler-builtins/blob/fb48f81544cf29e6ff7fb4468c8b2e1dacd42b79/crates/symbol-check/src/main.rs then messing around to see what flags get it to pass. Might be easier than repeating the try build process here. Why does this only apply to Windows, though, and only compiler-builtins? I'd expect other platforms to get the same paths in their debug info. |
|
(a) yes, i found the current problem by inspecting an object file, extracting |
|
it works w |
|
the objname issue is resolved when checked manually but compiler-builtins still comes as non-deterministic in the repro check run i did. archive still contains absolute build paths (e.g. the |
|
update on the i patched however, |
5ba7fa8 to
5b79843
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
both the intermediate compiler-rt archive and the final rlib now show only bare filenames, no absolute paths :D full terminal output |
| /// Resolve the archiver tool cc-rs/bootstrap configured for this target. | ||
| /// cc-rs checks both the dash and underscore forms of `AR_<target>`, | ||
| /// falling back to a plain `AR`, and finally the bare tool name on PATH. | ||
| fn find_archiver_tool(name: &str) -> PathBuf { | ||
| let target = env::var("TARGET").unwrap_or_default(); | ||
| let dash_var = format!("AR_{target}"); | ||
| let underscore_var = format!("AR_{}", target.replace('-', "_")); | ||
|
|
||
| let resolved = env::var_os(&dash_var) | ||
| .or_else(|| env::var_os(&underscore_var)) | ||
| .or_else(|| env::var_os("AR")) | ||
| .map(PathBuf::from); | ||
|
|
||
| match resolved { | ||
| // Swap the resolved archiver's directory in, but use `name` itself | ||
| // (llvm-ar vs llvm-lib) rather than assuming the configured AR is | ||
| // the right tool for both extraction and re-archiving. | ||
| Some(path) => match path.parent() { | ||
| Some(dir) if !dir.as_os_str().is_empty() => dir.join(format!("{name}.exe")), | ||
| _ => PathBuf::from(name), | ||
| }, | ||
| None => PathBuf::from(name), | ||
| } | ||
| } | ||
|
|
||
| /// The compiler-rt archive contains absolute object paths because the | ||
| /// archiver records whatever input paths it's given as member names. | ||
| /// Since compiler-builtins passes object files from OUT_DIR (absolute, | ||
| /// build-path-dependent), those become the archive's member names, | ||
| /// making the archive non-reproducible across build paths. | ||
| /// | ||
| /// We fix this by extracting all members and re-archiving them using only | ||
| /// their bare filenames, so the archive's metadata no longer depends on | ||
| /// where the build happened. | ||
| fn rearchive_with_bare_names() { | ||
| let out_dir = PathBuf::from(env::var("OUT_DIR").unwrap()); | ||
|
|
||
| for name in ["libcompiler-rt.a", "compiler-rt.lib"] { | ||
| let archive = out_dir.join(name); | ||
| if archive.exists() { | ||
| rebuild_archive_with_bare_names(&archive); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn rebuild_archive_with_bare_names(archive: &Path) { | ||
| let tmp_dir = archive.parent().unwrap().join("relib_tmp"); | ||
|
|
||
| if tmp_dir.exists() { | ||
| fs::remove_dir_all(&tmp_dir).expect("failed to remove temporary rearchive directory"); | ||
| } | ||
| fs::create_dir_all(&tmp_dir).expect("failed to create temporary rearchive directory"); | ||
|
|
||
| let ar = find_archiver_tool("llvm-ar"); | ||
| let lib = find_archiver_tool("llvm-lib"); | ||
|
|
||
| // Extract members into a temporary directory, then re-archive them using | ||
| // only their filenames. This avoids carrying over path-dependent member names | ||
| // from the original archive. | ||
| let status = Command::new(&ar) | ||
| .arg("x") | ||
| .arg(archive) | ||
| .current_dir(&tmp_dir) | ||
| .status() | ||
| .unwrap_or_else(|e| panic!("failed to execute {}: {e}", ar.display())); | ||
| assert!(status.success(), "{} failed to extract archive", ar.display()); | ||
|
|
||
| let mut objs: Vec<_> = fs::read_dir(&tmp_dir) | ||
| .expect("failed to read extraction directory") | ||
| .filter_map(Result::ok) | ||
| .map(|e| e.file_name()) | ||
| .filter(|name| { | ||
| Path::new(name) | ||
| .extension() | ||
| .map_or(false, |ext| ext == "o" || ext == "obj") | ||
| }) | ||
| .collect(); | ||
| assert!(!objs.is_empty(), "archive extraction produced no object files"); | ||
|
|
||
| // read_dir order isn't guaranteed stable across machines/runs; sort so | ||
| // the re-archived member order is deterministic. | ||
| objs.sort(); | ||
|
|
||
| // Build the replacement archive at a temp path first, so a failed | ||
| // llvm-lib invocation doesn't leave us without any archive at all. | ||
| let tmp_archive = archive.with_file_name(format!( | ||
| "{}.tmp", | ||
| archive.file_name().unwrap().to_string_lossy() | ||
| )); | ||
| if tmp_archive.exists() { | ||
| fs::remove_file(&tmp_archive).expect("failed to remove stale temp archive"); | ||
| } | ||
|
|
||
| // Re-archive from inside tmp_dir, passing only bare filenames, so | ||
| // llvm-lib stores bare names as the member names. | ||
| let status = Command::new(&lib) | ||
| .arg(format!("-out:{}", tmp_archive.display())) | ||
| .arg("-nologo") | ||
| .args(&objs) | ||
| .current_dir(&tmp_dir) | ||
| .status() | ||
| .unwrap_or_else(|e| panic!("failed to execute {}: {e}", lib.display())); | ||
| assert!(status.success(), "{} failed to rebuild archive", lib.display()); | ||
|
|
||
| fs::rename(&tmp_archive, archive).expect("failed to replace archive with rebuilt version"); | ||
|
|
||
| fs::remove_dir_all(&tmp_dir).expect("failed to remove temporary rearchive directory"); | ||
| } |
There was a problem hiding this comment.
This is quite a lot of logic to have in a build script. Could we put this functionality into rustc somehow and access it with a flag?
There was a problem hiding this comment.
i wasn't sure where the right abstraction would be, since this archive is produced by the external C toolchain rather than by rustc's own archive generation. were you picturing a rustc/bootstrap mechanism that build scripts could opt into, or something else? i'd definitely prefer moving the logic out of the build script if there's a cleaner/preferable place for it.
There was a problem hiding this comment.
Ah sorry right, I was thinking this was for the Rust bits.
Is the summary basically that we need an archive where lib.exe is invoked with relative paths rather than absolute? This feels like an option that cc might be able to handle, especially if it's going to be useful elsewhere. Maybe it should even be the default?
There was a problem hiding this comment.
oh shoot, this didnt appear when i loaded the website, but answers my zulip q, and that sounds like a better/cleaner approach, tho i dont know well enough about cc-rs atp, i'll get on it and happy to test it against this fix,
not sure about the api design rn tho
There was a problem hiding this comment.
Got a link to the Zulip thread? I haven't seen it.
There was a problem hiding this comment.
Actually, after thinking about this some more, could we use ar_archive_writer in compiler_builtins? If the goal is reproducibility then that's our most stable option since it's fully under our control.
There was a problem hiding this comment.
that seems interesting, i haven't tried or even thought of that
im happy to experiment and try that fix!
There was a problem hiding this comment.
Good thought, that seems reasonable to me.
@paradoxicalguy I think you're probably already aware but any kind of build system changes for c-b should go via the c-b repo, quite a bit more gets tested there. Or at least tested in a draft PR there if you want to write a run-make test here.
(There's also symcheck in that repo that could probably be extended for testing)
There was a problem hiding this comment.
On second thought, maybe that's also something that could be done in cc if a Cargo feature is enabled? Assuming whatever build script pattern we come up with would probably have to be duplicated anywhere else that cares about reproduceability, that would make it easier to share.
There was a problem hiding this comment.
i'll experiment with ar_archive_writer in compiler-builtins first and see how well it fits. if it ends up being a generally useful approach rather than something compiler-builtins-specific, we can revisit whether it makes sense to expose it through cc-rs
and yes, i'll open in c-b if we go ahead that way :D
|
Maybe r? tgross35 |
|
|
|
@rustbot author for now |
|
Reminder, once the PR becomes ready for a review, use |
View all comments
compiler_builtins.rlibdiffers between builds because clang-cl embeds the absolute object output path into theS_OBJNAMEcodeview record.the path comes from cc-rs passing an absolute
/Foargument (derived from cargo'sOUT_DIR) toaddDebugObjectName(), which llvm then writes verbatim into.debug$S.minimal repro:
-fdebug-compilation-dir=.makesS_OBJNAMEemit a relative path instead.needs CI verification on a windows dist job since local builds use cl.exe rather than the bundled clang-cl. this pr is only for testing