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

[Adrian Prantl via llvm-commits]

> 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>