[Lldb-commits] [lldb] r231310 - Introduce lldbassert(x)

[Zachary Turner]

On Thu, Mar 5, 2015 at 2:18 PM <jingham at apple.com> wrote:

>
> > On Mar 5, 2015, at 11:59 AM, Zachary Turner <zturner at google.com> wrote:
> >
> > 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.
> >
>
> 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.
>
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...


>
> > 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.
>
> 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."
>
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.

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 *still* 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"
*definitely* isn't a good reason.

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.


>
> >
> > 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.
>
> 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.
>
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.


>
> >
> > By making this simple replacement all stack traces will now have file
> and line number information, and be able to unwind through inlined
> functions.
>
> 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.
>
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.


>
> >
> > 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.
> >
>
> 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?
>
Documentation is organized by Directory/Filename and by Namespace.
Support stuff is split at the directory level between llvm/Support
<http://llvm.org/docs/doxygen/html/dir_b8a34fdf3d7436adb6938f4347ba32a4.html>
and llvm/ADT
<http://llvm.org/docs/doxygen/html/dir_a7dd73f244ee1af3dca2a8723843bc79.html>,
and at the namespace level is either in llvm
<http://llvm.org/docs/doxygen/html/namespacellvm.html> or llvm::sys
<http://llvm.org/docs/doxygen/html/namespacellvm_1_1sys.html>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20150305/ce5dd1e2/attachment.html>