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

Tue Nov 17 10:31:16 PST 2015

Thank you for your help. But I have no commit rights. Could you commit that patch?

17.11.2015, 19:55, "David Blaikie" <dblaikie at gmail.com>:
> On Tue, Nov 17, 2015 at 1:04 AM, Eugene Kosov <claprix at yandex.ru> wrote:
>> Finally, it looks like I understand the nature of my problem.
>>
>> Here are we initializing ArgStr.
>> lea    0x14005c6(%rip),%rcx        # 0x7fffef778d4b <.L.str21>
>> mov    %rcx,0x20(%rbx)
>>
>> (gdb) p (char*)0x7fffef778d4b
>> $5 = 0x7fffef778d4b <.L.str21> "force-align-stack"
>>
>> Note offset is 0x20 and the value is force-align-stack which was removed some time ago https://github.com/llvm-mirror/llvm/commit/8e2b613ef041e861891affc6434bf4bee846ebbe
>>
>> Going one frame down and here we're reading ArtStr on a different(!) offset of 0x18.
>> mov    0x18(%rbx),%r12
>> mov    0xe647e7(%rip),%rbp        # 0x7ffff55c40e0 <_ZL12GlobalParser>
>> cmpb   $0x0,(%r12)
>>
>> bt is
>> #0  llvm::cl::Option::addArgument (this=0x7fffefd18390 <ForceStackAlign>) at /tmp/buildd/llvm-toolchain-snapshot-3.8~svn252971/lib/Support/CommandLine.cpp:224
>> #1  0x00007fffee3787af in global constructors keyed to a () from /usr/local/lib/python2.7/dist-packages/lldb/_lldb.so
>>
>> What's _lldb.so?
>> $ ll -h /usr/local/lib/python2.7/dist-packages/lldb/_lldb.so
>> lrwxrwxrwx 1 root staff 19 Jan 15  2014 /usr/local/lib/python2.7/dist-packages/lldb/_lldb.so -> ../../../liblldb.so
>>
>> $ ll -h /usr/local/lib/liblldb.so
>> lrwxrwxrwx 1 root root   14 Jan 18  2014 /usr/local/lib/liblldb.so -> liblldb.so.3.5
>>
>> That's a binary I compiled almost two years ago!
>>
>> $ ldd /usr/bin/lldb
>>         linux-vdso.so.1 =>  (0x00007ffd92598000)
>>         liblldb-3.8.so => /usr/lib/x86_64-linux-gnu/liblldb-3.8.so (0x00007f300751e000)
>>
>> ldd claims lldb uses a new liblldb version but actually uses an old one from a different path! I'm was shocked when I discovered this! But that explains why this was not 'fixed' for months and there is no ticket in a bug tracker at all.
>>
>> So, there is no bug which my patch fixes or tries to fix. It's just a simple refactoring :)
>
> Awesome, thanks for the research. Please commit the cleanup/refactoring patch whenever it suits you.
>
> - Dave
>
>> 16.11.2015, 22:55, "David Blaikie" <dblaikie at gmail.com>:
>>
>>> 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
>>
>> --
>> Eugene

--
Eugene