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

Mon Nov 16 11:55:44 PST 2015

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

>
>
> 16.11.2015, 21:41, "David Blaikie" <dblaikie at gmail.com>:
> >  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)
>
> Basically, yes. Making Option class harder to use incorrectly.
>
> >>  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.
>
> Ok. If patch is fine we could call it refactoring and commit to trunk. It
> will make code a bit more up-to-date and robust. After testing the new lldb
> version from debian package, most probably I will have a crash again with
> probably more clear backtrace and open a ticket in a bugtracker. Or we can
> throw away this patch (at least for now) and I'll fill a ticket with info I
> have right now. Your think the second option is better, do you?
>

ish. Is there any advantage to committing this before you run the
build/test/reproduce the crash with the patch applied locally? Then report
the bug with the extra info & commit this patch?

- Dave

>
> >>  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
>
>
> --
> Eugene
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151116/fea297e5/attachment-0001.html>