[Lldb-commits] [PATCH] Change lldb_assert() to use llvm::sys::PrintBacktrace

[Enrico Granata]

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 <zturner at google.com> wrote:
> 
> 
> 
> On Wed, Mar 4, 2015 at 5:48 PM Enrico Granata <egranata at apple.com> wrote:
>>> On Mar 4, 2015, at 5:36 PM, Zachary Turner <zturner at google.com> 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*.
>  
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20150304/d9755d54/attachment.html>