Hello, There is my review. Note: I reviewed the library in previous round and voted no. So the current review was mostly review of the problems I found. - What is your evaluation of the design? The design is solid and minimalist as it suppose to be for such a library. Good separation for different backends. I still lack the out of the box "throw exception with stacktrace"/catch and print trace as I written in previous review but I don't think it is show stopper. i.e. "throw_with_trace" as shown in examples should be part of the library as it is one of most useful features. - What is your evaluation of the implementation? The implementation is OK but there are several issues (mostly in backend) I need to point out: I used this trivial program for run time testing #include <boost/stacktrace.hpp> #include <iostream> void foo() { std::cout << boost::stacktrace::stacktrace() << std::endl; } void bar() { foo(); } int main() { bar(); } Addr2Line Backend: --------------------------- In order to get the stacktrace the addr2line is called for each frame - i.e. stack of 4 functions results in 4(!!!) forks need to be fixed This is sample of stace output for trivial example, it can be reduced to 2 fork/exec at most [pid 7985] execve("/usr/bin/addr2line", ["addr2line", "-Cpe", "./a.out", "0x000000000040AFC2"], [/* 75 vars */]) = 0 [pid 7986] execve("/usr/bin/addr2line", ["addr2line", "-Cpe", "./a.out", "0x000000000040B02D"], [/* 75 vars */]) = 0 [pid 7987] execve("/usr/bin/addr2line", ["addr2line", "-Cpe", "./a.out", "0x000000000040B039"], [/* 75 vars */]) = 0 [pid 7988] execve("/usr/bin/addr2line", ["addr2line", "-Cpe", "/lib/x86_64-linux-gnu/libc.so.6", "0x00007EFD582A6830"], [/* 75 vars */]) = 0 [pid 7989] execve("/usr/bin/addr2line", ["addr2line", "-Cpe", "./a.out", "0x000000000040AEB9"], [/* 75 vars */]) = 0 This is something easy to fix and I expect it to be fixed before library is delivered to the official release. Libbacktrace Backend: ------------------------------ Linux: using libbacktrace without debug information results core dump - seems, the dump happens in ::backtrace_pcinfo happens for both g++ 4.8 and 5.4 Please make sure you can workaround this problem. This MUST be fixed. Windows/MinGW I build libbacktrace for mingw (tested it to make sure I can get traces using its API) However when I used boost.stacktrace with libbacktrace the program crashed every time in any configuration I used (with or without debug information) it worked with default build (without) but its usability is limited. This MUST be fixed (or if impossible mark as libbacktrace unusable for MINGW32) OS Support FreeBSD ------------------------------- FreeBSD - 100% unusable at this point - also I must say for FreeBSD it may be tricky due to : I needed to do some changes in CppCMS's backtrace library recently due to shift of latest freebsd to clang Take a look: https://github.com/artyom-beilis/cppcms/commit/18c3f05bfa9e4008aeac39322e8a0... https://github.com/artyom-beilis/cppcms/blob/master/booster/lib/backtrace/sr... It also may say that you WILL have problems with Mac OS X. $ c++ -v FreeBSD clang version 3.8.0 (tags/RELEASE_380/final 262564) (based on LLVM 3.8.0) Target: x86_64-unknown-freebsd11.0 Thread model: posix InstalledDir: /usr/bin $ uname -a FreeBSD freebsd11 11.0-RELEASE-p1 FreeBSD 11.0-RELEASE-p1 #0 r306420: Thu Sep 29 01:43:23 UTC 2016 root@releng2.nyi.freebsd.org:/usr/obj/usr/src/sys/GENERIC amd64 $ c++ -I ../boost_stacktrace/include/ -I ../boost_1_63_0/ -Wall -O0 -g st.cpp In file included from st.cpp:1: In file included from ../boost_stacktrace/include/boost/stacktrace.hpp:15: In file included from ../boost_stacktrace/include/boost/stacktrace/frame.hpp:205: In file included from ../boost_stacktrace/include/boost/stacktrace/detail/frame_unwind.ipp:36: ../boost_stacktrace/include/boost/stacktrace/detail/safe_dump_posix.ipp:36:63: error: use of undeclared identifier 'S_IFREG' const int fd = ::open(file, O_CREAT | O_WRONLY | O_TRUNC, S_IFREG | S_IWUSR | S_IRUSR); ^ ../boost_stacktrace/include/boost/stacktrace/detail/safe_dump_posix.ipp:36:73: error: use of undeclared identifier 'S_IWUSR' const int fd = ::open(file, O_CREAT | O_WRONLY | O_TRUNC, S_IFREG | S_IWUSR | S_IRUSR); ^ ../boost_stacktrace/include/boost/stacktrace/detail/safe_dump_posix.ipp:36:83: error: use of undeclared identifier 'S_IRUSR' const int fd = ::open(file, O_CREAT | O_WRONLY | O_TRUNC, S_IFREG | S_IWUSR | S_IRUSR); ^ In file included from st.cpp:1: In file included from ../boost_stacktrace/include/boost/stacktrace.hpp:15: In file included from ../boost_stacktrace/include/boost/stacktrace/frame.hpp:205: ../boost_stacktrace/include/boost/stacktrace/detail/frame_unwind.ipp:75:7: error: no member named '_Unwind_Backtrace' in the global namespace ::_Unwind_Backtrace(&boost::stacktrace::detail::unwind_callback, &state); ~~^ st.cpp:3:14: warning: treating Unicode character as whitespace [-Wunicode-whitespace] void foo() { std::cout << boost::stacktrace::stacktrace() << std::endl; } ^ st.cpp:3:17: warning: treating Unicode character as whitespace [-Wunicode-whitespace] void foo() { std::cout << boost::stacktrace::stacktrace() << std::endl; } ^ st.cpp:4:14: warning: treating Unicode character as whitespace [-Wunicode-whitespace] void bar() { foo(); } ^ 3 warnings and 4 errors generated. I have another suggestion regarding testing. I didn't run them but since you didn't catch crash with libbacktrace without debug mode you probably have all compiled with debug information I suggest to modify the set of tests that you run on entire matrix: {backend}x{debug mode on/off/external PDB/debug information}x{rdynamic on/off} - What is your evaluation of the documentation? Documentation is fairly good - What is your evaluation of the potential usefulness of the library? VERY useful one the bugs are fixed. - Did you try to use the library? With what compiler? Yes: I tested on Linux gcc 5.4/4.8/clang++ Windows MinGW 5.x; FreeBSD clang 3.8

Did you have any problems?

Lots - see notes above. - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? Several hours running code reading backeds code. - Are you knowledgeable about the problem domain? Yes I had written one for CppCMS that runs on many platforms. And finally, every review should attempt to answer this question: - Do you think the library should be accepted as a Boost library? Yes, Subject to: - Fix of build/run issues on all major platforms including MINGW and FreeBSD - it was too easy to find issues I really concerns me - it is show stopper for the library to be released as part of boost. - FIx crashes with libbacktrace - Fix of addr2line performance - no need to fork/exec for each frame Note: I understand that it is hard for the author to fix all stuff on all platforms - but stacktrace by definition is doing something non portable in portable way. Best Regards, Artyom Beilis