Page MenuHomePhabricator

Don't abort() in lldb_assert and document why.
ClosedPublic

Authored by aprantl on Mar 27 2019, 4:54 PM.

Details

Summary

See the changes to the website for the full rationale.

Having a policy document that explains when to use what method of error handling in LLDB will make on-boarding new community members easier and help make sense of all the different options. Let me know if there is a better location for this text and/or if you disagree with the contents!

rdar://problem/49356014

Diff Detail

Repository
rL LLVM

Event Timeline

aprantl created this revision.Mar 27 2019, 4:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 27 2019, 4:54 PM
JDevlieghere requested changes to this revision.Mar 27 2019, 5:00 PM
JDevlieghere added a subscriber: JDevlieghere.

Can you please also update the RST docs? (https://lldb.llvm.org/new-www/)

lldb/www/source.html
83 ↗(On Diff #192543)

What happens with asserts in release builds? If they don't cause the process to abort, then they're strictly worse than lldb_assert.

This revision now requires changes to proceed.Mar 27 2019, 5:00 PM
aprantl marked an inline comment as done.Mar 27 2019, 5:16 PM
aprantl added inline comments.
lldb/www/source.html
83 ↗(On Diff #192543)

The same thing that happens with them in LLVM; they disappear. I see the main benefit of assertions to guard against thoughtless refactoring, where the contract between components is broken. Ideally we'd encode the entire contract in the type system, but where that isn't possible we need assertions. But their primary use is when validating LLDB (ReleaseAsserts) against the testsuite.

aprantl updated this revision to Diff 192548.Mar 27 2019, 5:31 PM

Thank you for writing this up. I think there is a lot of confusion around this topic, and a document like this is a good step towards clearing it up.

Not really related to this patch, but since we're talking about lldb_assert... The thing that has always bugged me about this function is that it is really hard to use the way it's supposed to be used. Usually, you have to repeat the condition you are asserting twice. For instance, you often have to write something like this

lldb_assert(thing_that_should_not_happen);
// Now I cannot just assume assertion is true, because that may crash. So I still need to handle the situation somehow.
if(!thing_that_should_not_happen)
  return default_value_or_error_or_something;

At that point I just give up, and don't write the lldb_assert line at all.
Maybe if lldb_assert returned a value (possibly even marked with warn_unused_result), then one could "inline" the assert into the if statement, and things would look less awkward (?)

Hmm.. cool. I didn't know about this. Is there a timeline for moving this to the main website (and getting rid of the html docs)?

lldb/docs/resources/source.rst
73–78 ↗(On Diff #192548)

This looks like it tries to specify when Expected<T> and when Optional<T> should be used, but its not really clear to me what the distinction is.

The way I see it, both "Expected" and "Optional" can be used for "invalid input" and "functions that may fail", and the difference is whether there is a need to attach a error message or error code to explain why the failure happened.

81–82 ↗(On Diff #192548)

I am not sure this really explains the difference between lldb_assert and assert. A "failed internal consistency check" is definitely a "bug in lldb", and it can certainly "happen". Perhaps an example would help make things clearer?
I tried making one up, but I realized I couldn't (maybe that's the intention since you say they should be used sparingly?). The one place where I'd consider using lldb_assert is for things that are definitely a "bug", but they are not an "lldb bug", such as a compiler emitting DWARF that is obviously broken. However, this document makes it pretty clear this falls into the "invalid input" case.

88 ↗(On Diff #192548)

I am wondering whether we should also mention logging here. What I often do when there is no reasonable way to bubble an error up the stack or display it to the user is that I write it to the log. In that sense it is a form of error handling. WDYT?

aprantl updated this revision to Diff 192708.Mar 28 2019, 1:35 PM

Address review feedback!

While writing this I came to the conclusion that lldb_assert() is really a lazy way of handling errors. If we can we should replace it with error handling and perhaps logging.

davide added a subscriber: davide.Mar 28 2019, 2:35 PM

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

Thanks for summarizing your thoughts, Davide.

I think that what you wrote is a much better explanation of what I was trying to say with

Use these sparingly and only if error handling is not otherwise feasible.

I think that unless we can eliminate all uses of lldb_assert() from the code we should document it, but discourage its use. Do you have any suggestions for a better wording?

Thanks for summarizing your thoughts, Davide.

I think that what you wrote is a much better explanation of what I was trying to say with

Use these sparingly and only if error handling is not otherwise feasible.

I think that unless we can eliminate all uses of lldb_assert() from the code we should document it, but discourage its use. Do you have any suggestions for a better wording?

I do personally believe that your wording is reasonable. If it was me, I would just be a little stronger and say that new code should never use lldbassert, and if you're touching existing code you might consider replacing it with proper error handling.
I would also like to thank you personally for clarifying this whole situation. I am also responsible for using carelessly lldbassert in some places of the codebase, but your doc was good to make me think about it (and why to not use), and I guess it would be very useful for others as well [including new contributors].

Thank you for taking the time to write this up! I wish I could have read this when I started working on LLDB. :)

aprantl updated this revision to Diff 192730.Mar 28 2019, 3:43 PM

I do personally believe that your wording is reasonable. If it was me, I would just be a little stronger and say that new code should never use lldbassert, and if you're touching existing code you might consider replacing it with proper error handling.

Done!

davide accepted this revision.Mar 28 2019, 3:44 PM

LGTM

JDevlieghere accepted this revision.Mar 28 2019, 3:56 PM

Works for me.

Hmm.. cool. I didn't know about this. Is there a timeline for moving this to the main website (and getting rid of the html docs)?

Tanya has to manually modify the scripts that generate this. I've sent her an e-mail asking if anything is blocking the transition.

This revision is now accepted and ready to land.Mar 28 2019, 3:56 PM

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.

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.

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.

labath accepted this revision.Mar 29 2019, 12:10 AM

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.

I just realized that this can cause problems with tests that specifically check the handling of invalid/corrupted debug info (which we don't have many of right now, but we probably should). Normally, you would want to test that lldb gracefully recovers from such a situation, but then in a debug build this will just crash. Theoretically, we could make this behavior dependent on at runtime property (LLDB_ASSERT_SANE_COMPILER=True), but that may just make things too complicated.

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.

It should be fairly easy to whip up a helper function, which reports an error only once. Perhaps something like:

switch(form) {
...
default:
  static std::once_flag report_bad_dw_form;
  report_once(report_bad_dw_form, "Unknown form: {0}", form);
  return something;
}

This would even allow you to place the once_flag inside some object, and get the error reported once for each (e.g.) Module that we are processing.

Another problem with lldb_assert() is that in its current form it prints to stderr, which AFAIK is not even visible when LLDB is invoked from an IDE like Xcode.

I like Pavel's suggestion of having a report_once function.

It looks like everybody seems to agree that lldb_assert has some useful ideas but is flawed in its implementation. I'm going to land this patch and we can continue improving or building a replacement for lldb_assert independently from this.

Closed by commit rL357268: Don't abort() in lldb_assert and document why. (authored by adrian, committed by ). · Explain WhyMar 29 2019, 9:12 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 29 2019, 9:12 AM