Handle GlobalCtxt directly from librustc_interface query system #66791

cjgillot · 2019-11-26T22:26:15Z

This PR constructs the GlobalCtxt as a member of the Queries in librustc_interface.
This simplifies the code to construct it, at the expense of added complexity in the query control flow.
This allows to handle the arenas directly from librustc_interface.

Based on #66707

r? @Zoxc

The construction of the GlobalCtxt is moved from a generator's stack to the Queries struct. Since the GlobalCtxt requires the HIR Forest and the arenas to live longer, those are moved into Queries the same way. The resulting handling of objects is more brittle, because consumers of the Once objects need to be careful of their initialisation.

Zoxc · 2019-11-27T07:04:13Z

src/librustc_interface/passes.rs


-impl BoxedGlobalCtxt {
+impl<'gcx> BoxedGlobalCtxt<'gcx> {
    pub fn enter<F, R>(&mut self, f: F) -> R
    where
        F: for<'tcx> FnOnce(TyCtxt<'tcx>) -> R,


I think this could just be F: FnOnce(TyCtxt<'gcx>) -> R, now.

Zoxc · 2019-11-27T07:04:41Z

src/librustc_interface/passes.rs

+        ty::tls::enter_global(self.0, |tcx| f(tcx))
+    }
+
+    pub fn print_stats(&self) {


I'd just remove this function and let its caller call print_stats directly using enter.

src/librustc_interface/passes.rs

src/librustc_interface/queries.rs

src/librustc_interface/passes.rs

src/librustc_interface/queries.rs

src/librustc_interface/passes.rs

src/librustc/ty/context.rs

Zoxc · 2019-11-27T07:26:15Z

src/librustc_interface/queries.rs

@@ -74,6 +75,8 @@ pub struct Queries<'comp> {
    arenas: Once<AllArenas>,
    forest: Once<hir::map::Forest>,


You can add hir::map::Forest to the list of types in arena.rs with a [few] attribute then we can remove this field and allocate it from Arena.

src/librustc_interface/queries.rs

Zoxc · 2019-11-27T09:17:12Z

r? @Mark-Simulacrum

Zoxc · 2019-11-27T15:53:51Z

This looks good to me now.

Mark-Simulacrum · 2019-11-27T15:58:08Z

@bors r+

bors · 2019-11-27T15:58:10Z

📌 Commit 1e12f39 has been approved by Mark-Simulacrum

bjorn3 · 2019-11-27T23:36:29Z

src/librustc_interface/passes.rs

@@ -740,93 +741,77 @@ pub fn default_provide_extern(providers: &mut ty::query::Providers<'_>) {
    rustc_codegen_ssa::provide_extern(providers);
 }

-declare_box_region_type!(


Can this macro now be removed?

No, it's still used for the resolver.

@Zoxc

Handle GlobalCtxt directly from librustc_interface query system This PR constructs the `GlobalCtxt` as a member of the `Queries` in librustc_interface. This simplifies the code to construct it, at the expense of added complexity in the query control flow. This allows to handle the arenas directly from librustc_interface. Based on rust-lang#66707 r? @Zoxc

@Zoxc

Handle GlobalCtxt directly from librustc_interface query system This PR constructs the `GlobalCtxt` as a member of the `Queries` in librustc_interface. This simplifies the code to construct it, at the expense of added complexity in the query control flow. This allows to handle the arenas directly from librustc_interface. Based on rust-lang#66707 r? @Zoxc

@ghost

Rollup of 11 pull requests Successful merges: - #66379 (Rephrase docs in for ptr) - #66589 (Draw vertical lines correctly in compiler error messages) - #66613 (Allow customising ty::TraitRef's printing behavior) - #66766 (Panic machinery comments and tweaks) - #66791 (Handle GlobalCtxt directly from librustc_interface query system) - #66793 (Record temporary static references in generator witnesses) - #66808 (Cleanup error code) - #66826 (Clarifies how to tag users for assigning PRs) - #66837 (Clarify `{f32,f64}::EPSILON` docs) - #66844 (Miri: do not consider memory allocated by caller_location leaked) - #66872 (Minor documentation fix) Failed merges: r? @ghost

RalfJung · 2019-11-30T08:00:22Z

I think I need help to adjust Miri to these changes... we were using compiler.global_ctxt to enter the compiler's typing context. I tried to update that in rust-lang/miri#1086 but now we ICE immediately:

thread 'rustc' panicked at 'assertion failed: self.try_set(value).is_none()', src/librustc_data_structures/sync.rs:492:9
stack backtrace:
  [...]
  11: std::panicking::rust_panic_with_hook
             at src/libstd/panicking.rs:475
  12: std::panicking::begin_panic
  13: rustc::session::Session::init_features
  14: rustc_interface::passes::register_plugins
  15: rustc_interface::queries::Queries::register_plugins
  16: rustc_interface::queries::Queries::expansion
  17: rustc_interface::queries::Queries::prepare_outputs
  18: rustc_interface::queries::Queries::global_ctxt
  19: <miri::MiriCompilerCalls as rustc_driver::Callbacks>::after_analysis::{{closure}}
             at src/bin/miri.rs:37
  20: rustc_interface::queries::<impl rustc_interface::interface::Compiler>::enter
             at /rustc/8f1bbd69e13c9e04a4c2b75612bc0c31af972439/src/librustc_interface/queries.rs:335
  21: <miri::MiriCompilerCalls as rustc_driver::Callbacks>::after_analysis
             at src/bin/miri.rs:36

RalfJung · 2019-11-30T08:09:56Z

src/librustc_driver/lib.rs

+            if callbacks.after_analysis(compiler) == Compilation::Stop {
+                return early_exit();
+            }


Is it maybe a problem that now, after_analysis is called inside compiler.enter, so that if the callback needs access to the ctx and calls compiler.enter itself, we end up with nested compiler.enter?

That is indeed the problem. The Callbacks are supposed to take &'tcx Queries<'tcx> now to avoid that. So that has to be fixed before Miri can work again. Do you or @cjgillot want to fix that?

There should probably be an assertion to avoid calling enter twice too.

I prepared a PR at #66896 (and tested that this indeed is sufficient to fix Miri).

There should probably be an assertion to avoid calling enter twice too.

I didn't do that as I wouldn't know how.

Sorry for the delay. Do you still need me to do something?

RalfJung · 2019-11-30T13:52:20Z

Don't worry, I am sure you had better things to do on a Saturday morning. I think we are all set, except for that panic to give a better error when nesting "enter" calls.

@Zoxc

pass Queries to compiler callbacks rust-lang#66791 made it impossible to access the tcx in the callbacks; this should fix that. r? @Zoxc

cjgillot added 15 commits November 24, 2019 23:21

Isolate compiler queries inside the Queries type.

266ede1

Move linking ouside the interface queries.

5098ba6

Tidy.

27513a2

Fix test.

becfe5c

Remove wrong comment.

18bb912

Pass Queries by reference.

ea1b803

Simplify early exits.

7d01b6c

Don't move stuff out of Compiler::enter.

8ffc944

Superfluous lifetime.

9d5f721

Reduce righward drift.

68b1ac9

Review nits.

144d1c2

Make the HIR map own the Definitions.

58a9c73

Formatting.

7b71e9a

Move local Arena to Queries.

7e72b36

rust-highfive assigned Zoxc Nov 26, 2019