Re: [boost] Stacktrace library starts review today 14th Dec

Subject: Re: [boost] Stacktrace library starts review today 14th Dec
From: Klemens Morgenstern (klemens.morgenstern_at_[hidden])
Date: 2016-12-17 09:10:42

> Review guidelines
> =================
>
> Reviews should be submitted to the developer list
> (boost_at_[hidden]), preferably with '[stacktrace]' in the
> subject. Or if you don't wish to for some reason or are not
> subscribed to the developer list you can send them privately to me at
> 's_sourceforge at nedprod dot com'. If so, please let me know whether
> or not you'd like your review to be forwarded to the list.
>
> For your review you may wish to consider the following questions:
>
>        - What is your evaluation of the design?
Seems like the right approach to me. You have the stacktrace that stores 
the trace and you can get more information through the frame class.

Since the discussion about attaching a stack-trace to an exception got 
rolling, I'd like to propose two functions:

boost::stacktrace::throw_with_st<std::runtime_error>("bla"); //throws an 
exception derived from std::runtime_error

and

... catch(std::runtime_error & re) {
    boost::stacktrace::rethrow_with_st(re); //rethrows, but has the 
stacktrace attached
};

I don't think the latter would work with std::exception_ptr, but if it 
does, that would be even better.

>
>        - 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.
Seems to be the overall right way to me. However there are a few problems:

- backend_linux/add2line
Do I have a guarantee that this tool works right for every compiler on 
linux? I would instinctively think not. If I'm right here, there should 
at the very least be a macro which allows to change the program name.

Additionally I would require that <windows.h> is not included in 
backend_windows.hpp, but to go the route of boost/winapi or to put this 
include in the source. Or to rename backend_windows.hpp to 
backend_windows.ipp, so the intent is obvious.

Also, I get why there's the stack-trace function at the top of the 
stacktrace, and it would be the wrong approach to remove it manually. I 
would however like a workaround, so that the stacktrace doesn't include 
those calls.

Something like that maybe: (CaptureStackBackTrace is obtained through a 
fwd-declaration as in boost/winapi)

#define BOOST_STACKTRACE(Name) \
boost::stacktrace::stacktrace 
Name(boost::stacktrace::detail::empty_tag()); \
BOOST_FILL_STACKTRACE(Name);

#if defined(BOOST_STACKTRACE_WINDOWS)

#define BOOST_FILL_STACKTRACE(Obj) \
{ \
       boost::detail::winapi::ULONG_ hc = 0; \
::CaptureStackBackTrace(0,static_cast<boost::detail::winapi::ULONG_>( 
boost::stacktrace::stacktrace::size), \
Obj.native_handle(), &hc); \
}
#endif

The downside would be, that this wouldn't work well with the 
link-solution for noop. But maybe instead of using CaptureStackBackTrace 
directly, this could be done through a function-ptr given by the linked 
library, so that it can point to a noop.

It would also be awesome if you could provide a better build 
description, especially since this library may be used by other boost 
libraries in the future.

Something like that:

lib foo : bar.cpp : <stacktrace>off ; //not link to anything
lib foo : bar.cpp : <stacktrace>noop ; //link against the empty backend
lib foo : bar.cpp : <stacktrace>windbg ; //windows
...
lib foo : bar.cpp : <stacktrace>on ; //automatically select the default 
with debug, none with release.

I think if that is defined in stacktrace/build/Jamfile.v2 it would only 
be available when building boost or having boost through use-project. 
That would be very helpful.

>        - What is your evaluation of the documentation?
I think that about right.

>        - What is your evaluation of the potential usefulness of the
>          library?
>        - Did you try to use the library?  With what compiler?  Did you
>          have any problems?
Yes, mingw 6 (didn't work) and msvc 19, where it worked.

I got an error with msvc (RtlCaptureStackBackTrace not defined), because 
I hadn't defined
_WIN32_WINNT correclty. It might be useful to add an error/warning for that.
>        - How much effort did you put into your evaluation? A glance? A
>          quick reading? In-depth study?
Tried a simple example, executed the tests, looked through the 
implementation. ~2h.
>        - Are you knowledgeable about the problem domain?
No, i.e. not more than knowing what a stacktrace is.
>
> And finally, every review should attempt to answer this question:
>
>        - Do you think the library should be accepted as a Boost
> library?
>
Yes. I do think it can be improved by further additions, but the basis 
is sound.