Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions compiler/rustc_middle/src/query/plumbing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,21 @@ pub struct QueryVTable<'tcx, C: QueryCache> {
// Offset of this query's cache field in the QueryCaches struct
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: fn(tcx: TyCtxt<'tcx>, key: C::Key) -> C::Value,

/// Function pointer that calls `tcx.$query(key)` for this query and
/// discards the returned value.
///
/// This is a weird thing to be doing, and probably not what you want.
/// It is used for loading query results from disk-cache in some cases.
pub call_query_method_fn: fn(tcx: TyCtxt<'tcx>, key: C::Key),

/// Function pointer that actually calls this query's provider.
/// Also performs some associated secondary tasks; see the macro-defined
/// implementation in `mod invoke_provider_fn` for more details.
///
/// This should be the only code that calls the provider function.
pub invoke_provider_fn: fn(tcx: TyCtxt<'tcx>, key: C::Key) -> C::Value,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with the distinction between these functions, and the comments help but I'm still left wondering about the relationship between them. Especially given that they have the same signature. Does one call the other, or something?


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>,
Expand Down
15 changes: 10 additions & 5 deletions compiler/rustc_query_impl/src/execution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -416,7 +416,8 @@ fn execute_job_non_incr<'tcx, C: QueryCache, const FLAGS: QueryFlags>(
}

let prof_timer = qcx.tcx.prof.query_provider();
let result = qcx.start_query(job_id, query.depth_limit(), || query.compute(qcx, key));
// Call the query provider.
let result = qcx.start_query(job_id, query.depth_limit(), || query.invoke_provider(qcx, key));
let dep_node_index = qcx.tcx.dep_graph.next_virtual_depnode_index();
prof_timer.finish_with_query_invocation_id(dep_node_index.into());

Expand Down Expand Up @@ -459,18 +460,21 @@ fn execute_job_incr<'tcx, C: QueryCache, const FLAGS: QueryFlags>(

let (result, dep_node_index) = qcx.start_query(job_id, query.depth_limit(), || {
if query.anon() {
return dep_graph_data
.with_anon_task_inner(qcx.tcx, query.dep_kind(), || query.compute(qcx, key));
// Call the query provider inside an anon task.
return dep_graph_data.with_anon_task_inner(qcx.tcx, query.dep_kind(), || {
query.invoke_provider(qcx, key)
});
}

// `to_dep_node` is expensive for some `DepKind`s.
let dep_node = dep_node_opt.unwrap_or_else(|| query.construct_dep_node(qcx.tcx, &key));

// Call the query provider.
dep_graph_data.with_task(
dep_node,
(qcx, query),
key,
|(qcx, query), key| query.compute(qcx, key),
|(qcx, query), key| query.invoke_provider(qcx, key),
query.hash_result(),
)
});
Expand Down Expand Up @@ -547,7 +551,8 @@ fn try_load_from_disk_and_cache_in_memory<'tcx, C: QueryCache, const FLAGS: Quer
let prof_timer = qcx.tcx.prof.query_provider();

// The dep-graph for this computation is already in-place.
let result = qcx.tcx.dep_graph.with_ignore(|| query.compute(qcx, *key));
// Call the query provider.
let result = qcx.tcx.dep_graph.with_ignore(|| query.invoke_provider(qcx, *key));

prof_timer.finish_with_query_invocation_id(dep_node_index.into());

Expand Down
13 changes: 8 additions & 5 deletions compiler/rustc_query_impl/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,15 +107,18 @@ impl<'tcx, C: QueryCache, const FLAGS: QueryFlags> SemiDynamicQueryDispatcher<'t
}
}

// Don't use this method to compute query results, instead use the methods on TyCtxt.
/// Calls `tcx.$query(key)` for this query, and discards the returned value.
/// See [`QueryVTable::call_query_method_fn`] for details of this strange operation.
#[inline(always)]
fn execute_query(self, tcx: TyCtxt<'tcx>, key: C::Key) -> C::Value {
(self.vtable.execute_query)(tcx, key)
fn call_query_method(self, tcx: TyCtxt<'tcx>, key: C::Key) {
(self.vtable.call_query_method_fn)(tcx, key)
}

/// Calls the actual provider function for this query.
/// See [`QueryVTable::invoke_provider_fn`] for more details.
#[inline(always)]
fn compute(self, qcx: QueryCtxt<'tcx>, key: C::Key) -> C::Value {
(self.vtable.compute_fn)(qcx.tcx, key)
fn invoke_provider(self, qcx: QueryCtxt<'tcx>, key: C::Key) -> C::Value {
(self.vtable.invoke_provider_fn)(qcx.tcx, key)
}

#[inline(always)]
Expand Down
26 changes: 18 additions & 8 deletions compiler/rustc_query_impl/src/plumbing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -457,7 +457,9 @@ fn try_load_from_on_disk_cache<'tcx, C: QueryCache, const FLAGS: QueryFlags>(
panic!("Failed to recover key for {:?} with hash {}", dep_node, dep_node.hash)
});
if query.will_cache_on_disk_for_key(tcx, &key) {
let _ = query.execute_query(tcx, key);
// Call `tcx.$query(key)` for its side-effect of loading the disk-cached
// value into memory.
query.call_query_method(tcx, key);
}
}

Expand Down Expand Up @@ -625,14 +627,15 @@ 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 {
/// Defines an `invoke_provider` function that calls the query's provider,
/// to be used as a function pointer in the query's vtable.
///
/// To mark a short-backtrace boundary, the function's actual name
/// (after demangling) must be `__rust_begin_short_backtrace`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is helpful, thanks.

mod invoke_provider_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>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Pre-existing) This function has a strange name! Doesn't give any idea what it does, and is the same as the one from std. Is that deliberate? Really confusing. Would invoke_provider_inner or something like that be better?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this is probably very confusing if you don't already know how short backtraces work.

The backtrace handler looks for pairs of functions named __rust_begin_short_backtrace and __rust_end_short_backtrace in its stack trace, and will replace those stack frames (and everything in between) with [... omitted N frames ...], along with a message telling you to set RUST_BACKTRACE=full to disable short-backtrace mode.

If an ICE occurs in a query provider, we typically want to omit all of the query plumbing between the query caller and the query provider, because it's dozens of lines of irrelevant noise. To make that work, the function that calls the provider has to be named __rust_begin_short_backtrace. (There is also a matching specially named function on the other end of the query plumbing; see .)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To minimize runtime overhead and also make the short-backtraces more reliable, the specially-named functions are used in places where we were going to dynamically call a function pointer from a vtable anyway.

The downside is that those functions have to have unhelpful names, because function names are what the short-backtrace machinery uses to do its work. To compensate for that as much as possible, we define the strangely-named functions in modules that have more helpful names, and use comments to (hopefully) explain what's going on.

tcx: TyCtxt<'tcx>,
Expand All @@ -643,10 +646,13 @@ macro_rules! define_queries {

// 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);
});

// Erase the returned value, because `QueryVTable` uses erased values.
// For queries with `arena_cache`, this also arena-allocates the value.
provided_to_erased(tcx, provided_value)
}
}
Expand All @@ -666,8 +672,12 @@ macro_rules! define_queries {
} {
None
}),
execute_query: |tcx, key| erase::erase_val(tcx.$name(key)),
compute_fn: self::compute_fn::__rust_begin_short_backtrace,
call_query_method_fn: |tcx, key| {
// Call the query method for its side-effect of loading a value
// from disk-cache; the caller doesn't need the value.
let _ = tcx.$name(key);
},
invoke_provider_fn: self::invoke_provider_fn::__rust_begin_short_backtrace,
try_load_from_disk_fn: if_cache_on_disk!([$($modifiers)*] {
Some(|tcx, key, prev_index, index| {
// Check the `cache_on_disk_if` condition for this key.
Expand Down
Loading