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

Zachary Turner via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Thu Mar 28 16:29:24 PDT 2019


zturner added a comment.

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.


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

https://reviews.llvm.org/D59911





More information about the lldb-commits mailing list