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

Tue Nov 17 08:55:43 PST 2015

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151117/8317bd78/attachment.html>