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
12 changes: 12 additions & 0 deletions compiler/rustc_middle/src/query/plumbing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,18 @@ macro_rules! define_callbacks {
}

impl<'tcx> TaggedQueryKey<'tcx> {
/// Returns the name of the query this key is tagged with.
///
/// This is useful for error/debug output, but don't use it to check for
/// specific query names. Instead, match on the `TaggedQueryKey` variant.
pub fn query_name(&self) -> &'static str {
Copy link
Contributor

Choose a reason for hiding this comment

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

I was planning to make description return a (name, description) pair to avoid more of these large matches.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, and in that case we could hoist the tcx.sess.verbose_internals() check out of the macro-defined method, and let callers deal with it instead (probably via a helper method).

That would let print_query_stack avoid printing the query name twice when verbose-internals is enabled.

match self {
$(
TaggedQueryKey::$name(_) => stringify!($name),
)*
}
}

/// Formats a human-readable description of this query and its key, as
/// specified by the `desc` query modifier.
///
Expand Down
12 changes: 5 additions & 7 deletions compiler/rustc_middle/src/query/stack.rs
Original file line number Diff line number Diff line change
@@ -1,16 +1,14 @@
use std::fmt::Debug;

use rustc_span::def_id::DefId;

use crate::dep_graph::DepKind;
use crate::queries::TaggedQueryKey;

/// Description of a frame in the query stack.
///
/// This is mostly used in case of cycles for error reporting.
#[derive(Clone, Debug)]
pub struct QueryStackFrame<'tcx> {
/// The query and key of the query method call that this stack frame
/// corresponds to.
Copy link
Contributor

Choose a reason for hiding this comment

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

If the frame is just a tagged key now, then the whole separate notion of "frame" no longer makes sense, and the structure can be removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

True. And QueryJobInfo::frame can be renamed QueryJobInfo::tagged_key. Would be a good follow-up, either in this PR or another.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's better to leave the potential removal of QueryStackFrame to another PR, because it will have wider effects on other code, and because it's not yet clear to me that replacing frame with tagged_key is necessarily the right move.

///
/// Code that doesn't care about the specific key can still use this to
/// check which query it's for, or obtain the query's name.
pub tagged_key: TaggedQueryKey<'tcx>,
pub dep_kind: DepKind,
pub def_id: Option<DefId>,
}
7 changes: 4 additions & 3 deletions compiler/rustc_query_impl/src/execution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use rustc_errors::FatalError;
use rustc_middle::dep_graph::{DepGraphData, DepNodeKey, SerializedDepNodeIndex};
use rustc_middle::query::{
ActiveKeyStatus, CycleError, EnsureMode, QueryCache, QueryJob, QueryJobId, QueryKey,
QueryLatch, QueryMode, QueryState, QueryVTable,
QueryLatch, QueryMode, QueryStackFrame, QueryState, QueryVTable,
};
use rustc_middle::ty::TyCtxt;
use rustc_middle::verify_ich::incremental_verify_ich;
Expand Down Expand Up @@ -73,8 +73,9 @@ fn collect_active_query_jobs_inner<'tcx, C>(
let mut collect_shard_jobs = |shard: &HashTable<(C::Key, ActiveKeyStatus<'tcx>)>| {
for (key, status) in shard.iter() {
if let ActiveKeyStatus::Started(job) = status {
// This function is safe to call with the shard locked because it is very simple.
let frame = crate::plumbing::create_query_stack_frame(query, *key);
// It's fine to call `create_tagged_key` with the shard locked,
// because it's just a `TaggedQueryKey` variant constructor.
let frame = QueryStackFrame { tagged_key: (query.create_tagged_key)(*key) };
job_map.insert(job.id, QueryJobInfo { frame, job: job.clone() });
}
}
Expand Down
11 changes: 4 additions & 7 deletions compiler/rustc_query_impl/src/from_cycle_error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ use rustc_errors::codes::*;
use rustc_errors::{Applicability, Diag, MultiSpan, pluralize, struct_span_code_err};
use rustc_hir as hir;
use rustc_hir::def::{DefKind, Res};
use rustc_middle::dep_graph::DepKind;
use rustc_middle::queries::{QueryVTables, TaggedQueryKey};
use rustc_middle::query::CycleError;
use rustc_middle::query::erase::erase_val;
Expand Down Expand Up @@ -77,11 +76,10 @@ fn check_representability<'tcx>(tcx: TyCtxt<'tcx>, cycle_error: CycleError<'tcx>
let mut item_and_field_ids = Vec::new();
let mut representable_ids = FxHashSet::default();
for frame in &cycle_error.cycle {
if frame.node.dep_kind == DepKind::check_representability
&& let Some(field_id) = frame.node.def_id
&& let Some(field_id) = field_id.as_local()
&& let Some(DefKind::Field) = frame.node.tagged_key.def_kind(tcx)
if let TaggedQueryKey::check_representability(def_id) = frame.node.tagged_key
&& tcx.def_kind(def_id) == DefKind::Field
{
let field_id: LocalDefId = def_id;
let parent_id = tcx.parent(field_id.to_def_id());
let item_id = match tcx.def_kind(parent_id) {
DefKind::Variant => tcx.parent(parent_id),
Expand Down Expand Up @@ -110,8 +108,7 @@ fn variances_of<'tcx>(tcx: TyCtxt<'tcx>, cycle_error: CycleError<'tcx>) -> &'tcx
&cycle_error.cycle,
|cycle| {
if let Some(frame) = cycle.get(0)
&& frame.node.dep_kind == DepKind::variances_of
&& let Some(def_id) = frame.node.def_id
&& let TaggedQueryKey::variances_of(def_id) = frame.node.tagged_key
{
let n = tcx.generics_of(def_id).own_params.len();
ControlFlow::Break(tcx.arena.alloc_from_iter(iter::repeat_n(ty::Bivariant, n)))
Expand Down
21 changes: 12 additions & 9 deletions compiler/rustc_query_impl/src/job.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
use std::io::Write;
use std::iter;
use std::ops::ControlFlow;
use std::sync::Arc;
use std::{iter, mem};

use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_errors::{Diag, DiagCtxtHandle};
use rustc_hir::def::DefKind;
use rustc_middle::queries::TaggedQueryKey;
use rustc_middle::query::{
CycleError, QueryJob, QueryJobId, QueryLatch, QueryStackFrame, QueryWaiter,
};
Expand Down Expand Up @@ -88,8 +89,8 @@ pub(crate) fn find_cycle_in_stack<'tcx>(
panic!("did not find a cycle")
}

/// Finds the job closest to the root with a `DepKind` matching the `DepKind` of `id` and returns
/// information about it.
/// Finds the query job closest to the root that is for the same query method as `id`
/// (but not necessarily the same query key), and returns information about it.
#[cold]
#[inline(never)]
pub(crate) fn find_dep_kind_root<'tcx>(
Expand All @@ -99,12 +100,14 @@ pub(crate) fn find_dep_kind_root<'tcx>(
) -> (Span, String, usize) {
let mut depth = 1;
let mut info = &job_map.map[&id];
let dep_kind = info.frame.dep_kind;
// Two query stack frames are for the same query method if they have the same
// `TaggedQueryKey` discriminant.
let expected_query = mem::discriminant::<TaggedQueryKey<'tcx>>(&info.frame.tagged_key);
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it's worth adding a method to TaggedQueryKey to get the tag. It could even return a DepKind.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was doing that in an earlier draft, until I realised that there is no code that truly needs to obtain a DepKind from a TaggedQueryKey. So when I realised that this code could use mem::discriminant instead, I removed the TaggedQueryKey → DepKind method entirely.

I think both approaches are pretty close. If we had other call sites that really needed the DepKind, then I would probably not use mem::discriminant here. But as things currently stand, this approach lets us avoid having the DepKind method at all, which I think is a little better.

let mut last_info = info;

while let Some(id) = info.job.parent {
info = &job_map.map[&id];
if info.frame.dep_kind == dep_kind {
if mem::discriminant(&info.frame.tagged_key) == expected_query {
depth += 1;
last_info = info;
}
Expand Down Expand Up @@ -420,8 +423,8 @@ pub fn print_query_stack<'tcx>(
if Some(count_printed) < limit_frames || limit_frames.is_none() {
// Only print to stderr as many stack frames as `num_frames` when present.
dcx.struct_failure_note(format!(
"#{} [{:?}] {}",
count_printed, query_info.frame.dep_kind, description
"#{count_printed} [{query_name}] {description}",
query_name = query_info.frame.tagged_key.query_name(),
))
.with_span(query_info.job.span)
.emit();
Expand All @@ -431,8 +434,8 @@ pub fn print_query_stack<'tcx>(
if let Some(ref mut file) = file {
let _ = writeln!(
file,
"#{} [{:?}] {}",
count_total, query_info.frame.dep_kind, description
"#{count_total} [{query_name}] {description}",
query_name = query_info.frame.tagged_key.query_name(),
);
}

Expand Down
23 changes: 1 addition & 22 deletions compiler/rustc_query_impl/src/plumbing.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
use std::num::NonZero;

use rustc_data_structures::sync::{DynSend, DynSync};
use rustc_data_structures::unord::UnordMap;
use rustc_hir::def_id::DefId;
use rustc_hir::limit::Limit;
use rustc_index::Idx;
use rustc_middle::bug;
Expand All @@ -13,9 +11,7 @@ use rustc_middle::query::erase::{Erasable, Erased};
use rustc_middle::query::on_disk_cache::{
AbsoluteBytePos, CacheDecoder, CacheEncoder, EncodedDepNodeIndex,
};
use rustc_middle::query::{
QueryCache, QueryJobId, QueryKey, QueryMode, QueryStackFrame, QueryVTable, erase,
};
use rustc_middle::query::{QueryCache, QueryJobId, QueryMode, QueryVTable, erase};
use rustc_middle::ty::TyCtxt;
use rustc_middle::ty::codec::TyEncoder;
use rustc_middle::ty::tls::{self, ImplicitCtxt};
Expand Down Expand Up @@ -83,23 +79,6 @@ pub(crate) fn start_query<R>(
})
}

pub(crate) fn create_query_stack_frame<'tcx, C>(
vtable: &'tcx QueryVTable<'tcx, C>,
key: C::Key,
) -> QueryStackFrame<'tcx>
where
C: QueryCache<Key: QueryKey + DynSend + DynSync>,
QueryVTable<'tcx, C>: DynSync,
{
let def_id: Option<DefId> = key.key_as_def_id();

QueryStackFrame {
tagged_key: (vtable.create_tagged_key)(key),
dep_kind: vtable.dep_kind,
def_id,
}
}

pub(crate) fn encode_query_values<'tcx>(
tcx: TyCtxt<'tcx>,
encoder: &mut CacheEncoder<'_, 'tcx>,
Expand Down
Loading