[PATCH] D21818: Add artificial debug information to avoid compiler crash

Thu Sep 1 15:43:54 PDT 2016

> It looks like, at the moment, if someone accidentally removes this verifier check, the test suite still passes cleanly.

I spoke too soon. Sorry. If someone accidentally removes just the verifier check, this test should fail because the test is looking specifically for this one assertion message from the verifier and it would not be there. In my experiment, I reverted r267370 and therefore removed this test as well.

________________________________________
From: Gao, Yunzhong
Sent: Thursday, September 01, 2016 3:27 PM
To: Adrian Prantl
Cc: Robinson, Paul; David Blaikie; reviews+D21818+public+b40f0be86340e0e5 at reviews.llvm.org; llvm-commits at lists.llvm.org
Subject: RE: [PATCH] D21818: Add artificial debug information to avoid compiler crash

Ok, this test case was not very well constructed that if I just run through "opt -inline", it looks like the callee is being optimized away. But it is simple enough to modify the test case.

$ opt -inline -filetype=asm -o - test2.ll
!dbg attachment points at wrong subprogram for function

So, this test case does show that the verifier test cannot be safely relaxed. I have two follow-up questions.

1. It looks like, at the moment, if someone accidentally removes this verifier check, the test suite still passes cleanly. It seems that it would make sense to add a test that exercises the inliner?

Index: llvm/test/Verifier/callsite-dbgloc.ll
===================================================================

--- llvm/test/Verifier/callsite-dbgloc.ll       (revision 279786)
+++ llvm/test/Verifier/callsite-dbgloc.ll       (working copy)
@@ -1,4 +1,5 @@
 ; RUN: not llvm-as %s -o %t 2>&1 | FileCheck %s
+; RUN: not opt %s -inline -filetype=asm -o /dev/null 2>&1 | FileCheck %s
 ; Created and then edited from
 ;   extern void i();
 ;   void h() { i(); }

With this patch, if someone removes the verifier check, the second assertion "dbg pointing at wrong subprogram" would be triggered.

2. Is there still anything I can/should do to improve the stack-protector.ll test case from commit r274263?

Thanks,
- Gao


________________________________________
From: aprantl at apple.com [aprantl at apple.com]
Sent: Thursday, September 01, 2016 1:42 PM
To: Gao, Yunzhong
Cc: Robinson, Paul; David Blaikie; reviews+D21818+public+b40f0be86340e0e5 at reviews.llvm.org; llvm-commits at lists.llvm.org
Subject: Re: [PATCH] D21818: Add artificial debug information to avoid compiler crash

> On Sep 1, 2016, at 1:36 PM, Gao, Yunzhong <Yunzhong.Gao at sony.com> wrote:
>
> Hi Adrian,
> I tested with the attached test case with my modified compiler (see the previous email on the modifications),
> $ llc < test.ll -o -
> And the compilation succeeded.

That command line doesn’t sound as if the function was actually inlined though, right? You’ll probably want to run it through something like "opt -inline” first to ensure that __stack_chk_fail is inlined into test2.

>
> - Gao
>
>
> ________________________________________
> From: aprantl at apple.com [aprantl at apple.com]
> Sent: Thursday, September 01, 2016 9:42 AM
> To: Gao, Yunzhong
> Cc: Robinson, Paul; David Blaikie; reviews+D21818+public+b40f0be86340e0e5 at reviews.llvm.org; llvm-commits at lists.llvm.org
> Subject: Re: [PATCH] D21818: Add artificial debug information to avoid compiler crash
>
>> On Aug 26, 2016, at 4:53 PM, Gao, Yunzhong <Yunzhong.Gao at sony.com> wrote:
>>
>> Hi Adrian,
>> I did the following experiment.
>> Step#1, check out from trunk r279786
>> Step#2, revert the following commits: r267370, r267904 and r274263
>> Step#3, build and run regression test => all tests pass with no failure
>>
>> In addition,
>> I tested the compiler built above with this test added in r267370, llvm/test/Verifier/callsite-dbgloc.ll,
>> the compilation succeeds with no assertion failure. This test has a comment at the top of the file
>> which mentions a C example that was expected to get an assertion failure with "-O2 -g", but upon
>> my experiment, no assertion failure was triggered either.
>>
>> I tested the compiler built above with the test change introduced in r274263, and again, the test
>> passed with no assertion failure.
>>
>> What do you think of this experiment? Can you draw meaningful conclusions from this,
>> or am I missing some steps?
>
> The dangerous pattern is this:
>
> define void @foo !dbg !1 {
>  ...
>  call @__stack_chk_fail() ; Missing !dbg location here!
>  ...
> }
>
> define void @__stack_chk_fail() !dbg !2 {
>  some_insn, !dbg !3
>  ...
> }
>
> When some_insn gets inlined into @foo its debug location will still point to @__stack_chk_fail, instead of being proper inlined location rooted in @foo.
>
> Does your test exercise this case?
>
> -- adrian
>
>>
>> - Gao
>>
>>
>>
>> ________________________________________
>> From: aprantl at apple.com [aprantl at apple.com]
>> Sent: Thursday, August 25, 2016 10:52 AM
>> To: Robinson, Paul
>> Cc: Gao, Yunzhong; David Blaikie; reviews+D21818+public+b40f0be86340e0e5 at reviews.llvm.org; llvm-commits at lists.llvm.org
>> Subject: Re: [PATCH] D21818: Add artificial debug information to avoid compiler crash
>>
>>> On Aug 25, 2016, at 10:49 AM, Robinson, Paul <paul.robinson at sony.com> wrote:
>>>
>>> (re-adding llvm-commits which got lost from my previous)
>>>
>>>> -----Original Message-----
>>>> From: aprantl at apple.com [mailto:aprantl at apple.com]
>>>> Sent: Thursday, August 25, 2016 10:42 AM
>>>> To: Robinson, Paul
>>>> Cc: Gao, Yunzhong; David Blaikie;
>>>> reviews+D21818+public+b40f0be86340e0e5 at reviews.llvm.org
>>>> Subject: Re: [PATCH] D21818: Add artificial debug information to avoid
>>>> compiler crash
>>>>
>>>>
>>>>> On Aug 25, 2016, at 10:37 AM, Robinson, Paul <paul.robinson at sony.com>
>>>> wrote:
>>>>>> -----Original Message-----
>>>>>> From: llvm-commits [mailto:llvm-commits-bounces at lists.llvm.org] On
>>>> Behalf
>>>>>> Of Gao, Yunzhong via llvm-commits
>>>>>> Hi Adrian,
>>>>>> I would not say that attaching an artificial debug location should be
>>>>>> ruled out as
>>>>>> an option, and the original commit r274263 used that approach. But I
>>>> think
>>>>>> David
>>>>>> was suggesting that the verifier could be modified to consider this IR
>>>> as
>>>>>> valid
>>>>>> without the extra debug info (that was my interpretation of what David
>>>>>> said,
>>>>>> but I could be wrong) and therefore I did some investigation here,
>>>>>
>>>>> Right, the line-0 tactic increases debug-info size but the original
>>>>> complaint was because of a verifier failure.  If we can mark the
>>>>> offending artificial calls so that the verifier will allow them,
>>>>> then we don't pay the debug-info size cost in the object file.
>>>>>
>>>>> I'd be happier with a tactic that would automatically deal with this
>>>>> for all cases of artificial calls to hard-coded function names, but
>>>>> it's not immediately clear how to make that work.
>>>>
>>>> I added the verifier check because having an inlineable function call
>>>> without a debug location to a function with debug info inside a function
>>>> with debug info causes an assertion failure when the backend attempts to
>>>> construct the inline scopes.
>>>> Do we know that it is safe to relax the verifier here?
>>>>
>>>> -- adrian
>>>>
>>>>
>>> Well, maybe not, and that would be a good experiment for Gao to try--
>>> undo the original patch, so it doesn't mark the call with line-0, and
>>> make the called function inlineable, and see if it dies when generating
>>> the inlined scopes.
>>
>> Gao, I think the assertion we're looking for is:
>>
>> LexicalScopes::getOrCreateRegularScope()
>> assert(cast<DISubprogram>(Scope)->describes(MF->getFunction()));
>>
>>> --paulr
>>>
>>
>>
>
>
> <test.ll>


