-
Notifications
You must be signed in to change notification settings - Fork 104
Use cpptrace for prettier backtraces from exceptions
#3225
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
base: next
Are you sure you want to change the base?
Conversation
a0c128d to
19ae202
Compare
|
Lots of CMake pain here |
|
Thanks @ZedThree ! This does look nice, if the CMake pain can be overcome. I think if it works portably then it could replace all the MsgStack / AUTOTRACE code. |
|
I've removed That leaves uses like Same goes for uses like Here's an example with And here's what it looks like with Clearly the @bendudson Thoughts on completely removing CUDA tests are failing due to #3233 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
| bout::globals::mesh = Mesh::create(); | ||
| // Load from sources. Required for Field initialisation | ||
| bout::globals::mesh->load(); | ||
| bout::globals::mpi = new MpiWrapper(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: assigning newly created 'gsl::owner<>' to non-owner 'MpiWrapper *' [cppcoreguidelines-owning-memory]
bout::globals::mpi = new MpiWrapper();
^There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
Always generating the backtraces can slow the unit tests down by a factor x2-3 An alternative might be to store the `cpptrace:stacktrace` and only convert to string in `BoutException::what()`
- Add paths to `FetchContent` dirs in config file - Switch to custom versions of cpptrace/libdward-lite - Includes some cmake fixes in both projects
Required because cpptrace prevents in-source builds Also switch to using `subprocess.run` so we can better see errors
Seems to cause MPI problems?
dschwoerer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 👍
cpptrace seems not that tricky to build, but I am wondering whether we should allow to disable it? Would it only require to #ifdef part of one file? For example if you do not have internet while building? Should we try to bundle it into our release tars?
| The second utility BOUT++ has to help debugging is the message stack using the | ||
| `TRACE` macro. This is very useful for when a bug only occurs after a long time | ||
| of running, and/or only occasionally. The ``TRACE`` macro can simply be dropped | ||
| in anywhere in the code:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we not recommend -g?
Even with TRACE still around, most times -g is more helpful, imho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's a good point. TRACE is now very much for additional info, so I'll try and make sure it's clear that we recommend always using either Debug or RelWithDebInfo CMAKE_BUILD_TYPE, and moving the backtrace section up (your next comment)
| The output looks something like this: | ||
| Lastly, BOUT++ can also automatically print a backtrace in the event of a | ||
| crash. This is very useful to include if you ever need to report a bug to the | ||
| developers! The output looks something like this: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe move this up, as I think this is the most helpful information in general?
|
@dschwoerer Do you have any opinion on completely removing |
|
I do not use it. I either use gdb or add print statements 😇 It may add some value in a few cases, but I think in general they should be considered left over debug output, that was once for one person useful, but can go. Probably better to cleanup the code 👍 |
I think it's probably simpler if we just always require it. Setting |
|
I guess this should probably use a git submodule like fmt and mpark.variant, which are also required, rather than |
Yes, that would make it easier to include in the release! |
We have historically been reluctant to add too many external dependencies, but with CMake and spack making it easier to pull them in and build them, I thought I'd try this library for generating stack traces.
Two useful things:
addr2lineand instead can always enable backtracesand can even enable colour:
It also has its own signal handler we could perhaps use instead of ours (though I've not tested this out).
Lastly, this is maybe a complete enough solution that we could also remove
MsgStack/TRACE/AUTO_TRACE?