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

Subject: Re: [boost] Stacktrace library starts review today 14th Dec
From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2016-12-17 12:51:46

On Wed, Dec 14, 2016 at 11:43 AM, Niall Douglas
<s_sourceforge_at_[hidden]> 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.