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

Mon Oct 3 11:32:02 PDT 2016

On Tue, Sep 27, 2016 at 6:50 PM Gao, Yunzhong <Yunzhong.Gao at sony.com> wrote:

> Hi David,
>
> > 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?
>
> Sorry for taking a while to respond.
> Your suggestion makes sense. I looked into how to run just one pass, and it
> seems a bit tricky to run just the stack protector pass. One way is to use
> a combination of the -start-after and -stop-after options of llc.

Ah, sorry - I hadn't noticed this was a code gen pass, rather than a middle
end/IR pass.

> The downside
> is that the output is MIR instead of LLVM IR. FileCheck seems to accept
> it. Do
> you know of better ways to run only one pass?
>

No, MIR is meant to be the tool here, but I don't know if it's become
common enough that that's the canonical way to write such a test.

Sounds like just keeping it as a codegen pass, testing assembly out the
other end is fine. It's already got lots of test coverage in this form, so
adding more seems consistent which is fine.

But the test should test something more than "doesn't crash" - it should
verify that the debug information is emitted correctly, for example. But
that's perhaps a bit involved/annoying?

>
> Below is the patch to the test:
>
> Index: test/CodeGen/X86/stack-protector.ll
> ===================================================================
> --- test/CodeGen/X86/stack-protector.ll (revision 279786)
> +++ test/CodeGen/X86/stack-protector.ll (working copy)
> @@ -1,4 +1,6 @@
>  ; RUN: llc -mtriple=i386-pc-linux-gnu < %s -o - | FileCheck
> --check-prefix=LINUX-I386 %s
> +; RUN: llc -mtriple=i386-pc-linux-gnu < %s -o - -start-after safe-stack \
> +; RUN:   -stop-after stack-protector | FileCheck
> --check-prefix=LINUX-I386-IR %s
>  ; RUN: llc -mtriple=x86_64-pc-linux-gnu < %s -o - | FileCheck
> --check-prefix=LINUX-X64 %s
>  ; RUN: llc -code-model=kernel -mtriple=x86_64-pc-linux-gnu < %s -o - |
> FileCheck --check-prefix=LINUX-KERNEL-X64 %s
>  ; RUN: llc -mtriple=x86_64-apple-darwin < %s -o - | FileCheck
> --check-prefix=DARWIN-X64 %s
> @@ -3887,6 +3889,8 @@
>
>  define void @test32() #1 !dbg !7 {
>  entry:
> +; LINUX-I386-IR-LABEL: test32()
> +; LINUX-I386-IR: call void @__stack_chk_fail(), !dbg !{{[0-9]+}}
>    %0 = alloca [5 x i8], align 1
>    ret void
>  }
>
> - Gao
>
>
> ________________________________________
> From: David Blaikie [dblaikie at gmail.com]
> Sent: Friday, September 02, 2016 4:57 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
>
> 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
> <mailto: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<mailto: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<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
>
> 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><mailto: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><mailto:
> reviews%2BD21818%2Bpublic%2Bb40f0be86340e0e5 at reviews.llvm.org<mailto:
> reviews%252BD21818%252Bpublic%252Bb40f0be86340e0e5 at reviews.llvm.org>>;
> llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org><mailto:
> 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><mailto:aprantl at apple.com
> <mailto:aprantl at apple.com>> [aprantl at apple.com<mailto:aprantl at apple.com
> ><mailto: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><mailto:
> reviews%2BD21818%2Bpublic%2Bb40f0be86340e0e5 at reviews.llvm.org<mailto:
> reviews%252BD21818%252Bpublic%252Bb40f0be86340e0e5 at reviews.llvm.org>>;
> llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org><mailto:
> 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><mailto: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><mailto:
> aprantl at apple.com<mailto:aprantl at apple.com>> [aprantl at apple.com<mailto:
> aprantl at apple.com><mailto: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><mailto:
> reviews%2BD21818%2Bpublic%2Bb40f0be86340e0e5 at reviews.llvm.org<mailto:
> reviews%252BD21818%252Bpublic%252Bb40f0be86340e0e5 at reviews.llvm.org>>;
> llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org><mailto:
> 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><mailto: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><mailto:
> aprantl at apple.com<mailto:aprantl at apple.com>> [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: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><mailto:
> reviews%2BD21818%2Bpublic%2Bb40f0be86340e0e5 at reviews.llvm.org<mailto:
> reviews%252BD21818%252Bpublic%252Bb40f0be86340e0e5 at reviews.llvm.org>>;
> llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org><mailto:
> 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><mailto: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>> [mailto: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><mailto:
> reviews%2BD21818%2Bpublic%2Bb40f0be86340e0e5 at reviews.llvm.org<mailto:
> reviews%252BD21818%252Bpublic%252Bb40f0be86340e0e5 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><mailto: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><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/20161003/940a1943/attachment.html>