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 <ztur...@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 <gclay...@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 <ztur...@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 <gclay...@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 <ztur...@google.com> wrote: > > > > > > > > > > > > On Tue, Sep 20, 2016 at 1:55 PM Greg Clayton <gclay...@apple.com> wrote: > > > > > > > On Sep 20, 2016, at 1:45 PM, Zachary Turner <ztur...@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. > > > _______________________________________________ lldb-dev mailing list lldb-dev@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
