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

Added barrier to panic loop. #1055

Closed

Conversation

charles-r-earp
Copy link
Contributor

@charles-r-earp charles-r-earp commented Apr 26, 2023

Related to #1048.

On panic!, rustc_codegen_spirv emits a loop {}, but this can be optimized away with spirv-opt due to the AgressiveDCE pass. This PR inserts a memory_barrier inside the loop which prevents it from being removed.

This ensures that shaders that panic, ie from out of bounds accesses, will not silently continue execution but hang the app.

Unfortunately the wgpu compute example errors with UnsupportedInstruction(Function, MemoryBarrier), likely naga does not support barriers.

It does work using create_shader_module_spirv and enabling the features Features::SPIRV_SHADER_PASSTHROUGH | Features::TIMESTAMP_QUERY_INSIDE_PASSES.

Edit:
Replaced with https://github.com/charles-r-earp/rust-gpu/tree/panic-abort.

The idea is for panic! to emit OpKill / OpTerminateInvocation and replace that with a return from the entry point after inlining. Additionally it will use debug_printfln! if available so that it's easier to detect panics while developing.

@charles-r-earp charles-r-earp marked this pull request as draft April 26, 2023 23:48
@charles-r-earp
Copy link
Contributor Author

Is it safe to say this PR has been obsoleted by the following?

* [Lower aborts (incl. panics) to "return from entry-point", instead of infinite loops. #1070](https://github.com/EmbarkStudios/rust-gpu/pull/1070)

* [Add `debugPrintf`-based panic reporting, controlled via `spirv_builder::ShaderPanicStrategy`. #1080](https://github.com/EmbarkStudios/rust-gpu/pull/1080)

* [Show `panic!` messages via `debugPrintf`, even including some runtime arguments (`{u,i,f}32` as `{}` or `{:?}`). #1082](https://github.com/EmbarkStudios/rust-gpu/pull/1082)

Yes

@eddyb
Copy link
Contributor

eddyb commented Jul 21, 2023

Alright then, closing in that case - thanks for prompting all of this, btw, Rust-GPU panics would've probably stayed in a pretty bad shape for a while longer if not for #1048 and the ensuing discussions.

@eddyb eddyb closed this Jul 21, 2023
@charles-r-earp charles-r-earp deleted the panic-loop-barrier branch January 11, 2024 01:28
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.

2 participants