[stacktrace] review
Hello, This is my review of stacktrace library: Note that I have experience with the subject as I developed a similar library for the CppCMS framework [1]
- What is your evaluation of the design?
The design is straight forward and I don't see any major issues with the API. Also I have several suggestions in handling backtrace with catch from } catch (const traced& e) { std::cerr << e.what() << '\n'; if (e.trace) { std::cerr << "Backtrace:\n" << e.trace << '\n'; } } catch (const std::exception& e) { std::cerr << e.what() << '\n'; } In my library I use much simpler approach in terms of API [2] catch(std::exception const &e) { std::cerr << e.what() << std::endl; std::cerr << booster::trace(e); } Where booster::trace is a manipulator that prints strack trace if "e" contains backtrace information - much shorted code without additional catch. I find it very easy to use. Just a suggestion. I also like with_trace templated class idea.
- What is your evaluation of the implementation? Most of my personal concerns with this library are with the implementation and I would hugely appreciate feedback from others with substantial experience of using stacktracing "in anger" in non-trivial use case scenarios.
I mostly reviewed the backend. Linux: ========= 1st of all: I recommend mentioning in the documentation the option -rdynamic/-export-dynamic as otherwise dladdr would likely to fail - it is critical as many may miss one. Regarding use of add2line utility: 1. Failing to calling an external program in case of dladdr fails is bad idea. This behaviour is unexpected by the user. I understand the advantage of this as you can use debug information but should be done only per user specific request (i.e. compilation option) and not by default. 2. If you call add2line - use it for ALL addresses in backtrace at once and not for each single one! My recommendation is to provide alternative backed that uses other way to extract information not involving forking. Maybe try to analyze debug data or so - or require preparing initial map using nm tool and than loading it from file or something like that. Additionally calling the backed linux is bad - as same approach with slight modifications works on *BSD, Mac OS X and Solaris (RIP). Windows: ======== I have a question why you used COM instead of SymFromAddr functions? I understand thread safety issues but simple mutex can solve them. My concerns are regarding MinGW compilers. Have you tested it on MinGW compiler? It wouldn't work as it used MSVC debug information in general so maybe it is good idea to provide an alternative backend that extracts only address and DLL name so you can analyse the stacktrace later (using addr2line utility for example) Take a look on my code [1] regarding non-msvc windows compilers - it provides at least some useful information.
- What is your evaluation of the documentation?
The documentation is ok in general and straight forward as the library itself. What I do lack is the documentation of underlying backends - how do they work. 1. use of -rdynamic/-export-dynamic should be noted in "bold face" regarding the use of the library. 2. please do not call it "POSIX backend" - as there none of the stuff you use is actually POSIX... Also you'll probably need different backends for MINGW and mention it.
- What is your evaluation of the potential usefulness of the library?
Very Very useful.
- Did you try to use the library? With what compiler? Did you
have any problems? No I didn't - I mostly read the documentation and the backend code.
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
3-4 hours
- Are you knowledgeable about the problem domain?
Yes I am. I have written similar library for CppCMS project [1] - FINAL VOTE: Yes, Conditionally Conditions: Use of addr2line must not be enabled by default. [1] https://sourceforge.net/p/cppcms/code/HEAD/tree/framework/trunk/booster/boos... https://sourceforge.net/p/cppcms/code/HEAD/tree/framework/trunk/booster/lib/... [2] https://sourceforge.net/p/cppcms/code/HEAD/tree/framework/trunk/booster/boos... Best Regards, Artyom Beilis
2016-12-21 18:58 GMT+03:00 Artyom Beilis <artyom.beilis@gmail.com>: <...>
I have a question why you used COM instead of SymFromAddr functions? I understand thread safety issues but simple mutex can solve them.
The library is header only by default, so it's impossible to create a global mutex. This may also interact badly with libraries that use the same functions.
My concerns are regarding MinGW compilers. Have you tested it on MinGW compiler? It wouldn't work as it used MSVC debug information in general so maybe it is good idea to provide an alternative backend that extracts only address and DLL name so you can analyse the stacktrace later (using addr2line utility for example)
Yes, MinGW build requires fixes. I'll take care of that.
Take a look on my code [1] regarding non-msvc windows compilers - it provides at least some useful information.
- What is your evaluation of the documentation?
The documentation is ok in general and straight forward as the library itself. What I do lack is the documentation of underlying backends - how do they work.
Good point! Thanks!
1. use of -rdynamic/-export-dynamic should be noted in "bold face" regarding the use of the library.
Those flags are not required because addr2line can extract information from debug sections of the binary.
2. please do not call it "POSIX backend" - as there none of the stuff you use, not only from the export sections is actually POSIX...
Also you'll probably need different backends for MINGW and mention it.
+1
[1] https://sourceforge.net/p/cppcms/code/HEAD/tree/framework/trunk/booster/boos... https://sourceforge.net/p/cppcms/code/HEAD/tree/framework/trunk/booster/lib/...
[2] https://sourceforge.net/p/cppcms/code/HEAD/tree/framework/trunk/booster/boos...
Thanks for the review and for the links! -- Best regards, Antony Polukhin
I see that Boost.stacktrace is up for review March 17, 2017 - March 26, 2017 I've got some broad feedback. I'm in a race to comment before I go on vacation on Friday, so I'm sorry if I've missed something or misunderstood. I will try and follow the thread while on vacation - but not sure that will happen. 1) Resolving function names can be VERY expensive. Potentially it requires multi-megabyte files to be loaded and parsed. It is not clear to me that the resolution of frame.name() is going to be deferred. I have written a _lot_ of stack tracing code, and have usually in practice needed something that is fast and efficient to collect stack trace(s), but which allows deferring of the cost of resolving symbolic information until I really need it. In the extreme example I had some code which speculatively collected 100,000's of stack traces, but only symbolically resolved 10's of them. Thus the gathering of the 100,000's of traces had to be very very efficient (only the return addresses being saved) but the symbolic resolution did not have to be so fast. This also solved the issue of working within signals which I also had to do, indeed the code had been used to arguably work at an even lower level than signal handlers -- but more on that would get off topic. I think that the interface needs to reflect this. Or at a minimum clearly state that symbol resolution will be delayed until absolutely necessary -- I think that it is best if the interface reflects this. In addition to my own code, the company I currently work for has a multi-platform stack trace library which also defers symbols resolution. Also until the symbols are resolved I think that the space requirement for a frame should be kept to the minimum (i.e. one pointer). This may necessitate two classes e.g. raw_stacktrace and symbolic_stacktrace with a means to convert between them, the symbolic_stacktrace should be all that is needed by default i.e. for most simple case users, potentially that means that having stacktrace and raw_stacktrace is better (raw_stacktrace only having a single pointer for each raw_frame). 2) Tracing optimized code on some architectures can be really quite difficult. Especially when there are performance constraints (for example on Microsoft x86 should FPO information be loaded, should the whole .pdb file be loaded to walk the stack). So I would suggest a user specified function could be used to provide some choice in the algorithm for how walk the stack back, with some default. 3) Modules / libraries. Something else I have found useful is the ability to examine a frame and find out the code module e.g. a .dll or .so from it. That then raises the possibility of another object in addition to stacktrace and frame. Members that I have found useful on a module object are things like path, load address, length (maybe begin/end for the address in memory), name, version, copyright notice (this can get rather platform specific). A boost::stacktrace is definitely needed. I'm hoping that we'll get one that can meet more than the simple case needs. - Mark -- View this message in context: http://boost.2283326.n4.nabble.com/stacktrace-review-tp4690649p4692111.html Sent from the Boost - Dev mailing list archive at Nabble.com.
On 08/03/2017 19:59, mbartosik via Boost wrote:
I see that Boost.stacktrace is up for review March 17, 2017 - March 26, 2017
I've got some broad feedback. I'm in a race to comment before I go on vacation on Friday, so I'm sorry if I've missed something or misunderstood. I will try and follow the thread while on vacation - but not sure that will happen. [snip]
Thanks for the pre-review review. I don't want to disturb the Safe Numerics review, but here is the changelog in Stacktrace since the last review: * Reimplemented boost::stacktrace::basic_stacktrace class to accept allocators and to have a small size * Added outputting of the path to the binary object into the stacktrace, when source location is not available * Allowed to use `libbacktrace` - a defacto standard for GCC/CLANG backtracing that does not fork and has a permissive license * Optimized stacktrace and frames ostreaming * Removed backends and made the default tracing to have only platform basic functionality. Added macros to enable additional functionality. * Fixed (theoretically) MinGW compilation * Added functions for async-safe dumping into memory or descriptor * Changed signal handling example to be async-signal-safe * Allowed users to choose how many frames to skip during stack capture * Removed `boost::stacktrace::stacktrace` functions and other internals from printed stack traces * Updated docs: * examples now use Boost.Exception * removed all the references to "backend" * reduced macro count * "Build, Macros and Backends" section was rewritten and became "Configuration and Build" * added note that library is C++03 * added examples on storing traces into shared memory * updated all the human readable dumps Some of these address your concerns in your review, however quite a few points in your review have not been addressed, and some look quite important. Perhaps Antony might consider these before the 17th? Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
std::cout << symbol_location_ptr(f.address()); // outputs `some/path/test.exe` 2017-03-08 22:59 GMT+03:00 mbartosik via Boost <boost@lists.boost.org>:
I see that Boost.stacktrace is up for review March 17, 2017 - March 26, 2017
I've got some broad feedback. I'm in a race to comment before I go on vacation on Friday, so I'm sorry if I've missed something or misunderstood. I will try and follow the thread while on vacation - but not sure that will happen.
Thanks a lot for the feedback!
1) Resolving function names can be VERY expensive. Potentially it requires multi-megabyte files to be loaded and parsed. It is not clear to me that the resolution of frame.name() is going to be deferred. I have written a _lot_ of stack tracing code, and have usually in practice needed something that is fast and efficient to collect stack trace(s), but which allows deferring of the cost of resolving symbolic information until I really need it. In the extreme example I had some code which speculatively collected 100,000's of stack traces, but only symbolically resolved 10's of them. Thus the gathering of the 100,000's of traces had to be very very efficient (only the return addresses being saved) but the symbolic resolution did not have to be so fast. This also solved the issue of working within signals which I also had to do, indeed the code had been used to arguably work at an even lower level than signal handlers -- but more on that would get off topic.
I think that the interface needs to reflect this. Or at a minimum clearly state that symbol resolution will be delayed until absolutely necessary -- I think that it is best if the interface reflects this.
In addition to my own code, the company I currently work for has a multi-platform stack trace library which also defers symbols resolution.
Also until the symbols are resolved I think that the space requirement for a frame should be kept to the minimum (i.e. one pointer). This may necessitate two classes e.g. raw_stacktrace and symbolic_stacktrace with a means to convert between them, the symbolic_stacktrace should be all that is needed by default i.e. for most simple case users, potentially that means that having stacktrace and raw_stacktrace is better (raw_stacktrace only having a single pointer for each raw_frame).
boost::stacktrace::stacktrace class only stores raw pointers. Names resolving happens only when you call frame::name(). I'll update the docs to make that more explicit. I see no need in `symbolic_stacktrace` because such class * usable only if multiple frame::name() calls for the same frame happen (which is a very rare case) * could be easily build by users Tell me if I'm wrong or missed some point.
2) Tracing optimized code on some architectures can be really quite difficult. Especially when there are performance constraints (for example on Microsoft x86 should FPO information be loaded, should the whole .pdb file be loaded to walk the stack). So I would suggest a user specified function could be used to provide some choice in the algorithm for how walk the stack back, with some default.
How does such function shall look like? Could you please give an example?
3) Modules / libraries. Something else I have found useful is the ability to examine a frame and find out the code module e.g. a .dll or .so from it. That then raises the possibility of another object in addition to stacktrace and frame. Members that I have found useful on a module object are things like path, load address, length (maybe begin/end for the address in memory), name, version, copyright notice (this can get rather platform specific).
This is a part of Boost.DLL library. In boost_1_64 it will have a `boost::dll::symbol_location_ptr` so you can write the following code: stacktrace st; // no symbol names extraction, only storing frame addresses frame f = st[0]; // copying a pointer under the hood std::cout << f; // outputs `foo() at source/path/foo.cpp:42`. Only at this line function name and source location are retrieved.
A boost::stacktrace is definitely needed. I'm hoping that we'll get one that can meet more than the simple case needs.
- Mark
Great thanks for the feedback! Are there more usecases that the library does not meet? -- Best regards, Antony Polukhin
Minor example fixes for the previous mail: This is a part of the Boost.DLL library. In boost_1_64 it will have a `boost::dll::symbol_location_ptr` so you can write the following code: stacktrace st; // no symbol names extraction, only storing frame addresses frame f = st[0]; // copying a pointer under the hood std::cout << symbol_location_ptr(f.address()); // outputs `some/path/test.exe` std::cout << f; // outputs `foo() at source/path/foo.cpp:42`. Only at // this line function name and source location are retrieved. -- Best regards, Antony Polukhin
participants (4)
-
Antony Polukhin -
Artyom Beilis -
mbartosik -
Niall Douglas