Differential D59911
Don't abort() in lldb_assert and document why. Authored by aprantl on Mar 27 2019, 4:54 PM.
Details 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
Event TimelineComment Actions Can you please also update the RST docs? (https://lldb.llvm.org/new-www/)
Comment Actions 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. 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)?
Comment Actions 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. Comment Actions 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.
Comment Actions 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? Comment Actions 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. Comment Actions Thank you for taking the time to write this up! I wish I could have read this when I started working on LLDB. :) Comment Actions
Done! Comment Actions Works for me. Tanya has to manually modify the scripts that generate this. I've sent her an e-mail asking if anything is blocking the transition. Comment Actions 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. Comment Actions IMO, this example should be changed to return llvm::make_error<DWARFError>("Unsupported location list type");and let someone higher up handle this. Comment Actions 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. Comment Actions 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.
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. Comment Actions 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. | ||||||||||||||||||||||||