-
Notifications
You must be signed in to change notification settings - Fork 402
Use the dummy codegen backend #4648
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
| /// Also disable the MIR pass that inserts an alignment check on every pointer dereference. Miri | ||
| /// does that too, and with a better error message. | ||
| pub const MIRI_DEFAULT_ARGS: &[&str] = &[ | ||
| "-Zcodegen-backend=dummy", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dummy codegen backend is a builtin codegen backend that is only capable of emitting rlibs containing the crate metadata. For every other crate type it emits an error after codegen. This is fine for miri as for --crate-type bin, miri exits before codegen.
…apkin Allow codegen backends to indicate which crate types they support This way cargo will drop the unsupported crate types for crates that specify multiple crate types. This is a prerequisite for rust-lang/miri#4648.
…, r=WaffleLapkin Allow codegen backends to indicate which crate types they support This way cargo will drop the unsupported crate types for crates that specify multiple crate types. This is a prerequisite for rust-lang/miri#4648.
Rollup merge of #148177 - bjorn3:codegen_backend_crate_types, r=WaffleLapkin Allow codegen backends to indicate which crate types they support This way cargo will drop the unsupported crate types for crates that specify multiple crate types. This is a prerequisite for rust-lang/miri#4648.
ac1e234 to
ef6aed4
Compare
This comment has been minimized.
This comment has been minimized.
…es, r=JonathanBrouwer Handle default features and -Ctarget-features in the dummy backend This prevents a warning about ABI relevant target features not being set on x86 and arm. In addition it is required for miri to report correct features in is_*_feature_detected!() if miri switches to the dummy backend. Required for rust-lang/miri#4648
…es, r=JonathanBrouwer Handle default features and -Ctarget-features in the dummy backend This prevents a warning about ABI relevant target features not being set on x86 and arm. In addition it is required for miri to report correct features in is_*_feature_detected!() if miri switches to the dummy backend. Required for rust-lang/miri#4648
…es, r=JonathanBrouwer Handle default features and -Ctarget-features in the dummy backend This prevents a warning about ABI relevant target features not being set on x86 and arm. In addition it is required for miri to report correct features in is_*_feature_detected!() if miri switches to the dummy backend. Required for rust-lang/miri#4648
Rollup merge of #148253 - bjorn3:dummy_backend_target_features, r=JonathanBrouwer Handle default features and -Ctarget-features in the dummy backend This prevents a warning about ABI relevant target features not being set on x86 and arm. In addition it is required for miri to report correct features in is_*_feature_detected!() if miri switches to the dummy backend. Required for rust-lang/miri#4648
a4a79ee to
ff9ef3b
Compare
|
Almost there. With rust-lang/rust#148177 and rust-lang/rust#148253 merged, the entire miri test suite passes after blessing a single change to a query cycle error. Doctests are broken however due to those requiring a check build for bin crates first, which the dummy codegen backend doesn't yet allow. Opened rust-lang/rust#148299 for this. |
5f8c2f8 to
38dd4f6
Compare
|
I hadn't noticed the cargo-miri-test test suite before. It currently has trouble with non-rlib/bin crate types. In particular cargo thinks it needs to build crates only available as cdylib (despite those never being usable) and the |
Allow check builds with binaries for the dummy codegen backend This is likely the last part necessary for rust-lang/miri#4648. Miri needs to be able to do a regular check build for compile_fail doc tests to work.
95b1a48 to
cad7423
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we can get rid of the hacks that patch --emit and --extern, that would definitely neat.
However I am concerned about those "unsupported crate type" warnings, that seems like something Miri users will then also see in the real world. We also need to ensure crate graphs that include cdylib keep working.
test-cargo-miri/Cargo.toml
Outdated
| [dependencies] | ||
| byteorder = "1.0" | ||
| cdylib = { path = "cdylib" } | ||
| #cdylib = { path = "cdylib" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's up with this?
We added cdylib dependencies here because we got bug reports about Miri acting strange on them on real code, so we can't just stop supporting that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dependencies that are both an rlib and cdylib work just fine. However dependencies that are solely a cdylib won't work. But you wouldn't be able to use them anyway. Cargo for some reason insists on still building them, but you would never be able to use them in any other way than copying them from the target dir. It is not an artifact dependency that you can link against through a build script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#1705 was about a crate that mixed cdylib and other crate types.
But still, a cdylib-only crate might be odd or useless, but it worked fine so far so I am concerned about breaking it as people might be relying on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hyper was an rlib+cdylib crate, not a cdylib-only crate. rlib+cdylib still works. Digging through the linked issues, all of them seem to be for rlib+cdylib crates and none for cdylib-only crates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's what I just wrote in the 2nd comment. Hard to say what people rely on though.
What's the error one gets when having a cdylib-only crate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the error one gets when having a cdylib-only crate?
error: cannot produce cdylib for `cdylib v0.1.0 (/home/gh-bjorn3/miri/test-cargo-miri/cdylib)` as the target `aarch64-unknown-linux-gnu` does not support these crate types
I've now moved the cdylib package from a dependency to just a workspace member. This way we still check that the mere presence of a cdylib-only package doesn't break miri, only when using it as dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That error is kind of misleading since it's not about the target, it's about the codegen backend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is unfortunately hard coded in cargo. I thought it was also hard coded in the code parsing the rustc warnings, but looks like that is not the case after all: https://github.com/rust-lang/cargo/blob/4406c1b96413e79ee770f81506a0697fb8f11404/src/cargo/core/compiler/build_context/target_info.rs#L669-L670 I will open a PR to change the rustc warning and see if I can open a PR to change the cargo error.
| @@ -1,2 +1,6 @@ | |||
| UNSUPPORTED CRATE TYPE | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned in the review summary, this isn't great. A lot of crate graphs contain cdylibs, so we'd be showing this warning to users a lot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We used to have code here that skips the build if the crate type is cdylib. That was obviously wrong, I just didn't realize at the time that there can be multiple crate types. But we could maybe instead filter the crate types and remove cdylib (and anything else that's unsupported), and then skip the build if there's no crate type left?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've now pushed a commit to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should then also fix the cdylib-only-dependency, shouldn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't. The dummy codegen backend tells cargo that cdylib is not supported and then cargo reports this error before we can filter the --crate-type cdylib.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, unfortunate.
Allow check builds with binaries for the dummy codegen backend This is likely the last part necessary for rust-lang/miri#4648. Miri needs to be able to do a regular check build for compile_fail doc tests to work.
This comment has been minimized.
This comment has been minimized.
This allows getting rid of all the argument mangling hacks as well as the exported_non_generic_symbols override hack by doing cargo build instead of cargo check. This is also a prerequisite for using native jit mode support in miri that I hope to add to rustc in the future to be used with cg_clif too.
3233570 to
69bc70b
Compare
|
Thank you for contributing to Miri! |
…athanBrouwer Handle default features and -Ctarget-features in the dummy backend This prevents a warning about ABI relevant target features not being set on x86 and arm. In addition it is required for miri to report correct features in is_*_feature_detected!() if miri switches to the dummy backend. Required for rust-lang/miri#4648
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/bin/miri.rs
Outdated
| if config.opts.prints.is_empty() && self.target_crate { | ||
| // Avoid warnings about unsupported crate types | ||
| #[allow(rustc::bad_opt_access)] | ||
| config.opts.crate_types.retain(|&c| c == CrateType::Executable || c == CrateType::Rlib); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably only do this for target crates?
What happens if the list of crate types is empty after the filtering?
Also, why is it okay to allow bad_opt_access here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably only do this for target crates?
This is gated behind if self.target_crate.
What happens if the list of crate types is empty after the filtering?
Currently rustc treats that as implicit --crate-type lib, but in any case it shouldn't be possible to get an empty list here. Cargo will error if none of the crate types are bin or rlib as the dummy codegen backend only lists those crate types as supported.
Also, why is it okay to allow bad_opt_access here?
This is long before the TyCtxt gets created that you are normally supposed to access crate_types through.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently rustc treats that as implicit --crate-type lib, but in any case it shouldn't be possible to get an empty list here. Cargo will error if none of the crate types are bin or rlib as the dummy codegen backend only lists those crate types as supported.
Please add an assertion then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently rustc treats that as implicit --crate-type lib
Actually, it treats it as --crate-type bin just like if the user didn't pass --crate-type at all. I got it confused with the behavior when --crate-type is present on the cli, but all listed crate types are unsupported by the target or codegen backend.
In any case I added an assertion.
|
@rustbot ready |
This allows getting rid of all the argument mangling hacks as well as the exported_non_generic_symbols override hack by doing cargo build instead of cargo check. This is also a prerequisite for using native jit mode support in miri that I hope to add to rustc in the future to be used with cg_clif too.