[PATCH] D14711: fix lldb crash by changing const char * with StringRef

[David Blaikie via llvm-commits]

On Mon, Nov 16, 2015 at 10:24 AM, Eugene Kosov <claprix at yandex.ru> wrote:

> Using StringRef basically insert assertions in code. And we get assertion
> failure instead of a nullptr dereference on incorrect Option usage.
>

Oh, OK - so then it's not fixing any bugs, just causing the failure to
happen sooner rather than later? (& in a more defined way - an assertion
failure)


>
> I could create a test but I don't know what exactly I need to test. My
> patch introduces no changes to class interface. Now it's possible to call
> Option::setArgStr() with nullptr and that will work until the next
> O->ArgStr[0]. After the patch that will trigger and assertion failure
> inside of a StringRef constructor. How could I test that?
>

That's not really something we would test, no. There are GUnit "death"
tests which could test this, but we don't commonly write them. If the
behavior was undefined/null dereferincing before, and now it's failing an
assertion, I'd just take that as a refactor and not write a test.

But I would /not/ expect that to fix any bugs - and if it does, I would
rather not make the change until we understand the nature of the bugs and
fix them, otherwise we're likely to be hiding the bugs and making our lives
harder in the future.


>
> I'm argeed it's strange to call this patch a bug fix. But here is my
> problem. I'm not sure this patch will fix my crash. I just hope for this.


Hope is where I get concerned - I don't really like making changes based on
"hope". If bugs are fixed without understanding why, then they may just be
hidden and recur in some other, more problematic way.


> APT lldb crashed every time. But when I build it from sources like this
> http://lldb.llvm.org/tutorial.html it works just fine. I want to use the
> last clang and lldb but I can't buid it from sources every time because my
> laptop is too old and slow. For example I was creating and testing this
> simple patch for the whole day.
>
> We can call my patch just refactoring not a crash fix. And after
> installing the next debian packages I'll see whether my crash is gone.
>
> 16.11.2015, 20:54, "David Blaikie" <dblaikie at gmail.com>:
> > On Mon, Nov 16, 2015 at 9:47 AM, Eugene Kosov <claprix at yandex.ru> wrote:
> >> Hello.
> >>
> >> Here is the description of a crash
> http://lists.llvm.org/pipermail/llvm-dev/2015-November/092429.html
> >>
> >> I thinks its testable. Option::ArgStr, Option::HelpStr,
> Option::ValueStr must not be nullptr. We could make them private and put
> assertions in setter function. And test it. StringRef can not be created
> from nullptr string.
> >
> > Ah, so it's a matter of the current code observes a null/value
> initialized const char* whereas the new code will observe a default
> constructed StringRef which represents the empty string, not null?
> >
> > Could you include a test case in your patch? It's helpful to have test
> coverage in LLVM for LLVM features that our users (including in-project
> uses in places like Clang and LLDB) depend on.
> >
> > I'm not quite following if/why this is a valid state (or whether
> changing to StringRef is just working around a situation and demoting a
> crash to some other unexpected but non-crashy behavior), though - some more
> detail might be helpful, but I'm not sure.
> >
> >> 16.11.2015, 20:38, "David Blaikie" <dblaikie at gmail.com>:
> >>
> >>> What's the actual bug in LLDB's usage & how does this address it? Is
> it testable in LLVM? (unit tests?)
> >>>
> >>> On Mon, Nov 16, 2015 at 9:26 AM, Eugene via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
> >>>> kevgs created this revision.
> >>>> kevgs added a reviewer: llvm-commits.
> >>>> kevgs added a subscriber: llvm-commits.
> >>>> kevgs set the repository for this revision to rL LLVM.
> >>>>
> >>>> Description:
> http://lists.llvm.org/pipermail/llvm-dev/2015-November/092429.html
> >>>>
> >>>> Repository:
> >>>>   rL LLVM
> >>>>
> >>>> http://reviews.llvm.org/D14711
> >>>>
> >>>> Files:
> >>>>   include/llvm/Support/CommandLine.h
> >>>>   lib/Support/CommandLine.cpp
> >>>>   unittests/Support/CommandLineTest.cpp
> >>>>   utils/FileCheck/FileCheck.cpp
> >>>>
> >>>> _______________________________________________
> >>>> llvm-commits mailing list
> >>>> llvm-commits at lists.llvm.org
> >>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> >>
> >> --
> >> Eugene
>
> --
> Eugene
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151116/7d126ec7/attachment.html>