moved instrument pass from query to expansion pass

Cargo.lock

@@ -483,6 +483,7 @@ dependencies = [
  "if_chain",
  "itertools 0.9.0",
  "lazy_static 1.4.0",
+ "matches",
  "pulldown-cmark 0.7.0",
  "quine-mc_cluskey",
  "regex-syntax",
@@ -3625,8 +3626,8 @@ name = "rustc_coverage"
 version = "0.0.0"
 dependencies = [
  "log",
- "rustc",
  "rustc_ast",
+ "rustc_errors",
  "rustc_resolve",
  "rustc_span",
  "smallvec 1.0.0",

src/librustc_coverage/Cargo.toml

@@ -10,8 +10,8 @@ path = "lib.rs"
 
 [dependencies]
 log = "0.4"
-rustc = { path = "../librustc" }
+rustc_errors = { path = "../librustc_errors" }
 rustc_resolve = { path = "../librustc_resolve" }
 rustc_ast = { path = "../librustc_ast" }
 rustc_span = { path = "../librustc_span" }
-smallvec = { version = "1.0", features = ["union", "may_dangle"] }
+smallvec = { version = "1.0", features = ["union", "may_dangle"] }

src/librustc_coverage/coverage.rs

@@ -1,10 +1,10 @@
 //! Injects code coverage instrumentation into the AST.
 
-use rustc::util::common::ErrorReported;
 use rustc_ast::ast::*;
 use rustc_ast::mut_visit::{self, MutVisitor};
 use rustc_ast::ptr::P;
 use rustc_ast::token;
+use rustc_errors::ErrorReported;
 use rustc_resolve::Resolver;
 use rustc_span::symbol::sym;
 use rustc_span::symbol::Symbol;
@@ -150,11 +150,8 @@ impl CoverageVisitor<'_, '_> {
     }
 
     fn is_visiting_main(&self) -> bool {
-        if let Some(current_fn) = self.function_stack.last() {
-            *current_fn == sym::main
-        } else {
-            false
-        }
+        // len == 1 ensures a function is being visited and the function is not a nested function
+        self.function_stack.len() == 1 && *self.function_stack.last().unwrap() == sym::main
     }
 
     fn empty_tuple(&mut self, span: Span) -> P<Expr> {
@@ -165,7 +162,6 @@ impl CoverageVisitor<'_, '_> {
             id: self.next_ast_node_id(),
         })
     }
-}
 
     fn wrap_and_count_expr(
         &mut self,
@@ -321,6 +317,9 @@ impl MutVisitor for CoverageVisitor<'_, '_> {
     //        Rust calls parentheticals "Grouped expressions"
     // `?` operator which can invoke the equivalent of an early return (of `Err(...)`).
     //   * {expr_possibly_in_block}?   ->   coverage::counter(n, {expr_possibly_in_block})?
+    //   * Expression initializers for const/static values (where legal to invoke a function?) or
+    //     assign a coverage region to the entire file, and increment a counter if/when it's known
+    //     that the consts and statics were initialized for the file/mod.
     // Any others?
     //   * NOT let var: type = expr (since there's no branching to worry about)
     //   * NOT for or while loop expressions because they aren't optionally executed

src/librustc_interface/passes.rs

@@ -94,8 +94,8 @@ declare_box_region_type!(
 
 /// Runs the "early phases" of the compiler: initial `cfg` processing, loading compiler plugins,
 /// syntax expansion, secondary `cfg` expansion, synthesis of a test
-/// harness if one is to be provided, and injection of a dependency on the
-/// standard library and prelude.
+/// harness if one is to be provided, injection of a dependency on the
+/// standard library and prelude, and name resolution.
 ///
 /// Returns `None` if we're aborting after handling -W help.
 pub fn configure_and_expand(
@@ -138,43 +138,6 @@ pub fn configure_and_expand(
     result.map(|k| (k, resolver))
 }
 
-/// If enabled by command line option, this pass injects coverage statements into the AST to
-/// increment region counters and generate a coverage report at program exit.
-/// The AST Crate cannot be resolved until after the AST is instrumented.
-pub fn instrument(krate: ast::Crate, resolver: &mut Resolver<'_>) -> Result<ast::Crate> {
-    coverage::instrument(krate, resolver)
-}
-
-/// Performs name resolution on the expanded and optionally instrumented crate.
-pub fn resolve_crate(
-    sess: Lrc<Session>,
-    krate: ast::Crate,
-    resolver: &mut Resolver<'_>,
-) -> Result<ast::Crate> {
-    let sess = &*sess;
-
-    resolver.resolve_crate(&krate);
-
-    // FIXME(richkadel): Run tests and confirm that check_crate must go after resolve_crate(),
-    // and if not, can/should I move it into configure_and_expand_inner() (which would mean
-    // running this before instrumentation, if that's OK).
-
-    // FIXME(richkadel): original comment here with small modification:
-
-    // Needs to go *after* expansion and name resolution, to be able to check the results of macro
-    // expansion.
-    sess.time("complete_gated_feature_checking", || {
-        rustc_ast_passes::feature_gate::check_crate(
-            &krate,
-            &sess.parse_sess,
-            &sess.features_untracked(),
-            sess.opts.unstable_features,
-        );
-    });
-
-    Ok(krate)
-}
-
 impl BoxedResolver {
     pub fn to_resolver_outputs(resolver: Rc<RefCell<BoxedResolver>>) -> ResolverOutputs {
         match Rc::try_unwrap(resolver) {
@@ -443,10 +406,30 @@ fn configure_and_expand_inner<'a>(
     }
 
     if sess.opts.debugging_opts.ast_json {
-        // Note this version of the AST will not include injected code, such as coverage counters.
         println!("{}", json::as_json(&krate));
     }
 
+    let instrument_coverage = match sess.opts.debugging_opts.instrument_coverage {
+        Some(opt) => opt,
+        None => false,
+    };
+
+    if instrument_coverage {
+        krate = coverage::instrument(krate, &mut resolver)?;
+    }
+
+    resolver.resolve_crate(&krate);
+
+    // Needs to go *after* expansion to be able to check the results of macro expansion.
+    sess.time("complete_gated_feature_checking", || {
+        rustc_ast_passes::feature_gate::check_crate(
+            &krate,
+            &sess.parse_sess,
+            &sess.features_untracked(),
+            sess.opts.unstable_features,
+        );
+    });
+
     // Add all buffered lints from the `ParseSess` to the `Session`.
     sess.parse_sess.buffered_lints.with_lock(|buffered_lints| {
         info!("{} parse sess buffered_lints", buffered_lints.len());
@@ -455,16 +438,6 @@ fn configure_and_expand_inner<'a>(
         }
     });
 
-    // // Needs to go *after* expansion, to be able to check the results of macro expansion.
-    // sess.time("complete_gated_feature_checking", || {
-    //     rustc_ast_passes::feature_gate::check_crate(
-    //         &krate,
-    //         &sess.parse_sess,
-    //         &sess.features_untracked(),
-    //         sess.opts.unstable_features,
-    //     );
-    // });
-
     Ok((krate, resolver))
 }
 
@@ -748,7 +721,7 @@ impl<'tcx> QueryContext<'tcx> {
     }
 
     pub fn print_stats(&mut self) {
-        self.enter(ty::query::print_stats)
+        self.enter(|tcx| ty::query::print_stats(tcx))
     }
 }
 

src/librustc_interface/queries.rs

@@ -16,7 +16,6 @@ use rustc_middle::util::common::ErrorReported;
 use rustc_session::config::{OutputFilenames, OutputType};
 use rustc_session::{output::find_crate_name, Session};
 use rustc_span::symbol::sym;
-
 use std::any::Any;
 use std::cell::{Ref, RefCell, RefMut};
 use std::mem;
@@ -75,8 +74,6 @@ pub struct Queries<'tcx> {
     parse: Query<ast::Crate>,
     crate_name: Query<String>,
     register_plugins: Query<(ast::Crate, Lrc<LintStore>)>,
-    unresolved_expansion: Query<(ast::Crate, Steal<Rc<RefCell<BoxedResolver>>>, Lrc<LintStore>)>,
-    instrument: Query<(ast::Crate, Steal<Rc<RefCell<BoxedResolver>>>, Lrc<LintStore>)>,
     expansion: Query<(ast::Crate, Steal<Rc<RefCell<BoxedResolver>>>, Lrc<LintStore>)>,
     dep_graph: Query<DepGraph>,
     lower_to_hir: Query<(&'tcx Crate<'tcx>, Steal<ResolverOutputs>)>,
@@ -96,9 +93,7 @@ impl<'tcx> Queries<'tcx> {
             parse: Default::default(),
             crate_name: Default::default(),
             register_plugins: Default::default(),
-            unresolved_expansion: Default::default(),
             expansion: Default::default(),
-            instrument: Default::default(),
             dep_graph: Default::default(),
             lower_to_hir: Default::default(),
             prepare_outputs: Default::default(),
@@ -171,12 +166,10 @@ impl<'tcx> Queries<'tcx> {
         })
     }
 
-    /// Expands built-ins and other macros, but does not perform name resolution, pending potential
-    /// instrumentation.
-    pub fn unresolved_expansion(
+    pub fn expansion(
         &self,
     ) -> Result<&Query<(ast::Crate, Steal<Rc<RefCell<BoxedResolver>>>, Lrc<LintStore>)>> {
-        self.unresolved_expansion.compute(|| {
+        self.expansion.compute(|| {
             let crate_name = self.crate_name()?.peek().clone();
             let (krate, lint_store) = self.register_plugins()?.take();
             let _timer = self.session().timer("configure_and_expand");
@@ -193,46 +186,6 @@ impl<'tcx> Queries<'tcx> {
         })
     }
 
-    /// Returns a modified ast::Crate with injected instrumentation code, if requested by command
-    /// line option.
-    ///
-    /// The `instrument` pass/query depends on (and implies) `unresolved_expansion`.
-    pub fn instrument(
-        &self,
-    ) -> Result<&Query<(ast::Crate, Steal<Rc<RefCell<BoxedResolver>>>, Lrc<LintStore>)>> {
-        self.instrument.compute(|| {
-            let (krate, boxed_resolver, lint_store) = self.unresolved_expansion()?.take();
-            let instrument_coverage = match self.session().opts.debugging_opts.instrument_coverage {
-                Some(opt) => opt,
-                None => false,
-            };
-            if instrument_coverage {
-                boxed_resolver.borrow().borrow_mut().access(|resolver| {
-                    let _timer = self.session().timer("instrument_coverage");
-                    passes::instrument(krate, resolver)
-                })
-            } else {
-                Ok(krate)
-            }
-            .map(|krate| (krate, boxed_resolver, lint_store))
-        })
-    }
-
-    /// Updates the ast::Crate, first querying `instrument`, which queries `unresolved_expansion`.
-    /// After all expansions and instrumentation, performs name resolution on the final AST.
-    pub fn expansion(
-        &self,
-    ) -> Result<&Query<(ast::Crate, Steal<Rc<RefCell<BoxedResolver>>>, Lrc<LintStore>)>> {
-        self.expansion.compute(|| {
-            let (krate, boxed_resolver, lint_store) = self.instrument()?.take();
-            let result = boxed_resolver.borrow().borrow_mut().access(|resolver| {
-                let _timer = self.session().timer("resolve_expansion");
-                passes::resolve_crate(self.session().clone(), krate, resolver)
-            });
-            result.map(|krate| (krate, boxed_resolver, lint_store))
-        })
-    }
-
     pub fn dep_graph(&self) -> Result<&Query<DepGraph>> {
         self.dep_graph.compute(|| {
             Ok(match self.dep_graph_future()?.take() {

src/libstd/coverage.rs

@@ -12,15 +12,15 @@
 //! Example:
 //!
 //! ```
-//! main() {
-//!     let value = if (true) {
+//! fn main() {
+//!     let value = if true {
 //!         std::coverage::count(1, {
 //!             // any expression
 //!             1000
 //!         })
 //!     } else {
 //!         std::coverage::count(2, 500)
-//!     }
+//!     };
 //!     std::coverage::count_and_report(3, ())
 //! }
 //! ```
@@ -37,8 +37,18 @@ static mut COUNTERS: Option<Mutex<HashMap<u128, usize>>> = None;
 
 static INIT: Once = Once::new();
 
+// FIXME(richkadel): Thinking about ways of optimizing executing coverage-instrumented code, I may
+// be able to replace the hashmap lookup with a simple vector index, for most invocations, by
+// assigning an index to each hash, at execution time, and storing that index in an injected static
+// at the site of each counter. If the static is initialized to an unused index (say, u64::MAX), I
+// can pass it into the std::coverage::count() function as mutable, and update it to the next index
+// available. The vector would contain the hashed region IDs and corresponding counts.
+// I just need to be sure the change can be done atomically, and if the same counter is called from
+// multiple threads before the index is assigned, that the behavior is reasonable. (I suppose it
+// *could* even tolerate the possibility that two indexes were assigned. Printing the report could
+// sum the counts for any identical coverage region IDs, if we need to allow for that.)
 #[stable(feature = "coverage", since = "1.44.0")]
-#[inline]
+#[inline(always)]
 fn increment_counter(counter: u128) -> LockResult<MutexGuard<'static, HashMap<u128, usize>>> {
     let mut lock = unsafe {
         INIT.call_once(|| {
@@ -60,10 +70,27 @@ fn increment_counter(counter: u128) -> LockResult<MutexGuard<'static, HashMap<u1
 /// a match pattern arm statement that might not be in a block (but if it is, don't wrap both the
 /// block and the last statement of the block), or a closure statement without braces.
 ///
-///     coverage::count(234234, {some_statement_with_or_without_semicolon()})
+/// ```no_run
+///     # fn some_statement_with_or_without_semicolon() {}
+///     std::coverage::count(234234, {some_statement_with_or_without_semicolon()})
+/// ```
+// Adding inline("always") for now, so I don't forget that some llvm experts thought
+// it might not work to inject the llvm intrinsic inside this function if it is not
+// inlined. Roughly speaking, LLVM lowering may change how the intrinsic and
+// neighboring code is translated, in a way that behaves counterintuitively.
+// (Note that "always" is still only a "recommendation", so this still might not
+// address that problem.) Further investigation is needed, and an alternative
+// solution would be to just expand code into the projected inline version at
+// code injection time, rather than call this function at all.
 #[stable(feature = "coverage", since = "1.44.0")]
-#[inline]
+#[inline(always)]
 pub fn count<T>(counter: u128, result: T) -> T {
+    // FIXME(richkadel): replace increment_counter with a call to the LLVM intrinsic:
+    // ```
+    // declare void @llvm.instrprof.increment(i8* <name>, i64 <hash>,
+    //    i32 <num-counters>, i32 <index>)
+    // ```
+    // See: http://llvm.org/docs/LangRef.html#llvm-instrprof-increment-intrinsic
     let _ = increment_counter(counter);
     result
 }
@@ -73,6 +100,7 @@ pub fn count<T>(counter: u128, result: T) -> T {
 /// if the `main()` has one or more `return` statements. In this case, all returns and the last
 /// statement of `main()` (unless not reachable) should use this function.
 #[stable(feature = "coverage", since = "1.44.0")]
+#[inline(always)]
 pub fn count_and_report<T>(counter: u128, result: T) -> T {
     println!("Print the coverage counters:");
     let mut counters = increment_counter(counter).unwrap();

src/libstd/lib.rs

@@ -465,7 +465,6 @@ pub mod path;
 pub mod process;
 pub mod sync;
 pub mod time;
-pub mod coverage;
 
 #[stable(feature = "futures_api", since = "1.36.0")]
 pub mod task {