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