<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Nov 16, 2015 at 10:24 AM, Eugene Kosov <span dir="ltr"><<a href="mailto:claprix@yandex.ru" target="_blank">claprix@yandex.ru</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Using StringRef basically insert assertions in code. And we get assertion failure instead of a nullptr dereference on incorrect Option usage.<br></blockquote><div><br></div><div>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)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
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?<br></blockquote><div><br></div><div>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.<br><br>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
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.</blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> APT lldb crashed every time. But when I build it from sources like this <a href="http://lldb.llvm.org/tutorial.html" rel="noreferrer" target="_blank">http://lldb.llvm.org/tutorial.html</a> 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.<br>
<br>
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.<br>
<br>
16.11.2015, 20:54, "David Blaikie" <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>>:<br>
<div><div class="h5">> On Mon, Nov 16, 2015 at 9:47 AM, Eugene Kosov <<a href="mailto:claprix@yandex.ru">claprix@yandex.ru</a>> wrote:<br>
>> Hello.<br>
>><br>
>> Here is the description of a crash <a href="http://lists.llvm.org/pipermail/llvm-dev/2015-November/092429.html" rel="noreferrer" target="_blank">http://lists.llvm.org/pipermail/llvm-dev/2015-November/092429.html</a><br>
>><br>
>> 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.<br>
><br>
> 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?<br>
><br>
> 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.<br>
><br>
> 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.<br>
><br>
>> 16.11.2015, 20:38, "David Blaikie" <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>>:<br>
>><br>
>>> What's the actual bug in LLDB's usage & how does this address it? Is it testable in LLVM? (unit tests?)<br>
>>><br>
>>> On Mon, Nov 16, 2015 at 9:26 AM, Eugene via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
>>>> kevgs created this revision.<br>
>>>> kevgs added a reviewer: llvm-commits.<br>
>>>> kevgs added a subscriber: llvm-commits.<br>
>>>> kevgs set the repository for this revision to rL LLVM.<br>
>>>><br>
>>>> Description: <a href="http://lists.llvm.org/pipermail/llvm-dev/2015-November/092429.html" rel="noreferrer" target="_blank">http://lists.llvm.org/pipermail/llvm-dev/2015-November/092429.html</a><br>
>>>><br>
>>>> Repository:<br>
>>>>   rL LLVM<br>
>>>><br>
>>>> <a href="http://reviews.llvm.org/D14711" rel="noreferrer" target="_blank">http://reviews.llvm.org/D14711</a><br>
>>>><br>
>>>> Files:<br>
>>>>   include/llvm/Support/CommandLine.h<br>
>>>>   lib/Support/CommandLine.cpp<br>
>>>>   unittests/Support/CommandLineTest.cpp<br>
>>>>   utils/FileCheck/FileCheck.cpp<br>
>>>><br>
>>>> _______________________________________________<br>
>>>> llvm-commits mailing list<br>
>>>> <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
>>>> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
>><br>
>> --<br>
>> Eugene<br>
<br>
</div></div>--<br>
Eugene<br>
</blockquote></div><br></div></div>