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

Tue Nov 17 11:03:35 PST 2015

Sure, committed in r253360

On Tue, Nov 17, 2015 at 10:31 AM, Eugene Kosov <claprix at yandex.ru> wrote:

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