> 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)


> 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

Reply via email to