Skip to content

Update ShaderPanicStrategy::DebugPrintfThenExit documentation for Vulkan SDK changes. #354

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

Merged
merged 1 commit into from
Aug 3, 2025

Conversation

eddyb
Copy link
Collaborator

@eddyb eddyb commented Aug 1, 2025

I noticed a few changes in the Vulkan Validation Layer documentation and ended up with these items:

I'm not sure how happy I am with the result, but it tries the offer both an easy set of env vars
(VK_LOADER_LAYERS_ENABLE=VK_LAYER_KHRONOS_validation VK_LAYER_PRINTF_ONLY_PRESET=1 VK_LAYER_PRINTF_TO_STDOUT=1)
and detailed explanations of all the moving parts, in case that doesn't suffice.

Honestly, we should probably have a separate document, and/or move this to debug_printf!
(and merely refer to it from this setting), since users trying that out would also need this info.


As a sneak peek, this is a screenshot of the new docs (click to open its full size):

/// - it uses `VK_EXT_debug_utils` internally, exposing it via `log`
/// - with e.g. `env_logger`, `RUST_LOG=info` suffices for `debugPrintf`
/// messages (as they specifically have the "info" level)
/// - other `log`/`tracing` subscribers should be configured similarly
#[cfg_attr(feature = "clap", clap(skip))]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Firestar99 I noticed this in passing, am I right to think this means cargo-gpu cannot make use of this mode? IMO it's invaluable to debugging panics, so we should try to make it easier and more visible (tbf this entire enum is user-facing API on top of ~6 possible strings that get passed to rustc_codegen_spirv via codegen args, but I'm not sure where it would be best placed).

(oh and I would be fine with not exposing UNSOUND_DO_NOT_USE_UndefinedBehaviorViaUnreachable to users at all, I haven't really heard of anyone going for it, so it can stay behind RUSTGPU_CODEGEN_ARGS or w/e instead - then the choices implied by ShaderPanicStrategy get simpler)

Copy link
Member

@Firestar99 Firestar99 Aug 3, 2025

Choose a reason for hiding this comment

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

I think I disabled it since the original cargo-gpu (before my refactors) didn't support it and clap didn't work out of the box, the support for enums with values doesn't seem to cleanly work. But it gets even sillier:

  • clap is used to parse args from cmdline, which is disabled
  • serde is used to read the [metadata.cargo-gpu] from the crate's Cargo.toml
  • if you use cargo-gpu as a library, you just get a spirv-builder you need to configure manually

I agree this is a mess, and we should eventually figure out a unified way to represent these properties. Since I made #347 I thought of two directions to take things:

  • encourage the Cargo.toml path, so that tools like cargo gpu clippy could pick up the properties, and make the library path pick those up by default as well
  • if we make capabilities and extensions not pre-defined but picked up as they are used, as discussed in "link-time capabilites" on zulip, then there's no need to pre-declare anything per crate. If all settings are link-time settings, then there's no need to declare them in the Cargo.toml for a cargo gpu clippy to pick them up, since it doesn't link anything. And all the linking settings could just be properties of spirv-builder.

@LegNeato
Copy link
Collaborator

LegNeato commented Aug 3, 2025

I think this is probably the first place we need to have a trait for folks to implement or EII or whatever. It should not be this hard to printf from an end-user perspective! But the docs look good.

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.

🚀

@LegNeato LegNeato added this pull request to the merge queue Aug 3, 2025
Merged via the queue into Rust-GPU:main with commit 688103f Aug 3, 2025
13 checks passed
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