gfx-rs / wgpu

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

Bundles still have lifetime constraints #6433

Open fyellin opened 2 weeks ago

fyellin commented 2 weeks ago

Although wgpu::RenderPass and wgpu::RenderBundleEncoder both implement the trait wpu::util::RenderEncoder, they have different expectations for the lifetime of their common methods. For methods such as set_pipeline, set_bind_group, and set_vertex_buffer, RenderPass has no expectations for the lifetime of the argument. RenderBundleEncoder requires that the argument live at least as long as self.

There is no way of enforcing this lifetime restriction in the C interface or the Python interface to this code. This is documented in this bug at wgpu-native.

Attached is RenderBundleEncoderBug.zip which contains RenderBundleEncoder.py. It is run by calling python RenderBundleEncoderBug.py <flag> where for this bug, flag is one of null-pipeline, null-bind-group, null-vertex-buffer, null-index-buffer, null-indirect-buffer. These show that nulling any of these values has no affect on a RenderPass but will cause the RenderBundleEncoder to panic.

My preference is that this lifetime restriction in RenderBundleEncoder (and RenderEncoder) be removed. Whatever techniqueRenderPass uses to keep track of the objects (and keep them alive?) can certainly be applied to RenderBundleEncoder.

Alternatively, RenderBundleEncoder shouldn't panic. It should never panic except for a serious non-recoverable error. this is an error that can easily be reported back to the user.

Wumpf commented 1 week ago

Those lifetime constraints got lifted for Render & Compute pass not too long ago, see https://github.com/gfx-rs/wgpu/pull/5884 and related PRs. Very similar work has to be done for bundles. However, I thought that most of it was already in place. But obviously there's still something missing. A suite of tests similar to what was added for render & compute pass is in order, fixing those and then removing the those lifetimes from the public api

fyellin commented 1 week ago

I recently wrote some test code for python that tests deleting objects in the user code even though they area still in use by wgpu. Everything worked perfectly except for RenderBundleEncoder. Behind the scenes, I had to force RenderBundleEncoder to retain a pointer to the objects it depended on.

Should I file a different bug about the fact that RenderBundle and RenderBundleEncoder panic when they find something they don't like, rather than returning an error? Or is this part of the same cleanup? Passing the wrong arguments formats to createRenderBundleEncoder() or forgetting to call set_pipeline() will cause a crash.

Wumpf commented 1 week ago

part of the same cleanup, I think this issue captures this well enough already. Generally, violating the lifetime constraints that the rust borrow checker tries to uphold (and naturally can't be automatically enforced in a C interface) is undefined behavior. So it's less of a crash bug but more of a "those lifetimes really shouldn't be there" issue (: (well and ofc from a different point of view, wgpu-native is currently not correctly implementing the contract of webgpu.h, which it could do easily if those lifetime constraints weren't there)