Report errors when CommandEncoder.finish is called
#7820
Conversation
wgpu-core/src/command/mod.rs
Outdated
| macro_rules! pass_base { | ||
| ($pass:expr, $scope:expr $(,)?) => { | ||
| match (&$pass.parent, &$pass.base.error) { | ||
| // Attempting to record a command on a finished encoder raises a | ||
| // validation error. | ||
| (&None, _) => return Err(EncoderStateError::Ended).map_pass_err($scope), | ||
|
|
||
| // Attempting to record a command on an open but invalid pass (i.e. | ||
| // a pass with a stored error) fails silently. (The stored error | ||
| // will be transferred to the parent encoder when the pass is ended, | ||
| // and then raised as a validation error when `finish()` is called | ||
| // for the parent). | ||
| (&Some(_), &Some(_)) => return Ok(()), | ||
|
|
||
| // Happy path | ||
| (&Some(_), &None) => &mut $pass.base, | ||
| } | ||
| }; | ||
| } | ||
| pub(crate) use pass_base; | ||
|
|
||
| macro_rules! pass_try { | ||
| ($base:expr, $scope:expr, $res:expr $(,)?) => { | ||
| match $res.map_pass_err($scope) { | ||
| Ok(val) => val, | ||
| Err(err) => { | ||
| $base.error.get_or_insert(err); | ||
| return Ok(()); | ||
| } | ||
| } | ||
| }; | ||
| } | ||
| pub(crate) use pass_try; |
There was a problem hiding this comment.
Could these 2 macros be methods on the pass/base instead? Ideally we shouldn't have hidden control flow inside macros.
There was a problem hiding this comment.
There are two difficulties here, a major one and a minor one. The major difficulty is that we want early-return behavior with a return value of Ok(()). The minor difficulty is that implementing pass_base! as a helper on BasePass has borrow checker problems in some cases (when it's not done inline, &mut pass.base becomes &mut pass for purposes of conflicting mutable borrows).
I do appreciate the point about hidden control flow in macros though. The name pass_try! is a reference to try! which was the precursor to ?, but perhaps that's too obscure to be useful documentation.
Before the current version I had a version where base_mut returned Result<&mut BasePass, Result<(), Err>>. Then the caller could do if let Err(e) = ... { return e; }. I dislike nested result/option types, but unlike control flow in a macro, at least the complexity is obvious. I'm not opposed to going back to that. Or it might be possible to do something with a closure here like I did for the encoder operations.
There was a problem hiding this comment.
I see, I think we can keep the macros then. It might be useful if they had some docs on them mentioning the hidden control flow and maybe the rationale for their existence (effectively copying the paragraph in your comment).
Leaving this up to you - I'll approve the PR!
There was a problem hiding this comment.
Yes, these should definitely have comments, I've added some.
|
I said in the meeting this morning that the change to report destroyed resources on submit was not in this PR, but it actually is. There's a helper here that decides which errors will be deferred. It's a bit of a kludge and risks the match statement going stale, but on the other hand, I don't think anything catastrophic happens if it's wrong, we just report an error at the wrong time. |
andyleiserson
force-pushed
the
errors-on-finish
branch
from
cbaf110 to
66358b2
Compare
This has the rest of the error reporting changes for #7391. I don't think there is urgency to get these changes in before the train leaves, but it's probably better to take either all of them or none of them than to take some of them, so here's a PR with all of them. It can be reviewed commit-by-commit if that's easier.
As in the other PRs, turning on the "hide whitespace" option will substantially reduce the diff.
Testing
Enables relevant CTS tests, and updates some wgpu tests that are sensitive to the timing of error reporting.
Squash or Rebase? Rebase, but check for fixups that should be squashed first.
Checklist
cargo fmt.taplo format.cargo clippy --tests. If applicable, add:--target wasm32-unknown-unknowncargo xtask testto run tests.CHANGELOG.mdentry.