Refactor the way bootstrap invokes `cargo miri` #123192

RalfJung · 2024-03-29T11:29:43Z

Instead of basically doing cargo run --manifest-path=<cargo-miri's manifest> -- miri, let's invoke the cargo-miri binary directly. That means less indirections, and also makes it easier to e.g. run the libcore test suite in Miri. (But there are still other issues with that.)

Also also adjusted Miri's stage numbering so that it is consistent with rustc/rustdoc.

This also makes ./x.py test miri honor --no-doc.

And this fixes #123177 by moving where we handle parallel_compiler.

rustbot · 2024-03-29T11:29:51Z

r? @clubby789

rustbot has assigned @clubby789.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2024-03-29T11:29:53Z

The Miri subtree was changed

cc @rust-lang/miri

RalfJung · 2024-03-29T11:30:26Z

src/bootstrap/src/bin/rustc.rs

@@ -150,7 +150,7 @@ fn main() {
        {
            cmd.arg("-Ztls-model=initial-exec");
        }
-    } else if std::env::var("MIRI").is_err() {


This was added here but at least in local testing is not needed any more. Let's see what CI says.

Cc @Urgau

If it's not needed anymore, that probably mean that Miri is respecting the RUSTFLAGS set by bootstrap (or always passing a target). It's probably not worth checking if there are any --cfg bootstrap or --check-cfg args being passed, since we will quickly see if CI fails.

Good to see this hack disappear.

Miri always passes a target, but that has always been the case. Nothing recently changed wrt RUSTFLAGS either.

But maybe somehow this interacts with the other changes in this PR.

RalfJung · 2024-03-29T11:31:31Z

src/bootstrap/src/core/build_steps/run.rs

@@ -121,7 +121,6 @@ impl Step for ReplaceVersionPlaceholder {

 #[derive(Debug, Clone, PartialEq, Eq, Hash)]
 pub struct Miri {
-    stage: u32,


I realized this field is unnecessary. At least it seems unnecessary, I don't fully understand how this make_run vs run stuff is meant to be used.

src/bootstrap/src/core/build_steps/run.rs

RalfJung · 2024-03-30T12:54:10Z

src/bootstrap/src/core/build_steps/compile.rs

-        // `cfg` option for rustc, `features` option for cargo, for conditional compilation
-        cargo.rustflag("--cfg=parallel_compiler");
-        cargo.rustdocflag("--cfg=parallel_compiler");
-    }


I have traced back every caller of this function and verified that they all call prepare_tool_cargo with one of the modes that triggers the flag.

src/bootstrap/src/core/builder.rs

RalfJung · 2024-03-30T13:53:26Z

src/bootstrap/src/core/build_steps/run.rs

@@ -135,29 +133,35 @@ impl Step for Miri {
    }

    fn make_run(run: RunConfig<'_>) {
-        run.builder.ensure(Miri {
-            stage: run.builder.top_stage,
-            host: run.build_triple(),


run.build_triple() returns builder.cmd.build; I assume that's always the same as builder.build.build?

run.build_triple() returns builder.cmd.build

run.build_triple() returns builder.build.build already.

Ah yes, you are right... and builder.cmd.build does not exist.
But some places use builder.config.build. That is hopefully the same as builder.build.build.

Yes, they are identical.

src/bootstrap/src/core/builder.rs

onur-ozkan · 2024-03-30T14:10:21Z

src/bootstrap/src/core/build_steps/test.rs

-            "run",
-            "src/tools/miri/cargo-miri",
+            target_compiler,
+            Mode::ToolStd, // it's unclear what to use here, we're not building anything just doing a smoke test!


ToolBootstrap is the right choice if you don't build anything.

ref:

rust/src/bootstrap/src/core/build_steps/tool.rs

Lines 79 to 87 in 40116ad

match self.mode {

Mode::ToolRustc => {

builder.ensure(compile::Std::new(compiler, compiler.host));

builder.ensure(compile::Rustc::new(compiler, target));

}

Mode::ToolStd => builder.ensure(compile::Std::new(compiler, target)),

Mode::ToolBootstrap => {} // uses downloaded stage0 compiler libs

_ => panic!("unexpected Mode for tool build"),

}

That doesn't work for stages > 0 though, because of this assertion:

rust/src/bootstrap/src/core/builder.rs

Lines 1461 to 1462 in e990cde

let use_snapshot = mode == Mode::ToolBootstrap;

assert!(!use_snapshot || stage == 0 || self.local_rebuild);

src/bootstrap/src/core/builder.rs

RalfJung · 2024-03-30T14:31:39Z

src/bootstrap/src/core/build_steps/test.rs

+        // we'd have stageN/bin/rustc and stageN/bin/rustdoc be effectively different stage
+        // compilers, which isn't what we want. Rustdoc should be linked in the same way as the
+        // rustc compiler it's paired with, so it must be built with the previous stage compiler.
+        let host_compiler = builder.compiler(stage - 1, host);


I realized this is still not quite consistent with rustdoc. In rustdoc the stage adjustment is done in the tool step, not the test step. But the tool steps are macro-generated for a bunch of tools and I don't want to do a change that could break clippy or so.^^

And arguably rustdoc is the weird one here; ./x.py build compiler/rustc --stage 0 and ./x.py build miri --stage 0 should be consistent (and they are with this PR); ./x.py build rustdoc --stage 0 is a NOP that does nothing.

…he cargo-miri smoke test and Miri sysroot build

onur-ozkan

Changes looks good to me, if you don't plan to do further changes you can r=me.

onur-ozkan · 2024-03-30T22:49:51Z

@bors rollup=iffy

RalfJung · 2024-03-31T07:14:44Z

I think this is good to go.
@bors r=onur-ozkan

bors · 2024-03-31T07:14:47Z

📌 Commit 5b7b4b6 has been approved by onur-ozkan

It is now in the queue for this repository.

ChrisDenton · 2024-03-31T18:55:26Z

See https://learn.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-search-order#search-order-for-unpackaged-apps

Skip to the seventh bullet point and onwards. Rust doesn't do anything to help so it just does the default thing.

Edit: sorry for the echo, i didn't see the above post

RalfJung · 2024-03-31T20:07:44Z

Miri isn't really different from regular rustc here though, so it would be rather strange if it needed special treatment... How does bootstrap usually provide the right libraries?

I'm not sure if I'll be able to debug this by just throwing spaghetti at the CI runners until it sticks. This may need someone with a Windows machine to play around with the code.

We now have two invocations of cargo-miri shortly after each other: first to build the sysroot, then to run cargo miri test in the test-cargo-miri crate. I reverted the first invocation back to the old way (that I no longer want to use, if possible, as there's too many wrappers of nesting cargo inside each other), which is still working. The second invocation fails. This should be all the same binaries and very similar environment variables (and it seems like environment variables don't really matter anyway, except for PATH?), the only difference is

The first invocation is executed via cargo run --manifest-path=.../cargo-miri/Cargo.toml -- <arguments>
The second invocation directly calls the cargo-miri binary, instead of invoking it via cargo run.

Is cargo run doing some magic to make more libraries available to the program, like adding some folder to the PATH?

RalfJung · 2024-03-31T20:18:23Z

Hm, I also found something interesting in the clippy code. Sadly with no comments explaining it. Instead of calling add_rustc_lib_path (which would be a NOP on Windows), they add some stuff to helpers::dylib_path_var(), which expands to PATH on Windows... But then they also immediately overwrite the PAH again with something else. I'm not sure if the clippy code works on Windows, but I'll try doing the "adding stuff to helpers::dylib_path_var() part", maybe that helps.

I thought it would obviously be better to call add_rustc_lib_path instead of inlining it, but maybe its Windows behavior isn't actually helpful?

danielframpton · 2024-03-31T20:50:21Z

The basic difference I can see between rustc.exe and miri.exe is that rustc.exe depends on the std and rustc_driver next to the binary, while miri.exe does not. By that I mean:

build\x86_64-pc-windows-msvc\stage1\bin\ contains (ignoring other things):

build\x86_64-pc-windows-msvc\stage1\bin\cargo-miri.exe
build\x86_64-pc-windows-msvc\stage1\bin\miri.exe
build\x86_64-pc-windows-msvc\stage1\bin\rustc.exe
build\x86_64-pc-windows-msvc\stage1\bin\rustc_driver-7c717ce3fea54cb2.dll
build\x86_64-pc-windows-msvc\stage1\bin\std-16ca2320259178a1.dll

build\x86_64-pc-windows-msvc\stage2\bin\ contains (ignoring other things):

build\x86_64-pc-windows-msvc\stage2\bin\rustc.exe
build\x86_64-pc-windows-msvc\stage2\bin\rustc_driver-0c0d6e343a54bdfc.dll
build\x86_64-pc-windows-msvc\stage2\bin\std-86a64ad04678906d.dll

The dependencies for rustc.exe point to the dlls in the same location:

link /dump /dependents build\x86_64-pc-windows-msvc\stage1\bin\rustc.exe shows:

rustc_driver-7c717ce3fea54cb2.dll
std-16ca2320259178a1.dll

link /dump /dependents build\x86_64-pc-windows-msvc\stage2\bin\rustc.exe shows:

rustc_driver-0c0d6e343a54bdfc.dll
std-86a64ad04678906d.dll

However, the dependencies for miri.exe in stage1\bin are the dlls in stage2\bin:

link /dump /dependents build\x86_64-pc-windows-msvc\stage1\bin\miri.exe shows these deps:

rustc_driver-0c0d6e343a54bdfc.dll
std-86a64ad04678906d.dll

RalfJung · 2024-03-31T21:00:03Z

@danielframpton thanks! So sounds to me like the staging mismatch I mentioned here and here is actually a problem on Windows. In rustc we are doing a stage increment in the Assemble step, which I guess arranges for things to be lined up properly on Windows. But Miri has nothing like that so we have to work around it by adding more things to the PATH I guess.

Linux also has a bit of stage weirdness here in that we are indeed adding the stage2 compiler's lib paths when running the stage 1 Miri. I don't know why there's an off-by-1 here. But we did this with add_rustc_lib_path which does nothing on Windows.

The latest patch adds self.rustc_lib_paths(run_compiler) to the PATH the same way it added it to the lib dir on Linux. So maybe that's actually the right call then. CI is looking promising, actually. EDIT: yes indeed, all tests passed. :) (and then it ran into the exit 1 I had added deliberately)

RalfJung · 2024-03-31T21:06:49Z

FWIW it's also strange that on Windows the Miri binary is in stage1/stage2, not in stage1-tools or stage1-tools-bin or so, like it is on Linux. I'm just not going to question that...

danielframpton · 2024-03-31T21:14:23Z

The stage1/bin location is where the binary is actually run from (when it fails). It is also in stage1-tools-bin and the dlls it actually depends have also been copied into that folder.

I noticed that clippy-driver.exe is in stage2-tools-bin and stage2/bin.

RalfJung · 2024-03-31T21:21:21Z

The stage1/bin location is where the binary is actually run from (when it fails). It is also in stage1-tools-bin and the dlls it actually depends have also been copied into that folder.

Oh, interesting. On Linux it's run from the tools-bin directory.

So if we were running it from the stage1-tools-bin folder things would actually work? Very strange.

I noticed that clippy-driver.exe is in stage2-tools-bin and stage2/bin.

Hm... but the build steps for Miri and Clippy are generated by the same macro.

Does clippy also have the off-by-1, where the clippy-driver.exe in stage1 needs the libs from stage2?

RalfJung · 2024-03-31T21:22:57Z

@onur-ozkan I think we have something that works on Windows, though I don't yet fully understand what exactly causes the stage mismatch. Are you fine with landing b08b06e ? That's the only major difference compared to before (the other difference is just a bit more logging).

(Don't approve right now, we still have the Windows builder in PR CI to double-check that this does indeed all work now.)

RalfJung · 2024-03-31T21:28:59Z

Oh, interesting. On Linux it's run from the tools-bin directory.

This may be a staging difference, not a platform difference. I do most of my testing with x.py test miri --stage 1, which then builds the stage 0 Miri and runs it to test a stage 1 sysroot. And we have this:

rust/src/bootstrap/src/core/build_steps/tool.rs

Lines 822 to 840 in 288daeb

    
           if (false $(|| !$add_bins_to_sysroot.is_empty())?) && $sel.compiler.stage > 0 { 
        
               let bindir = $builder.sysroot($sel.compiler).join("bin"); 
        
               t!(fs::create_dir_all(&bindir)); 
        
               #[allow(unused_variables)] 
        
               let tools_out = $builder 
        
                   .cargo_out($sel.compiler, Mode::ToolRustc, $sel.target); 
        
               $(for add_bin in $add_bins_to_sysroot { 
        
                   let bin_source = tools_out.join(exe(add_bin, $sel.target)); 
        
                   let bin_destination = bindir.join(exe(add_bin, $sel.compiler.host)); 
        
                   $builder.copy_link(&bin_source, &bin_destination); 
        
               })? 
        
               let tool = bindir.join(exe($tool_name, $sel.compiler.host)); 
        
               tool 
        
           } else { 
        
               tool 
        
           }

So for stages other than 0 we do some adjustment of the path returned for where to find the tool. That adjustment is strangely also tied to whether add_bins_to_sysroot is empty. And that adjustment breaks running the tool on Windows as it copies the binary to a location that has the libraries from the wrong stage.

@onur-ozkan you wrote that code in 6d99d6a. Is it possible that this copies clippy/miri to the wrong sysroot, i.e. not the one that has the libraries they need?

Specifically, let bindir = $builder.sysroot($sel.compiler).join("bin"); is suspicious. That's the sysroot of the compiler that was just used to build this tool, which is one stage down from the programs that the tool will be used to run.

EDIT: I can confirm that indeed when I use --stage 2, it runs the Miri from stage1/bin with the libraries from stage2, same as on Windows. Except that on Linux this just works because add_rustc_lib_path actually does its job...

RalfJung · 2024-03-31T22:27:43Z

Okay Windows CI is still happy. :) PR is ready to land again.

RalfJung · 2024-03-31T22:28:07Z

Thanks a lot @danielframpton for the help!

onur-ozkan · 2024-04-01T07:05:34Z

@onur-ozkan you wrote that code in 6d99d6a. Is it possible that this copies clippy/miri to the wrong sysroot, i.e. not the one that has the libraries they need?

Those binaries copied from stageN-tools-bin into stageN/bin. So expect that to be fairly correct.

@bors r+

bors · 2024-04-01T07:05:37Z

📌 Commit 85d460e has been approved by onur-ozkan

It is now in the queue for this repository.

bors · 2024-04-01T07:20:00Z

⌛ Testing commit 85d460e with merge 871df0d...

RalfJung · 2024-04-01T08:06:53Z

Those binaries copied from stageN-tools-bin into stageN/bin. So expect that to be fairly correct.

I'm pretty sure that's wrong. We usually copy e.g. from stage0-rustc to stage1/bin, so tools should also have such a stage increment I think.
But anyway what's a separate issue.

bors · 2024-04-01T09:20:55Z

☀️ Test successful - checks-actions
Approved by: onur-ozkan
Pushing 871df0d to master...

rust-timer · 2024-04-01T12:58:37Z

Finished benchmarking commit (871df0d): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.6%	[-1.6%, -1.6%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.6%	[-1.6%, -1.6%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 667.101s -> 667.778s (0.10%)
Artifact size: 315.69 MiB -> 315.75 MiB (0.02%)

x.py test miri: respect --no-doc / --doc

fd7909a

rustbot assigned clubby789 Mar 29, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Mar 29, 2024

RalfJung commented Mar 29, 2024

View reviewed changes

RalfJung force-pushed the bootstrap-test-miri branch 2 times, most recently from 161b0e0 to cd2dbe9 Compare March 30, 2024 11:54

RalfJung commented Mar 30, 2024

View reviewed changes

src/bootstrap/src/core/build_steps/run.rs Show resolved Hide resolved

RalfJung mentioned this pull request Mar 30, 2024

Running ./x.py test miri --stage 0 twice rebuilds miri, cargo-miri, and rustdoc #123177

Closed

RalfJung commented Mar 30, 2024

View reviewed changes

RalfJung force-pushed the bootstrap-test-miri branch 3 times, most recently from 25f030b to 2c7ec0e Compare March 30, 2024 12:59

onur-ozkan reviewed Mar 30, 2024

View reviewed changes

src/bootstrap/src/core/builder.rs Show resolved Hide resolved

RalfJung force-pushed the bootstrap-test-miri branch from 2c7ec0e to 24ba73c Compare March 30, 2024 13:52

RalfJung commented Mar 30, 2024

View reviewed changes

RalfJung force-pushed the bootstrap-test-miri branch from 24ba73c to 1dd0f30 Compare March 30, 2024 13:54

onur-ozkan reviewed Mar 30, 2024

View reviewed changes

RalfJung commented Mar 30, 2024

View reviewed changes

RalfJung force-pushed the bootstrap-test-miri branch 2 times, most recently from e990cde to f4ff9c6 Compare March 30, 2024 14:47

prepare_tool_cargo: add support for a miri-test mode, and use it in t…

2a93942

…he cargo-miri smoke test and Miri sysroot build

RalfJung force-pushed the bootstrap-test-miri branch from f4ff9c6 to 5b7b4b6 Compare March 30, 2024 16:09

onur-ozkan approved these changes Mar 30, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

fix not finding the right libraries on Windows

b08b06e

RalfJung force-pushed the bootstrap-test-miri branch from 877d577 to 294fa81 Compare March 31, 2024 21:11

checktools: make it easier to trace what is happening

85d460e

RalfJung force-pushed the bootstrap-test-miri branch from 294fa81 to 85d460e Compare March 31, 2024 22:27

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 1, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 1, 2024

bors merged commit 871df0d into rust-lang:master Apr 1, 2024
12 checks passed

rustbot added this to the 1.79.0 milestone Apr 1, 2024

RalfJung deleted the bootstrap-test-miri branch April 1, 2024 09:36

	match self.mode {
	Mode::ToolRustc => {
	builder.ensure(compile::Std::new(compiler, compiler.host));
	builder.ensure(compile::Rustc::new(compiler, target));
	}
	Mode::ToolStd => builder.ensure(compile::Std::new(compiler, target)),
	Mode::ToolBootstrap => {} // uses downloaded stage0 compiler libs
	_ => panic!("unexpected Mode for tool build"),
	}

	let use_snapshot = mode == Mode::ToolBootstrap;
	assert!(!use_snapshot \|\| stage == 0 \|\| self.local_rebuild);

Refactor the way bootstrap invokes cargo miri #123192

Refactor the way bootstrap invokes cargo miri #123192

Conversation

RalfJung commented Mar 29, 2024 • edited

rustbot commented Mar 29, 2024

rustbot commented Mar 29, 2024

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

RalfJung Mar 30, 2024 • edited

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

RalfJung Mar 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onur-ozkan left a comment

Choose a reason for hiding this comment

onur-ozkan commented Mar 30, 2024

RalfJung commented Mar 31, 2024

bors commented Mar 31, 2024

ChrisDenton commented Mar 31, 2024 • edited

This comment has been minimized.

RalfJung commented Mar 31, 2024 • edited

RalfJung commented Mar 31, 2024

danielframpton commented Mar 31, 2024

RalfJung commented Mar 31, 2024 • edited

This comment has been minimized.

RalfJung commented Mar 31, 2024

danielframpton commented Mar 31, 2024 • edited

RalfJung commented Mar 31, 2024 • edited

RalfJung commented Mar 31, 2024

RalfJung commented Mar 31, 2024 • edited

RalfJung commented Mar 31, 2024

RalfJung commented Mar 31, 2024

onur-ozkan commented Apr 1, 2024

bors commented Apr 1, 2024

bors commented Apr 1, 2024

RalfJung commented Apr 1, 2024 • edited

bors commented Apr 1, 2024

rust-timer commented Apr 1, 2024

Overall result: no relevant changes - no action needed

Refactor the way bootstrap invokes `cargo miri` #123192

Refactor the way bootstrap invokes `cargo miri` #123192

RalfJung commented Mar 29, 2024 •

edited

RalfJung Mar 30, 2024 •

edited

RalfJung Mar 30, 2024 •

edited

ChrisDenton commented Mar 31, 2024 •

edited

RalfJung commented Mar 31, 2024 •

edited

RalfJung commented Mar 31, 2024 •

edited

danielframpton commented Mar 31, 2024 •

edited

RalfJung commented Mar 31, 2024 •

edited

RalfJung commented Mar 31, 2024 •

edited

RalfJung commented Apr 1, 2024 •

edited