All: Throwing from destructors
Created by: ibaned
Early signs of this are seen in #449 and #577, but I think its a broader problem than those issues make it seem. This is not a clang-specific problem but rather goes down to the C++ standard.
Basically, because destructors need to be safe to call during the stack unwinding as a response to an exception being throw, they should not themselves be able to throw exceptions.
It is worthwhile to read libMesh's experience with this problem using GCC 6.1, see libMesh/libmesh#1073. This quote from that discussion sums up the relevant language in the various C++ standards as well as the sensible solution of taking out any throw statements from destructors:
No, not undefined behavior. In the C++03 standard, sections 15.2 and 15.5.1 make it clear that an exception which escapes a destructor results in an immediate call to std::terminate().
In C++11 it's even a bit looser, as best as I can tell: if you override the default "noexcept" specification on your destructor, and your destructor isn't currently being called as part of stack unwinding during another exception throw, and the object being destroyed isn't static or TLS, then the exception is allowed to propagate and be handled normally; otherwise you get std::terminate().
Nitpicking aside, though, after a little more thought my vote is to just remove the libmesh_parallel_only() macro call here. The hypothetical advantage of maybe being able to more easily debug MPI sync failures at LibMeshInit destruction time isn't worth the definite annoyance of a false positive warning or of trying to work around the warning.
Here is a warning thrown by GCC 6.1 on Teuchos:
/projects/AppComp/alexa/nightly/cee-compute011/src/Trilinos/packages/teuchos/core/src/Teuchos_TestForException.hpp:183:28: warning: throw will always call terminate() [-Wterminate]
throw Exception(omsgstr); \
^
/projects/AppComp/alexa/nightly/cee-compute011/src/Trilinos/packages/teuchos/core/src/Teuchos_TestForException.hpp:305:3: note: in expansion of macro ‘TEUCHOS_TEST_FOR_EXCEPTION’
TEUCHOS_TEST_FOR_EXCEPTION(throw_exception_test, std::logic_error, "Error!")
^~~~~~~~~~~~~~~~~~~~~~~~~~
/projects/AppComp/alexa/nightly/cee-compute011/src/Trilinos/packages/teuchos/core/src/Teuchos_RCPNode.cpp:655:5: note: in expansion of macro ‘TEUCHOS_TEST_FOR_EXCEPT’
TEUCHOS_TEST_FOR_EXCEPT(0==rcp_node_list());^M
^~~~~~~~~~~~~~~~~~~~~~~
/projects/AppComp/alexa/nightly/cee-compute011/src/Trilinos/packages/teuchos/core/src/Teuchos_TestForException.hpp:183:28: note: in C++11 destructors default to noexcept
throw Exception(omsgstr); \
^
/projects/AppComp/alexa/nightly/cee-compute011/src/Trilinos/packages/teuchos/core/src/Teuchos_TestForException.hpp:305:3: note: in expansion of macro ‘TEUCHOS_TEST_FOR_EXCEPTION’
TEUCHOS_TEST_FOR_EXCEPTION(throw_exception_test, std::logic_error, "Error!")
^~~~~~~~~~~~~~~~~~~~~~~~~~
/projects/AppComp/alexa/nightly/cee-compute011/src/Trilinos/packages/teuchos/core/src/Teuchos_RCPNode.cpp:655:5: note: in expansion of macro ‘TEUCHOS_TEST_FOR_EXCEPT’
TEUCHOS_TEST_FOR_EXCEPT(0==rcp_node_list());^M
^~~~~~~~~~~~~~~~~~~~~~~
The way I see it, here are the options Teuchos has:
- Remove
TEST_FOR_EXCEPT
calls from destructors. - Replace those calls with the equivalent of
assert()
. - Try to override the
noexcept
specification. Note that doing this will not guarantee a proper unwinding exception in all cases.
#449 and #577 seem to indicate that there are Teuchos tests which rely on exception throwing from destructors to cause unwinding. Given that this is only standards-conforming with C++11 and even then only under certain special conditions, those tests should be looked at carefully to see if they actually conform to the restrictions mentioned in the standard and if not then expected behavior should be re-evaluated and tests changed.
@trilinos/teuchos @MicheldeMessieres @bartlettroscoe @mhoemmen