Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace Attr::SpvDebugLine with a better basic debug source location attribute. #9

Merged
merged 1 commit into from
Nov 6, 2024

Conversation

eddyb
Copy link
Collaborator

@eddyb eddyb commented Nov 6, 2024

Partially modeled on a subset of the NonSemantic.Shader.DebugInfo.100 SPIR-V extended instruction set, but more with Rust-GPU's needs in mind, this extends the existing file:line:col debuginfo to support both:

  • ranges (within a single file - comparable to rustc's internal Span)
  • "inlined at" debuginfo (call site debug location + callee name)
    • more expressive than NonSemantic.Shader.DebugInfo.100 in one key way: for some reason, that extension only seems to allow distinguishing call sites by line number which seems borderline incorrect (wrt DWARF)

There is nothing in the spirt library itself that can create such extended debuginfo (even though in theory e.g. NonSemantic.Shader.DebugInfo.100 support could do that), and instead Rust-GPU is expected to produce it by converting its own custom instructions.

The Rust-GPU side of this already exists since it's part of the polaris/lodestar demo branches (IIRC this specifically happened for recursion emulation, where FuncCall being outside Blocks meant).


Example from a relatively messy inlining stack:
image

Ideally large amounts of deduplication could be done by having a "debug context tracker" in print that only needs to show changes (i.e. entering/leaving a group of inlined instructions, and individual location debuginfo that could still differ between instructions), but that's not done in this PR.

The status quo on the Rust-GPU side was already quite verbose, and arguably worse than this at times (or at least more "syntaxful"), so this will do for the time being.

@eddyb eddyb added this to the 0.5.0 milestone Nov 6, 2024
Comment on lines +2153 to +2171
// HACK(eddyb) the syntaxes used are taken from VSCode, i.e.:
// https://github.com/microsoft/vscode/blob/6b924c5/src/vs/workbench/contrib/terminalContrib/links/browser/terminalLinkParsing.ts#L75-L91
// (using the most boring syntax possible for every situation).
let is_quoted = s.ends_with('"');
let is_range = (start_line, start_col) != (end_line, end_col);
write!(
s,
"{}{start_line}{}{start_col}",
if is_quoted { ',' } else { ':' },
if is_quoted && is_range { '.' } else { ':' }
)
.unwrap();
if is_range {
s += "-";
if start_line != end_line {
write!(s, "{end_line}.").unwrap();
}
write!(s, "{end_col}").unwrap();
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the more "subjective" part of this change (based on comments in VSCode's source code, also linked in the // HACK... comment for posterity).

Since multiple syntaxes are possible for (almost) every situation, I tried to mix and match them to reduce variation or redundancy wherever possible.

@eddyb eddyb enabled auto-merge November 6, 2024 13:54
Copy link
Collaborator

@LegNeato LegNeato left a comment

Choose a reason for hiding this comment

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

A couple of things:

  1. Feels like we should have a test here if we don't need to add too much plumbing? Not sure how much work that is.
  2. Is there any way we can use tracing and wire it up with a provider that outputs in this format? I know rustc has its own debug/error machinery as well as tracing, so perhaps both are preferable.
  3. Doc comments would really help us newcomers follow the code.

Feel free to ignore me, still ramping on spirt.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/print/mod.rs Outdated Show resolved Hide resolved
src/spv/lower.rs Show resolved Hide resolved
@eddyb
Copy link
Collaborator Author

eddyb commented Nov 6, 2024

Feels like we should have a test here if we don't need to add too much plumbing? Not sure how much work that is.

Sadly the only user of this right now would be Rust-GPU, and at least with the implementation I have for converting to this new encoding, none of its tests that depend on "showing the entire call chain, including inlined calls, for a Rust-GPU diagnostic", changed output.

Is there any way we can use tracing and wire it up with a provider that outputs in this format? I know rustc has its own debug/error machinery as well as tracing, so perhaps both are preferable.

This is for source code locations for the code being compiled, not in the host (rustc/rustc_codegen_spirv/spirt), and also not information the code being compiled can access itself (so not like panic::Location or anything tracing might use).

What this replaces is rustc_codegen_spirv's clunkier custom instructions that look like "pushes" and "popes" (to/from a virtual stack of "inlined call frames"), making it possible to access the same information by inspecting some attributes without extra context and custom handling code etc.

Doc comments would really help us newcomers follow the code.

The critical one is on DbgSrcLoc, did you have something else in mind? SPIR-T is not very deeply documented, but I've tried to at least have top-level doc comments on e.g. type definitions (and variants, where it's not just trivially wrapping some different type) since the first public release.

Copy link
Collaborator

@LegNeato LegNeato left a comment

Choose a reason for hiding this comment

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

Nope, that is sufficient for now. I I think once the dust settles we'll need a book-like guide and do a sweep fo everything public to make sure the docs are easy to follow.

@eddyb eddyb added this pull request to the merge queue Nov 6, 2024
Merged via the queue into Rust-GPU:main with commit 000ec78 Nov 6, 2024
6 checks passed
@eddyb eddyb deleted the dbg-attr branch November 6, 2024 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants