give rustc_driver users a chance to overwrite the default sysroot #122993

RalfJung · 2024-03-24T11:05:51Z

and then use that in Miri.

This lets us get rid of an annoying arg-patching hack, and may help with rust-lang/miri#3404.

Unfortunately the computation of real_rust_source_base_dir depended on knowing the default sysroot, so I had to move that around a bit. I have no idea how all this Session and Options stuff works so I hope this makes sense.

rustbot · 2024-03-24T11:05:58Z

r? @Nadrieril

rustbot has assigned @Nadrieril.
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-24T11:05:59Z

The Miri subtree was changed

cc @rust-lang/miri

RalfJung · 2024-03-24T11:06:51Z

compiler/rustc_metadata/src/rmeta/decoder.rs

@@ -1604,25 +1608,34 @@ impl<'a, 'tcx> CrateMetadataRef<'a> {
            })
        }

+        let real_rust_source_base_dir = sess.real_rust_source_base_dir();


This is the only place where real_rust_source_base_dir gets called. Should I just move that logic into this file?

Also, should I be worried about perf here? We now re-compute real_rust_source_base_dir each time imported_source_file gets called. Not sure what would be the right place for a cache though.

Before #84233 this got computed after the config callback, in build_session. That would be a lot better. But it seems that was changed for good reasons.

Nadrieril · 2024-03-24T23:01:05Z

I tried to follow but I don't have the context to review this

r? compiler

RalfJung · 2024-03-25T07:18:02Z

FWIW this is blocking cargo miri test for all crates that have doctests, so would be good to get it landed quickly. :)

oli-obk · 2024-03-25T08:39:13Z

@bors try @rust-timer queue

bors · 2024-03-25T08:40:22Z

⌛ Trying commit dedcc8b with merge 92652ed...

give rustc_driver users a chance to overwrite the default sysroot and then use that in Miri. This lets us get rid of an annoying arg-patching hack, and may help with rust-lang/miri#3404. Unfortunately the computation of `real_rust_source_base_dir` depended on knowing the default sysroot, so I had to move that around a bit. I have no idea how all this `Session` and `Options` stuff works so I hope this makes sense.

RalfJung · 2024-03-25T08:49:38Z

I guess this is now doing file system accesses in the middle of some rustc code that is probably in a query? Which is probably not okay as queries are supposed to be pure?

bors · 2024-03-25T10:37:33Z

☀️ Try build successful - checks-actions
Build commit: 92652ed (92652ed58253f3d8f5d0717f1e80a37cc8b65c56)

RalfJung · 2024-03-25T10:57:26Z

FWIW we're going with a different solution for rust-lang/miri#3404 for now. So while cleaning up the config hook interaction with opts.maybe_sysroot may still be nice it is no longer pressing.

WaffleLapkin · 2024-03-25T11:47:26Z

cc @nnethercote since you recently did adjacent refactors

RalfJung · 2024-03-25T12:53:22Z

FWIW we're going with a different solution for rust-lang/miri#3404 for now. So while cleaning up the config hook interaction with opts.maybe_sysroot may still be nice it is no longer pressing.

That turns out not to work currently, it is blocked on cuviper/autocfg#26.

We have another work-around for rust-lang/miri#3404 (just change what rustdoc does), but I think I'd still like to get rid of the argument-parsing-sysroot hack in the Miri driver, which will need either something like this PR (letting the config callback overwrite the default sysroot) or rust-lang/miri#3411.

rust-timer · 2024-03-25T13:44:49Z

Finished benchmarking commit (92652ed): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Warning ⚠: The following benchmark(s) failed to build:

serde-1.0.136

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	872.7%	[10.2%, 10404.6%]	260
Regressions ❌ (secondary)	999.8%	[8.4%, 10336.5%]	261
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	872.7%	[10.2%, 10404.6%]	260

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)	3.2%	[1.8%, 6.2%]	4
Improvements ✅ (primary)	-5.4%	[-11.2%, -1.0%]	18
Improvements ✅ (secondary)	-4.2%	[-8.0%, -2.0%]	7
All ❌✅ (primary)	-5.4%	[-11.2%, -1.0%]	18

Cycles

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)	609.6%	[5.8%, 6978.9%]	260
Regressions ❌ (secondary)	704.7%	[8.0%, 6923.1%]	261
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	609.6%	[5.8%, 6978.9%]	260

Binary size

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

Bootstrap: 671.665s -> 897.895s (33.68%)
Artifact size: 315.02 MiB -> 312.92 MiB (-0.67%)

RalfJung · 2024-03-25T13:50:40Z

Hey I think I get the award for the biggest perf regression ever 😂

michaelwoerister · 2024-03-25T14:03:10Z

I guess this is now doing file system accesses in the middle of some rustc code that is probably in a query? Which is probably not okay as queries are supposed to be pure?

Yes, that would be a problem. Maybe real_rust_source_base_dir could be turned into its own query and then feed as an input into the query system like similar cases:

rust/compiler/rustc_interface/src/queries.rs

Lines 161 to 171 in dda2372

    
           let feed = tcx.feed_local_crate(); 
        
           feed.crate_name(crate_name); 
        
           let feed = tcx.feed_unit_query(); 
        
           feed.features_query(tcx.arena.alloc(rustc_expand::config::features( 
        
               sess, 
        
               &pre_configured_attrs, 
        
               crate_name, 
        
           ))); 
        
           feed.crate_for_resolver(tcx.arena.alloc(Steal::new((krate, pre_configured_attrs)))); 
        
           feed.output_filenames(Arc::new(outputs));

RalfJung · 2024-03-25T19:07:40Z

Hm... yeah, maybe, though that does sound quite complicated.^^ (to someone with no experience with adding new queries, and this 'feed' stuff seems quite sketchy)

Before #84233 we had a simple cache in the Session, but that somehow didn't work because it affects the crate hash or so. Is there no place outside of Config that we can put a cache such that a change invalidates the incremental cache but does not change the crate hash? Also, I'm not even sure if that is needed, #84233 only talks about -remap-path-prefix needing this treatment but not real_rust_source_base_dir.

bjorn3 · 2024-03-25T19:18:34Z

Is there no place outside of Config that we can put a cache such that a change invalidates the incremental cache but does not change the crate hash?

For options we have TRACKED_NO_CRATE_HASH, which real_rust_source_base_dir has been marked as in that PR. This would add the hash to dep_tracking_hash without adding it to crate_hash. This doesn't work for non-options though I think. Feeding it as query is probably the easiest option. Query feeding is basically providing inputs to the incr comp system from which regular queries can then derive (cached) results.

RalfJung · 2024-03-25T19:20:39Z

Yeah a TRACKED_NO_CRATE_HASH is kind of a hack as Options is (in my understanding) meant to reflect user-provided inputs, which real_rust_source_base_dir is not.

michaelwoerister · 2024-03-26T11:22:00Z

Hm... yeah, maybe, though that does sound quite complicated.^^

I can offer to implement the "real_rust_source_base_dir to query" change in a separate PR. However, I wouldn't be able to get to that before tomorrow.

oli-obk · 2024-03-26T12:22:34Z

Adding the query is not the issue, the problem I see is that most of our decoding infra uses Session and not TyCtxt, so we're not able to call queries at all. I'm doing some cleanups right now that will enable this or make it significantly simpler at least. Maybe just cache it with a Once in Session for now?

RalfJung · 2024-03-26T12:27:44Z

This is not urgent any more, rustdoc got patched to avoid the problematic arg-passing cases. And I think for handling --sysroot long-term I prefer the solution in rust-lang/miri#3411, though that is blocked on some crates being updated.

So... it may not be worth doing all that work just for this PR.

Maybe just cache it with a Once in Session for now?

We can just precompute it into the session. That's what happened before #84233. But somehow that was a problem wrt incremental builds or so.

michaelwoerister · 2024-03-26T12:42:43Z

Yeah, all of the [TRACKED] stuff is to account for the fact that queries have access to the session without any actual dependency tracking, so we make sure that things in the various options are virtually constant as far as incr. comp. is concerned. However, all of that is easy to get wrong and preferably none of that data is directly available to queries.

Maybe just cache it with a Once in Session for now?

That sounds like a bug to me because it circumvents dependency tracking. Maybe I'm missing something?

oli-obk · 2024-03-26T13:30:29Z

Since it's not urgent. Let's block it on my unfinished refactor

@rustbot blocked

klensy · 2024-03-26T13:41:10Z

compiler/rustc_metadata/src/rmeta/decoder.rs

@@ -1604,25 +1608,34 @@ impl<'a, 'tcx> CrateMetadataRef<'a> {
            })
        }

+        let real_rust_source_base_dir = sess.real_rust_source_base_dir();


This fn called a lot, you should wrap real_rust_source_base_dir into some once_cell.

Replace some `CrateStore` trait methods with hooks. Just like with the `CrateStore` trait, this avoids the cyclic definition issues with `CStore` being defined after TyCtxt, but needing to be used in TyCtxt. This is work towards unblocking rust-lang#122993 (the next step is giving `Span` deserialization access to the `TyCtxt`)

RalfJung · 2024-03-29T19:37:38Z

I'm going to close this PR, I think rust-lang/miri#3411 is an easier way to handle this and may even be nicer -- it makes all the Miri sysroot handling very explicit by just checking how the Miri driver is invoked.

rustbot assigned Nadrieril Mar 24, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 24, 2024

RalfJung changed the title ~~give rustc_driver users a change to overwrite the default sysroot~~ give rustc_driver users a chance to overwrite the default sysroot Mar 24, 2024

RalfJung force-pushed the default-sysroot branch from a715bad to d5c7ac0 Compare March 24, 2024 11:06

RalfJung commented Mar 24, 2024

View reviewed changes

give rustc_driver users a chance to overwrite the default sysroot

dedcc8b

RalfJung force-pushed the default-sysroot branch from d5c7ac0 to dedcc8b Compare March 24, 2024 11:09

RalfJung mentioned this pull request Mar 24, 2024

cargo miri test is broken for doctests (Option 'sysroot' given more than once) rust-lang/miri#3404

Closed

rustbot assigned lcnr and unassigned Nadrieril Mar 24, 2024

This was referenced Mar 25, 2024

Add TRACKED_NO_CRATE_HASH and use it for --remap-path-prefix #84233

Merged

Allow targets to override default codegen backend #116793

Merged

oli-obk assigned oli-obk and unassigned lcnr Mar 25, 2024

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 25, 2024

This comment has been minimized.

Sign in to view

RalfJung mentioned this pull request Mar 25, 2024

Support argfiles in all our argument munching rust-lang/miri#3412

Open

2 tasks

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Mar 25, 2024

rustbot added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 26, 2024

klensy reviewed Mar 26, 2024

View reviewed changes

oli-obk mentioned this pull request Mar 26, 2024

Replace some CrateStore trait methods with hooks. #123099

Merged

RalfJung closed this Mar 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

give rustc_driver users a chance to overwrite the default sysroot #122993

give rustc_driver users a chance to overwrite the default sysroot #122993

RalfJung commented Mar 24, 2024

rustbot commented Mar 24, 2024

rustbot commented Mar 24, 2024

RalfJung Mar 24, 2024

RalfJung Mar 25, 2024

RalfJung Mar 25, 2024 •

edited

Loading

Nadrieril commented Mar 24, 2024

RalfJung commented Mar 25, 2024

oli-obk commented Mar 25, 2024

This comment has been minimized.

bors commented Mar 25, 2024

RalfJung commented Mar 25, 2024 via email

bors commented Mar 25, 2024

This comment has been minimized.

RalfJung commented Mar 25, 2024

WaffleLapkin commented Mar 25, 2024

RalfJung commented Mar 25, 2024

rust-timer commented Mar 25, 2024

RalfJung commented Mar 25, 2024

michaelwoerister commented Mar 25, 2024

RalfJung commented Mar 25, 2024 •

edited

Loading

bjorn3 commented Mar 25, 2024

RalfJung commented Mar 25, 2024 •

edited

Loading

michaelwoerister commented Mar 26, 2024

oli-obk commented Mar 26, 2024

RalfJung commented Mar 26, 2024

michaelwoerister commented Mar 26, 2024

oli-obk commented Mar 26, 2024

klensy Mar 26, 2024

RalfJung commented Mar 29, 2024

give rustc_driver users a chance to overwrite the default sysroot #122993

give rustc_driver users a chance to overwrite the default sysroot #122993

Conversation

RalfJung commented Mar 24, 2024

rustbot commented Mar 24, 2024

rustbot commented Mar 24, 2024

RalfJung Mar 24, 2024

Choose a reason for hiding this comment

RalfJung Mar 25, 2024

Choose a reason for hiding this comment

RalfJung Mar 25, 2024 • edited Loading

Choose a reason for hiding this comment

Nadrieril commented Mar 24, 2024

RalfJung commented Mar 25, 2024

oli-obk commented Mar 25, 2024

This comment has been minimized.

bors commented Mar 25, 2024

RalfJung commented Mar 25, 2024 via email

bors commented Mar 25, 2024

This comment has been minimized.

RalfJung commented Mar 25, 2024

WaffleLapkin commented Mar 25, 2024

RalfJung commented Mar 25, 2024

rust-timer commented Mar 25, 2024

Overall result: ❌ regressions - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

Binary size

RalfJung commented Mar 25, 2024

michaelwoerister commented Mar 25, 2024

RalfJung commented Mar 25, 2024 • edited Loading

bjorn3 commented Mar 25, 2024

RalfJung commented Mar 25, 2024 • edited Loading

michaelwoerister commented Mar 26, 2024

oli-obk commented Mar 26, 2024

RalfJung commented Mar 26, 2024

michaelwoerister commented Mar 26, 2024

oli-obk commented Mar 26, 2024

klensy Mar 26, 2024

Choose a reason for hiding this comment

RalfJung commented Mar 29, 2024

RalfJung Mar 25, 2024 •

edited

Loading

RalfJung commented Mar 25, 2024 •

edited

Loading

RalfJung commented Mar 25, 2024 •

edited

Loading