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

David Blaikie via llvm-commits llvm-commits at lists.llvm.org
Fri Sep 2 16:57:33 PDT 2016


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?

- David

On Fri, Sep 2, 2016 at 4:49 PM Gao, Yunzhong <Yunzhong.Gao at sony.com> wrote:

> Hi David,
>
> Here is a timeline of events from my perspective. I would not be surprised
> if
> I have misunderstood something here; Adrian please feel free to correct me.
>
> 1. [In the old days] If a caller function has debug metadata, and a callee
> function has debug metadata, but the call instruction has no debug
> metadata,
> and if inlining takes place, the LLVM backend runs into an assertion
> failure:
>
>   !dbg attachment points at wrong subprogram for function
>
>   (rdar://problem/25878916)
>
> 2. r267370 adds a verifier check, so in the case described above, one would
> get a verifier failure instead of a backend assertion.
>
> 3. But the compiler may insert call instructions to special functions. So
> to avoid
> the verifier check failure added in event#2 above, r267904 and r274263
> attempt to attach artificial debug metadata to these artificial call
> instructions.
>
> 4. David noticed that the test added in r274263 does not have any CHECK
> lines.
> The test is actually only checking that the verifier check does not get
> triggered. David asks whether the verifier check can be improved to allow
> this case instead of adding more debug info.
>
> 5. Gao did some experiment and found that if the verifier check is relaxed,
> then we get the backend assertion failure from event#1. The remaining
> question
> is what should be done about the test case discussed in event#4.
>
> Please advise.
> - Gao
>
>
> ________________________________________
> From: David Blaikie [dblaikie at gmail.com]
> Sent: Thursday, September 01, 2016 7:59 PM
> To: Gao, Yunzhong; Adrian Prantl
> Cc: Robinson, Paul;
> 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
>
> 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.
>
> Anyone able/care to summarize?
>
> On Thu, Sep 1, 2016 at 3:43 PM Gao, Yunzhong <Yunzhong.Gao at sony.com
> <mailto:Yunzhong.Gao at sony.com>> wrote:
> > 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<mailto:
> reviews%2BD21818%2Bpublic%2Bb40f0be86340e0e5 at reviews.llvm.org>;
> llvm-commits at lists.llvm.org<mailto: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<mailto:aprantl at apple.com> [aprantl at apple.com
> <mailto: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<mailto:
> reviews%2BD21818%2Bpublic%2Bb40f0be86340e0e5 at reviews.llvm.org>;
> llvm-commits at lists.llvm.org<mailto: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<mailto:
> 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<mailto:aprantl at apple.com> [aprantl at apple.com
> <mailto: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<mailto:
> reviews%2BD21818%2Bpublic%2Bb40f0be86340e0e5 at reviews.llvm.org>;
> llvm-commits at lists.llvm.org<mailto: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
> <mailto: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<mailto:aprantl at apple.com> [aprantl at apple.com
> <mailto: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<mailto:
> reviews%2BD21818%2Bpublic%2Bb40f0be86340e0e5 at reviews.llvm.org>;
> llvm-commits at lists.llvm.org<mailto: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
> <mailto: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> [mailto:
> 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<mailto:
> reviews%2BD21818%2Bpublic%2Bb40f0be86340e0e5 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
> <mailto:paul.robinson at sony.com>>
> >>>> wrote:
> >>>>>> -----Original Message-----
> >>>>>> From: llvm-commits [mailto:llvm-commits-bounces at lists.llvm.org
> <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>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160902/9287fa14/attachment.html>


More information about the llvm-commits mailing list