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
#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
