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

Add debugPrintf-based panic reporting, controlled via spirv_builder::ShaderPanicStrategy. #1080

Merged
merged 1 commit into from
Jul 18, 2023

Conversation

eddyb
Copy link
Contributor

@eddyb eddyb commented Jul 14, 2023

This is a followup to my earlier PR about panics (and the only thing I want to really get into 0.9):

There are better docs on spirv_builder::ShaderPanicStrategy, but this is the basic usage example:

    SpirvBuilder::new("examples/shaders/sky-shader", "spirv-unknown-vulkan1.1")
        .shader_panic_strategy(spirv_builder::ShaderPanicStrategy::DebugPrintfThenExit {
            print_inputs: true,
            print_backtrace: true,
        })

(that is, to enable emitting all the information we can right now - print_inputs will show values of entry-point inputs, to identify the invocation, while print_backtrace will collect the static "inlined call frames" information we track for diagnostics, into the message)

And this is what it looks like with an artificially induced panic in sky-shader (plus some extra functions that just call the next one in the chain, to show off a non-trivial backtrace):


If you look at the diff, you may notice most of it is to the inliner: yupp it had more bugs, but not only that, the entire strategy for tracking debuginfo was wrong because it's fundamentally an outside-in inliner, instead of the usual inside-out model (I'm honestly surprised any of the past diagnostics demos worked at all).

The other changes I've made were to the ash runner, hopefully they're not too disruptive (cc @DJMcNab).
This is how I was testing (after sprinkling sky-shader with extra panics):

VK_LAYER_ENABLES=VK_VALIDATION_FEATURE_ENABLE_DEBUG_PRINTF_EXT cargo run -p example-runner-ash --release -- -d

Haven't been able to break anything with it, so I'm relatively confident the inliner changes aren't semantically meaningful (after all, the whole point is to tweak debuginfo to make it more accurate, not to change anything else).


EDIT: also check out this other PR, which further improved the output and figured out how to use it with wgpu:

@eddyb eddyb requested review from fu5ha and VZout as code owners July 14, 2023 22:20
@eddyb eddyb requested a review from repi July 15, 2023 06:12
@eddyb eddyb marked this pull request as draft July 15, 2023 10:35
@eddyb

This comment was marked as resolved.

@eddyb eddyb marked this pull request as ready for review July 18, 2023 19:26
@eddyb eddyb enabled auto-merge (rebase) July 18, 2023 19:26
@eddyb eddyb merged commit 4252427 into EmbarkStudios:main Jul 18, 2023
7 checks passed
@eddyb eddyb deleted the whose-panic-is-it-anyway branch July 18, 2023 20:17
@eddyb
Copy link
Contributor Author

eddyb commented Jul 20, 2023

For anyone seeing this PR later, the output, wgpu compatibility, and docs, were improved in:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants