[Boost-users] [Stacktrace] Review Manager report

Subject: [Boost-users] [Stacktrace] Review Manager report
From: Niall Douglas (s_sourceforge_at_[hidden])
Date: 2017-01-02 11:14:06

Firstly happy new year to everyone, and many thanks to all those who
participated in the peer review of proposed Boost.Stacktrace before
Christmas. I received the following formal reviews:

1. Vladimir Batov (YES)
2. Klemens Morgenstern (YES)
3. Andrey Semashev (NO)
4. Nat Goodspeed (YES)
5. Artyom Beilis (NO)
6. Barrett Adair (YES)

Extensive comments without a formal acceptance recommendation were also
made by:

1. Peter Dimov
2. Edward Diener

There was a very noticeable lack of consensus on the strengths and
weaknesses of the proposed library which has made summarising all these
reviews into a coherent report with concrete conditions for acceptance
very difficult. Antony also responded to many of the concerns throughout
the review, agreeing with some points, disagreeing with others.

I think this lack of consensus stemmed from multiple opinions on what a
stacktrace library ought to provide:

Minimum viable product: A portable backtrace() and backtrace_symbols()
implementation with a C++ STL idiomatic API. The stack backtrace capture
implementation must not allocate memory and be "fast". Note that
backtrace_symbols() does NOT symbolise into strings containing source
file name and line number. It also allocates memory
(backtrace_symbols_fd() does not).

Opinion 1: Portable serialisation of the stacktrace with instruction
pointers converted to offsets into SO/DLL binary paths so a later
external process can use those in combination with their debug symbols
to retrieve source file names and line numbers. Ideally the
serialisation formats supported would include those consumable by well
known tooling for this purpose e.g. addr2line or llvm-symbolizer.

Opinion 2: That Stacktrace can in process use debug symbols to retrieve
source file names and line numbers. This could be implemented by
invoking addr2line or equivalent as a child process which has the
enormous advantage of being async safe, but has disadvantages in almost
every other area. Or it could be implemented by one's own DWARF parser
but with the huge caveat that memory could be badly corrupted by now.

Opinion 3: That Stacktrace be "async safe" in that backtraces can be
safely captured and parsed from a signal handler or SEH/TEH handler
callback or point of C++ exception throw. *This implies no memory
allocation during the execution of the handler*.

>From the reviews, I think there is consensus that the minimum viable
product is achieved in Stacktrace though many mentioned that
improvements are needed to the documentation and most of the negative
feedback was on features not in the minimum viable product described
above. I therefore recommend the ACCEPTANCE of proposed Boost.Stacktrace
into Boost with the following conditions:

1. That the documentation more clearly set out what Stacktrace does and
especially does not do, and that the default facilities provided are
those of the Minimum Viable Product described above i.e. no source file
nor line number discovery, no external dependencies on libraries not
always provided on the target platform, no invocation of child
processes, no async safety during stacktrace parsing. Stacktrace ought
to be pared back to the minimum viable product featureset with no
external dependencies at all.

2. That suitable predefined macros opt in to additional functionality
such as serialisation of offset translated backtraces, retrieval of
source code path and line number etc. Enabling these may introduce
dependencies on third party libraries not always present on a system
such as libunwind, or on third party helper utilities such as addr2line.
I would recommend that the APIs the use of which are dependent on such
third party items are entirely compiled out of Stacktrace when the macro
enabling them is not defined.

3. That the documentation describe ONLY the default minimum viable
product functionality in its tutorial and quick start. All extra
functionality enabled by macros is to be relocated into a separate
documentation section away from the main documentation e.g.
"Advanced/optional features".

4. That APIs made available only when a given macro has turned them on
are documented as such in the API reference docs.

I'd like to congratulate Antony on getting yet another library of his
into Boost. I hope some of his magic rubs off on me when I bring Outcome
to review later this year!

Regards,
Niall

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