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

Subject: Re: [boost] Stacktrace library starts review today 14th Dec
From: Antony Polukhin (antoshkka_at_[hidden])
Date: 2016-12-17 12:05:07

2016-12-17 17:10 GMT+03:00 Klemens Morgenstern <klemens.morgenstern_at_[hidden]>:
>
>> 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.

I'm not providing functions and classes for embedding stacktraces into
exceptions... because there are too many ways to do that, they all
differ and there's no way to make all the users happy. So if the user
wishes to embed stacktrace - he has to write 5-15 lines of code in a
way he'd like it.

>>        - 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.

Most of the compilers use DWARF debugging because all the tools
(debuggers,addr2line) understand it.
The problem is that addr2line may not be installed. I'll clarify
addr2line requirement in the docs and will try to find another way of
getting debug info from address.

> 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.

On windows COM stuff is used a lot for backtracing. Moving it into the
boost/winapi may take a lot of time and effort. I'll do that some day,
but this is not a high priority issue for me.

> 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.

That top level frame may dissappear/reapper depending on the compiler
version and even compiler flags. I was experimenting with skipping the
first frame or forcing inlinement. I've found no good way to resolve
that issue.

> 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.

Calling a function pointer produces stack frame :(

> 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.

Just include the header <boost/stacktrace.hpp> and that's it. Nothing
to be done in Jamfile.v2

>>        - 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.

I'll fix that. There may be more such errors. Fortunately they all
could be easily captured by the Boost testing matrix (if the library
would be accepted into the Boost),

Thank you for the review!

-- 
Best regards,
Antony Polukhin