> On 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?
Well no, it is indeed terrible: changing die.GetName() to return StringRef make the code a lot safer. When StringRef is used everywhere, this problem disappear by itself. StringRef(const char *) is a very unsafe API, that is not intended to be used *inside* the library (or very sporadically). So if you build a StringRef out of a const char *, you’re supposed to be in a place that deals with “uncontrolled” input and you need to sanitize it. (Also StringRef(const char *) is “costly": it calls strlen) — Mehdi > 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
