Auto merge of #151739 - Zalathar:short-backtrace, r=fee1-dead

Use fewer intermediate functions for short backtraces in queries

If we make sure that `compute_fn` in the query's vtable is actually named `__rust_begin_short_backtrace`, we can avoid the need for some additional intermediate functions and stack frames.

This is similar to how the `get_query_incr` and `get_query_non_incr` functions are actually named `__rust_end_short_backtrace`.

---

Before/after comparison: #151739 (comment)

---

- Earlier draft of this PR: #151719
- Introduction of this backtrace-trimming: #108938

Author: bors

compiler/rustc_middle/src/query/plumbing.rs

@@ -45,7 +45,7 @@ pub struct QueryVTable<'tcx, C: QueryCache> {
     pub query_cache: usize,
     pub will_cache_on_disk_for_key_fn: Option<WillCacheOnDiskForKeyFn<'tcx, C::Key>>,
     pub execute_query: fn(tcx: TyCtxt<'tcx>, k: C::Key) -> C::Value,
-    pub compute: fn(tcx: TyCtxt<'tcx>, key: C::Key) -> C::Value,
+    pub compute_fn: fn(tcx: TyCtxt<'tcx>, key: C::Key) -> C::Value,
     pub try_load_from_disk_fn: Option<TryLoadFromDiskFn<'tcx, C::Key, C::Value>>,
     pub is_loadable_from_disk_fn: Option<IsLoadableFromDiskFn<'tcx, C::Key>>,
     pub hash_result: HashResult<C::Value>,

compiler/rustc_query_impl/src/lib.rs

@@ -29,7 +29,7 @@ use rustc_query_system::query::{
 };
 use rustc_span::{ErrorGuaranteed, Span};
 
-use crate::plumbing::{__rust_begin_short_backtrace, encode_all_query_results, try_mark_green};
+use crate::plumbing::{encode_all_query_results, try_mark_green};
 use crate::profiling_support::QueryKeyStringCache;
 
 #[macro_use]
@@ -123,7 +123,7 @@ where
 
     #[inline(always)]
     fn compute(self, qcx: QueryCtxt<'tcx>, key: Self::Key) -> Self::Value {
-        (self.vtable.compute)(qcx.tcx, key)
+        (self.vtable.compute_fn)(qcx.tcx, key)
     }
 
     #[inline(always)]

compiler/rustc_query_impl/src/plumbing.rs

@@ -566,18 +566,6 @@ macro_rules! expand_if_cached {
     };
 }
 
-/// Don't show the backtrace for query system by default
-/// use `RUST_BACKTRACE=full` to show all the backtraces
-#[inline(never)]
-pub(crate) fn __rust_begin_short_backtrace<F, T>(f: F) -> T
-where
-    F: FnOnce() -> T,
-{
-    let result = f();
-    std::hint::black_box(());
-    result
-}
-
 // NOTE: `$V` isn't used here, but we still need to match on it so it can be passed to other macros
 // invoked by `rustc_with_all_queries`.
 macro_rules! define_queries {
@@ -636,6 +624,32 @@ macro_rules! define_queries {
                 }
             }
 
+            /// Defines a `compute` function for this query, to be used as a
+            /// function pointer in the query's vtable.
+            mod compute_fn {
+                use super::*;
+                use ::rustc_middle::queries::$name::{Key, Value, provided_to_erased};
+
+                /// This function would be named `compute`, but we also want it
+                /// to mark the boundaries of an omitted region in backtraces.
+                #[inline(never)]
+                pub(crate) fn __rust_begin_short_backtrace<'tcx>(
+                    tcx: TyCtxt<'tcx>,
+                    key: Key<'tcx>,
+                ) -> Erased<Value<'tcx>> {
+                    #[cfg(debug_assertions)]
+                    let _guard = tracing::span!(tracing::Level::TRACE, stringify!($name), ?key).entered();
+
+                    // Call the actual provider function for this query.
+                    let provided_value = call_provider!([$($modifiers)*][tcx, $name, key]);
+                    rustc_middle::ty::print::with_reduced_queries!({
+                        tracing::trace!(?provided_value);
+                    });
+
+                    provided_to_erased(tcx, provided_value)
+                }
+            }
+
             pub(crate) fn make_query_vtable<'tcx>()
                 -> QueryVTable<'tcx, queries::$name::Storage<'tcx>>
             {
@@ -652,22 +666,7 @@ macro_rules! define_queries {
                         None
                     }),
                     execute_query: |tcx, key| erase::erase_val(tcx.$name(key)),
-                    compute: |tcx, key| {
-                        #[cfg(debug_assertions)]
-                        let _guard = tracing::span!(tracing::Level::TRACE, stringify!($name), ?key).entered();
-                        __rust_begin_short_backtrace(||
-                            queries::$name::provided_to_erased(
-                                tcx,
-                                {
-                                    let ret = call_provider!([$($modifiers)*][tcx, $name, key]);
-                                    rustc_middle::ty::print::with_reduced_queries!({
-                                        tracing::trace!(?ret);
-                                    });
-                                    ret
-                                }
-                            )
-                        )
-                    },
+                    compute_fn: self::compute_fn::__rust_begin_short_backtrace,
                     try_load_from_disk_fn: should_ever_cache_on_disk!([$($modifiers)*] {
                         Some(|tcx, key, prev_index, index| {
                             // Check the `cache_on_disk_if` condition for this key.

src/tools/run-make-support/src/lib.rs

@@ -47,6 +47,7 @@ pub use crate::assertion_helpers::{
     assert_contains, assert_contains_regex, assert_count_is, assert_dirs_are_equal, assert_equals,
     assert_not_contains, assert_not_contains_regex,
 };
+pub use crate::command::CompletedProcess;
 // `diff` is implemented in terms of the [similar] library.
 //
 // [similar]: https://github.com/mitsuhiko/similar

tests/run-make/short-ice/rmake.rs

@@ -12,40 +12,53 @@
 // - FIXME(#143198): On `x86_64-pc-windows-msvc`: full backtrace sometimes do not contain matching
 //   count of short backtrace markers (e.g. 5x end marker, but 3x start marker).
 
-use run_make_support::rustc;
+use run_make_support::CompletedProcess;
 
 fn main() {
-    let rust_test_1 =
-        rustc().set_backtrace_level("1").input("src/lib.rs").arg("-Ztreat-err-as-bug=1").run_fail();
-    let rust_test_2 = rustc()
-        .set_backtrace_level("full")
-        .input("src/lib.rs")
-        .arg("-Ztreat-err-as-bug=1")
-        .run_fail();
+    // Run the same command twice with `RUST_BACKTRACE=1` and `RUST_BACKTRACE=full`.
+    let configure_rustc = || {
+        let mut rustc = run_make_support::rustc();
+        rustc.input("src/lib.rs").arg("-Ztreat-err-as-bug=1");
+        rustc
+    };
+    let rustc_bt_short = configure_rustc().set_backtrace_level("1").run_fail();
+    let rustc_bt_full = configure_rustc().set_backtrace_level("full").run_fail();
 
-    let mut rust_test_log_1 = rust_test_1.stderr_utf8();
-    rust_test_log_1.push_str(&rust_test_1.stdout_utf8());
-    let rust_test_log_1 = rust_test_log_1.as_str();
+    // Combine stderr and stdout for subsequent checks.
+    let concat_stderr_stdout =
+        |proc: &CompletedProcess| format!("{}\n{}", proc.stderr_utf8(), proc.stdout_utf8());
+    let output_bt_short = &concat_stderr_stdout(&rustc_bt_short);
+    let output_bt_full = &concat_stderr_stdout(&rustc_bt_full);
 
-    let mut rust_test_log_2 = rust_test_2.stderr_utf8();
-    rust_test_log_2.push_str(&rust_test_2.stdout_utf8());
-    let rust_test_log_2 = rust_test_log_2.as_str();
+    // Count how many lines of output mention symbols or paths in
+    // `rustc_query_system` or `rustc_query_impl`, which are the kinds of
+    // stack frames we want to be omitting in short backtraces.
+    let rustc_query_count_short = count_lines_with(output_bt_short, "rustc_query_");
+    let rustc_query_count_full = count_lines_with(output_bt_full, "rustc_query_");
 
-    let rustc_query_count_full = count_lines_with(rust_test_log_2, "rustc_query_");
+    // Dump both outputs in full to make debugging easier, especially on CI.
+    // Use `--no-capture --force-rerun` to view output even when the test is passing.
+    println!("=== BEGIN SHORT BACKTRACE ===\n{output_bt_short}\n=== END SHORT BACKTRACE === ");
+    println!("=== BEGIN FULL BACKTRACE ===\n{output_bt_full}\n=== END FULL BACKTRACE === ");
 
     assert!(
-        rust_test_log_1.lines().count() < rust_test_log_2.lines().count(),
-        "Short backtrace should be shorter than full backtrace.\nShort backtrace:\n\
-        {rust_test_log_1}\nFull backtrace:\n{rust_test_log_2}"
+        output_bt_short.lines().count() < output_bt_full.lines().count(),
+        "Short backtrace should be shorter than full backtrace"
     );
+
+    let n_begin = count_lines_with(output_bt_full, "__rust_begin_short_backtrace");
+    let n_end = count_lines_with(output_bt_full, "__rust_end_short_backtrace");
+    assert!(n_begin + n_end > 0, "Full backtrace should contain short-backtrace markers");
     assert_eq!(
-        count_lines_with(rust_test_log_2, "__rust_begin_short_backtrace"),
-        count_lines_with(rust_test_log_2, "__rust_end_short_backtrace"),
-        "Full backtrace should contain the short backtrace markers.\nFull backtrace:\n\
-        {rust_test_log_2}"
+        n_begin, n_end,
+        "Full backtrace should contain equal numbers of begin and end markers"
+    );
+
+    assert!(
+        rustc_query_count_short + 5 < rustc_query_count_full,
+        "Short backtrace should have omitted more query plumbing lines \
+        (actual: {rustc_query_count_short} vs {rustc_query_count_full})"
     );
-    assert!(count_lines_with(rust_test_log_1, "rustc_query_") + 5 < rustc_query_count_full);
-    assert!(rustc_query_count_full > 5);
 }
 
 fn count_lines_with(s: &str, search: &str) -> usize {