Subject: Re: [boost] Stacktrace library starts review today 14th Dec
From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2016-12-17 16:01:15
On Sat, Dec 17, 2016 at 9:32 PM, Antony Polukhin <antoshkka_at_[hidden]> 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.