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

Thu Mar 28 16:11:04 PDT 2019

jingham added a comment.

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;

We should be running the testsuite against the in development compiler, so if the compiler adds a new location list format but somebody forgets to tell us, if we are lucky it will get emitted in some test's code, and that test will die with this assertion.  Then we will know we have to go add that format.  Since we can run the testsuite against other compilers we can widen the lookout by doing that for any compilers we care about.

If somebody runs lldb against the output of a new compiler in the field and that has a location list format we don't support, then we can recover, so unreachable is not appropriate.  And we do return false when we don't understand the format, so everybody upstream will recover.  In fact, in my understanding lldbassert explicitly says "you can assert in testing but you have to recover from this condition".  So that seems alright.

However, in this case it might be good to print a message saying "you have some debug info I don't understand" at the point where we didn't understand it, as being easier to debug than a downstream message.  Logging is another way to do this, but that involves a couple of round trips with whoever encounters the problem, since the user would see some downstream error (I couldn't print the value of this variable) then we have to figure out that it is a DWARF problem and not some other variable reconstruction problem and get them to turn on that log...

In this case it might be better to print an error at the problem site.  You could certainly do this with an assert and a printf.  lldbassert just provides a convenient way to do that.

So I would say: use lldbassert for recoverable errors that you would want to catch at the error site in testing, and would want to warn about (as opposed to logging) at the error site in the field.

However, I do agree that lldbasserts should be used sparingly.  I'm thinking of the gdb "complaints" which would print a message for everything that wasn't copasetic in debug info.  In practice you would either get no complaints, or you would get a new compiler that did one or two bad things MANY times, and so you would get a spew of complaints and you really wouldn't be able to use the debugger w/o turning them off altogether.

So you have to make sure that they are not used in situations where they are going to be noisy.

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

https://reviews.llvm.org/D59911