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.

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

Reviews should be submitted to the developer list (boost@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_ststd::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 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_castboost::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.

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

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 On 14 Dec 2016 at 8:43, Niall Douglas wrote:

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 (boost@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

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