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

Thu Mar 5 20:14:42 PST 2015

And FWIW, please don't take this as me pointing fingers, calling anyone
out, or criticizing previous decisions.  It's really not about that.  I get
that things end up the way they do for lots of different reasons.  But we
should be able to detach and evaluate things objectively.  And at least
from my point of view, I think that we -- as a community -- really need to
stop being so averse to understanding or making changes in LLVM.  I don't
see LLDB as a project which uses LLVM.  I see LLVM as a project which LLDB
is one component of.  All working towards the same goal.  From this angle,
it only makes sense that we should *never* invent an abstraction in LLDB if
the same abstraction could be useful to someone else who uses LLVM.  And we
should *never* invent an abstraction in LLDB when an existing LLVM
abstraction sucks or doesn't meet our needs.  We should fix it.  Other
people might think it sucks too.  So when I call it out in a CL or
something, it's not me being "difficult", it's me trying to make the
changes as useful as possible to as many people as possible.

When I started on this project 9 months ago, I knew what LLVM was only in
name.  I'd never touched it or even thought of working on it.  The point is
-- it's not that scary (the only thing that's scary is having to adjust my
editor settings every time I go edit LLVM code.  But...).  Even better,
most of us have the benefit of working in physical proximity to people who
are experts in all areas of LLVM.  So when someone is not sure if LLVM
supports something, there's someone nearby who can answer the question.
And if not, well there's #llvm and #lldb IRC.  And I'll even do the dirty
work.  I'll test peoples' patches on Windows.  I'll test my own patches on
all 3 platforms.  I spent 3 hours today trying to learn how to mess with
Xcode projects.  None of this stuff benefits me directly, but I think it's
*that* important.

On Thu, Mar 5, 2015 at 2:43 PM Zachary Turner <zturner at google.com> wrote:

> 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/20150306/7f45dadd/attachment.html>