gfx-rs / wgpu

A cross-platform, safe, pure-Rust graphics API.
https://wgpu.rs
Apache License 2.0
11.46k stars 855 forks source link

Make `compile_fxc` accept `full_stage: &CStr` #5836

Closed workingjubilee closed 1 week ago

workingjubilee commented 1 week ago

As discussed in https://github.com/gfx-rs/wgpu/pull/5812#issuecomment-2174536813 the reason that it's acceptable to pass this String to an LPCSTR argument is because it's secretly a CStr. Make it be what it thinks it is, as otherwise, despite this having only one caller, it's not typesafe.

workingjubilee commented 1 week ago
Elabajaba commented 1 week ago

Should I cherrypick this one for backporting as well?

workingjubilee commented 1 week ago

No idea how a patch for DX12 on Windows is affecting Vulkan on Linux...?

@Elabajaba Unlike the other patch, where any version of wgpu with compile_fxc and without that patch is actively inducing reads of garbage data on Windows (or segfaults, which are much less worse), this one shouldn't affect the actually-compiled programs. Rather, it affects whether compile_fxc is an unsafe fn and whether its callers must have secret magic knowledge. As this function is not public outside this codebase, the change here will have no effect on resulting programs, but will make it less likely a future change incorrectly violates invariants.

Rather, it affects whether compile_fxc is an unsafe fn and whether its callers must have secret magic knowledge.

Currently: compile_fxc is not an unsafe fn, but its callers require secret knowledge of what happens to their strings, so it should be.

With this patch: compile_fxc does not need to be an unsafe fn as its callers no longer require secret knowledge of what happens to their strings.

workingjubilee commented 1 week ago

"so is that a yes or no" "idk lol I don't know what your policy is for backporting type-safety fixes?"

teoxoy commented 1 week ago

No idea how a patch for DX12 on Windows is affecting Vulkan on Linux...?

One of the tests is intermittently failing.