applying /Brepro in bootstrap on MSVC and adding a run-make test.

[paradoxicalguy]

View all comments

as per the discussion, this PR applies /Brepro only for bootstrap when building rustc on MSVC, rather than enabling it for all Rust binaries.

it also adds a run-make test that compiles a small PE executable, parses its IMAGE_DEBUG_DIRECTORY using the object crate, and verifies that TimeDateStamp values remain identical across repeated /Brepro builds.

r?@Urgau

[rustbot]

compiler-builtins is developed in its own repository. If possible, consider making this change to rust-lang/compiler-builtins instead.

cc @tgross35

[paradoxicalguy]

oh no

[tgross35]

Whatever the goal is here, would it be possible to write a run-make test that uses gimli/object to check this? Exact binary contents tend to be a bit fragile, as illustrated here...

[tgross35]

Also is there any interaction with trim-paths? #111540

[paradoxicalguy]

(a) for the test, i'll investigate if i can write a run-make test that inspects the relevant PE/COFF metadata using object
(b) im not sure right now, i'll also check any interactions w trim paths

[Urgau]

There seems to be some serious downsides to using /Brepro unconditionally, it apparently breaks incremental linking¹², unsure if it's even possible to use from rustc.

If we do enable it, we probably want to enable it only if -Cincremental isn't passed.

Let's ping some of our Windows target maintainer on this matter, @ChrisDenton @dpaoliello @Fulgen301 @lambdageek.

Footnotes

1. https://stackoverflow.com/questions/75891687/potential-downsides-of-brepro-msvc-linker-option ↩

2. https://reviews.llvm.org/D51635 ↩

[Urgau]

Buck2 seems to enable it unconditionally, but they also disable incremental linking.

[paradoxicalguy]

oh i didn't know that :'(, i'll read those linked pages and wait for target maintainers for their approaches too

[Fulgen301]

I'm not a fan:

• Although popular, this switch is still undocumented, so we shouldn't enable it for every build without some guarantee that it won't go horribly bonkers in the next patch release.
• Why is this in the function that configured debug info? /Brepro is for reproducibility in the resulting binaries, which includes setting the PE header timestamps to deterministic values. Whether a build is reproducible or not shouldn't depend on whether debuginfo is enabled.
• Incremental linking fundamentally doesn't work with reproducible builds due to the extra information the linker adds to the binaries. This is a worse debug profile experience for something that, at least in my opinion, few people will care about during development.

I also don't think such major changes should be done in a PR; this should be a Major Change Proposal at the minimum and / or integrate with existing efforts for reproducible builds.

[paradoxicalguy]

• mcp, not sure this needs one, genuinely asking rather than assuming
  is the concern mainly the incremental-linking regression, or the broader precedent of changing default windows behavior? if it's the latter, i'll look into starting one after other target maintainers have also discussed this. 

and will move this out of debuginfo fn regardless.
open to whatever mitigates the undocumented flag risk too

[paradoxicalguy]

cc @bjorn3

[bjorn3]

The above discussion makes it seem to me like we shouldn't enable it by default inside rustc itself, but could make bootstrap pass it when building everything. We don't use incremental linking for rustc itself anyway.

[paradoxicalguy]

then i'll get to it now, thanks :-)

[Fulgen301]

is the concern mainly the incremental-linking regression, or the broader precedent of changing default windows behavior? if it's the latter, i'll look into starting one after other target maintainers have also discussed this.

Both; /Brepro doesn't have a (known) way to disable it, so users could not opt into incremental linking again, or disable reproducible builds if they needed it. That alone makes me think a MCP is a good idea, because it's quite a major change and should have room to be discussed. (And MCPs aren't heavy.) It feels like something that should be gated behind something unstable and tested first, though I am not very familiar with the best process here.

That's only about all Windows programs in the wild though; enabling it for only rustc should be fine, like bjorn said.

[paradoxicalguy]

@tgross35 were you hyperlinking something here

[ChrisDenton]

If we did want to more directly support this outside of rustc itself then think it would be better to have it be a cargo option or subcommand. Reproducible builds are not something that needs to be implicit and it requires setting more than just /Brepro, as you're no doubt aware. It's more a whole build system thing (rustc could help in some way but, if so, it should do it via an explicit flag imho).

[paradoxicalguy]

just want to make sure i understand correctly: you are saying the narrow "bootstrap passes /Brepro when building rustc itself" approach (what @bjorn3 suggested) is still fine as a self-contained fix,
and the "explicit flag/subcommand" point is about a broader, separate effort for reproducible builds in general? or does this change the shape of what's being proposed here too?
want to make sure i'm not coding up something that's already been steered away from before i put the patch together

[ChrisDenton]

just want to make sure i understand correctly: you are saying the narrow "bootstrap passes /Brepro when building rustc itself" approach (what @bjorn3 suggested) is still fine as a self-contained fix

Yes, absolutely.

My only point was that reproducible builds are ultimately a build system concern and bootstrap is the build system for rustc so it definitely makes sense to put it there.

[tgross35]

@tgross35 were you hyperlinking something here

No; I was referring to how it took multiple attempts to get the desired result :)

[paradoxicalguy]

checked the interaction directly: --remap-path-prefix (trim-paths) and -Clink-arg=/Brepro coexist, compiled and linked cleanly together, and /Brepro's timestamp effect still applies with remap-path-prefix active. so no overlap expected or observed.

[paradoxicalguy]

applied /Brepro conditionally in bootstrap, confirmed its working via two independent full rebuilds (coff TimeDateStamp are identical). while writing the run-make test, hitting an interesting problem: the test produces identical timestamps even with /Brepro removed(not sure why), so expanding the fix to something closer to a real build (lto, more codegen) to properly observe the nondeterminism the flag is addressing. will report back once the test reliably distinguishes the two cases.

[paradoxicalguy]

(i hadn't removed this pr's change in my local clone, thats why the coff TimeDateStamp were coming identical even after removing it in run-make test's arg😭)

[paradoxicalguy]

• Calculate job matrix (web logs, enhanced plaintext logs)

is it because of msvc not being msvc or msvc* something like that

[Kobzol]

@bors try jobs=msvc

[rust-bors]

☀️ Try build successful (CI)
Build commit: 8f072d0 (8f072d051b779dabc0ba832156761f5820cd3978)
Base parent: 55b6bd8 (55b6bd8ecbed23599e966a04508a8a13cbfe90b3)

[paradoxicalguy]

its done :D
bors' new pfp looking fire btw

[Urgau]

@Fulgen301 could you take a look at the PR? I don't access to a Windows machine, and this is pretty Windows specific.

[paradoxicalguy]

@Fulgen301

[Fulgen301]

GitHub did not report the pings at all, yay...

[Fulgen301]

This should also look at the TimeDateStamp field in the COFF file header, currently it only looks at the debug section.

View changes since the review

[paradoxicalguy]

whoops, i'll include that

[paradoxicalguy]

done
x86_64-pc-windows-msvc and i686-pc-windows-msvc; both pass locally too

[paradoxicalguy]

need a msvc bors run on this

[tgross35]

@bors try jobs=msvc

[rust-bors]

☀️ Try build successful (CI)
Build commit: 7d958e4 (7d958e45d1625eaeb0e7a5896dc1404418e3ad8d)
Base parent: 29e68fe (29e68fe2295f8fc2feb52b8cb0b61a055842fdcf)

[paradoxicalguy]

cc @Urgau @Fulgen301

[ChrisDenton]

I'm happy to approve this from a Windows pov, thanks for working on it! Though if @Kobzol has any concerns from a bootstrap perspective then feel free to r-.

@bors r+

[rust-bors]

📌 Commit ae29b37 has been approved by ChrisDenton

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

Reason for tree closure: spurious failures

[paradoxicalguy]

yay (/≧▽≦)/

[Fulgen301]

What Chris said.

Thanks!