Support when `build-std` is set for Cargo #3078

ghost · 2023-09-23T23:47:13Z

See #2705. This is my first contribution to a rust-lang project, and I am not extremely familiar with the internals of Miri and Cargo, so I apologize if there obvious deficiencies in this PR (besides the ones I am about to list, which are:)

in the case that build-std is set, cargo-miri will not set MIRI_SYSROOT, which may be problematic for things that expect MIRI_SYSROOT to always be set after cargo miri setup
--print-syroot is ignored by cargo miri setup when build-std is set (but what else can it do)
the Miri driver parses the value of --crate-name from the CLI arguments using a dedicated function, but ideally we'd want to borrow functionality from the arg module in cargo-miri?
it may be possible for a user to name their crate std or core or something, in which case, if build-std is also set, the Miri driver will naively assume that crate is really part of the standard library and disable debug assertions, enable overflow checks, etc. (I'm not actually sure if this can happen or not; someone please clarify if I'm wrong here)
there may be some special flags we set for the Miri sysroot that I have forgotten to pass along in the build-std case

I have created a test crate with build-std set and a target spec for the PlayStation 2 CPU, with the expectation that Miri will see that build-std is set and skip building the sysroot. I am not actually able to test this at the moment.

ghost · 2023-09-24T18:02:28Z

src/bin/miri.rs

+                | "compiler_builtins"
+            ) {
+                // We are compiling the standard library.
+                args.push("-Cdebug-assertions=off".into());


I have also written a version of this which attempts to fix existing "-Cdebug-assertions", "-Coverflow-checks", etc. options if they are present (and also accounting for, e.g., if "-C" and "debug-assertions" are separate arguments), and only resorts to appending new arguments if the options are not present. However, I'm not sure if this extra complexity is warranted, because I think rustc can handle duplicate codegen options just fine, so probably the only benefit would be to make the rustc invocation "nicer" for someone debugging the interaction between miri and rustc

RalfJung · 2023-09-24T22:05:55Z

Thanks for the PR! :) It will be reviewed eventually but that might take a bit, at least I am rather busy currently.

I am quite concerned about that list of crate names in the Miri driver, that sounds like something that we really want to avoid.

RalfJung · 2023-10-09T16:32:53Z

cargo-miri/src/util.rs

+
+/// Checks if Cargo's build-std setting is enabled for one or more crates.
+pub fn is_build_std_set() -> bool {
+    let cmd = cargo()


You need to pass cargo_extra_flags() as arguments to ensure that if the user sets --cfg unstable.build-std that will be respected. Also what about -Zbuild-std? I don't think I understand how these flags even interact.

It might be worth having a little helper function that invokes cargo config to get the value of an arbitrary key.

RalfJung · 2023-10-09T16:33:29Z

cargo-miri/src/util.rs

+        .output()
+        .expect("failed to get Cargo config");
+    // TODO: Cargo will exit with an error code if the `unstable.build-std` key is not set. Should
+    // we still return false if an unrelated error has occurred?


Ideally we'd distinguish these cases. Which status code does cargo return when the key is absent? The least we can do is raise an error on any unexpected status code.

Well, I think Cargo returns 101 for most errors (unless I am misunderstanding your question). This includes when the key is absent but also, e.g., when the key a.b cannot be resolved in the first place because a exists and is not a table. I'm sure there are many other error cases for cargo config get which return 101.

Relevant Cargo code: https://github.com/rust-lang/cargo/blob/f5418fcfebe7d01536b891c8b2ceb32c4eaf098c/src/cargo/util/errors.rs#L324C1-L341C2

Okay, that's unfortunate.

But still you can then treat only error 101 as "absent" and all other codes as actual errors.

RalfJung · 2023-10-09T16:35:30Z

cargo-miri/src/phases.rs

+            println!("WARNING: `MIRI_NO_STD` has no effect when Cargo has `build-std` enabled.");
+        }
+    }
+    let cargo_cmd = match subcommand {


Why did you move the cargo_cmd logic up? This is confusing now, since the match has too many jobs.

Also we want to call setup before any of the other logic here. I think this should be more like

let miri_sysroot = if has_build_std { Some(setup(...)) } else { None };

RalfJung · 2023-10-09T16:36:36Z

src/bin/miri.rs

-                    "Miri was invoked in 'target' mode without `MIRI_SYSROOT` or `--sysroot` being set"
-                    )
-            });
+        if env::var_os("MIRI_STD_AWARE_CARGO").is_some() {


I would strongly prefer if we could avoid having such hacks in the Miri driver. This should all be taken care of by cargo-miri.

cargo-miri already injects itself into every invocation from cargo, so it should be entirely possible to do any further command-line argument adjustments there rather than doing them inside the Miri driver.

RalfJung · 2023-10-09T16:38:25Z

src/bin/miri.rs

+                | "compiler_builtins"
+            ) {
+                // We are compiling the standard library.
+                args.push("-Cdebug-assertions=off".into());


Why do we want this? If the user has enabled debug assertions then we should honor that. We should behave like regular cargo run, which for build-std will also apply flags to the std build.

Miri no longer sets -Cdebug-assertions=on either, so I see no reason why we'd start messing with -C flags again.

RalfJung · 2023-10-09T16:38:49Z

src/bin/miri.rs

+            // See `cargo_miri::phase_rustc`.
+            if crate_name == "panic_abort" {
+                args.push("-Cpanic=abort".into());
+            }


Is this really needed? If build-std with regular rustc doesn't need it then we shouldn't need it either. AFAIK build-std will simply only build the panic runtime that is actually needed for this crate and this avoids the issue that comes up when building the panic runtime for a sysroot.

RalfJung · 2023-10-09T16:42:29Z

test-cargo-miri/Cargo.toml

@@ -16,6 +16,7 @@ issue_1567 = { path = "issue-1567" }
 issue_1691 = { path = "issue-1691" }
 issue_1705 = { path = "issue-1705" }
 issue_1760 = { path = "issue-1760" }
+issue_2705 = { path = "issue-2705" }


I was expecting we would add something somewhere that calls cargo miri run -Zbuild-std; why is this a separate crate?

bors · 2024-01-07T14:29:02Z

☔ The latest upstream changes (presumably #3259) made this pull request unmergeable. Please resolve the merge conflicts.

norepimorphism added 2 commits September 23, 2023 19:15

take first steps toward build-std support

f163dd3

oops that was embarrassing

01b95d8

ghost commented Sep 24, 2023

View reviewed changes

RalfJung reviewed Oct 9, 2023

View reviewed changes

RalfJung added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Oct 9, 2023

RalfJung mentioned this pull request Oct 27, 2023

Tracking Issue for cargo config rust-lang/cargo#9301

Open

13 tasks

ghost closed this by deleting the head repository Jan 15, 2024

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support when `build-std` is set for Cargo #3078

Support when `build-std` is set for Cargo #3078

ghost commented Sep 23, 2023 •

edited by ghost

Loading

ghost Sep 24, 2023

RalfJung commented Sep 24, 2023 •

edited

Loading

RalfJung Oct 9, 2023

RalfJung Oct 9, 2023

ghost Oct 23, 2023

RalfJung Oct 27, 2023

RalfJung Oct 9, 2023

RalfJung Oct 9, 2023

RalfJung Oct 9, 2023

RalfJung Oct 9, 2023

RalfJung Oct 9, 2023

bors commented Jan 7, 2024

Support when build-std is set for Cargo #3078

Support when build-std is set for Cargo #3078

Conversation

ghost commented Sep 23, 2023 • edited by ghost Loading

Choose a reason for hiding this comment

RalfJung commented Sep 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Jan 7, 2024

Support when `build-std` is set for Cargo #3078

Support when `build-std` is set for Cargo #3078

ghost commented Sep 23, 2023 •

edited by ghost

Loading

RalfJung commented Sep 24, 2023 •

edited

Loading