[lldb-dev] LLDB Evolution - Final Form

[Greg Clayton via lldb-dev]

I agree that for case #1, we must handle that by checking the pointer. Same thing for #2. For #3 we just need to fix the bug in clang. Our case in more of a different issue.

The cause of most crashes for us is in the clang::ASTContext class as we try to create types from DWARF. clang::ASTContext is used mainly by the compiler and this has a very rigid set of assumptions it can make about what inputs it gets. We create types like enums, CXXRecordDecl, Typedefs, arrays, etc by making types in the clang::ASTContext as we parse the DWARF. In the debugger, we are often creating classes from DWARF where information can be missing or not emitted. The -gline-tables-only feature emits DWARF, but takes the contents of all functions and removes all debug info children. This means you might have a function definition for say "bool Foo::operator <(const Foo&)" that has no parameters because the DWARF is truncated. This is one example of things that would make clang::ASTContext crash as it says you must have arguments for an "operator <" function which is understandable. We found that and fix the assert crash. But there are many many things in clang::ASTContext that mostly work, but as soon as we get fancier DWARF from a variety of people and test every variation, we end up crashing due to assertions. And the functions on the clang::ASTContext and many of the clang::Decl and clang::Type stuff don't have a ton of documentation saying "you must satisfy X or I will assert". Many of the assertions do make sense and are the kinds of things you want to see when debugging the creation of new types while you have a debug build. But in production, we would rather be able to try and survive just about anything.


> On Sep 21, 2016, at 8:25 AM, Zachary Turner <zturner at google.com> wrote:
> 
> BTW, one more comment about this.  If you've got a situation where LLDB is using LLVM in a way that makes LLDB crash, there are 3 possibilities:
> 
> 1) LLVM / Clang is vending a pointer and we're supposed to be checking it for null.
> 2) We used the LLVM / Clang API incorrectly causing it to return us a null pointer.
> 3) There is a bug in the LLVM / Clang API.
> 
> Case #1 is the only case where we checking for null is the correct fix.  Blindly adding a null check and calling it a day is making the code *worse* IMO.  If it's case 2 then we need to understand where in LLDB we are doing the wrong thing and fix that.  If it's case 3 we need to dive into the LLVM / Clang code and fix the real problem (presumably with help from someone with expertise).  But "not crashing" is not the same as "fixing the bug".
> 
> FWIW, I expect a lot of these types of problems to fall somewhere between categories 2 and 3.  LLDB is one of (if not the only) user of the clang external ast source so that entire subsystem is not as battle hardened as the rest of clang / llvm.
> 
> On Tue, Sep 20, 2016 at 2:31 PM Greg Clayton <gclayton at apple.com> wrote:
> We should avoid crashing if there is a reasonable work around when the input is bad. StringRef with NULL is easy, just put NULL and zero length and don't crash. Just because it is documented, doesn't mean it needs to stay that way, but I am not going to fight that battle.
> 
> We should make every effort to not crash if we can. If it is way too difficult, document the issue and make sure clients know that this is the way things are. StringRef does this and we accept it. Doesn't mean it won't crash us. I just hate seeing the crash logs where we have:
> 
> StringRef s(die.GetName());
> 
> It shouldn't crash IMHO, but we know it does and we now code around it. Yes, we put in code like:
> 
> StringRef s;
> const char *cstr = die.GetName();
> if (cstr)
>     s = cstr;
> 
> Is this nice code? I am glad it makes it simple for the LLVM side, but I would rather write:
> 
> StringRef s(die.GetName());
> 
> Maybe I will subclass llvm::StringRef as lldb::StringRef and override the constructor.
> 
> 
> > On Sep 20, 2016, at 2:24 PM, Zachary Turner <zturner at google.com> wrote:
> >
> > Well, but StringRef for example is well documented.  So it seems to me like there's an example of a perfectly used assert.  It's documented that you can't use null, and if you do it asserts.  Just like strlen.
> >
> > The issue I have with "you can't ever assert" is that it brings it into an absolute when it really shouldn't be.  We already agreed (I think) that certain things that are well documented can assert.  But when we talk in absolutes, it tends to sway people that they should always do that thing, even when it's not the most appropriate solution.  And I think some of that shows in the LLDB codebase where you've got hugely complicated logic that is very hard to follow, reason about, or test, because no assumptions are ever made about any of the inputs.  Even when they are internal inputs that are entirely controlled by us.
> >
> > On Tue, Sep 20, 2016 at 2:19 PM Greg Clayton <gclayton at apple.com> wrote:
> > Again, strlen is a stupid example as it is well documented. All of llvm and clang are not.
> > > On Sep 20, 2016, at 1:59 PM, Zachary Turner <zturner at google.com> wrote:
> > >
> > >
> > >
> > > On Tue, Sep 20, 2016 at 1:55 PM Greg Clayton <gclayton at apple.com> wrote:
> > >
> > > > On Sep 20, 2016, at 1:45 PM, Zachary Turner <zturner at google.com> wrote:
> > > >
> > > > I do agree that asserts are sometimes used improperly.  But who's to say that the bug was the assert, and not the surrounding code?  For example, consider this code:
> > > >
> > > > assert(p);
> > > > int x = *p;
> > >
> > > Should be written as:
> > >
> > > assert(p);
> > > if (!p)
> > >     do_something_correct();
> > > else
> > >     int x = *p;
> > >
> > > >
> > > > Should this assert also not be here in library code?  I mean it's obvious that the program is about to crash if p is invalid.  Asserts should mean "you're about to invoke undefined behavior", and a crash is *better* than undefined behavior.  It surfaces the problem so that you can't let it slip under the radar, and it also alerts you to the point that the UB is invoked, rather than later.
> > > >
> > > > What about this assert?
> > > >
> > > > assert(ptr);
> > > > int x = strlen(ptr);
> > > >
> > > > Surely that assert is ok right?  Do we need to check whether ptr is valid EVERY SINGLE TIME we invoke strlen, or any other function for that matter?  The code would be a disastrous mess.
> > >
> > > Again, check before you call if this is in a shared library! What is so hard about that? It is called software that doesn't crash.
> > >
> > > assert(ptr)
> > > int x = ptr ? strlen(ptr) : 0;
> > >
> > > I find it hard to believe that you are arguing that you cannot EVER know ANYTHING about the state of your program.  :-/
> > >
> > > This is like arguing that you should run a full heap integrity check every time you perform a memory write, just to be sure you aren't about to crash.
> > >
> > > If you make a std::vector<>, do we need to verify that its internal pointer is not null before we write to it?   Probably not, right?  Why not?  Because it has a specification of how it works, and it is documented that you can construct one, you can use it.
> > >
> > > It's ok to document how functions work, and it is ok to assume that functions work the way they claim to work.
> >
>