Simplify stack trace collection code in C++ #3110
Conversation
cpp/src/Ice/Exception.cpp
Outdated
| #elif defined(ICE_BACKTRACE) | ||
| backTraceEnabled = true; | ||
| #endif | ||
| return collectStackTraces(); |
There was a problem hiding this comment.
If I read this correctly ICE_LIBBACKTRACE is the only case where collectStackTraces has the possibility to return false (aside from when there is no library support). ICE_BACKTRACE is always true and ICE_DBGHELP will throw an exception.
I think we should just throw if bstate == NULL and have this function return void. Returning a bool here doesn't make much sense. If it returns false you have no idea why or what you're supposed to do.
The few places we're call it we don't even check the return value.
There was a problem hiding this comment.
We check the return value in the test.
There was a problem hiding this comment.
Fixed. It now returns void and the test assumes stack trace collection is available for the current platform/build.
| @@ -539,6 +466,59 @@ Ice::Exception::ice_stackTrace() const | |||
| return getStackTrace(*_stackFrames); | |||
| } | |||
|
|
|||
| bool | |||
| Ice::Exception::ice_enableStackTraceCollection() | |||
There was a problem hiding this comment.
Is the ice_ prefix really necessary?
There was a problem hiding this comment.
We use this ice_ prefix for all other member functions. No ice_ prefix would be inconsistent.
| } | ||
| }; | ||
|
|
||
| Init init; | ||
| #endif | ||
|
|
||
| inline bool collectStackTraces() noexcept |
There was a problem hiding this comment.
Should we be using a memory barrier to access these global variables. Isn't there a chance they'll be stale otherwise.
There was a problem hiding this comment.
Added mutex lock.
| if (!bstate) | ||
| { | ||
| // Leaked, as libbacktrace does not provide an API to free this state. | ||
| bstate = backtrace_create_state(0, 1, ignoreErrorCallback, 0); |
There was a problem hiding this comment.
It would be nice to get an error message out of the callback if possible.
There was a problem hiding this comment.
It's unrelated to this PR and it's not clear where we'd send this error message.
This PR simplifies the stack trace collection code in C++, and moves the initialization to a static function on Ice::Exception.
It also fixes #3048.