[lldb-dev] LLDB Evolution - Final Form

Zachary Turner via lldb-dev lldb-dev at lists.llvm.org
Tue Sep 20 14:46:20 PDT 2016


Occasionally (and in my experience *very* occasionally), you need to treat
"" as different from null.  But the frequency with which that is really
necessary is much lower than people realize.  It just seems high because of
the prevalence of raw pointers.

For every other case, you can use withNullAsEmpty()  (mostly used as a
helper for porting and when dealing with native APIs, which is to say quite
a bit right now, but in the long run, almost never (evidenced by the fact
that it only got added to LLVM a few days ago and nobody has ever needed
it).

On Tue, Sep 20, 2016 at 2:41 PM Mehdi Amini <mehdi.amini at apple.com> wrote:

>
> > On 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?
>
> 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 <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.
> >>
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20160920/4ab2a581/attachment-0001.html>


More information about the lldb-dev mailing list