-
Notifications
You must be signed in to change notification settings - Fork 58
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
SR error handling #112
Comments
Does it make sense to try continuing parsing a module upon seeing the first recoverable error? We know for sure this module is no good, and we know for sure that changes are needed. We might as well just return with the first encountered error, in which case it's the same as unrecoverable. |
It depends on whether we expect anyone to be doing real-time correction of the errors. There are definitely benefits to just erroring out as soon as we see the first error, but it provides much less information to a user. |
How do you see this real-time correction happening, and in what cases? Do you expect that the vector of correctable errors we return is complete, and by addressing each individual instruction the user will get a pass next time? If the user still have to iterate multiple times, then they might as well iterate on an error by error basis. |
Now I've explained it I think it's less likely to be useful, and really the chances that we get an incorrect SPIR-V module should be really small, as they will almost always be generated from GLSL/HLSL/etc. However, in the general case, providing as many errors as possible per pass is generally much more useful, consider programming rust, if it only gave you the first error you hit it would take absolutely forever to fix any problems. |
I think we can look at WASM for inspiration here, not Rust. Having an error in SPIR-V binary could mean that user's high-level code is wrong, but more likely this would happen if there is a bug in the high-level to SPIR-V translator. We shouldn't expect these bugs to be iteratively fixed in batches :) |
Currently, there are situations where
rspirv
can panic, it should not be possible to cause a panic from inputs to the libraries (any observed panic should be regarded as a bug). It also errors out very quickly for a specific class of error. I am proposing a new error handling strategy:All issues with user input should be classed into three types: warnings, recoverable errors and unrecoverable errors. These are treated in very different ways:
Warnings
These are for when a single valid alternative exists, e.g. Missing required capabilities. These will be logged to a store (a
Vec
) which is returned with the module on successful and unsuccessful parsing. These are probably quite rare and in practice may all get subsumed into a validation pass, so this category may not be necessary.Recoverable Errors
These are for when there is an error with the SPIR-V which means that a valid module cannot be produced, but there is a sensible way to continue with the parsing. They are logged to a store (another
Vec
), and returned instead of the module. Most errors fall into this category as we can usually just replace the offending instruction/type/constant with a dummy value. These should all be instrumented with the instruction and offset where they occurred.Unrecoverable Errors
This case should be avoided as it gives the least information to the user, however, if an error occurs and no reasonable corrective action can be taken, then a
Result::Err
should be propagated up the call stack and returned instead of the module, along with the list of recoverable errors encountered up to that point. This kind of error includes, encountering the end of the stream of words mid instruction, or receiving an instruction where the number of operands specified in the binary doesn't match the number encountered or expected.Any comments on this approach, or alternatives, would be greatly appreciated.
The text was updated successfully, but these errors were encountered: