[boost] Stacktrace library starts review today 14th Dec

[Niall Douglas]

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!

Stacktrace is an optionally header-only library providing four
implementation backends, libunwind (POSIX only), windbg (Windows
only), backtrace (from the C library on most POSIX implementations)
and a null backend. At its very simplest it lets you capture the
stack backtrace for the calling thread and to print it to a
std::ostream& of your choice. The basic_stacktrace<> class quacks
like a STL container of frame object instances. The rest of the API
pretty much follows STL design principles for the most part. Use is
therefore unsurprising.

You can find the documentation at
http://apolukhin.github.io/stacktrace/index.html and the github repo
at https://github.com/apolukhin/stacktrace.

Review guidelines
=================

Reviews should be submitted to the developer list
(bo...@lists.boost.org), 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?

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

- What is your evaluation of the documentation?
- 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?
- How much effort did you put into your evaluation? A glance? A
quick reading? In-depth study?
- Are you knowledgeable about the problem domain?

And finally, every review should attempt to answer this question:

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

Be sure to say this explicitly so that your other comments don't
obscure your overall opinion.

Even if you do not wish to give a full review any technical comment
regarding the library is welcome as part of the review period and
will help me as the review manager decide whether the library should
be accepted as a Boost library. Any questions about the use of the
library are also welcome.

Finally, thanks to Edward whose announcement of the Synapse library
review I borrowed heavily from as I thought it very well structured.
Hopefully the above is just as clear.

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

[Vladimir Batov]

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

Yes. I personally very much believe the library should be accepted. I
myself use a similar facility to identify and to address a problem
promptly when reported by a customer. I am fortunate to only work on
Linux. Having a multi-platform community-reviewed community-tested
facility would be a much better solution.

I have not used the library. However, when it's in Boost I'll make use
of it immediately. Can't say much about implementation either. However,
the interface and the output seemed straightforward and sensible...
Although in my implementation I decided not to report the superfluous

0# boost::stacktrace::detail::backend::backend(void**, unsigned long)

Additionally the number of macros seemed surprisingly high and I might
say worrisome. Are they really that unavoidable? After all, it's almost
2017 outside. Secondly, when it crashes out there on the customer site,
I want as much info as I can get. So, if the default mode is configured
accordingly and, consequently, eases the deployment, that'd be of great
plus.

[Antony Polukhin]

2016-12-14 12:55 GMT+03:00 Vladimir Batov <Vladimi...@constrainttec.com>:
<...>

> I have not used the library. However, when it's in Boost I'll make use of it
> immediately. Can't say much about implementation either. However, the
> interface and the output seemed straightforward and sensible... Although in
> my implementation I decided not to report the superfluous
>
> 0# boost::stacktrace::detail::backend::backend(void**, unsigned long)

I've tried to do the same thing and failed:
* BOOST_FORCEINLINE may be ignored by compilers (and even worse -
produces a warning on some platforms when it is ignored).
* skipping predefined frames count fail too - depending on the
compiler/flags/version/platform different inlinement heuristics are
used and a chance of skipping useful frames appears

I'll try to do some more tweaking with BOOST_FORCEINLINE + warning
suppression. It may get better, but in some cases will continue to
output internals in backtraces.

> Additionally the number of macros seemed surprisingly high and I might say
> worrisome. Are they really that unavoidable?

By default everything works out-of-the-box and you do not need to
define macro. You will need those macros only for very experienced
tuning.

I can drop BOOST_STACKTRACE_USE_UNWIND and
BOOST_STACKTRACE_USE_WINDBG, but users may wish to make assertions on
those macro and even may wish to have different code:

void foo() {
#if defined(BOOST_STACKTRACE_USE_UNWIND) || defined(BOOST_STACKTRACE_USE_NOOP)
// Async safe code, do our own async safe printing
#else
// Not async safe, run only in non-production builds
#endif

}


> Secondly, when it crashes out there on the customer site, I want as
> much info as I can get.

Yes, that's the default behavior.

> I myself use a similar facility to identify and to address a problem promptly when reported by a customer. I am fortunate to only work on Linux.

Please, take a quick look at the Linux implementation
https://github.com/apolukhin/stacktrace/blob/master/include/boost/stacktrace/detail/backend_linux.hpp#L229
If your implementation has a more advanced technique for detecting
source file/line - I'd really appreciate a hint.


Thank you for the review!

--
Best regards,
Antony Polukhin

[Gavin Lambert]

On 15/12/2016 08:01, Antony Polukhin wrote:
> I've tried to do the same thing and failed:
> * BOOST_FORCEINLINE may be ignored by compilers (and even worse -
> produces a warning on some platforms when it is ignored).
> * skipping predefined frames count fail too - depending on the
> compiler/flags/version/platform different inlinement heuristics are
> used and a chance of skipping useful frames appears
>
> I'll try to do some more tweaking with BOOST_FORCEINLINE + warning
> suppression. It may get better, but in some cases will continue to
> output internals in backtraces.

Is it possible to identify internal code by address rather than frame
count? At least the start address of internal methods should be readily
obtainable, although lengths are perhaps more problematic.

Perhaps the same mechanisms used to turn trace addresses into
module/function information could also be used to filter out internal
frames after the fact?

[Vladimir Batov]

On 12/15/2016 06:01 AM, Antony Polukhin wrote:
> ...

> Please, take a quick look at the Linux implementation
> https://github.com/apolukhin/stacktrace/blob/master/include/boost/stacktrace/detail/backend_linux.hpp#L229
> If your implementation has a more advanced technique for detecting
> source file/line - I'd really appreciate a hint.

My version is nowhere advanced... I humbly use the facilities provided
by backtrace(), backtrace_symbols() and abi::__cxa_demangle() (GNU
extensions). Then, while traversing the array returned by
backtrace_symbols(), I skip the very first entry. For advanced you might
probably like to have a look at the backtrace(), backtrace_symbols()
source code.

[Antony Polukhin]

2016-12-15 3:44 GMT+03:00 Edward Diener <eldi...@tropicsoft.com>:
> In the documentation for stacktrace I do not see anything about what level
> of C++ is required for a compiler using the stacktrace library or any
> information on what compilers have been tested when using the library.

It's C++98. Tested on latest clang(c++03, c++11), gcc(c++98, c++11,
c++1y), msvc.


> In the section of "Build, Macros and Backends" I do not think there is
> enough information about using a particular backend, other than saying that
> you need to link with such-and-such. Does stacktrace "automatically" find
> the header files it needs from these backends ? Even so I think some
> information about where it looks for the backend header files need to be
> given. I also think that some information of what libraries it looks to link
> with needs to be given. I am assuming it links with the static versions of
> these backend libraries unless I define either BOOST_STACKTRACE_LINK and
> BOOST_ALL_DYN_LINK, or BOOST_STACKTRACE_DYN_LINK. What if I want to use the
> header-only version but I want to link with the shared library backend ? I
> also assume that if I link with the static library backend I should probably
> be linking with the RTL static library, while if I link with the shared
> library backend I should also be linking with the RTL shared library.
>
> What about 32-bit versus 64-bit use of stacktrace ? Do they both work as
> advertised with the correct backends, whether static or shared ? I think the
> "Build, Macros and Backends" need rto be expanded to cover alol these
> topics.

All the headers are found automatically and the library is header only
if you do not define BOOST_STACKTRACE_LINK or
BOOST_STACKTRACE_DYN_LINK.

Backend is chosen automatically in header only mode. You need to do
nothing, unless you encounter some problems on default setup (like you
OS is POSIX but not LSB Core Specification 4.1) or unless you wish to
disable stacktraces.

If you define BOOST_STACKTRACE_LINK or BOOST_STACKTRACE_DYN_LINK you
have to manually link with one of the backends.


I'll try to improve the "Build, Macros and Backends" section to be more clear.

--
Best regards,
Antony Polukhin

[Antony Polukhin]

2016-12-15 3:27 GMT+03:00 Vladimir Batov <Vladimi...@constrainttec.com>:
> On 12/15/2016 06:01 AM, Antony Polukhin wrote:
>>
>> ...
>> Please, take a quick look at the Linux implementation
>>
>> https://github.com/apolukhin/stacktrace/blob/master/include/boost/stacktrace/detail/backend_linux.hpp#L229
>> If your implementation has a more advanced technique for detecting
>> source file/line - I'd really appreciate a hint.
>
>
> My version is nowhere advanced... I humbly use the facilities provided by
> backtrace(), backtrace_symbols() and abi::__cxa_demangle() (GNU extensions).
> Then, while traversing the array returned by backtrace_symbols(), I skip the
> very first entry. For advanced you might probably like to have a look at the
> backtrace(), backtrace_symbols() source code.

I did that... and that code was not async-signal-safe and was not
extracting information from debug sections :(

--
Best regards,
Antony Polukhin

[Niall Douglas]

On 14 Dec 2016 at 22:01, Antony Polukhin wrote:

> 2016-12-14 12:55 GMT+03:00 Vladimir Batov <Vladimi...@constrainttec.com>:
> <...>
> > I have not used the library. However, when it's in Boost I'll make use of it
> > immediately. Can't say much about implementation either. However, the
> > interface and the output seemed straightforward and sensible... Although in
> > my implementation I decided not to report the superfluous
> >
> > 0# boost::stacktrace::detail::backend::backend(void**, unsigned long)
>
> I've tried to do the same thing and failed:
> * BOOST_FORCEINLINE may be ignored by compilers (and even worse -
> produces a warning on some platforms when it is ignored).
> * skipping predefined frames count fail too - depending on the
> compiler/flags/version/platform different inlinement heuristics are
> used and a chance of skipping useful frames appears
>
> I'll try to do some more tweaking with BOOST_FORCEINLINE + warning
> suppression. It may get better, but in some cases will continue to
> output internals in backtraces.

The way I've always done it in my stacktrace implementations is to
force *noinline* the capture function, then ignore the first stack
frame. Unlike force inline, compilers may not ignore force noinline,
so this works and produces the desired result.

Niall

--
ned Productions Limited Consulting
http://www.nedproductions.biz/
http://ie.linkedin.com/in/nialldouglas/

[Klemens Morgenstern]

> Review guidelines
> =================
>
> Reviews should be submitted to the developer list
> (bo...@lists.boost.org), 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.

[Antony Polukhin]

2016-12-17 17:10 GMT+03:00 Klemens Morgenstern <klemens.m...@gmx.net>:
>
>> Review guidelines
>> =================
>>
>> Reviews should be submitted to the developer list
>> (bo...@lists.boost.org), 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

[Andrey Semashev]

On Wed, Dec 14, 2016 at 11:43 AM, Niall Douglas
<s_sour...@nedprod.com> wrote:
> The formal review of the Stacktrace library by Antony Polukhin starts
>

> Stacktrace is an optionally header-only library providing four
> implementation backends, libunwind (POSIX only),

From the looks of it, it's not using libunwind but unwind facilities from libdl.

> - What is your evaluation of the design?
> - What is your evaluation of the implementation?

Design and implementation:

1. `basic_stacktrace` is a template, which has its static size as a
template parameter. This complicates using `basic_stacktrace` with
Boost.Exception as an attached attribute and in similar contexts,
where the type of `basic_stacktrace` is important and templates are
inconvenient/not suitable. The upside of this decision is that the
stacktrace does not allocate memory for the array of pointers, but in
return the `stacktrace` object is quite big (800 bytes), which can be
a problem. I would rather prefer `stacktrace` to not be templated on
its capacity and let the size and capacity be runtime values. You
already maintain its actual size in runtime anyway. The implementation
could include a small buffer optimization (e.g. up to 8 entries are
stored in-place, a dynamic buffer is allocated for more entries). Or
maybe just use `std::vector` internally.
2. The semantics of `basic_stacktrace` is too overloaded.
2.1. `basic_stacktrace` seem to mimic a container of `frame`s, yet
the default constructor creates a non-empty container with the current
stacktrace. This coupling is unnecessary. Let `stacktrace` be a pure
container without any further logic, i.e. default-constructing a
stacktrace creates an empty container of size and capacity equal to 0.
Make a separate free function for obtaining the current stacktrace
(e.g. `template< typename Backend = default_backend >
basic_stacktrace< Backend > current_stacktrace(std::size_t
max_depth)`; see below on the backends).
2.2. Remove the hash code from the container, provide algorithms for
its calculation instead (i.e. provide `hash_value()` overload for
`frame`, `hash_range()` should work for the stacktrace then; provide a
specialization for `std::hash`).
3. The semantics of the backend are unclear. For some reason it
manages the data that is presumably owned by the stacktrace. I think
it is misplaced to be embedded in the stacktrace itself. I quickly
skimmed through implementation and it seems the backend contains no
data (besides the pointer to the buffer and the number of entries,
which should belong to the `stacktrace`, and the hash value, which I
don't think should be stored at all). This implies that the backend is
actually a set of algorithms (please, correct me if I'm wrong). In
that case it would be reasonable to:
3.1. Design the backend as a class with static algorithms a-la
`std::char_traits` or various traits from Boost.Intrusive.
3.2. Document this concept, with all required algorithms, and make
it a template argument for the `stacktrace` and the `frame`.
Alternatively, you could pass it to all algorithms working with the
stacktraces and frames, but I think it makes more sense to relate the
contents of the `stacktrace` and `frame` with the particular backend
that was used to generate it.
3.3. Provide multiple backends, as you already do, but name them
appropriately and implement as separate types (i.e. `windows_backend`,
`unwind_backend`, etc.) For every supported platform, select the
default backend that makes most sense and provide a typedef
`default_backend`.
3.4. The `frame`, the `basic_stacktrace` and each backend should be
separate components. In particular, they should not include each other
unless necessary, and interact using well-defined and documented
interfaces. This opens the possibility of user-defined backends.
4. The documented mechanism for embedding a stacktrace into an
exception seem to duplicate the same functionality from
Boost.Exception. I think that reinvents the wheel, and should be
removed. I'd recommend working towards better support for
Boost.Exception and provide examples of integration with it. (Note:
I'm not referring to the topic of integrating with
`BOOST_THROW_EXCEPTION` here.)
5. http://apolukhin.github.io/stacktrace/boost_stacktrace/getting_started.html#boost_stacktrace.getting_started.global_control_over_stacktrace_o.
I'm not sure how global customization and how it is related to
defining `BOOST_STACKTRACE_DEFAULT_MAX_DEPTH`, but formatting
customization surely must not require the user to define any macros.
Formatting is typically implemented with a locale facet, which I think
is what should be used in this case as well. The facet may support
format customization. I would suggest separate configuration of the
`frame` format and `stacktrace` format (as the list of strings
produced by formatting frames according to ther format). BTW, the
`operator<<` for `basic_stacktrace` and `frame` don't check that the
stream is good before proceeding.
6. `basic_stacktrace` is missing some of the typedefs required for
containers, such as `value_type`, `size_type`, `difference_type`. The
`reference` typedef is a value type, which I think makes the class
incompatible with the container concept. `#include <cstddef>,
<iterator>` are missing in stacktrace.hpp.
7. `const_iterator` need not have a pointer to the backend. If the
backend is a template parameter, it can have just a pointer to the
frame within the `stacktrace` container and use static algorithms from
the backend. Or, actually, it doesn't need any algorithms, if
everything is implemented by the frame.
8. Apparently, on Linux every call to `frame::source_file()` and
`frame::source_line()` forks a process. I don't think this is
documented anywhere, while it has serious reprecussions, starting from
that `addr2line` has to be availeble and executable on the system.
This is also a potential security issue because the process is run
with the host process priviledges, and uses path resolution (i.e. a
forged `addr2line` process can be executed with the host process
rights). For me, this one issue is an immediate blocker.
9. Although I don't think this is documented, AFAIU the library does
not support loading debug symbols from external files, at least not on
Linux. If that's truly the case, the usefullness of the library is
severely reduced because distributed software is typically stripped,
with optional debug symbols available in separate files.

In general I think the library is not sufficiently well designed. The
classes' concepts are not well defined and separated. Regarding the
implementation, the critical problem is forking a process under the
hood.

> - What is your evaluation of the documentation?

Documentation:

1. In general, the docs feel very terse. In some sections it feels
there's not enough explanation given. I'll try listing the specific
places below.
1.1. http://apolukhin.github.io/stacktrace/boost_stacktrace/getting_started.html#boost_stacktrace.getting_started.enabling_and_disabling_stacktrac
- I did not understand what it means to "enable/disable stacktraces
for a whole project". Until that point it felt like there is no need
for any special action to be able to use the library in any place of
my code, so what changes when `BOOST_STACKTRACE_LINK` is defined?
1.2. http://apolukhin.github.io/stacktrace/boost_stacktrace/getting_started.html#boost_stacktrace.getting_started.saving_stacktraces_by_specified_
- I think, `frame` type is insufficiently described, and before the
section describing format customization it would be helpful to have a
section describing that class, what it offers, etc.
1.3. http://apolukhin.github.io/stacktrace/boost_stacktrace/getting_started.html#boost_stacktrace.getting_started.getting_function_information_fro
- It is not clear from the example where that `my_signal_handler`
comes from. It seems, the example relies on some previous examples,
but doesn't mention that or which those previous examples are. Better
make the example more self-contained.
2. The example in
http://apolukhin.github.io/stacktrace/boost_stacktrace/getting_started.html#boost_stacktrace.getting_started.handle_terminates_aborts_and_seg
is incorrect. I believe, streaming into `cout` is not allowed from the
signal handler. The example should be corrected or removed. (Oh, there
is a footnote about it way down at the bottom of the page. It is
easily missed, and does not fix the example. People will read the
example, perhaps copy/paste it in their projects, so the example has
to be valid.)
3. Although I have limited knowledge myself, English could be improved
in a few places. I could suggest corrections, if needed and noone with
better English offer help.

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

I tried to compile the library and tests on Kubuntu 16.10 x86_64, gcc
6.2 with the following comand line:

bjam -j 8 --toolset=gcc "cxxflags=-std=c++0x
-Wno-unused-local-typedefs -ftemplate-backtrace-limit=0" variant=debug
debug-symbols=on threading=multi runtime-link=shared link=static
libs/stacktrace/test

Two tests have failed:

testing.capture-output
bin.v2/libs/stacktrace/test/autodetect_ho.test/gcc-6.2.0/debug/link-static/threading-multi/autodetect_ho.run
====== BEGIN OUTPUT ======
0# 0x559ff868b3b9
1# 0x559ff868f309
2# 0x559ff868f337
3# 0x559ff8687728
4# 0x559ff868aab8
5# __libc_start_main
6# 0x559ff868715a

' 0# 0x559ff868b3b9
1# 0x559ff868f4dc
2# 0x559ff868f108
3# 0x559ff868f28a
4# 0x559ff868f0f4
5# 0x559ff868f28a
6# 0x559ff868f0f4
7# 0x559ff868f28a
8# 0x559ff868f0f4
9# 0x559ff868f28a
10# 0x559ff868f0f4
11# 0x559ff868f28a
12# 0x559ff868f0f4
13# 0x559ff868f28a
14# 0x559ff868f0f4
15# 0x559ff868f28a
16# 0x559ff868f0f4
17# 0x559ff868f28a
18# 0x559ff8687966
19# 0x559ff868aabd
20# __libc_start_main
21# 0x559ff868715a
'

0# 0x559ff868b3b9
1# 0x559ff868f1cc
2# 0x559ff868f28a
3# 0x559ff868f0f4
4# 0x559ff868f28a
5# 0x559ff868f0f4
6# 0x559ff868f28a
7# 0x559ff868f0f4
8# 0x559ff868f28a
9# 0x559ff868f0f4
10# 0x559ff868f28a
11# 0x559ff868f0f4
12# 0x559ff868f28a
13# 0x559ff868f0f4
14# 0x559ff868f28a
15# 0x559ff868f0f4
16# 0x559ff868f28a
17# 0x559ff8687966
18# 0x559ff868aabd
19# __libc_start_main
20# 0x559ff868715a

libs/stacktrace/test/test.cpp(67): test 'ss1.str().find("foo1") !=
std::string::npos' failed in function 'void test_nested()'
libs/stacktrace/test/test.cpp(68): test 'ss1.str().find("foo2") !=
std::string::npos' failed in function 'void test_nested()'
libs/stacktrace/test/test.cpp(69): test 'ss2.str().find("foo1") !=
std::string::npos' failed in function 'void test_nested()'
libs/stacktrace/test/test.cpp(70): test 'ss2.str().find("foo2") !=
std::string::npos' failed in function 'void test_nested()'
4 errors detected.

EXIT STATUS: 1
====== END OUTPUT ======

testing.capture-output
bin.v2/libs/stacktrace/test/unwind_ho.test/gcc-6.2.0/debug/link-static/threading-multi/unwind_ho.run
====== BEGIN OUTPUT ======
0# 0x557c10ce13b9
1# 0x557c10ce5309
2# 0x557c10ce5337
3# 0x557c10cdd728
4# 0x557c10ce0ab8
5# __libc_start_main
6# 0x557c10cdd15a

' 0# 0x557c10ce13b9
1# 0x557c10ce54dc
2# 0x557c10ce5108
3# 0x557c10ce528a
4# 0x557c10ce50f4
5# 0x557c10ce528a
6# 0x557c10ce50f4
7# 0x557c10ce528a
8# 0x557c10ce50f4
9# 0x557c10ce528a
10# 0x557c10ce50f4
11# 0x557c10ce528a
12# 0x557c10ce50f4
13# 0x557c10ce528a
14# 0x557c10ce50f4
15# 0x557c10ce528a
16# 0x557c10ce50f4
17# 0x557c10ce528a
18# 0x557c10cdd966
19# 0x557c10ce0abd
20# __libc_start_main
21# 0x557c10cdd15a
'

0# 0x557c10ce13b9
1# 0x557c10ce51cc
2# 0x557c10ce528a
3# 0x557c10ce50f4
4# 0x557c10ce528a
5# 0x557c10ce50f4
6# 0x557c10ce528a
7# 0x557c10ce50f4
8# 0x557c10ce528a
9# 0x557c10ce50f4
10# 0x557c10ce528a
11# 0x557c10ce50f4
12# 0x557c10ce528a
13# 0x557c10ce50f4
14# 0x557c10ce528a
15# 0x557c10ce50f4
16# 0x557c10ce528a
17# 0x557c10cdd966
18# 0x557c10ce0abd
19# __libc_start_main
20# 0x557c10cdd15a

libs/stacktrace/test/test.cpp(67): test 'ss1.str().find("foo1") !=
std::string::npos' failed in function 'void test_nested()'
libs/stacktrace/test/test.cpp(68): test 'ss1.str().find("foo2") !=
std::string::npos' failed in function 'void test_nested()'
libs/stacktrace/test/test.cpp(69): test 'ss2.str().find("foo1") !=
std::string::npos' failed in function 'void test_nested()'
libs/stacktrace/test/test.cpp(70): test 'ss2.str().find("foo2") !=
std::string::npos' failed in function 'void test_nested()'
4 errors detected.

EXIT STATUS: 1
====== END OUTPUT ======

I didn't try using the library in my projects.

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

I read the docs and the implementation. I think, about 3 hours of study.

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

In general, I think a stacktrace library would be useful. I have
reservations about this particular library, though.

> - Are you knowledgeable about the problem domain?

I can't say I have a deep knowledge. I know what a stacktrace is and
have experience with gdb and other debuggers wrt. stacktraces. I did
not implement a stacktrace library myself.

> And finally, every review should attempt to answer this question:
>
> - Do you think the library should be accepted as a Boost
> library?

There are a few points in my review that I'm uncertain about (perhaps,
I misunderstood some bits of implementation or rationale), but at the
same time there are critical points that I think affect usability of
the library, at least for me. In particular, process forking under the
hood is a no go for me. Some design choices seem questionable to me,
like unclear separation of concepts and stacktrace implementation. I
don't think those issues are easy to resolve after the review, so my
vote is NO, the library should not be accepted in the current form.

That said, I'd like to thank Antony for the submission and encourage
him to continue his work on the library. The library does try to fill
an important gap.

[Niall Douglas]

On 17 Dec 2016 at 20:05, Antony Polukhin wrote:

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

addr2line is also borked on some distros. I've found llvm-symbolizer
to be much more reliable.

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

As I mentioned in a previous post, you want to use noinline to force
a guaranteed first entry to strip. As it's a header only library,
you'll get this perversity:

__declspec(noinline) inline void make_backtrace(...)

... which is completely legal because you want inline linkage
semantics, but to never inline the function.

Niall

--
ned Productions Limited Consulting
http://www.nedproductions.biz/
http://ie.linkedin.com/in/nialldouglas/

[Antony Polukhin]

2016-12-17 20:51 GMT+03:00 Andrey Semashev <andrey....@gmail.com>:
<...>

> That said, I'd like to thank Antony for the submission and encourage
> him to continue his work on the library. The library does try to fill
> an important gap.

Thank you for the review!

I'd like to discuss a proposed changes. Thing that I'm worried about:
* replacing class `stacktrace` with vector<void*> will definitely
break the most common use case std::cout << get_stacktrace(); as it's
not a good idea to overload ostream operator for vector of void
pointers
* addr2line and forking is not nice. Is linking with a GPL licensed
code an acceptable alternative?
* removing `basic_stacktrace` will definitely remove all the
possibilities for optimization of caching COM/parsed DWARF. This is a
quick win that later may turn into big troubles.

--
Best regards,
Antony Polukhin

[Klemens Morgenstern]

Am 17.12.2016 um 18:05 schrieb Antony Polukhin:
> 2016-12-17 17:10 GMT+03:00 Klemens Morgenstern <klemens.m...@gmx.net>:
>>
>
>>> - 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.

Thus (in addition to what Niall said) it should be configurable.

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

But that might be a problem. I would expect the stacktrace to be
essentially the same on different compilers. I get why stripping is not
a goot solution, but that seems problematic to me.

>
>> 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 :(

What I meant was, having a pointer to CaptureStackBackTrace or an empty
replacement. So it would be essentially the same as calling it directly.
Thus you'd have a direct call to the sys-function. This could also be
done with an alias as in compiler-extensions.

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

Yeah, but what are those binaries for then?

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

No Problem, it really fills a need.
Any Idea how to get it to work with mingw?

[Antony Polukhin]

2016-12-17 21:17 GMT+03:00 Niall Douglas <s_sour...@nedprod.com>:
> On 17 Dec 2016 at 20:05, Antony Polukhin wrote:
>
>> 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.
>
> addr2line is also borked on some distros. I've found llvm-symbolizer
> to be much more reliable.

I'll take a look at it. Thanks!

>> > 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.
>
> As I mentioned in a previous post, you want to use noinline to force
> a guaranteed first entry to strip. As it's a header only library,
> you'll get this perversity:
>
> __declspec(noinline) inline void make_backtrace(...)
>
> ... which is completely legal because you want inline linkage
> semantics, but to never inline the function.

Oh, now I've got that. I've been confused by the `__declspec(noinline)
inline`. Thanks again!
Does the `BOOST_NOINLINE inline` solution work well on GCC, CLANG and
other compilers?

--
Best regards,
Antony Polukhin

[Antony Polukhin]

2016-12-17 21:38 GMT+03:00 Klemens Morgenstern <klemens.m...@gmx.net>:
<...>

> Any Idea how to get it to work with mingw?

Try to remove the "Rtl", so it would just become
CaptureStackBackTrace. If that helps - please, tell me.

--
Best regards,
Antony Polukhin

[Andrey Semashev]

On Sat, Dec 17, 2016 at 9:32 PM, Antony Polukhin <anto...@gmail.com> wrote:
>
> I'd like to discuss a proposed changes. Thing that I'm worried about:
> * replacing class `stacktrace` with vector<void*> will definitely
> break the most common use case std::cout << get_stacktrace(); as it's
> not a good idea to overload ostream operator for vector of void
> pointers

No, I wasn't suggesting to replace `stacktrace` with `vector<void*>`.
I was suggesting it as a possible implementation of `stacktrace` (i.e.
an internal data member of the `stacktrace` class). And BTW, that
should probably be `vector<frame>` to support proper container
semantics in `stacktrace`.

I've given it some more thought after I wrote the review, and it
occurred to me that the main reason for the current implementation of
`basic_stacktrace` as an array is probably the intention to allow its
use in signal handlers. For some reason it didn't occur to me while I
was writing the review. That is a fair goal, although I'm having a
hard time thinking of what can be done with a stacktrace from a signal
handler. I guess, the only thing that can be done is dumping it into a
file, but the library does not provide such a facility (the
`operator<<` is not suitable for this).

Of course, `vector` is not an option in a signal handler, but the
runtime-sized `basic_stacktrace` can still be used in this case. The
idea is to offer a way to provide an external storage for the
`basic_stacktrace` object, which would be an array of `frame`s in this
case. It could look something like this:

void my_signal_handler(int sig)
{
boost::stacktrace::frame frames[10];
boost::stacktrace::stacktrace bt{ frames, 10 }; // bt.size() == 0
&& bt.capacity() == 10
boost::stacktrace::fill_stacktrace(bt); // makes bt.size() <= 10
}

Internally, `stacktrace` should be smart enough to handle three cases
wrt. the buffer of frames:

1. The `stacktrace` object refers to an external buffer. It should not
deallocate the frames.
2. The `stacktrace` object owns a small amount of frames stored in the
internal array (e.g. up to 8 frames). The frames should be destroyed
with the `stacktrace` object.
3. The `stacktrace` object owns a large amount of frames, allocated
dynamically on heap. The frames should be destroyed and deallocated
with the `stacktrace` object.

Copying and assignment should also work with these three states.

> * addr2line and forking is not nice. Is linking with a GPL licensed
> code an acceptable alternative?

I think it would be difficult to get it accepted, but probably
possible if it's a separate header, on which nothing from the rest of
the library depends. Of course, the most preferable solution is to
have the whole library licensed under BSL.

Maybe there is a different implementation of the functionality
provided by addr2line? Niall mentioned a tool from LLVM, maybe its
code can be reused (and I'm assuming it's licensed under BSD).

> * removing `basic_stacktrace` will definitely remove all the
> possibilities for optimization of caching COM/parsed DWARF. This is a
> quick win that later may turn into big troubles.

Again, I wasn't suggesting to remove `stacktrace` - on the contrary,
the stacktrace should be represented with a distinct type. What I'm
having the problem with is what semantics you put into that type.

[Andrey Semashev]

Another alternative is to add an allocator template argument to
`basic_stacktrace` and use a stack-based allocator in signal handlers.

[Thomas Trummer]

> On 15 Dec 2016, at 08:08, Antony Polukhin <anto...@gmail.com> wrote:
>
> It's C++98. Tested on latest clang(c++03, c++11), gcc(c++98, c++11,
> c++1y), msvc.

Just to make sure I’m not doing anything stupid: macOS is currently not supported (it fails to compile)?

Thomas

[Niall Douglas]

On 18 Dec 2016 at 0:01, Andrey Semashev wrote:

> I've given it some more thought after I wrote the review, and it
> occurred to me that the main reason for the current implementation of
> `basic_stacktrace` as an array is probably the intention to allow its
> use in signal handlers. For some reason it didn't occur to me while I
> was writing the review. That is a fair goal, although I'm having a
> hard time thinking of what can be done with a stacktrace from a signal
> handler. I guess, the only thing that can be done is dumping it into a
> file, but the library does not provide such a facility (the
> `operator<<` is not suitable for this).

In the past I've parsed the backtrace to figure out what call
sequence led to the fault causing the signal handler and taken action
based on that. In case you're interested, it was a capability based
security model where the stacktrace signature was used to cache what
capabilities the calling code had. On first time seeing a stacktrace
signature, you needed to dig down the stacktrace and figure out what
perms it had based on what had called what. You might think thread
local data would be a lot easier, but a thread can overwrite its data
to fake its security context. Faking a stack backtrace is
considerably harder.

There are other applications for parsing a backtrace in a signal
handler too. They are fairly niche I suppose, but I've used the
technique a few times in my career and more than I've ever used
virtual inheritance, so it's not a never used technique.

> > * addr2line and forking is not nice. Is linking with a GPL licensed
> > code an acceptable alternative?
>
> I think it would be difficult to get it accepted, but probably
> possible if it's a separate header, on which nothing from the rest of
> the library depends. Of course, the most preferable solution is to
> have the whole library licensed under BSL.
>
> Maybe there is a different implementation of the functionality
> provided by addr2line? Niall mentioned a tool from LLVM, maybe its
> code can be reused (and I'm assuming it's licensed under BSD).

Unfortunately the LLVM library is extensive and extracting just the
code you need to do DWARF table lookups is non-trivial. addr2line
uses a LGPL libdwarf library, again non trivial to extract just what
you need which is a tiny subset of a DWARF utility library.

Many years ago to work around async safety and back when DbgHelp.dll
had two, totally incompatible versions and each user's system could
have either, I found a superb COFF and DWARF parsing library written
by some Russian guy which was malloc-free and async-safe and made
looking up a source file and line number really easy. I tried finding
it again for Antony last month, but after 20 minutes I gave up. I
last used that library maybe a decade ago.

It actually wouldn't be too much work to write your own DWARF parser
to extract source file and line number. The DWARF spec is well
documented, and it's basically just five levels of searching tables
and jumping down to the next layer if memory serves. One could write
such a parser without too much work, but most of the effort would
actually go on debugging it. After all, it would need to cope with
stripped symbols, non-Intel symbols, big endian vs little endian,
weird symbols generated by bugs in a particular compiler version and
so on.

https://github.com/aclements/libelfin could chop some time off
development, or you could just go ahead and embed a copy as a git
subrepo as it's MIT licence.

Niall

--
ned Productions Limited Consulting
http://www.nedproductions.biz/
http://ie.linkedin.com/in/nialldouglas/

[Niall Douglas]

On 17 Dec 2016 at 21:43, Antony Polukhin wrote:

> >> > 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.
> >
> > As I mentioned in a previous post, you want to use noinline to force
> > a guaranteed first entry to strip. As it's a header only library,
> > you'll get this perversity:
> >
> > __declspec(noinline) inline void make_backtrace(...)
> >
> > ... which is completely legal because you want inline linkage
> > semantics, but to never inline the function.
>
> Oh, now I've got that. I've been confused by the `__declspec(noinline)
> inline`. Thanks again!
> Does the `BOOST_NOINLINE inline` solution work well on GCC, CLANG and
> other compilers?

For the major compilers targeting their native ecosystem I've found
they work as expected. Though the markup is perverse, and you might
want to add an explanatory comment :).

Niall

--
ned Productions Limited Consulting
http://www.nedproductions.biz/
http://ie.linkedin.com/in/nialldouglas/

[Antony Polukhin]

2016-12-18 12:42 GMT+03:00 Thomas Trummer <th.tr...@gmail.com>:
>
>> On 15 Dec 2016, at 08:08, Antony Polukhin <anto...@gmail.com> wrote:
>>
>> It's C++98. Tested on latest clang(c++03, c++11), gcc(c++98, c++11,
>> c++1y), msvc.
>
> Just to make sure I’m not doing anything stupid: macOS is currently not supported (it fails to compile)?

I have not tried it on MacOS. if you have an access to the platform
and some free time, I'd appreciate help with debugging.

--
Best regards,
Antony Polukhin

_______________________________________________

[Peter Dimov]

Antony Polukhin wrote:
> I have not tried it on MacOS. if you have an access to the platform and
> some free time, I'd appreciate help with debugging.

According to

http://www.nullptr.me/2013/04/14/generating-stack-trace-on-os-x/

Mac OS has ::backtrace:

https://developer.apple.com/legacy/library/documentation/Darwin/Reference/ManPages/man3/backtrace.3.html

It doesn't have addr2line, but it has atos, as suggested in

https://spin.atomicobject.com/2013/01/13/exceptions-stack-traces-c/

[Andrey Semashev]

On 12/18/16 16:56, Niall Douglas wrote:
> On 18 Dec 2016 at 0:01, Andrey Semashev wrote:
>
>>> * addr2line and forking is not nice. Is linking with a GPL licensed
>>> code an acceptable alternative?
>>
>> I think it would be difficult to get it accepted, but probably
>> possible if it's a separate header, on which nothing from the rest of
>> the library depends. Of course, the most preferable solution is to
>> have the whole library licensed under BSL.
>>
>> Maybe there is a different implementation of the functionality
>> provided by addr2line? Niall mentioned a tool from LLVM, maybe its
>> code can be reused (and I'm assuming it's licensed under BSD).
>
> Unfortunately the LLVM library is extensive and extracting just the
> code you need to do DWARF table lookups is non-trivial. addr2line
> uses a LGPL libdwarf library, again non trivial to extract just what
> you need which is a tiny subset of a DWARF utility library.

LGPL is actually better than GPL. If I'm not mistaken, it is possible to
make a BSL-licensed backend of Boost.Stacktrace that uses the
LGPL-licensed libdwarf.

> Many years ago to work around async safety and back when DbgHelp.dll
> had two, totally incompatible versions and each user's system could
> have either, I found a superb COFF and DWARF parsing library written
> by some Russian guy which was malloc-free and async-safe and made
> looking up a source file and line number really easy. I tried finding
> it again for Antony last month, but after 20 minutes I gave up. I
> last used that library maybe a decade ago.
>
> It actually wouldn't be too much work to write your own DWARF parser
> to extract source file and line number. The DWARF spec is well
> documented, and it's basically just five levels of searching tables
> and jumping down to the next layer if memory serves. One could write
> such a parser without too much work, but most of the effort would
> actually go on debugging it. After all, it would need to cope with
> stripped symbols, non-Intel symbols, big endian vs little endian,
> weird symbols generated by bugs in a particular compiler version and
> so on.
>
> https://github.com/aclements/libelfin could chop some time off
> development, or you could just go ahead and embed a copy as a git
> subrepo as it's MIT licence.

If Boost.Stacktrace has its own implementation, that's even better. I
guess, it's up to Antony to decide if he has time to implement this.

Thanks, Niall.

[Andrey Semashev]

On 12/18/16 16:56, Niall Douglas wrote:

> On 17 Dec 2016 at 21:43, Antony Polukhin wrote:
>

>>> __declspec(noinline) inline void make_backtrace(...)

>>
>> Oh, now I've got that. I've been confused by the `__declspec(noinline)
>> inline`. Thanks again!
>> Does the `BOOST_NOINLINE inline` solution work well on GCC, CLANG and
>> other compilers?
>
> For the major compilers targeting their native ecosystem I've found
> they work as expected. Though the markup is perverse, and you might
> want to add an explanatory comment :).

If I'm not misremembering, Intel compiler issues a warning in this case.

[Niall Douglas]

The formal review of the Stacktrace library ends soon. Thanks to all
those who contributed to the interesting discussion, however formal
reviews have only been made by:

1. Vladimir Batov (YES)
2. Klemens Morgenstern (YES)
3. Andrey Semashev (NO)
4. Nat Goodspeed (YES)

(If I missed a review, please say)

Could anyone who hasn't made a review yet please make one soon? We
really need to get the number of reviews up above five as a minimum.
And for a single purpose well defined library such as Stacktrace
eight reviews is hardly a big ask.

We also never resolved how much async safety Stacktrace's parsing
routines ought to have. I'll be happy to keep accepting reviews up to
the New Year, so if you get some spare time over the Christmas break,
write a Stacktrace review!!!

Thanks,
Niall