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

[Enrico Granata]

> 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

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.

In this case we could get away with the lesser interface (FILE*), that’s true. However, if - say - we ended up deciding that the output of lldbassert() should go to the user’s terminal AND to a log file, this would be trivial to do with a StreamTee, but it would require more fiddling with a FILE*

> If we need to interoperate with StreamString, then we should implement an overload **in LLVM** that returns an std::string.
> 
> On Wed, Mar 4, 2015 at 5:30 PM Enrico Granata <egranata at apple.com <mailto:egranata at apple.com>> wrote:
>> On Mar 4, 2015, at 5:18 PM, Zachary Turner <zturner at google.com <mailto:zturner at google.com>> wrote:
>> 
>> Hmm, I'm not sure I agree.  Whether it prints to a Stream or directly to stderr is kind of an implementation detail.  Not very important since it just ends up to stdout or stderr anwyay and we don't do anything else with the backtrace except print it and throw it away.  
>> 
>> llvm already has functionality built in to serve exactly this purpose.  Why shouldn't we use it?  Not only are we sure that it's implemented on all platforms that LLVM supports,
> 
> If that is a concern, I posit that we should implement Host::Backtrace() on all platforms
> 
> The alternative of course would be to get rid of Host::Backtrace() entirely, and use the similar LLVM facility - but given how our own facility uses Streams instead of FILE*, I don’t think that is actually a good change
> 
>> but the format is consistent on all of these platforms, and anyway why reinvent the wheel?
> 
> Except in this case we already have the wheel
> 
> 
>> 
>> On Wed, Mar 4, 2015 at 5:15 PM Enrico Granata <granata.enrico at gmail.com <mailto:granata.enrico at gmail.com>> wrote:
>> ================
>> Comment at: source/Utility/LLDBAssert.cpp:14
>> @@ -13,1 +13,3 @@
>> +
>> +#include "llvm/Support/Signals.h"
>> 
>> ----------------
>> I would not do this.
>> Printing to a Stream is the LLDB way to do this, no reason for switching to this LLVM API
>> 
>> ================
>> Comment at: source/Utility/LLDBAssert.cpp:36
>> @@ -37,1 +35,3 @@
>> +        llvm::sys::PrintStackTrace(stderr);
>> +        fprintf(stderr, "please file a bug report against lldb reporting this failure log, and as many details as possible\n");
>>      }
>> ----------------
>> Printing to stderr is probably a good idea
>> But, again, I prefer to stick to the LLDB host layer
>> 
>> It's probably fine to reimplement Host::Backtrace() in terms of LLVM APIs if it can be done generally and with decent performance, but I don't see much in terms of added value in this change
>> 
>> http://reviews.llvm.org/D8069 <http://reviews.llvm.org/D8069>
>> 
>> EMAIL PREFERENCES
>>   http://reviews.llvm.org/settings/panel/emailpreferences/ <http://reviews.llvm.org/settings/panel/emailpreferences/>
>> 
>> 
> 
>> _______________________________________________
>> lldb-commits mailing list
>> lldb-commits at cs.uiuc.edu <mailto:lldb-commits at cs.uiuc.edu>
>> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits <http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits>
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20150304/37385636/attachment.html>