Quantcast

[stacktrace] review

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[stacktrace] review

Artyom Beilis-2
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/booster/backtrace.h
https://sourceforge.net/p/cppcms/code/HEAD/tree/framework/trunk/booster/lib/backtrace/src/backtrace.cpp

[2] https://sourceforge.net/p/cppcms/code/HEAD/tree/framework/trunk/booster/booster/backtrace.h#l271


Best Regards,

Artyom Beilis

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [stacktrace] review

Antony Polukhin
2016-12-21 18:58 GMT+03:00 Artyom Beilis <[hidden email]>:
<...>
> 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/booster/backtrace.h
> https://sourceforge.net/p/cppcms/code/HEAD/tree/framework/trunk/booster/lib/backtrace/src/backtrace.cpp
>
> [2] https://sourceforge.net/p/cppcms/code/HEAD/tree/framework/trunk/booster/booster/backtrace.h#l271

Thanks for the review and for the links!

--
Best regards,
Antony Polukhin

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Stacktrace] review

Barrett Adair
In reply to this post by Artyom Beilis-2
On Wed, Dec 14, 2016 at 2:43 AM, Niall Douglas
<[hidden email]> wrote:

> The formal review of the Stacktrace library by Antony Polukhin starts
> today 14th Dec and will conclude before Christmas. I appreciate we
> are likely a bit tired out from the many library reviews recently and
> of course it's Christmas, but given the lack of a portable way to
> work with stack backtraces, which you inevitably need to do
> eventually in any non-toy production application, Stacktrace needs
> your review!
>
> For your review you may wish to consider the following questions:
>
> What is your evaluation of the design?

I like it.

> What is your evaluation of the implementation?

I've never implemented a stacktrace reader, so I don't have much to add.
The code did not raise any red flags for me, though.

> What is your evaluation of the documentation?

Documentation is solid.

> What is your evaluation of the potential usefulness of the library?

Very useful. I will likely use this at work.

> Did you try to use the library?  With what compiler?  Did you have any problems?

Yes, GCC 5.3, no problems. This stacktrace made me smile:

0# boost::stacktrace::detail::backend::backend(void**, unsigned long)
 1# s::~s()
 2# void __gnu_cxx::new_allocator<s>::destroy<s>(s*)
 3# void std::allocator_traits<std::allocator<s>
>::destroy<s>(std::allocator<s>&, s*)
 4# std::_Sp_counted_ptr_inplace<s, std::allocator<s>,
(__gnu_cxx::_Lock_policy)2>::_M_dispose()
 5# std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release()
 6# std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count()
 7# std::__shared_ptr<s, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr()
 8# std::shared_ptr<s>::~shared_ptr()
 9# hello()::{lambda()#1}::~hello()
10# std::_Function_base::_Base_manager<hello()::{lambda()#1}>::_M_destroy(std::_Any_data&,
std::integral_constant<bool, false>)
11# std::_Function_base::_Base_manager<hello()::{lambda()#1}>::_M_manager(std::_Any_data&,
std::_Function_base::_Base_manager<hello()::{lambda()#1}> const&,
std::_Manager_operation)
12# std::_Function_base::~_Function_base()
13# std::function<void ()>::~function()
14# void std::_Destroy<std::function<void ()> >(std::function<void ()>*)
15# void std::_Destroy_aux<false>::__destroy<std::function<void
()>*>(std::function<void ()>*, std::function<void ()>*)
16# void std::_Destroy<std::function<void ()>*>(std::function<void
()>*, std::function<void ()>*)
17# void std::_Destroy<std::function<void ()>*, std::function<void ()>
>(std::function<void ()>*, std::function<void ()>*,
std::allocator<std::function<void ()> >&)
18# std::vector<std::function<void ()>,
std::allocator<std::function<void ()> > >::~vector()
19# hello()
20# main
21# __libc_start_main
22# _start

> How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?

About 45 minutes total. I skimmed the code, the documentation, and the
test cases. Then,
I made a little test program.

> Are you knowledgeable about the problem domain?

No.

> Do you think the library should be accepted as a Boost library?

Yes, unequivocally. The concerns about async safety are interesting,
but ultimately
this library is useful to me (and likely many others) regardless of
signal support.

Barrett

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [stacktrace] review

mbartosik
In reply to this post by Artyom Beilis-2
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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [stacktrace] review

Boost - Dev mailing list
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/


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [stacktrace] review

Boost - Dev mailing list
In reply to this post by mbartosik
std::cout << symbol_location_ptr(f.address()); // outputs `some/path/test.exe`

2017-03-08 22:59 GMT+03:00 mbartosik via Boost <[hidden email]>:
> 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

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [stacktrace] review

Boost - Dev mailing list
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

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Loading...