On Sat, Dec 17, 2016 at 9:32 PM, Antony Polukhin <antoshkka@gmail.com> wrote:
I'd like to discuss a proposed changes. Thing that I'm worried about: * replacing class `stacktrace` with vector<void*> will definitely break the most common use case std::cout << get_stacktrace(); as it's not a good idea to overload ostream operator for vector of void pointers
No, I wasn't suggesting to replace `stacktrace` with `vector<void*>`. I was suggesting it as a possible implementation of `stacktrace` (i.e. an internal data member of the `stacktrace` class). And BTW, that should probably be `vector<frame>` to support proper container semantics in `stacktrace`. I've given it some more thought after I wrote the review, and it occurred to me that the main reason for the current implementation of `basic_stacktrace` as an array is probably the intention to allow its use in signal handlers. For some reason it didn't occur to me while I was writing the review. That is a fair goal, although I'm having a hard time thinking of what can be done with a stacktrace from a signal handler. I guess, the only thing that can be done is dumping it into a file, but the library does not provide such a facility (the `operator<<` is not suitable for this). Of course, `vector` is not an option in a signal handler, but the runtime-sized `basic_stacktrace` can still be used in this case. The idea is to offer a way to provide an external storage for the `basic_stacktrace` object, which would be an array of `frame`s in this case. It could look something like this: void my_signal_handler(int sig) { boost::stacktrace::frame frames[10]; boost::stacktrace::stacktrace bt{ frames, 10 }; // bt.size() == 0 && bt.capacity() == 10 boost::stacktrace::fill_stacktrace(bt); // makes bt.size() <= 10 } Internally, `stacktrace` should be smart enough to handle three cases wrt. the buffer of frames: 1. The `stacktrace` object refers to an external buffer. It should not deallocate the frames. 2. The `stacktrace` object owns a small amount of frames stored in the internal array (e.g. up to 8 frames). The frames should be destroyed with the `stacktrace` object. 3. The `stacktrace` object owns a large amount of frames, allocated dynamically on heap. The frames should be destroyed and deallocated with the `stacktrace` object. Copying and assignment should also work with these three states.
* addr2line and forking is not nice. Is linking with a GPL licensed code an acceptable alternative?
I think it would be difficult to get it accepted, but probably possible if it's a separate header, on which nothing from the rest of the library depends. Of course, the most preferable solution is to have the whole library licensed under BSL. Maybe there is a different implementation of the functionality provided by addr2line? Niall mentioned a tool from LLVM, maybe its code can be reused (and I'm assuming it's licensed under BSD).
* removing `basic_stacktrace` will definitely remove all the possibilities for optimization of caching COM/parsed DWARF. This is a quick win that later may turn into big troubles.
Again, I wasn't suggesting to remove `stacktrace` - on the contrary, the stacktrace should be represented with a distinct type. What I'm having the problem with is what semantics you put into that type.