enable no exception build and update exception handling for DataTypeUtils #3265
Conversation
… utils Signed-off-by: Ashwini Khade <askhade@microsoft.com>
Signed-off-by: Ashwini Khade <askhade@microsoft.com>
Signed-off-by: Ashwini Khade <askhade@microsoft.com>
Signed-off-by: Ashwini Khade <askhade@microsoft.com>
Signed-off-by: Ashwini Khade <askhade@microsoft.com>
Signed-off-by: Ashwini Khade <askhade@microsoft.com>
Signed-off-by: Ashwini Khade <askhade@microsoft.com>
CMakeLists.txt
Outdated
| if(ONNX_DISABLE_EXCEPTIONS) | ||
| add_compile_definitions("ONNX_NO_EXCEPTIONS") | ||
| endif() |
There was a problem hiding this comment.
Should you have flags to explicitly fail the build if there's an attempt to use exceptions?
e.g. -fno-exceptions for gcc/clang, /EH-s-c for MSVC
onnx/checker.h
Outdated
| #define fail_check(...) std::cerr << ONNX_NAMESPACE::MakeString(__VA_ARGS__);\ | ||
| abort(); |
There was a problem hiding this comment.
What's the reason for not using ONNX_THROW or ONNX_THROW_EX here?
There was a problem hiding this comment.
I updated these macros first and then decided to add ONNX_THROW and ONNX_THROW_EX . I should have gone back to these macros again and updated them ... updated them now.
| TEST(FunctionAPITest, TypeContextTest) { | ||
| // Does not test registering schema twice when no exceptions are enabled. |
There was a problem hiding this comment.
Would maybe be simpler to just put all the 'register twice' tests and infrastructure inside an #ifndef ONNX_NO_EXCEPTIONS.
| #ifndef ONNX_NO_EXCEPTIONS | ||
| ONNX_NAMESPACE::OpSchemaRegistry::OpSchemaRegisterOnce unused(schema); | ||
| (void)unused; | ||
| #endif | ||
| } |
There was a problem hiding this comment.
Why doesn't RegisterTwiceFloatSchema need a similar update?
There was a problem hiding this comment.
This test was not written properly - RegisterTwiceFloatSchema would register the CustomFunc for the first time and then it was registered for the second time here. Anyways I updated both the test to remove a lot of common code and make the intention of the tests clear.
Signed-off-by: Ashwini Khade <askhade@microsoft.com>
onnx/common/common.h
Outdated
| #define ONNX_HANDLE_EXCEPTION(func) | ||
|
|
||
| #else | ||
| #define ONNX_THROW(...) throw std::runtime_error(ONNX_NAMESPACE::MakeString(__VA_ARGS__)); |
Signed-off-by: Ashwini Khade <askhade@microsoft.com>
| ContextDependentFuntionTest(); | ||
| } | ||
|
|
||
| // Does not test registering schema twice when no exceptions are enabled. |
There was a problem hiding this comment.
The two tests here are actually testing different aspects. The first tests a simple function where the parameter is of a fixed type (float), while the second tests a function where the parameter may be one of several types (float or double). The name "RegisterTwiceSchema" may be confusing, but it is meant to register a function called "CustomFunTwice". Does running these two tests cause a problem (such as same function name being registered twice)? If so, I suggest just renaming the functions as "CustomFunTwice1" and "CustomFunTwice2" or use a static boolean to ensure it is registered only once. Thanks!
Signed-off-by: Ashwini Khade <askhade@microsoft.com>
CMakeLists.txt
Outdated
| if(ONNX_WERROR) | ||
| if(MSVC) | ||
| string(APPEND CMAKE_CXX_FLAGS " /WX") | ||
| elseif(MSVC) |
There was a problem hiding this comment.
Doesn't elseif need a different condition?
Not sure about the 'else(MSVC)' on line 94 as the cmake doco doesn't mention anything about repeating the condition from the 'if' in the 'else' or 'endif'.
There was a problem hiding this comment.
it was a typo It should be else and not elseif. Originally ONNX_WERROR did not have any effect on windows and mac platforms... with this change it should take effect on all 3 platforms...
else does allow condition.
"Per legacy, the else() and endif() commands admit an optional argument. If used, it must be a verbatim repeat of the argument of the opening if command."
However to keep it consistent with the rest of the document I will remove the condition
Signed-off-by: Ashwini Khade <askhade@microsoft.com>
Signed-off-by: Ashwini Khade <askhade@microsoft.com>
Signed-off-by: Ashwini Khade <askhade@microsoft.com>
Signed-off-by: Ashwini Khade <askhade@microsoft.com>
Signed-off-by: Ashwini Khade <askhade@microsoft.com>
Adding an option for creating a no exception build for ONNX. This gives runtimes an ability to build onnx without exceptions.
This is restricted to c++ only. Exceptions are required with python bindings. When exceptions are disabled we abort at the site of first exception.
This PR also updates error handling for DataTypeUtils. We throw an exception when any error is encountered. As stated above when exceptions are disabled we abort instead.
Also adding windows CI for testing no exception builds.
Signed-off-by: Ashwini Khade askhade@microsoft.com