rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
97.65k stars 12.63k forks source link

-Zremap-cwd-prefix is not applied to the sysroot rlib paths #112586

Closed danakj closed 1 year ago

danakj commented 1 year ago

This breaks deterministic builds on Windows (see https://github.com/rust-lang/rust/issues/88982#issuecomment-1589485353), where the PDB ends up including the paths to the object files. If a relative path is given to --sysroot, the path is turned into an absolute path for each rlib, and -Zremap-cwd-prefix is not applied.

danakj commented 1 year ago

cc: @michaelwoerister

bjorn3 commented 1 year ago

Does the linker have a way to remap the paths of object files as they will end up in the .pdb file?

danakj commented 1 year ago

I don't think so, no.

What lld-link does have is a way to specify "for relative source file path X, map it to PREFIX\X to make an absolute path for debugging". This is /pdbsourcepath, but it is not the same as stripping paths, it requires source paths to be relative to use this.

lld-link has no way to remove parts of paths given to it. link.exe does not either that I can find.

(The paths to the object files is not used in debugging, I believe? Just the paths to source files, which are currently remapped through -Zremap-cwd-prefix.)

bjorn3 commented 1 year ago

If it is not used that begs the question why it is even recorded in the first place :) On a more useful note, the crate loader uses std::fs::canonicalize to resolve symlinks and prevent those from causing crates to be potentially loaded multiple times which would error. Only the canonicalized path is then recorded for use by the linker code. I guess it would be possible to check in the linker code if a library path has the working directory as prefix and turn it back into a relative path or something like that. I guess it could look at the --sysroot argument and it's canonicalized path to also allow producing paths like ../../path/to/sysroot/lib/rustlib/x86_64-unknown-linux-gnu/lib/libstd-*.rlib.

danakj commented 1 year ago

I feel the same but I don't make the rules for the PDBs. Ok thank you for the pointer, I will have a look at how to do this.

bjorn3 commented 1 year ago

@rustbot label +A-reproducibility +O-windows-msvc

danakj commented 1 year ago

A PR to apply path remapping to linker command line outputs: https://github.com/rust-lang/rust/pull/112597

I verified that the Chromium build stops seeing any delta (from these paths) with the patch applied and building the same target in two different directories.

The Windows tests for bins can't yet be enabled by this PR tho because https://github.com/rust-lang/rust/issues/112587 needs to be addressed first.

michaelwoerister commented 1 year ago

Before we jump into a fix, I would find it helpful to get a more complete picture of the issue and possible solutions. I have some questions:

If these are paths that tell the linker where to find files on disk, then I suspect we can't remap them on the rustc side.

danakj commented 1 year ago

The paths that are appearing in the PDB are:

  1. The stdlib rlib files: /home/foo/bar/.../rustlib/x86_64-pc-windows-msvc/lib/libstd.rlib etc.
  2. The bin crate's object file, /tmp/<random>/symbols.o

The paths to other arguments (/LIBPATH, /NATVIS, /OUT) are either already relative (unlikely) or are not appearing in the PDB, at least with lld-link.

Providing them as relative paths to the linker works, the binary differences go away.

The PDB and EXE are affected, but the deltas in the PDB are the paths (which is easy to see) while the deltas in the EXE are less clear. However they are derived from the paths, because when the paths in the PDB are fixed, the deltas in the PDB and EXE both go away.

I will have to try link.exe to verify.

If these are paths that tell the linker where to find files on disk, then I suspect we can't remap them on the rustc side.

I believe we need to differentiate remap-path-prefix (can make arbitrary paths) and remap-cwd-prefix (must make a valid path).

Here's the high level way I think we can do so, from the session options:

    /// Returns a type to perform mapping of file paths as specified by the
    /// user. These paths should not be used for IO as they may map to invalid
    /// locations that are designed to just be more clear in diagnostics.
    pub fn file_path_mapping(&self) -> FilePathMapping {
        FilePathMapping::new(&self.remap_path_prefix)
    }

    /// Returns a type to perform mapping of file paths as specified by the
    /// user. These paths may be used for IO and are required to produce a valid
    /// path to the same file or directory.
    pub fn file_path_mapping_for_io(&self) -> FilePathMapping {
        FilePathMapping::new_for_io(&self.remap_path_prefix)
    }

The majority of uses cases would use file_path_mapping. The cmd line args to other tools (the linker) would use file_path_mapping_for_io.

danakj commented 1 year ago

Here's an example of the diffs on the EXE and PDB without any fixes:

Differences of files in build directories:
dbgcore.dll                    : None
dbghelp.dll                    : None
msdia140.dll                   : None
msvcp140.dll                   : None
test_control_flow_guard.exe    : DIFFERENT (unexpected): 8 out of 557056 bytes are different (0.00%)
  0x747a0   : dc47070052534453745bebf84928f5a84c4c44205044422e0100000074657374 '.G..RSDSt[..I(..LLD PDB.....test'
              dc47070052534453c00a38f31fc2ad584c4c44205044422e0100000074657374 '.G..RSDS..8....XLLD PDB.....test'
                              ^^^^^  +++++++                                              ^  +++
test_control_flow_guard.exe.pdb: DIFFERENT (unexpected): 418551 out of 23691264 bytes are different (1.77%)
  0x14c6000 : ffffffff770931010100000006000b8e0700000008000000289f010038570300 '....w.1.................(...8W..'
              ffffffff770931010100000006000b8e0700000008000000e49f010038570300 '....w.1.....................8W..'
                                                              ^^                                        ^
  0x14c6080 : 2f746d702f727573746336454f6668572f73796d626f6c732e6f002f746d702f '/tmp/rustc6EOfhW/symbols.o./tmp/'
              2f746d702f7275737463454a55667a412f73796d626f6c732e6f002f746d702f '/tmp/rustcEJUfzA/symbols.o./tmp/'
                                     ^^^   +++                                             ^^ ^^
  0x14c60a0 : 727573746336454f6668572f73796d626f6c732e6f0000000100000001000000 'rustc6EOfhW/symbols.o...........'
              7275737463454a55667a412f73796d626f6c732e6f0000000100000001000000 'rustcEJUfzA/symbols.o...........'
                           ^^^   +++                                                  ^^ ^^
  0x14c8860 : 736b746f702f52656c656173652f6c6f63616c5f72757374635f737973726f6f 'sktop/Release/local_rustc_sysroo'
              736b746f702f52656c656173652e322f6c6f63616c5f72757374635f73797372 'sktop/Release.2/local_rustc_sysr'
                                        ++++                                                 ++
  0x14c8880 : 742f6c69622f727573746c69622f7838365f36342d70632d77696e646f77732d 't/lib/rustlib/x86_64-pc-windows-'
              6f6f742f6c69622f727573746c69622f7838365f36342d70632d77696e646f77 'oot/lib/rustlib/x86_64-pc-window'
              ++++                                                              ++
  0x14c88a0 : 6d7376632f6c69622f6c69627374642e726c6962000000003300000001000000 'msvc/lib/libstd.rlib....3.......'
              732d6d7376632f6c69622f6c69627374642e726c696200003300000001000000 's-msvc/lib/libstd.rlib..3.......'
              ++++                                                              ++
  0x14c8940 : 702f52656c656173652f6c6f63616c5f72757374635f737973726f6f742f6c69 'p/Release/local_rustc_sysroot/li'
              702f52656c656173652e322f6c6f63616c5f72757374635f737973726f6f742f 'p/Release.2/local_rustc_sysroot/'
                                ++++                                                     ++
  0x14c8960 : 622f727573746c69622f7838365f36342d70632d77696e646f77732d6d737663 'b/rustlib/x86_64-pc-windows-msvc'
              6c69622f727573746c69622f7838365f36342d70632d77696e646f77732d6d73 'lib/rustlib/x86_64-pc-windows-ms'
              ++++                                                              ++
  0x14c8980 : 2f6c69622f6c69627374642e726c6962000000003400000001000000bcfc0100 '/lib/libstd.rlib....4...........'
              76632f6c69622f6c69627374642e726c696200003400000001000000bcfc0100 'vc/lib/libstd.rlib..4...........'
              ++++                                                              ++
  0x14c8a20 : 656173652f6c6f63616c5f72757374635f737973726f6f742f6c69622f727573 'ease/local_rustc_sysroot/lib/rus'
              656173652e322f6c6f63616c5f72757374635f737973726f6f742f6c69622f72 'ease.2/local_rustc_sysroot/lib/r'
                      ++++                                                          ++

Here is the difference once the stdlib rlibs are fixed:

test_control_flow_guard.exe    : DIFFERENT (unexpected): 8 out of 557056 bytes are different (0.00%)
  0x747a0   : dc4707005253445354d1b3b3a21595364c4c44205044422e0100000074657374 '.G..RSDST......6LLD PDB.....test'
              dc470700525344532239900cba42d0374c4c44205044422e0100000074657374 '.G..RSDS"9...B.7LLD PDB.....test'
                              ^^^^^^^^^^ + ^ ^                                          ^^   + ^
test_control_flow_guard.exe.pdb: DIFFERENT (unexpected): 24 out of 23699456 bytes are different (0.00%)
  0x14c6080 : 2f746d702f7275737463304b47427a642f73796d626f6c732e6f002f746d702f '/tmp/rustc0KGBzd/symbols.o./tmp/'
              2f746d702f72757374636850343945422f73796d626f6c732e6f002f746d702f '/tmp/rustchP49EB/symbols.o./tmp/'
                                  ++++  ^^ ^                                              ^^^^^
  0x14c60a0 : 7275737463304b47427a642f73796d626f6c732e6f0000000100000001000000 'rustc0KGBzd/symbols.o...........'
              72757374636850343945422f73796d626f6c732e6f0000000100000001000000 'rustchP49EB/symbols.o...........'
                        ++++  ^^ ^                                                   ^^^^^
  0x168c000 : 942e310154d1b3b30100000054d1b3b3a21595364c4c44205044422e52000000 '..1.T.......T......6LLD PDB.R...'
              942e31012239900c010000002239900cba42d0374c4c44205044422e52000000 '..1."9......"9...B.7LLD PDB.R...'
              942e31012239900c010000002239900cba42d0374c4c44205044422e52000000 '..1."9......"9...B.7LLD PDB.R...

And once the /tmp/.../symbols.o is made deterministic, the output is deterministic wrt paths.

michaelwoerister commented 1 year ago

Here's the high level way I think we can do so, from the session options:

For reference, we already have something similar to deal with different versions of a filename needed in different situations: https://github.com/rust-lang/rust/blob/3ed2a10d173d6c2e0232776af338ca7d080b1cd4/compiler/rustc_span/src/lib.rs#L282 https://github.com/rust-lang/rust/blob/3ed2a10d173d6c2e0232776af338ca7d080b1cd4/compiler/rustc_span/src/lib.rs#L176

danakj commented 1 year ago

Interesting, it does not seem that FileName is used in link.rs at all, it works strictly with Path. Including from CodegenResults::CrateInfo::used_crate_source which is a map of CrateSource which are (PathBuf, PathKind) and PathKind is:

pub enum PathKind {
    Native,
    Crate,
    Dependency,
    Framework,
    ExternFlag,
    All,
}
michaelwoerister commented 1 year ago

Could /LIBPATH be used to make paths in the sysroot relative? I.e. we pass /LIBPATH:<path-to-sysroot> libstd.rlib instead of <path-to-sysroot>\libstd.rlib.

danakj commented 1 year ago

We already do pass /LIBPATH:<path-to-sysroot>, which is done via add_library_search_dirs()

https://github.com/rust-lang/rust/blob/3ed2a10d173d6c2e0232776af338ca7d080b1cd4/compiler/rustc_codegen_ssa/src/back/link.rs#L1959

And if we do that and just give the sysroot libs without any path, yes the paths become deterministic in the PDB.

Is there a risk of conflicting stdlibs in different search paths, or presumably that would be unsupported?

danakj commented 1 year ago

The challenge here is the stdlib rlibs are found in CrateSource which contains the absolute path. We'd need to either:

  1. Track the LIBPATHs we've added, and if the path prefix is in a LIBPATH, drop the prefix.
  2. Change CodegenResults to have relative stdlib crates in CrateSource (I'm not sure how they get in there yet).
michaelwoerister commented 1 year ago

Changing how linkers are invoked has often had unintended side effects in the past, so we'll want to be careful. But overall, I think /libpath + relative path could be a viable fix here.

danakj commented 1 year ago

Ok, from looking at how CrateSource is constructed, it feels fraught to me to try to change stdlib paths in there to be relative. So I will look at tracking /LIBPATH.

Notably, it is added to the linker before the rlibs are.

michaelwoerister commented 1 year ago

Yes, I think it would be a good idea to do a quick implementation just to verify that it actually solves the problem we're trying to fix. Once that's confirmed, we can think about how to implement it cleanly.

danakj commented 1 year ago

Awkwardly, the /LIBPATH we give is relative to --sysroot. For me it is:

Add /LIBPATH local_rustc_sysroot/lib/rustlib/x86_64-pc-windows-msvc/lib

Then the path to the rlib is absolute, but can't do prefix matching against /LIBPATH:

/home/danakj/s/c/src/out_desktop/Release.2/local_rustc_sysroot/lib/rustlib/x86_64-pc-windows-msvc/lib/libstd.rlib starts_with local_rustc_sysroot/lib/rustlib/x86_64-pc-windows-msvc/lib = false
danakj commented 1 year ago

We can canonicalize the user-specified sysroot to make it consistent with the one we find by default. Something like this from session.rs:

    let sysroot =
        filesearch::get_or_default_sysroot(sopts.maybe_sysroot.clone()).expect("Failed finding sysroot");

And then get_or_default can defer to the user-specified one if present, but also canonicalize it (and fix it up for gcc).

danakj commented 1 year ago

Ah so /LIBPATH does show up in the PDB, and making it absolute then causes the same problem but for that. The entire linker command line is present in the PDB (see also /NXCOMPAT is present in the diff's context).

  0x14c41a0 : 2d70632d77696e646f77732d6d7376632f6c6962202f4e58434f4d504154202f '-pc-windows-msvc/lib /NXCOMPAT /'
              36342d70632d77696e646f77732d6d7376632f6c6962202f4e58434f4d504154 '64-pc-windows-msvc/lib /NXCOMPAT'
              ++++                                                              ++
  0x14c41c0 : 4c4942504154483a2f686f6d652f64616e616b6a2f732f632f7372632f6f7574 'LIBPATH:/home/danakj/s/c/src/out'
              202f4c4942504154483a2f686f6d652f64616e616b6a2f732f632f7372632f6f ' /LIBPATH:/home/danakj/s/c/src/o'
              ++++                                                              ++
  0x14c41e0 : 5f6465736b746f702f52656c656173652f6c6f63616c5f72757374635f737973 '_desktop/Release/local_rustc_sys'
              75745f6465736b746f702f52656c656173652e322f6c6f63616c5f7275737463 'ut_desktop/Release.2/local_rustc'
              ++++                                 ++++                         ++                ++

This was previously avoided by giving a relative --sysroot which is used verbatim in /LIBPATH.

danakj commented 1 year ago

Ok so recapping:

If I could find where the stdlib rlibs are added, as they are taking the sysroot and making something canonical, maybe that's the right answer then.. looking.

danakj commented 1 year ago

https://github.com/rust-lang/rust/blob/3ed2a10d173d6c2e0232776af338ca7d080b1cd4/compiler/rustc_metadata/src/locator.rs#L621-L642

This looks wrong. The non-stdlib crates are not an absolute path with relative --extern paths. But this is not the source of the paths either.

danakj commented 1 year ago

Staring at this code did give me an idea tho, to canonicalize the /LIBPATH (internally) and the rlib path (internally) in order to decide to drop the (canonical) matching prefix.

danakj commented 1 year ago

That works.

In MSVC Linker:

     fn link_rlib(&mut self, lib: &Path) {
+        let mut canonical_lib_dir = try_canonicalize(lib).unwrap_or_else(|_| lib.to_path_buf());
+        canonical_lib_dir.pop();
+        for libpath in &self.libpaths {
+            if canonical_lib_dir == *libpath {
+                // If the rlib is in a directory specified by /LIBPATH, then drop the directory
+                // from the command line. This ensures that the stdlib rlibs which are added to the
+                // command line by rustc do not appear as absolute paths when the sysroot is a
+                // relative path, in order to produce deterministic outputs (in this case, a linker
+                // command line) which do not include the current working directory.
+                self.cmd.arg(lib.file_name().expect("rlib has no file name path component"));
+                return;
+            }
+        }
         self.cmd.arg(lib);

and

     fn include_path(&mut self, path: &Path) {
+        self.libpaths.push(try_canonicalize(path).unwrap_or_else(|_| path.to_path_buf()));
         let mut arg = OsString::from("/LIBPATH:");
         arg.push(path);
         self.cmd.arg(&arg);

What do you think?

Here's the delta with this approach, which is just caused by the /tmp/.../symbols.o path.

test_control_flow_guard.exe    : DIFFERENT (unexpected): 8 out of 557056 bytes are different (0.00%)
  0x747a0   : dc470700525344534206862028fe47c64c4c44205044422e0100000074657374 '.G..RSDSB.. (.G.LLD PDB.....test'
              dc470700525344536dd05a1f40921cac4c4c44205044422e0100000074657374 '.G..RSDSm.Z.@...LLD PDB.....test'
                              ++++++++ ++ ^ ^^                                          ^^^^^
test_control_flow_guard.exe.pdb: DIFFERENT (unexpected): 24 out of 23699456 bytes are different (0.00%)
  0x14c6080 : 2f746d702f72757374633156576e6d392f73796d626f6c732e6f002f746d702f '/tmp/rustc1VWnm9/symbols.o./tmp/'
              2f746d702f72757374634a626c4c6e532f73796d626f6c732e6f002f746d702f '/tmp/rustcJblLnS/symbols.o./tmp/'
                                  ^^ ^^^^^  ^                                             ^^^^ ^
  0x14c60a0 : 72757374633156576e6d392f73796d626f6c732e6f0000000100000001000000 'rustc1VWnm9/symbols.o...........'
              72757374634a626c4c6e532f73796d626f6c732e6f0000000100000001000000 'rustcJblLnS/symbols.o...........'
                        ^^ ^^^^^  ^                                                  ^^^^ ^
  0x168c000 : 942e310142068620010000004206862028fe47c64c4c44205044422e52000000 '..1.B.. ....B.. (.G.LLD PDB.R...'
              942e31016dd05a1f010000006dd05a1f40921cac4c4c44205044422e52000000 '..1.m.Z.....m.Z.@...LLD PDB.R...'
              942e31016dd05a1f010000006dd05a1f40921cac4c4c44205044422e52000000 '..1.m.Z.....m.Z.@...LLD PDB.R...
danakj commented 1 year ago

Taking @petrochenkov 's feedback however, we can do this up in link.rs instead of in linker.rs, though it makes it more general rather than scoped to MSVC then.

In link.rs add_static_crate():

@@ -2693,9 +2693,24 @@ fn add_static_crate<'a>(
 ) {
     let src = &codegen_results.crate_info.used_crate_source[&cnum];
     let cratepath = &src.rlib.as_ref().unwrap().0;
+    let canonical_sysroot_lib_path = {
+        let lib_path = sess.target_filesearch(PathKind::All).get_lib_path();
+        try_canonicalize(&lib_path).unwrap_or(lib_path)
+    };

     let mut link_upstream = |path: &Path| {
-        cmd.link_rlib(&fix_windows_verbatim_for_gcc(path));
+        let mut canonical_path_dir = try_canonicalize(path).unwrap_or_else(|_| path.to_path_buf());
+        canonical_path_dir.pop();
+
+        let rlib_path = if canonical_path_dir == canonical_sysroot_lib_path {
+            // If the sysroot is a relative path, the sysroot libraries must also be specified as a
+            // relative path to the linker, else the linker command line is non-deterministic and
+            // it shows up in the PDB file generated by the MSVC linker.
+            Path::new(path.file_name().expect("rlib has no file name path component")).to_path_buf()
+        } else {
+            fix_windows_verbatim_for_gcc(path)
+        };
+        cmd.link_rlib(&rlib_path);
     };

This also works, and only affects sysroot rlibs.

Again, the binary diffs for two runs in different working dirs with the above approach:

test_control_flow_guard.exe    : DIFFERENT (unexpected): 8 out of 557056 bytes are different (0.00%)
  0x747a0   : dc470700525344530ae7fa14e645ca134c4c44205044422e0100000074657374 '.G..RSDS.....E..LLD PDB.....test'
              dc47070052534453926d0c2d2a65b3ac4c4c44205044422e0100000074657374 '.G..RSDS.m.-*e..LLD PDB.....test'
                              ++++ ++++ ^^^^                                             + ^^^
test_control_flow_guard.exe.pdb: DIFFERENT (unexpected): 24 out of 23699456 bytes are different (0.00%)
  0x14c6080 : 2f746d702f7275737463396b4149544f2f73796d626f6c732e6f002f746d702f '/tmp/rustc9kAITO/symbols.o./tmp/'
              2f746d702f7275737463784a6872784e2f73796d626f6c732e6f002f746d702f '/tmp/rustcxJhrxN/symbols.o./tmp/'
                                  ^^^^ ^^^^^ ^                                            ^^^^^^
  0x14c60a0 : 7275737463396b4149544f2f73796d626f6c732e6f0000000100000001000000 'rustc9kAITO/symbols.o...........'
              7275737463784a6872784e2f73796d626f6c732e6f0000000100000001000000 'rustcxJhrxN/symbols.o...........'
                        ^^^^ ^^^^^ ^                                                 ^^^^^^
  0x168c000 : 942e31010ae7fa14010000000ae7fa14e645ca134c4c44205044422e52000000 '..1..............E..LLD PDB.R...'
              942e3101926d0c2d01000000926d0c2d2a65b3ac4c4c44205044422e52000000 '..1..m.-.....m.-*e..LLD PDB.R...'
              942e3101926d0c2d01000000926d0c2d2a65b3ac4c4c44205044422e52000000 '..1..m.-.....m.-*e..LLD PDB.R...
danakj commented 1 year ago

https://github.com/rust-lang/rust/pull/112597/commits/7e07271eaf6f8ae1f3b2c3ffeb46bf1c13f36bc1 uses the above strategy in a (IMO) clean way.

michaelwoerister commented 1 year ago

Looks promising!

Here is my recap:

I'll give the concrete fix a closer look tomorrow. I also want to give others a chance to weigh in.

danakj commented 1 year ago

The relationship to prefix remapping is a bit murky to me. It's probably unrelated?

Yeah. It is unrelated because it is actually the --sysroot value that matters here. For Chromium, it looked related because we put a sysroot under the current working directory, so --remap-cwd-prefix would work. But --sysroot could also be somewhere else and the rlib paths should/could be relative to that then.

If the sysroot is on another drive, then it will have to be an absolute path, and thus making the rlibs relative to it will also make them an absolute path. So the solution we landed on here should work.

Thanks for your help with this.

michaelwoerister commented 1 year ago

@danakj, how are you building these test binaries? When I'm using a relative path for the sysroot, I'm running into trouble because Cargo will use a different working directory for each rustc invocation.

danakj commented 1 year ago

I am running it through Chromium GN - the invocations all happen from the single output directory (not unlike CMake).

It sounds like a relative sysroot with Cargo will just not work by design (of Cargo) then. All the folks interested in reproducible builds are using other systems though - Buck, GN, or Bazel I guess.

Let me share the commands in case they help.

Here's the high level thing from the chromium repo:

rm -f out_desktop/Release/test_control_flow_guard.exe out_desktop/Release.2/test_control_flow_guard.exe && \
autoninja -C out_desktop/Release test_control_flow_guard.exe && \
autoninja -C out_desktop/Release.2 test_control_flow_guard.exe && \
tools/determinism/compare_build_artifacts.py --first-build-dir out_desktop/Release \
    --second-build-dir out_desktop/Release.2 --target-platform win

With these GN args in both out_desktop/Release and out_desktop/Release.2 (for my convenience I am doing this work on Linux and cross-compiling for Windows):

use_goma = true
dcheck_always_on = true
enable_rust = true
symbol_level = 1
enable_nacl = false
is_debug = false
is_component_build = false

target_os = "win"

Here's the rustc invocation that it results in:

./../third_party/rust-toolchain/bin/rustc -Clinker=../../third_party/llvm-build/Release+Asserts/bin/lld-link \
-Clink-arg=--rsp-quoting=posix --crate-name test_control_flow_guard \
../../build/rust/tests/test_control_flow_guard/test_control_flow_guard.rs --crate-type bin --edition=2021 \
-Coverflow-checks=on -Cdefault-linker-libraries -Dunsafe_op_in_unsafe_fn -Zdep-info-omit-d-target -Zmacro-backtrace \
-Zremap-cwd-prefix=. --target=x86_64-pc-windows-msvc -Cembed-bitcode=no -Cpanic=abort -Zpanic_abort_tests \
--cfg cr_rustc_revision=\"a2b1646c597329d0a25efa3889b66650f65de1de-5-llvmorg-17-init-12166-g7586aeab\" \
-Copt-level=s -Cdebuginfo=1 -Ccontrol-flow-guard -Clink-arg=libcmt.lib --sysroot=local_rustc_sysroot \
--emit=dep-info=./test_control_flow_guard.exe.d,link -o ./test_control_flow_guard.exe -Clink-arg=/CETCOMPAT \
-Clink-arg=/WX -Clink-arg=--color-diagnostics -Clink-arg=/call-graph-profile-sort:no -Clink-arg=/TIMESTAMP:1685854800 \
-Clink-arg=/lldignoreenv -Clink-arg=/pdbpagesize:8192 -Clink-arg=/OPT:REF -Clink-arg=/OPT:ICF \
-Clink-arg=/INCREMENTAL:NO -Clink-arg=/FIXED:NO -Clink-arg=/OPT:NOLLDTAILMERGE -Clink-arg=/PROFILE \
-Clink-arg=/PDBSourcePath:o:\fake\prefix -Clink-arg=/OPT:ICF -Clink-arg=/DEBUG:GHASH \
-Clink-arg=/pdbaltpath:%_PDB% -Clink-arg=/NATVIS:../../build/config/c++/libc++.natvis \
-Clink-arg=/DEFAULTLIB:libcpmt.lib -Clink-arg=/FIXED:NO -Clink-arg=/ignore:4199 -Clink-arg=/ignore:4221 \
-Clink-arg=/NXCOMPAT -Clink-arg=/DYNAMICBASE -Clink-arg=/INCREMENTAL:NO -Clink-arg=/SUBSYSTEM:CONSOLE,5.02 \
-Clink-arg=/guard:cf -Clink-arg=legacy_stdio_definitions.lib \
-Clink-arg=-libpath:../../third_party/llvm-build/Release+Asserts/lib/clang/17/lib/windows \
-Clink-arg=/winsysroot:../../third_party/depot_tools/win_toolchain/vs_files/27370823e7 -Clink-arg=/MACHINE:X64 \
-Clink-arg=/PDB:./test_control_flow_guard.exe.pdb @test_control_flow_guard.exe.rsp

And the test_control_flow_guard.exe.rsp:

-Ldependency=local_rustc_sysroot/lib/rustlib/x86_64-pc-windows-msvc/lib
-Lnative=obj/buildtools/third_party/libc++/libc++
-Clink-arg=obj/buildtools/third_party/libc++/libc++/algorithm.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/any.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/atomic.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/barrier.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/bind.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/charconv.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/chrono.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/condition_variable.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/condition_variable_destructor.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/exception.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/format.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/functional.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/future.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/hash.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/ios.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/ios.instantiations.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/iostream.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/legacy_pointer_safety.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/locale.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/memory.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/mutex.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/mutex_destructor.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/new.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/optional.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/random.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/random_shuffle.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/regex.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/d2fixed.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/d2s.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/f2s.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/shared_mutex.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/stdexcept.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/string.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/strstream.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/system_error.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/thread.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/typeinfo.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/valarray.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/variant.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/vector.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/verbose_abort.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/locale_win32.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/support.obj
-Clink-arg=obj/buildtools/third_party/libc++/libc++/thread_win32.obj
-Clink-arg=../../third_party/llvm-build/Release+Asserts/lib/clang/17/lib/windows/clang_rt.builtins-x86_64.lib
-ladvapi32
-lcomdlg32
-ldbghelp
-ldnsapi
-lgdi32
-lmsimg32
-lodbc32
-lodbccp32
-loleaut32
-lshell32
-lshlwapi
-luser32
-lusp10
-luuid
-lversion
-lwininet
-lwinmm
-lwinspool
-lws2_32
-ldelayimp
-lkernel32
-lole32

Oh, and to verify the cause and solution originally, I had replaced -Clinker=../../third_party/llvm-build/Release+Asserts/bin/lld-link with -Clinker=../../link.py and run the rustc commands manually from each output directory. The link.py script would make the sysroot paths relative, and move the /tmp file out to a deterministic place. In that case, the tools/determinism/compare_build_artifacts.py step finds no differences anymore.

This is my hacky link.py:

#!/usr/bin/env python3
# Copyright 2023 The Chromium Authors
# Use of this source code is governed by a BSD-style license that can be
# found in the LICENSE file.

import subprocess
import os
import shutil
import sys

def drop_path(cwd, r: str):
  stdpath = os.path.join('local_rustc_sysroot', 'lib', 'rustlib', 'x86_64-pc-windows-msvc', 'lib')
  if '/' + str(stdpath) in r:
    return os.path.basename(r)
  else:
    return r

def main():
  cwd = os.getcwd()
  rest = [drop_path(cwd, r) for r in sys.argv[1:]]

  args = []
  for r in rest:
    if r.startswith('/tmp'):
      shutil.copy(r, 'my.o')
      args.append('my.o')
    else:
      args.append(r)

  lld = os.path.join('..', '..', 'third_party', 'llvm-build', 'Release+Asserts', 'bin', 'lld-link')
  return subprocess.call([lld] + args)

if __name__ == '__main__':
  sys.exit(main())
danakj commented 1 year ago

We do have the tests in here which are disabled too but I don't think they print out what differed so they will just still fail until the /tmp issue is resolved. But those are some command lines you could run as well and then use other tools to diff. Like https://source.chromium.org/chromium/chromium/src/+/main:tools/determinism/compare_build_artifacts.py

michaelwoerister commented 1 year ago

Thanks for the info, @danakj! Maybe we can let some of that inspire improved reproducibility testing when we also fix the object file in /tmp issue. Does lld-link produce PDBs deterministically?

danakj commented 1 year ago

Yes it does! As long as the command line is reproducible.

There's an extra flag that helps compared to link.exe, which is /PDBSourcePath:o:\fake\prefix, and I am not totally sure if link.exe is reproducible without that. link.exe does share the /pdbaltpath:%_PDB% which is also required.

danakj commented 1 year ago

This issue is fixed, https://github.com/rust-lang/rust/issues/112587 remains for https://github.com/rust-lang/rust/issues/88982

Thank you for all the help here!