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

[Eugene Kosov via llvm-commits]

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

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?

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