Skip to content
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

Open
Skepfyr opened this issue Oct 7, 2019 · 5 comments
Open

SR error handling #112

Skepfyr opened this issue Oct 7, 2019 · 5 comments

Comments

@Skepfyr
Copy link
Contributor

Skepfyr commented Oct 7, 2019

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.

@kvark
Copy link
Member

kvark commented Oct 8, 2019

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.

@Skepfyr
Copy link
Contributor Author

Skepfyr commented Oct 8, 2019

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.

@kvark
Copy link
Member

kvark commented Oct 8, 2019

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.

@Skepfyr
Copy link
Contributor Author

Skepfyr commented Oct 8, 2019

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.

@kvark
Copy link
Member

kvark commented Oct 8, 2019

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants