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

[jingham at apple.com]

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

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

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

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

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

Jim


> 
> On Thu, Mar 5, 2015 at 11:37 AM <jingham at apple.com> wrote:
> There are other use cases (e.g. for adding a backtrace to log messages) where we want to put the backtrace out in ways lldb is used to outputting information - e.g. lldb::Stream's.  So I don't think it is true that "all we want to do is get a backtrace to the screen."
> 
> Jim
> 
> > On Mar 4, 2015, at 5:22 PM, Zachary Turner <zturner at google.com> wrote:
> >
> > As mentioned in the other thread, I think the StreamString thing is kind of irrelevant.  The goal here is to get the backtrace to the screen.  I would argue the reverse, that we should only be reinventing facilities in LLDB when there's a good reason to do so.  In this case, I don't think being able to use a StreamString is a compelling enough reason.  On the other hand, having a consistent backtrace format which supports all platforms is definitely a compelling reason to use the LLVM facility.  So I do think it makes sense here.
> >
> > On Wed, Mar 4, 2015 at 5:12 PM Enrico Granata <egranata at apple.com> wrote:
> >> On Mar 4, 2015, at 4:39 PM, Zachary Turner <zturner at google.com> wrote:
> >>
> >> BTW, I have just uploaded http://reviews.llvm.org/D8068 to LLVM which implements backtracing of self on Windows (previously only backtracing of other threads was supported).  So once that goes through, if we switch this code to using llvm::sys::PrintBacktrace(),
> >
> > I don’t think we should make this change.
> > Host::Backtrace prints to an lldb_private::Stream which is our preferred API for accumulating output - the LLVM version of this seems to print to a FILE* which is much less general
> >
> > I understand using LLVM facilities where it makes sense, but we should not be doing so blindly when the net effect is a loss of functionality for us
> >
> > If you can implement Host::Backtrace in general on all platforms via LLVM, feel free to do so - but we should still go through lldb's Host::Backtrace for this, and Host::Backtrace should still use our Streams
> >
> >> we should have a standard backtrace format across all platforms that LLVM supports.
> >>
> >> I like this feature now that I understand the use case, thanks for introducing it.
> >>
> >
> > We had a power outage here in Cupertino, which delayed my reply
> > Glad to hear we all agree on this :-)
> >
> >> On Wed, Mar 4, 2015 at 3:31 PM Siva Chandra <sivachandra at google.com> wrote:
> >> On Wed, Mar 4, 2015 at 2:59 PM, Enrico Granata <egranata at apple.com> wrote:
> >> > +#ifdef LLDB_CONFIGURATION_DEBUG
> >> > +#define lldbassert(x) assert(x)
> >> > +#else
> >> > +#define lldbassert(x) lldb_private::lldb_assert(x, #x, __FUNCTION__, __FILE__, __LINE__)
> >> > +#endif
> >>
> >> Why should we have this ifdef? As in, why shouldn't we use lldb_assert
> >> unconditionally, always (giving us the benefit of backtraces always)?
> >> _______________________________________________
> >> lldb-commits mailing list
> >> lldb-commits at cs.uiuc.edu
> >> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
> > Thanks,
> > - Enrico
> > 📩 egranata@.com ☎️ 27683
> >
> >
> >
> >
> > _______________________________________________
> > lldb-commits mailing list
> > lldb-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>