[Lldb-commits] [PATCH] D59911: Don't abort() in lldb_assert and document why.

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D59911#1446946 <https://reviews.llvm.org/D59911#1446946>, @zturner wrote:

> In D59911#1446942 <https://reviews.llvm.org/D59911#1446942>, @jingham wrote:
>
> > In D59911#1446745 <https://reviews.llvm.org/D59911#1446745>, @davide wrote:
> >
> > > My (maybe unpopolar) opinion on the subject is that "soft assertions" are a way to cleanse your conscience of guilt, but they don't work really well in practice.
> > >  When I started working on lldb, I was a fairly strong proponent of assertions everywhere. My view changed somewhat radically over the course of the past 18 months, and I would like to summarize some points here.
> > >
> > > 1. Vendors (some) ship a release or two of debuggers every 6 months. Even if you just look at open source, llvm gets released every 6 months and distributions downstream package just the release version. Not everybody runs gentoo or exherbo or compiles his toolchain from svn. For these people the debugger has just to work. Assertions are always compiled out in release mode so this is not an issue. I strongly agree that for internal interfaces they have to be used as liberally as possible, mostly because they catch bugs in development. llvm libraries tend to use assertions galore, and developers put them in "just in case", and I think this is really not something we should do in lldb.
> > > 2. Errors have to be handled, properly. I don't think returning a default constructed object in case something goes bad is better than throwing an error. We now have rich/proper error handling in llvm to make sure the process blows up if the error is not checked. This is a good thing.
> > > 3. The above points are IMHO the only two needed ways of handling invariants/invalid input. Anything else in the middle is just going to cause confusion to new and existing developer. Adrian (or anybody else), do you have any real example where something wasn't either a user error or a strong invariant that couldn't be violated? I personally didn't, hence I don't understand the need for "soft" assertions here.
> >
> >
> > Here is a concrete example of the sort of thing I thought lldbassert was for.   In DWARFExpression::AddressRangeForLocationListEntry if we come across a LocationList format we don't understand, we do:
> >
> >   default:
> >     // Not supported entry type
> >     lldbassert(false && "Not supported location list type");
> >     return false;
>
>
> IMO, this example should be changed to
>
>   return llvm::make_error<DWARFError>("Unsupported location list type");
>
>
> and let someone higher up handle this.


We may be talking at cross-purposes...

I think it would be fine to convert this FUNCTION to return an llvm::Error that enforces the error gets handled properly.  But that's orthogonal to whether we do an lldbassert before returning the make_error (or "return false").

The whole point of lldbasserts as I understood it was is to catch problems that we can and should always recover from IRL as efficiently as possible in the testsuite.

If this function returned an error, and all the code above was doing the right thing, then some one in the call stack will deal with the return and finally end up producing some user-visible failure (i.e. "can't view variable X").   However, you will only catch the failure in the testsuite if the place where we ran across this bad format happened to be a variable that a test asserted.  Then we would see a test case failure and would go debug it and figure out what we needed to do, and all would be good.

But if we put an lldbassert before the return, then we wouldn't have to rely on what exactly the test checked.  As long as we passed through this code with the unexpected format we would assert, the test would fail at that point, and we  would know something went wrong and that we should go fix it.  So the chance that the testsuite would catch this problem for us is increased by the lldbassert.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59911/new/

https://reviews.llvm.org/D59911