Clarify names of QueryVTable functions for "executing" a query
#152434
Conversation
rustbot
added
A-query-system
|
|
||
| /// Function pointer that actually calls this query's provider, | ||
| /// while also marking a short-backtrace boundary and doing some tracing. | ||
| pub invoke_provider_fn: fn(tcx: TyCtxt<'tcx>, key: C::Key) -> C::Value, |
There was a problem hiding this comment.
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?
| /// This function would be named `invoke_provider`, 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>( |
There was a problem hiding this comment.
(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?
There was a problem hiding this comment.
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 .)
There was a problem hiding this comment.
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.
This comment has been minimized.
This comment has been minimized.
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
I've done a fairly substantial overhaul of the comments to hopefully make things clearer. I also changed the signature of |
| /// 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`. |
There was a problem hiding this comment.
This is helpful, thanks.
|
Nice improvements. r=me with the comment nit. |
This also changes the signature of `call_query_method` to not return a value, because its only caller immediately discards the value anyway.
|
@bors r=nnethercote |
rust-bors
bot
added
S-waiting-on-bors
Clarify names of `QueryVTable` functions for "executing" a query In the query system, there are several layers of functions involved in “executing” a query, with very different responsibilities at each layer, making it important to be able to tell them apart. This PR renames two of the function pointers in `QueryVTable`, along with their associated helper functions, to hopefully do a better job of indicating what their actual responsibilities are. r? nnethercote
Clarify names of `QueryVTable` functions for "executing" a query In the query system, there are several layers of functions involved in “executing” a query, with very different responsibilities at each layer, making it important to be able to tell them apart. This PR renames two of the function pointers in `QueryVTable`, along with their associated helper functions, to hopefully do a better job of indicating what their actual responsibilities are. r? nnethercote
Clarify names of `QueryVTable` functions for "executing" a query In the query system, there are several layers of functions involved in “executing” a query, with very different responsibilities at each layer, making it important to be able to tell them apart. This PR renames two of the function pointers in `QueryVTable`, along with their associated helper functions, to hopefully do a better job of indicating what their actual responsibilities are. r? nnethercote
Rollup of 17 pull requests Successful merges: - #142415 (Add note when inherent impl for a alias type defined outside of the crate) - #142680 (Fix passing/returning structs with the 64-bit SPARC ABI) - #150768 (Don't compute FnAbi for LLVM intrinsics in backends) - #151152 (Add FCW for derive helper attributes that will conflict with built-in attributes) - #151814 (layout: handle rigid aliases without params) - #151863 (Borrowck: simplify diagnostics for placeholders) - #152159 (Add note for `?Sized` params in int-ptr casts diag) - #152434 (Clarify names of `QueryVTable` functions for "executing" a query) - #152478 (Remove tm_factory field from CodegenContext) - #152498 (Partially revert "resolve: Update `NameBindingData::vis` in place") - #152316 (fix: add continue) - #152394 (Correctly check if a macro call is actually a macro call in rustdoc highlighter) - #152425 (Port #![test_runner] to the attribute parser) - #152481 (Use cg_ssa's produce_final_output_artifacts in cg_clif) - #152485 (fix issue#152482) - #152495 (Clean up some subdiagnostics) - #152502 (Implement `BinaryHeap::from_raw_vec`)
Rollup merge of #152434 - Zalathar:call-query, r=nnethercote Clarify names of `QueryVTable` functions for "executing" a query In the query system, there are several layers of functions involved in “executing” a query, with very different responsibilities at each layer, making it important to be able to tell them apart. This PR renames two of the function pointers in `QueryVTable`, along with their associated helper functions, to hopefully do a better job of indicating what their actual responsibilities are. r? nnethercote
3 participants
In the query system, there are several layers of functions involved in “executing” a query, with very different responsibilities at each layer, making it important to be able to tell them apart.
This PR renames two of the function pointers in
QueryVTable, along with their associated helper functions, to hopefully do a better job of indicating what their actual responsibilities are.r? nnethercote