Then I think your proposal should change from adjusting this one spot in the code on an ad-hoc basis to actually removing Host::Backtrace() in favor of the LLVM facility.
I will let others weigh in on that change. Sent from my iPhone > On Mar 4, 2015, at 6:01 PM, Zachary Turner <[email protected]> wrote: > > > > On Wed, Mar 4, 2015 at 5:48 PM Enrico Granata <[email protected]> wrote: >>> On Mar 4, 2015, at 5:36 PM, Zachary Turner <[email protected]> wrote: >>> >>> We don't have the wheel though. For example, it's not implemented on >>> Android and it's not implemented on Windows. I could duplicate a bunch of >>> code from LLVM, but why? The code is already there in LLVM. And what >>> about the Android people? There's currently about 3 completely different >>> implementations of backtracing (MacOSX, FreeBSD, Linux). All of them print >>> backtraces in a different format. We can call 1 function and have >>> backtraces in the same format for everyone, including Windows and Android, >>> right now. This seems like kind of a straightforward case of code reuse to >>> me, and a clear win, so I'm not sure why it's that contentious. >> >> Two things worry me about this: >> >> 1) we have a sanctioned LLDB facility that retrieves a host backtrace, but >> we’re saying that it does not work on all platforms, and instead of making >> it work on all platforms, we’re going to use another API, but have no plan >> to either improve or remove the existing LLDB API for this >> >> Given that we have an LLDB API to do this task, we should use it. If it is >> not an API that we can reasonably support on all platforms we care about, >> then we should remove it in favor of one that we can support on all >> platforms we care about. But we should not be ad-hoc choosing to use one or >> another way to do this task > I'm more than happy to remove Host::Backtrace in favor of > llvm::sys::PrintBacktrace(). It is called from 3 locations in the codebase, > and I believe I can replace all 3 of them by adding a simple overload to llvm > that takes an std::string by reference. > > Having a sanctioned API doesn't mean we can't improve on things, or move to a > different sanctioned API. This API is not exposed through the public > interface, so we have no guarantee or requirement to maintain compatibility. > And there is a better one in LLVM. > > "Sanctioned" shouldn't mean "now and forever no matter what new developments > arise". It should mean "this is what we use because it happens to be the > best thing, but if something better comes along, then by all means, go for it" > >> >> 2) the LLDB API interoperates nicely with our Streams, whereas the LLVM one >> only supports FILE*. In general, Streams are a much nicer interface to work >> with than FILE* are. > LLVM is open source too, why can't we modify it? I modify it all the time, > I'm quite confident I could push a change to LLVM that adds > PrintStackTrace(std::string &output). I believe that should address the > concerns surrounding FILE*. > >
_______________________________________________ lldb-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
