[boost] [stacktrace] review

Subject: [boost] [stacktrace] review
From: Artyom Beilis (artyom.beilis_at_[hidden])
Date: 2016-12-21 10:58:16

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