require a consistent codegen backend when -Zllvm-target-feature is present - #160618
require a consistent codegen backend when -Zllvm-target-feature is present#160618RalfJung wants to merge 2 commits into
Conversation
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
34238f6 to
6bf460d
Compare
| checksum_hash_algorithm: Option<SourceFileHashAlgorithm> = (None, parse_cargo_src_file_hash, [TRACKED], | ||
| "hash algorithm of source files used to check freshness in cargo (`blake3` or `sha256`)"), | ||
| codegen_backend: Option<String> = (None, parse_opt_string, [TRACKED], | ||
| codegen_backend: Option<String> = (None, parse_opt_string, [TRACKED] { TARGET_MODIFIER: CodegenBackend }, |
There was a problem hiding this comment.
This would encode the codegen backend path in the crate metadata if a path is passed to -Zcodegen-backend, right? That would both hurt reproducibility and give false positives if the codegen backend is at a different location on a different machine.
The CodegenBackend trait has a name method that can be used instead.
There was a problem hiding this comment.
Yeah looks like that is what happens.
Unfortunately I don't know how to record that name rather than the actual value of the flag...
There was a problem hiding this comment.
No. Maybe @davidtwco @azhogin have advice for how to best retain a "normalized" value for a target modifier, rather than the actual flag value.
In #160593 I am adding the concept of a "synthetic" target modifier that we could also use, but it's kind of hacky the way I did it so I'll hold off on feedback on that PR before adding more such things.
There was a problem hiding this comment.
The actually loaded codegen backend is not stored in Session, right? So recording its name() might be even more annoying than what I had to do in #160593. :/
There was a problem hiding this comment.
I'm not sure if there's a way to avoid this with the current infrastructure. In #160167, I change to encoding the type of the field itself rather than the string that the user provides (which is subsequently re-parsed), which could make it easier to serialise a reduced form of it (just whether it is LLVM/GCC/Cranelift w/out the specific path) but that would require a little bit of tweaking on what I have.
This comment has been minimized.
This comment has been minimized.
6bf460d to
ce5650d
Compare
rustbot
added
S-waiting-on-author
This comment has been minimized.
This comment has been minimized.
ce5650d to
89fe5bb
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
89fe5bb to
ab764ef
Compare
ab764ef to
6c38dd1
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. |
6 participants
Finishes the implementation of #157753 by ensuring that we don't mix codegen backends when using backend-specific target features (as other backends would ignore those).
The 2nd commit customizes the error we get to be a bit more helpful. I don't know how to do that with the diagnostic structs, so I converted this to use the builder API, which also deduplicated some strings.
r? @folkertdev
Cc @davidtwco @azhogin I hope I am using target modifiers correctly :D