[Lldb-commits] [PATCH] D18646: Fix DWO breakage in r264909

Fri Apr 1 10:44:23 PDT 2016

> On Apr 1, 2016, at 3:43 AM, Tamas Berghammer <tberghammer at google.com> wrote:
> 
> tberghammer added a comment.
> 
> This assert is NOT verifying the debug info itself. It is verifying that LLDB asked the right SymbolFileDWARFDwo to find a DIE based on a DIERef. Independently of the debug info (what can be garbage) passed in to LLDB this assert should never fire if LLDB has no major bug in the DWO handling logic.
> 
> Considering that we have ~10k assert in LLVM, ~6k assert in clang and ~1k assert in LLDB you should compile your release binary without asserts if you want to avoid some crash what is caused by them.
> 
> In this concrete case if we return an empty DWARFDIE from this function then the caller of the function will crash (with a SEGV) as it is not prepared to handle the case when no DWARFDIE can be found based on a DIERef as it should never happen if LLDB works correctly (independently of the debug info).

We do have places in the DWARF reader where we assert for nonsensical input, e.g. if you get to SymbolFileDwarf then the SymbolContext that you used to get there has to have a compile unit in it, otherwise you shouldn't find your way there.  I don't know this particular section of code well enough to comment assertively here if this is or is not in this category.    

But one of the problems we have with clang, for instance, is that clang's assumption was that it was going to build up its data structures by parsing code, and it has defenses at the parse stage to reject input that would result in invalid types.  So it seems eminently reasonable to assert if anything is wrong with for example the types it has already processed rather than do any work to try to back out.  But lldb by necessity comes in below the layer of these defenses, since we transcode DWARF, not source code, into types, and so we come a cropper with asserts that weren't really fatal, but were reasonable for the normal mode of use in the compiler.

By analogy in lldb, the rule for debug info, it is wise to assume there's going to be some junk in the debug info that you will need to sort through.  Even if you have to scotch a whole compile unit's debug information because you can't make sense of it, that should not be a fatal error in the debugger.  An assert that is provably only  triggered by a programming error in that API's usage is sometimes acceptable.  But unless you're really sure there's no way bad data could get you to the assert, then we should try to back out instead.

> 
> I think the very high level question we are discussing is what should LLDB do when it notices that it has a bug in itself. In terms of a compiler displaying an "internal compiler error" and the exiting/crashing is definitely the good solution (to avoid miss compilation) but what is the expected behavior for an interactive application. If you want to be protected against every possible issue then we should remove all assert from LLDB, convince the LLVM and clang guys to do the same and protect every pointer deference with a null check just in case some of the previous function calls returned NULL. I don't think this is the approach we are wanting to go down as it will add a lot of overhead to LLDB and make a lot of bug silent what is very problematic in case of a debugger where displaying incorrect information is almost as bad as crashing the IDE.

Yea, we shouldn't silently swallow errors when we come across something that isn't what the code expected.  Nor should we check every single pointer everywhere by reflex, that would be awkward as you say.  But the design for lldb should be that we strenuously attempt to always return a "nope couldn't do that" error if at all possible.  And callers should be prepared to handle that.  The debugger has lots of independent parts.  So if the expression parser has an error, or some type is mangled so we can't realize it, there are lots of other ways to look at data, and lots of other types to use which alternatives are rendered unavailable to the user if we crash on error.  And a long debug session is pretty stateful, so just putting lldb out of process, letting it crash and then maybe auto-reattaching is only a partial solution at best.

Jim

> 
> PS: Pavel changed the assert to an lldb_assert in the meantime.
> 
> 
> Repository:
>  rL LLVM
> 
> http://reviews.llvm.org/D18646
> 
> 
> 