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

Loader should not use C++ exceptions #52

Closed
daveh-lunarg opened this issue May 24, 2019 · 5 comments · Fixed by #80
Closed

Loader should not use C++ exceptions #52

daveh-lunarg opened this issue May 24, 2019 · 5 comments · Fixed by #80
Assignees

Comments

@daveh-lunarg
Copy link
Contributor

The existing loader code includes C++ exceptions as a part of it's error handling. OpenXR nominally follows the Google C++ style guide, which weakly prohibits use of exceptions.

In order to maintain widest possible compatibility, including with other projects which follow the Google style guide, the exception handling code should be removed from the loader.

This public issue is to track resolution of the same topic in the OpenXR WG, issue 974.

@daveh-lunarg daveh-lunarg self-assigned this Jun 12, 2019
@daveh-lunarg
Copy link
Contributor Author

Assigned myself, but feel free to jump in anyone. I'm buried on another project for a couple weeks, will plan to work on this in July.

@zheqiMicrosoft
Copy link
Contributor

I can volunteer to make this change. From what we’ve been able to gather, there are three categories of exception throwing in the code.

  1. Exceptions are thrown and caught directly by the function that throws. These instances can be changed to early return with a corresponding XrResult and logging, if any. See src\api_layers\api_dump.cpp(473) in function ApiDumpLayerXrCreateApiLayerInstance.
  2. Throws that are the result of a catch(..) with some “unknown error” or “bad alloc” logging. These can be if defed out as the bad allocs likely come from the standard library and Chromium already has an OOM handler.
  3. Throw in core_validation.cpp. This is only used by the validation layer that is not part of loader code. We can ignore these instances for now.

All catches should be able to be if defed out.

@daveh-lunarg , do you agree with this assessment and next steps?

@daveh-lunarg
Copy link
Contributor Author

Yes, I agree with all 3, though I would go further and say don't #ifdef the exception code, just remove it completely. I'll add a new issue to track cleaning up the throw(s) in core_validation.cpp, so it doesn't get lost.

Also, prevent exceptions being thrown by the json library by clearing the JSON_USE_EXCEPTION definition in external\jsoncpp\dist\json\json.h.

@rpavlik
Copy link
Contributor

rpavlik commented Jun 28, 2019

Instead of editing external\jsoncpp\dist\json\json.h I'd prefer setting JSON_USE_EXCEPTION=0 in CMake: that way we could use system-provided jsoncpp or the vendored copy equivalently.

Removing the throws and replacing with error returns seems fine. My concern about removing all the try-catch blocks (without making all functions nothrow) is that there are standard library containers used that may throw, and as a result, without these try-catch blocks you might have an exception escape through the C ABI. Tagging in @brycehutchings since he was the last person I talked with about exceptions and C ABI, hoping he can offer some guidance.

@brycehutchings
Copy link
Contributor

I think when disabling exception support, exceptions just results in a crash if an exception would have been thrown, which can be okay if we are careful that exceptions are for just for code defects and OOM. If that is all correct then using std::string and containers is okay as long as we don't rely on catching the exceptions and recovering.

Just peeking at JSON_USE_EXCEPTION=0, it looks like it will result in an abort and "are used only for pre-condition violations and internal logic errors." so a malicious JSON file can't cause a crash 👍.

We could use a similar pattern as the Json library, which will be nice to avoid EVERY function having to return an error code just because it might have OOM. We just need to be judicious that it not be used for errors that could be caused by the loader's various input (file, registry, function parameters, etc).

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

Successfully merging a pull request may close this issue.

4 participants