> On Sep 20, 2016, at 2:41 PM, Mehdi Amini <mehdi.am...@apple.com> wrote: > >> >> 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)
That is fine, but someone is still making the initial StringRef with the "const char *" so strlen is still being called. Strings from DWARF come from a string table so we will always call strlen. We might reduce the number of times strlen is called though and that is good. > — > 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
