Skip to content
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.

Sign up for GitHub

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

[cargo-miri] Don't skip rlib crates #1709

Merged
merged 3 commits into from Feb 14, 2021
Merged

[cargo-miri] Don't skip rlib crates #1709

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8011282
[cargo-miri] Don't skip `rlib` crates
Feb 13, 2021
304cd3e
Make the test more consistent with other tests
Feb 14, 2021
0737de8
Revert the lock file version change
Feb 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 5 additions & 8 deletions cargo-miri/bin.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -596,16 +596,13 @@ fn phase_cargo_rustc(args: env::Args) {
let target_crate = is_target_crate();
let print = get_arg_flag_value("--print").is_some(); // whether this is cargo passing `--print` to get some infos

// rlib and cdylib are just skipped, we cannot interpret them and do not need them
// cdylib is just skipped, we cannot interpret it and do not need it
// for the rest of the build either.
match get_arg_flag_value("--crate-type").as_deref() {
Some("rlib") | Some("cdylib") => {
if verbose {
eprint!("[cargo-miri rustc] (rlib/cdylib skipped)");
}
return;
if get_arg_flag_value("--crate-type").as_deref() == Some("cdylib") {
if verbose {
eprint!("[cargo-miri rustc] (cdylib skipped)");
}
_ => {},
return;
Comment on lines -601 to +605
Copy link
Author

@ghost ghost Feb 14, 2021 •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found that skipping the crate if get_arg_flag_value("--crate-type") is something is probably wrong, because Cargo passes multiple --crate-type to the compiler.

If I add

issue_1567::use_the_dependency();

to test-cargo-miri/src/lib.rs, the test will fail:

## Running `cargo miri` tests
A libstd for Miri is now available in `/my/home/directory/.cache/miri/HOST`.
Testing `cargo miri run` (no isolation)...
--- BEGIN stdout ---
--- END stdout ---
--- BEGIN stderr ---
error: extern location for issue_1567 does not exist: /my/home/directory/miri/test-cargo-miri/target/x86_64-unknown-linux-gnu/debug/deps/libissue_1567.rmeta
 --> src/lib.rs:6:5
  |
6 |     issue_1567::use_the_dependency();
  |     ^^^^^^^^^^

error: aborting due to previous error

error: could not compile `cargo-miri-test`

To learn more, run the command again with --verbose.
--- END stderr ---

TEST FAIL: exit code was 101

I think that is because Cargo passes both --crate-type cdylib and --crate-type rlib to the compiler and only invokes the compiler once. If cargo-miri skips the crate here, the rlib won't be produced.

Maybe just dropping --crate-type cdylib and continuing to compile with only --crate-type rlib (or skip the compilation if there's no more --crate-type) makes more sense.

(EDIT: Hit #1705 when experimenting with that... Trying to fix that together...)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(EDIT: Hit #1705 when experimenting with that... Trying to fix that together...)

So I suggest we tackle that in a future MR. Good catch though, this should be added to the test suite then (together with a crate-type = ["lib", "staticlib", "cdylib"] crate that triggers this behavior).

}

let store_json = |info: CrateRunInfo| {
Expand Down
5 changes: 5 additions & 0 deletions test-cargo-miri/Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion test-cargo-miri/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ edition = "2018"

[dependencies]
byteorder = "1.0"
issue_1567 = { path ="issue-1567" }
issue_1567 = { path = "issue-1567" }
issue_1691 = { path = "issue-1691" }

[dev-dependencies]
rand = { version = "0.7", features = ["small_rng"] }
Expand Down
8 changes: 8 additions & 0 deletions test-cargo-miri/issue-1691/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
[package]
name = "issue_1691"
version = "0.1.0"
authors = ["Miri Team"]
edition = "2018"

[lib]
crate-type = ["rlib"]
3 changes: 3 additions & 0 deletions test-cargo-miri/issue-1691/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
pub fn use_me() -> bool {
true
}
2 changes: 1 addition & 1 deletion test-cargo-miri/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@
/// assert!(cargo_miri_test::make_true());
/// ```
pub fn make_true() -> bool {
true
issue_1691::use_me()
}