<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Mar 4, 2015, at 5:36 PM, Zachary Turner <<a href="mailto:zturner@google.com" class="">zturner@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class="">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.<br class=""><div class=""><br class=""></div></div></div></blockquote><div><br class=""></div><div>Two things worry me about this:</div><div><br class=""></div><div>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</div><div><br class=""></div><div>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</div><div><br class=""></div><div>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.</div><div><br class=""></div><div>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*</div><div><br class=""></div><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="">If we need to interoperate with StreamString, then we should implement an overload **in LLVM** that returns an std::string.</div></div><br class=""><div class="gmail_quote">On Wed, Mar 4, 2015 at 5:30 PM Enrico Granata <<a href="mailto:egranata@apple.com" class="">egranata@apple.com</a>> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><div class=""><blockquote type="cite" class=""><div class="">On Mar 4, 2015, at 5:18 PM, Zachary Turner <<a href="mailto:zturner@google.com" target="_blank" class="">zturner@google.com</a>> wrote:</div><br class=""><div class=""><div dir="ltr" class="">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. <br class=""><div class=""><br class=""></div><div class="">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,</div></div></div></blockquote><div class=""><br class=""></div></div></div><div style="word-wrap:break-word" class=""><div class=""><div class="">If that is a concern, I posit that we should implement Host::Backtrace() on all platforms</div><div class=""><br class=""></div><div class="">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</div></div></div><div style="word-wrap:break-word" class=""><div class=""><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""> but the format is consistent on all of these platforms, and anyway why reinvent the wheel?</div></div></div></blockquote><div class=""><br class=""></div></div></div><div style="word-wrap:break-word" class=""><div class=""><div class="">Except in this case we already have the wheel</div><br class=""><blockquote type="cite" class=""><div class=""></div></blockquote></div></div><div style="word-wrap:break-word" class=""><div class=""><blockquote type="cite" class=""><div class=""><br class=""><div class="gmail_quote">On Wed, Mar 4, 2015 at 5:15 PM Enrico Granata <<a href="mailto:granata.enrico@gmail.com" target="_blank" class="">granata.enrico@gmail.com</a>> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">================<br class="">
Comment at: source/Utility/LLDBAssert.cpp:<u class=""></u>14<br class="">
@@ -13,1 +13,3 @@<br class="">
+<br class="">
+#include "llvm/Support/Signals.h"<br class="">
<br class="">
----------------<br class="">
I would not do this.<br class="">
Printing to a Stream is the LLDB way to do this, no reason for switching to this LLVM API<br class="">
<br class="">
================<br class="">
Comment at: source/Utility/LLDBAssert.cpp:<u class=""></u>36<br class="">
@@ -37,1 +35,3 @@<br class="">
+ llvm::sys::PrintStackTrace(<u class=""></u>stderr);<br class="">
+ fprintf(stderr, "please file a bug report against lldb reporting this failure log, and as many details as possible\n");<br class="">
}<br class="">
----------------<br class="">
Printing to stderr is probably a good idea<br class="">
But, again, I prefer to stick to the LLDB host layer<br class="">
<br class="">
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<br class="">
<br class="">
<a href="http://reviews.llvm.org/D8069" target="_blank" class="">http://reviews.llvm.org/D8069</a><br class="">
<br class="">
EMAIL PREFERENCES<br class="">
<a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank" class="">http://reviews.llvm.org/<u class=""></u>settings/panel/<u class=""></u>emailpreferences/</a><br class="">
<br class="">
<br class="">
</blockquote></div></div></blockquote></div></div><div style="word-wrap:break-word" class=""><div class=""><blockquote type="cite" class=""><div class="">
_______________________________________________<br class="">lldb-commits mailing list<br class=""><a href="mailto:lldb-commits@cs.uiuc.edu" target="_blank" class="">lldb-commits@cs.uiuc.edu</a><br class=""><a href="http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits" target="_blank" class="">http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits</a><br class=""></div></blockquote></div><br class=""></div></blockquote></div>
</div></blockquote></div><br class=""></body></html>