<div dir="ltr"><div><span style="font-size:13.1999998092651px">On Thu, Mar 5, 2015 at 2:18 PM <</span><a href="mailto:jingham@apple.com" target="_blank" style="font-size:13.1999998092651px">jingham@apple.com</a><span style="font-size:13.1999998092651px">> wrote:</span><br></div><div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
> On Mar 5, 2015, at 11:59 AM, Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br>
><br>
> Right, but I was just talking about that specific instance of replacing Host::Backtrace() with llvm::sys::PrintStackTrace().  There is not a good reason for that specific instance of Host::Backtrace to remain as-is instead of being replaced with llvm::sys::PrintStackTrace.  Even independently of whether we delete Host::Backtrace entirely in favor of llvm::sys::PrintStacktrace across the rest of the codebase.  There's nothing particualrly useful or special about having a StreamString at that call-site.<br>
><br>
<br>
I agree with Enrico here, we should be consistent in our usage.  So if we're going to use the llvm facility we should do it everywhere, otherwise people new to the code have to guess why we did different things in different places.<br></blockquote><div>I guess what frustrates me is that why isn't this the first response?  Instead, the first response is "no we don't need this, we already have it".  That's not a sufficient answer.  Not everyone has it.  And even if everyone does have it, code health and technical debt are important considerations that I feel too often take a backseat to "eh well, it works for us".  For example...</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> As for the larger issue of whether to replace Host::Backtrace across the codebase, llvm is open source and has an easy to work with community.  I'm always surprised when I see things like "the right method signature isn't available, let's go re-implement hundreds of lines of code on 4 different platforms".  All we need to do is add the method with the right signature.  I've already done that and it's been committed. It would be great for the community if people were more open to and comfortable with working in both codebases.<br>
<br>
That's not an accurate characterization of what actually happened.  More likely it was, "I need a backtrace and and I can write the 10 lines of code for Darwin (and probably most Linux'es) in much less time that it would take to go figure out whether llvm has such a thing."<br></blockquote><div>I would argue that no matter how much time it takes to figure out whether LLVM has such a thing, the time is worth it.  For example, does the Darwin and Linux backtrace implementation print filenames and line numbers, and can it backtrace through inlined functions?  I'm pretty sure the answer to that is no, because it doesn't use llvm-symbolizer.  The LLVM implementation does though.  Maybe it didn't at the time, but that's the whole point of using library code.  Someone can make improvements to it and you get them for free.  </div><div><br></div><div>Even if it takes 10 minutes to write the implementation on Darwin and a day to figure out whether it exists in LLVM, it's still worth it.  Even if it doesn't exist at all in LLVM, it's <b>still</b> worth it to ask whether it should, and implement it in LLVM if the answer is yes.  There is no good reason to implement a different flavor of the same abstraction in LLDB if LLVM already has it.  And "because it took too much time to check" <b>definitely</b> isn't a good reason.</div><div><br></div><div>Keep in mind I'm not arguing this for myself just because Windows is the odd one out and what works on Darwin is often likely to work on other OSes except Windows.  Turns out I had to implement this on Windows anyway even in LLVM.  So the amount of work for me here was the same.  I'm arguing it because it's the right thing to do.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
><br>
> It's true that we can't add an signature that takes a StreamString&, but that's the cost of adding unnecessary abstractions (for reference: llvm::raw_ostream supports printf style formatting, and everything else that StreamString supports.  I'm still at a loss for why we don't use it, but that's a discussion for another day).  Because we're stuck in this boat though doesn't mean we should continue adding unnecessary technical debt.<br>
<br>
As for Stream->llvm::raw_stream, regardless of the relative merits ab initio, converting all uses of StringStream over to some llvm functionality seems like lots of code churn for no benefit at this point.  Neither is hard to learn so it's not like this is actually going to save lldb developers any appreciable time.  This just seems like make work.<br></blockquote><div>Yes, at this point it might be churn.  But that doesn't mean we can't deprecate it and move to llvm streams.  I've seen a number of points made about why StreamString is preferred, surrounding things like IO manipulators that modify the state of the stream (like std::hex) and the fact that StreamString has a Printf method.  Turns out those aren't even actual problems, because LLVM streams don't have manipulators like std::hex and you can use printf formatting with LLVM streams with stream << format(...).  So while it might indeed be churn to just go and replace everything with llvm streams, that's not an excuse to continue making the problem worse (in my opinion).  But still, this is getting sidetracked and not really related to the issue, it was just an example of the cost of adding unnecessary abstractions instead of re-using what we already have.  </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
><br>
> By making this simple replacement all stack traces will now have file and line number information, and be able to unwind through inlined functions.<br>
<br>
If there's a version of the llvm functionality that returns a string, then by all means we should use it either for host backtrace or call it and then stuff the string into the log directly.  File * won't do for this, so it needs to be a string.  But again, having one place, the assert, that has lovely backtraces, and others - that come through logging or error reporting in commands, that doesn't, does not seem like a great change.<br></blockquote><div>The version I committed to LLVM returns a raw_ostream&.  This is just as good as a string except it adds 1 extra line of code on the LLDB side.  I have a patch locally which deletes Host::Backtrace and replaces all calls with this function.  I plan to commit it later today or tomorrow.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
><br>
> We should always be using llvm library functionality where it's available. And as Chandler said, not because anyone thinks the LLVM side is perfect, but  because we are all part of the same project, and we need to benefit from each other as well as share each others' costs.<br>
><br>
<br>
BTW, I looked on the llvm web site, but I couldn't find the docs specific to llvm's support libraries.  Can you point me at them so I can keep that info bookmarked?<br></blockquote><div>Documentation is organized by Directory/Filename and by Namespace.</div><div>Support stuff is split at the directory level between <a href="http://llvm.org/docs/doxygen/html/dir_b8a34fdf3d7436adb6938f4347ba32a4.html">llvm/Support</a> and <a href="http://llvm.org/docs/doxygen/html/dir_a7dd73f244ee1af3dca2a8723843bc79.html">llvm/ADT</a>, and at the namespace level is either in <a href="http://llvm.org/docs/doxygen/html/namespacellvm.html">llvm</a> or <a href="http://llvm.org/docs/doxygen/html/namespacellvm_1_1sys.html">llvm::sys</a></div></div></div></div>