feat: add cpptrace for better stack traces #4042
Conversation
|
@kdrienCG Is there a discussion somewhere about whether we want to add this dependency? |
|
@rrsettgast yes, we discussed this internally. There is value in having this TPL for debugging deeper issues. We can talk if you wish. |
@herve-gross That decision being made without consulting others on the core team is inappropriate. The inclusion of this package covers the fundamental problem in the approach to c++ exceptions in an HPC simulation code. There are simpler solutions...like output stack on the throw. |
| catch( geos::Exception & e ) | ||
| }; | ||
|
|
||
| auto onGeosException = [&]( geos::Exception & e ) | ||
| { // GEOS generated exceptions management | ||
| #ifdef GEOS_USE_CPPTRACE | ||
| std::string const stacktrace = StackTrace::formatStackTrace( cpptrace::from_current_exception() ); |
There was a problem hiding this comment.
Looks like the MACRO dispach will occur at each new lambda-exception. Maybe worth passing a callback through the lambda's interface and dispatch all below ? In case we add more of this and the duplication spread
There was a problem hiding this comment.
in order to not modify the stacktrace, do you agree that we should:
#ifdef GEOS_USE_CPPTRACE
#define GEOS_FORMATTED_STACKTRACE StackTrace::formatStackTrace( cpptrace::from_current_exception() )
#else
#define GEOS_FORMATTED_STACKTRACE LvArray::system::stackTrace( true )
#endif
and then,
std::string const stacktrace = GEOS_FORMATTED_STACKTRACE;
| #define GEOS_COMMON_LOGGER_STACKTRACE_HPP | ||
|
|
||
| #include "common/GeosxConfig.hpp" // For the following guards | ||
| #ifdef GEOS_USE_CPPTRACE |
There was a problem hiding this comment.
Since all the calls involving cpptrace are static I would move anything related to choosing whether cpptrace is used to the .cpp file to leave the header unencumbered by these macros.
|
Hi @rrsettgast, |
I assume you mean in the case of the std wrappers, so I will restrict my response to this case. You don't need to throw a |
(Requires TPL PR #347)
This PR aims to improve the stack trace output when an error is thrown.
When GEOS crashes from a C++ exception, the stack trace we log is not really useful: it points at
main.cppinstead of the code that actually threw, missing on valuable informations for devs. This is not a bug in the error handler, it is how the C++ runtime works: by the time wecatch, the stack between thethrowand the handler has already been unwound, so we cannot recover it ourselves. And we can't instrument every throw site either, since many exceptions come from the standard library itself (likestd::map::atfor example), which we can't modify.cpptrace solves this by hooking into the exception "machinery" to capture the stack at throw time, so we get a proper trace even when
throwhappens inside thestdlibrary, which is why we pull it as a dependency rather than rolling our own.Here is an example defining a
myThrowingFunction()incorrectly using amapdirectly (written in themain.cppfor convenience)Without cpptrace:
With cpptrace:
(truncated to fit the PR description)
We have a detailled stack trace with the name of the function that throws and the correct line and character position.