<div dir="ltr">Ah, Thanks for the summary - so it seems like a test should test /just/ the pass that creates the artificial call, and verify that the call is created with a debug location, when it wasn't before?<br><br>- David</div><br><div class="gmail_quote"><div dir="ltr">On Fri, Sep 2, 2016 at 4:49 PM Gao, Yunzhong <<a href="mailto:Yunzhong.Gao@sony.com">Yunzhong.Gao@sony.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi David,<br>
<br>
Here is a timeline of events from my perspective. I would not be surprised if<br>
I have misunderstood something here; Adrian please feel free to correct me.<br>
<br>
1. [In the old days] If a caller function has debug metadata, and a callee<br>
function has debug metadata, but the call instruction has no debug metadata,<br>
and if inlining takes place, the LLVM backend runs into an assertion failure:<br>
<br>
  !dbg attachment points at wrong subprogram for function<br>
<br>
  (rdar://problem/25878916)<br>
<br>
2. r267370 adds a verifier check, so in the case described above, one would<br>
get a verifier failure instead of a backend assertion.<br>
<br>
3. But the compiler may insert call instructions to special functions. So to avoid<br>
the verifier check failure added in event#2 above, r267904 and r274263<br>
attempt to attach artificial debug metadata to these artificial call instructions.<br>
<br>
4. David noticed that the test added in r274263 does not have any CHECK lines.<br>
The test is actually only checking that the verifier check does not get<br>
triggered. David asks whether the verifier check can be improved to allow<br>
this case instead of adding more debug info.<br>
<br>
5. Gao did some experiment and found that if the verifier check is relaxed,<br>
then we get the backend assertion failure from event#1. The remaining question<br>
is what should be done about the test case discussed in event#4.<br>
<br>
Please advise.<br>
- Gao<br>
<br>
<br>
________________________________________<br>
From: David Blaikie [<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>]<br>
Sent: Thursday, September 01, 2016 7:59 PM<br>
To: Gao, Yunzhong; Adrian Prantl<br>
Cc: Robinson, Paul; <a href="mailto:reviews%2BD21818%2Bpublic%2Bb40f0be86340e0e5@reviews.llvm.org" target="_blank">reviews+D21818+public+b40f0be86340e0e5@reviews.llvm.org</a>; <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
Subject: Re: [PATCH] D21818: Add artificial debug information to avoid compiler crash<br>
<br>
This whole thread seems to have taken many twists & turns - and I'm having trouble piecing it back together to figure out what's going on and what direction we should take.<br>
<br>
Anyone able/care to summarize?<br>
<br>
On Thu, Sep 1, 2016 at 3:43 PM Gao, Yunzhong <<a href="mailto:Yunzhong.Gao@sony.com" target="_blank">Yunzhong.Gao@sony.com</a><mailto:<a href="mailto:Yunzhong.Gao@sony.com" target="_blank">Yunzhong.Gao@sony.com</a>>> wrote:<br>
> It looks like, at the moment, if someone accidentally removes this verifier check, the test suite still passes cleanly.<br>
<br>
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.<br>
<br>
________________________________________<br>
From: Gao, Yunzhong<br>
Sent: Thursday, September 01, 2016 3:27 PM<br>
To: Adrian Prantl<br>
Cc: Robinson, Paul; David Blaikie; <a href="mailto:reviews%2BD21818%2Bpublic%2Bb40f0be86340e0e5@reviews.llvm.org" target="_blank">reviews+D21818+public+b40f0be86340e0e5@reviews.llvm.org</a><mailto:<a href="mailto:reviews%252BD21818%252Bpublic%252Bb40f0be86340e0e5@reviews.llvm.org" target="_blank">reviews%2BD21818%2Bpublic%2Bb40f0be86340e0e5@reviews.llvm.org</a>>; <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><mailto:<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>><br>
Subject: RE: [PATCH] D21818: Add artificial debug information to avoid compiler crash<br>
<br>
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.<br>
<br>
$ opt -inline -filetype=asm -o - test2.ll<br>
!dbg attachment points at wrong subprogram for function<br>
<br>
So, this test case does show that the verifier test cannot be safely relaxed. I have two follow-up questions.<br>
<br>
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?<br>
<br>
Index: llvm/test/Verifier/callsite-dbgloc.ll<br>
===================================================================<br>
--- llvm/test/Verifier/callsite-dbgloc.ll       (revision 279786)<br>
+++ llvm/test/Verifier/callsite-dbgloc.ll       (working copy)<br>
@@ -1,4 +1,5 @@<br>
 ; RUN: not llvm-as %s -o %t 2>&1 | FileCheck %s<br>
+; RUN: not opt %s -inline -filetype=asm -o /dev/null 2>&1 | FileCheck %s<br>
 ; Created and then edited from<br>
 ;   extern void i();<br>
 ;   void h() { i(); }<br>
<br>
With this patch, if someone removes the verifier check, the second assertion "dbg pointing at wrong subprogram" would be triggered.<br>
<br>
2. Is there still anything I can/should do to improve the stack-protector.ll test case from commit r274263?<br>
<br>
Thanks,<br>
- Gao<br>
<br>
<br>
________________________________________<br>
From: <a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a><mailto:<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>> [<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a><mailto:<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>>]<br>
Sent: Thursday, September 01, 2016 1:42 PM<br>
To: Gao, Yunzhong<br>
Cc: Robinson, Paul; David Blaikie; <a href="mailto:reviews%2BD21818%2Bpublic%2Bb40f0be86340e0e5@reviews.llvm.org" target="_blank">reviews+D21818+public+b40f0be86340e0e5@reviews.llvm.org</a><mailto:<a href="mailto:reviews%252BD21818%252Bpublic%252Bb40f0be86340e0e5@reviews.llvm.org" target="_blank">reviews%2BD21818%2Bpublic%2Bb40f0be86340e0e5@reviews.llvm.org</a>>; <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><mailto:<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>><br>
Subject: Re: [PATCH] D21818: Add artificial debug information to avoid compiler crash<br>
<br>
> On Sep 1, 2016, at 1:36 PM, Gao, Yunzhong <<a href="mailto:Yunzhong.Gao@sony.com" target="_blank">Yunzhong.Gao@sony.com</a><mailto:<a href="mailto:Yunzhong.Gao@sony.com" target="_blank">Yunzhong.Gao@sony.com</a>>> wrote:<br>
><br>
> Hi Adrian,<br>
> I tested with the attached test case with my modified compiler (see the previous email on the modifications),<br>
> $ llc < test.ll -o -<br>
> And the compilation succeeded.<br>
<br>
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.<br>
<br>
><br>
> - Gao<br>
><br>
><br>
> ________________________________________<br>
> From: <a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a><mailto:<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>> [<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a><mailto:<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>>]<br>
> Sent: Thursday, September 01, 2016 9:42 AM<br>
> To: Gao, Yunzhong<br>
> Cc: Robinson, Paul; David Blaikie; <a href="mailto:reviews%2BD21818%2Bpublic%2Bb40f0be86340e0e5@reviews.llvm.org" target="_blank">reviews+D21818+public+b40f0be86340e0e5@reviews.llvm.org</a><mailto:<a href="mailto:reviews%252BD21818%252Bpublic%252Bb40f0be86340e0e5@reviews.llvm.org" target="_blank">reviews%2BD21818%2Bpublic%2Bb40f0be86340e0e5@reviews.llvm.org</a>>; <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><mailto:<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>><br>
> Subject: Re: [PATCH] D21818: Add artificial debug information to avoid compiler crash<br>
><br>
>> On Aug 26, 2016, at 4:53 PM, Gao, Yunzhong <<a href="mailto:Yunzhong.Gao@sony.com" target="_blank">Yunzhong.Gao@sony.com</a><mailto:<a href="mailto:Yunzhong.Gao@sony.com" target="_blank">Yunzhong.Gao@sony.com</a>>> wrote:<br>
>><br>
>> Hi Adrian,<br>
>> I did the following experiment.<br>
>> Step#1, check out from trunk r279786<br>
>> Step#2, revert the following commits: r267370, r267904 and r274263<br>
>> Step#3, build and run regression test => all tests pass with no failure<br>
>><br>
>> In addition,<br>
>> I tested the compiler built above with this test added in r267370, llvm/test/Verifier/callsite-dbgloc.ll,<br>
>> the compilation succeeds with no assertion failure. This test has a comment at the top of the file<br>
>> which mentions a C example that was expected to get an assertion failure with "-O2 -g", but upon<br>
>> my experiment, no assertion failure was triggered either.<br>
>><br>
>> I tested the compiler built above with the test change introduced in r274263, and again, the test<br>
>> passed with no assertion failure.<br>
>><br>
>> What do you think of this experiment? Can you draw meaningful conclusions from this,<br>
>> or am I missing some steps?<br>
><br>
> The dangerous pattern is this:<br>
><br>
> define void @foo !dbg !1 {<br>
>  ...<br>
>  call @__stack_chk_fail() ; Missing !dbg location here!<br>
>  ...<br>
> }<br>
><br>
> define void @__stack_chk_fail() !dbg !2 {<br>
>  some_insn, !dbg !3<br>
>  ...<br>
> }<br>
><br>
> 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.<br>
><br>
> Does your test exercise this case?<br>
><br>
> -- adrian<br>
><br>
>><br>
>> - Gao<br>
>><br>
>><br>
>><br>
>> ________________________________________<br>
>> From: <a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a><mailto:<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>> [<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a><mailto:<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>>]<br>
>> Sent: Thursday, August 25, 2016 10:52 AM<br>
>> To: Robinson, Paul<br>
>> Cc: Gao, Yunzhong; David Blaikie; <a href="mailto:reviews%2BD21818%2Bpublic%2Bb40f0be86340e0e5@reviews.llvm.org" target="_blank">reviews+D21818+public+b40f0be86340e0e5@reviews.llvm.org</a><mailto:<a href="mailto:reviews%252BD21818%252Bpublic%252Bb40f0be86340e0e5@reviews.llvm.org" target="_blank">reviews%2BD21818%2Bpublic%2Bb40f0be86340e0e5@reviews.llvm.org</a>>; <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><mailto:<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>><br>
>> Subject: Re: [PATCH] D21818: Add artificial debug information to avoid compiler crash<br>
>><br>
>>> On Aug 25, 2016, at 10:49 AM, Robinson, Paul <<a href="mailto:paul.robinson@sony.com" target="_blank">paul.robinson@sony.com</a><mailto:<a href="mailto:paul.robinson@sony.com" target="_blank">paul.robinson@sony.com</a>>> wrote:<br>
>>><br>
>>> (re-adding llvm-commits which got lost from my previous)<br>
>>><br>
>>>> -----Original Message-----<br>
>>>> From: <a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a><mailto:<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>> [mailto:<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a><mailto:<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>>]<br>
>>>> Sent: Thursday, August 25, 2016 10:42 AM<br>
>>>> To: Robinson, Paul<br>
>>>> Cc: Gao, Yunzhong; David Blaikie;<br>
>>>> <a href="mailto:reviews%2BD21818%2Bpublic%2Bb40f0be86340e0e5@reviews.llvm.org" target="_blank">reviews+D21818+public+b40f0be86340e0e5@reviews.llvm.org</a><mailto:<a href="mailto:reviews%252BD21818%252Bpublic%252Bb40f0be86340e0e5@reviews.llvm.org" target="_blank">reviews%2BD21818%2Bpublic%2Bb40f0be86340e0e5@reviews.llvm.org</a>><br>
>>>> Subject: Re: [PATCH] D21818: Add artificial debug information to avoid<br>
>>>> compiler crash<br>
>>>><br>
>>>><br>
>>>>> On Aug 25, 2016, at 10:37 AM, Robinson, Paul <<a href="mailto:paul.robinson@sony.com" target="_blank">paul.robinson@sony.com</a><mailto:<a href="mailto:paul.robinson@sony.com" target="_blank">paul.robinson@sony.com</a>>><br>
>>>> wrote:<br>
>>>>>> -----Original Message-----<br>
>>>>>> From: llvm-commits [mailto:<a href="mailto:llvm-commits-bounces@lists.llvm.org" target="_blank">llvm-commits-bounces@lists.llvm.org</a><mailto:<a href="mailto:llvm-commits-bounces@lists.llvm.org" target="_blank">llvm-commits-bounces@lists.llvm.org</a>>] On<br>
>>>> Behalf<br>
>>>>>> Of Gao, Yunzhong via llvm-commits<br>
>>>>>> Hi Adrian,<br>
>>>>>> I would not say that attaching an artificial debug location should be<br>
>>>>>> ruled out as<br>
>>>>>> an option, and the original commit r274263 used that approach. But I<br>
>>>> think<br>
>>>>>> David<br>
>>>>>> was suggesting that the verifier could be modified to consider this IR<br>
>>>> as<br>
>>>>>> valid<br>
>>>>>> without the extra debug info (that was my interpretation of what David<br>
>>>>>> said,<br>
>>>>>> but I could be wrong) and therefore I did some investigation here,<br>
>>>>><br>
>>>>> Right, the line-0 tactic increases debug-info size but the original<br>
>>>>> complaint was because of a verifier failure.  If we can mark the<br>
>>>>> offending artificial calls so that the verifier will allow them,<br>
>>>>> then we don't pay the debug-info size cost in the object file.<br>
>>>>><br>
>>>>> I'd be happier with a tactic that would automatically deal with this<br>
>>>>> for all cases of artificial calls to hard-coded function names, but<br>
>>>>> it's not immediately clear how to make that work.<br>
>>>><br>
>>>> I added the verifier check because having an inlineable function call<br>
>>>> without a debug location to a function with debug info inside a function<br>
>>>> with debug info causes an assertion failure when the backend attempts to<br>
>>>> construct the inline scopes.<br>
>>>> Do we know that it is safe to relax the verifier here?<br>
>>>><br>
>>>> -- adrian<br>
>>>><br>
>>>><br>
>>> Well, maybe not, and that would be a good experiment for Gao to try--<br>
>>> undo the original patch, so it doesn't mark the call with line-0, and<br>
>>> make the called function inlineable, and see if it dies when generating<br>
>>> the inlined scopes.<br>
>><br>
>> Gao, I think the assertion we're looking for is:<br>
>><br>
>> LexicalScopes::getOrCreateRegularScope()<br>
>> assert(cast<DISubprogram>(Scope)->describes(MF->getFunction()));<br>
>><br>
>>> --paulr<br>
>>><br>
>><br>
>><br>
><br>
><br>
> <test.ll><br>
<br>
<br>
</blockquote></div>