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

Tue Aug 23 14:53:41 PDT 2016

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


> -----Original Message-----
> From: aprantl at apple.com [mailto:aprantl at apple.com]
> Sent: Tuesday, August 23, 2016 2:42 PM
> To: Gao, Yunzhong
> Cc: David Blaikie;
> reviews+D21818+public+b40f0be86340e0e5 at reviews.llvm.org; Eric
> Christopher; llvm-commits
> Subject: Re: [PATCH] D21818: Add artificial debug information to avoid
> compiler crash
> 
> Can you remind me again why fixing the frontend to attach an artificial debug
> location (line 0) is not an option here?
> 
> -- adrian
> > On Aug 23, 2016, at 2:39 PM, Gao, Yunzhong <Yunzhong.Gao at sony.com>
> wrote:
> >
> > Hi David,
> >
> >> perhaps that's sufficient to have the verifier test that just says
> >> "doesn't consider this invalid".
> >
> > Sorry for taking a while to respond. I was exploring possible
> > approaches to improve the verifier test to avoid considering the said case
> as invalid.
> >
> > One option would be to identify all the call instructions that the
> > compiler may artificially insert, and the verifier could create a whitelist of
> these "special"
> > function names. I did not spend a lot of time investigating this
> > option because I feel that having the verifier checking a list of
> > hardcoded function names has a hackish feeling to me, and also
> > hardcoding these function names would not distinguish a compiler-inserted
> call and a user-created call.
> >
> > A second option is to attach an attribute to the call instructions
> > that the compiler inserts, and then the verifier could check on the
> > existence of such an attribute. This approach avoids inserting a dummy
> > debug info entry, at the cost of attaching an extra attribute in IR.
> > Would this seem like a preferable approach? I am attaching the patch.
> >
> > I did not touch the test file from the previous commit r274263 that
> > makes sure the verifier tests do not fail in this patch, but advice is
> > appreciated how I should modify the test.
> >
> > Thanks,
> > - Gao
> >
> >
> > ________________________________________
> > From: David Blaikie [dblaikie at gmail.com]
> > Sent: Monday, July 11, 2016 3:28 PM
> > To: Gao, Yunzhong
> > Cc: Adrian Prantl;
> > reviews+D21818+public+b40f0be86340e0e5 at reviews.llvm.org; Eric
> > Christopher; llvm-commits
> > Subject: Re: [PATCH] D21818: Add artificial debug information to avoid
> > compiler crash
> >
> > If it's really just a case of the verifier erroneously failing on this input and
> the input isn't unique/interesting in any other way (ie: all optimizations,
> codegen, etc, wouldn't see it as 'different' or interesting at all) then perhaps
> that's sufficient to have the verifier test that just says "doesn't consider this
> invalid".
> >
> > On Fri, Jul 8, 2016 at 4:05 PM, Gao, Yunzhong
> <Yunzhong.Gao at sony.com<mailto:Yunzhong.Gao at sony.com>> wrote:
> > Hi Adrian, David,
> > I can add CHECK lines to the test just like what was added for the other
> functions in the same file.
> > If there is a better way to test verifier errors or a better place to add such
> test, could you advise?
> > I can understand what David is saying, but I would need help in order
> > to make the test more specific,
> > - Gao
> >
> >
> >
> > ________________________________________
> > From: aprantl at apple.com<mailto:aprantl at apple.com>
> > [aprantl at apple.com<mailto:aprantl at apple.com>]
> > Sent: Friday, July 08, 2016 2:47 PM
> > To: David Blaikie
> > Cc:
> >
> reviews+D21818+public+b40f0be86340e0e5 at reviews.llvm.org<mailto:revie
> ws
> > %2BD21818%2Bpublic%2Bb40f0be86340e0e5 at reviews.llvm.org>; Gao,
> > Yunzhong; Eric Christopher; llvm-commits
> > Subject: Re: [PATCH] D21818: Add artificial debug information to avoid
> > compiler crash
> >
> > On Jul 8, 2016, at 2:44 PM, David Blaikie
> <dblaikie at gmail.com<mailto:dblaikie at gmail.com><mailto:dblaikie at gmail.co
> m<mailto:dblaikie at gmail.com>>> wrote:
> >
> >
> >
> > On Tue, Jun 28, 2016 at 3:28 PM, Yunzhong Gao via llvm-commits <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>>> wrote:
> > ygao created this revision.
> > ygao added reviewers: aprantl, echristo, dblaikie.
> > ygao added a subscriber: llvm-commits.
> > Herald added a subscriber: mehdi_amini.
> >
> > Hi,
> > The compiler crashed with the following test program:
> >
> > /* test.c
> > * To reproduce:
> > * $ clang -c -g -fstack-protector-strong test.c
> > * (fatal error: error in backend: Broken function found, compilation
> > aborted!)
> > * (need  an assertion-enabled build of the compiler) */ void
> > __stack_chk_fail(void) { return; }
> >
> > void foo() {
> >  char bar[5];
> > }
> > /* end of test.c */
> >
> > This is not a legitimate input C file because __stack_chk_fail is a reserved
> function name.
> >
> > Is there a legitimate case this fix addresses? Might be good to have such a
> test case so we don't lose the coverage if/when this issue is fixed.
> >
> > If there isn't, then perhaps we should fix the original issue (allowing this
> invalid construct) rather than adding downstream workarounds?
> >
> > IIRC, this patch fixed an IR Verifier error triggered by the StackProtector
> pass which used to insert a function call without attaching a valid DebugLoc to
> the call.
> >
> > -- adrian
> >
> >
> > On the other hand, it seems that the compiler should be able to give a
> > nicer diagnostic or otherwise fail more gracefully instead of having
> > to crash on invalid input (in the spirit of all "internal compiler error"s are
> compiler bugs).
> >
> > The proposed patch here attempts to add artificial debug information
> > when the compiler generates the call to __stack_chk_fail. Does this look
> like a reasonable thing to do?
> >
> > Many thanks in advance,
> > - Gao
> >
> > http://reviews.llvm.org/D21818
> >
> > Files:
> >  lib/CodeGen/StackProtector.cpp
> >  test/CodeGen/X86/stack-protector.ll
> >
> > Index: test/CodeGen/X86/stack-protector.ll
> >
> ==========================================================
> =========
> > --- test/CodeGen/X86/stack-protector.ll
> > +++ test/CodeGen/X86/stack-protector.ll
> > @@ -3880,6 +3880,17 @@
> >   ret i32 %call
> > }
> >
> > +define void @__stack_chk_fail() #1 !dbg !6 {
> > +entry:
> > +  ret void
> > +}
> > +
> > +define void @test32() #1 !dbg !7 {
> > +entry:
> > +  %0 = alloca [5 x i8], align 1
> > +  ret void
> > +}
> > +
> > declare double @testi_aux()
> > declare i8* @strcpy(i8*, i8*)
> > declare i32 @printf(i8*, ...)
> > @@ -3899,3 +3910,16 @@
> > attributes #3 = { ssp "stack-protector-buffer-size"="33" } attributes
> > #4 = { ssp "stack-protector-buffer-size"="5" } attributes #5 = { ssp
> > "stack-protector-buffer-size"="6" }
> > +
> > +!llvm.dbg.cu<http://llvm.dbg.cu><http://llvm.dbg.cu/> = !{!0}
> > +!llvm.module.flags = !{!3, !4} !llvm.ident = !{!5}
> > +
> > +!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1)
> > +!1 = !DIFile(filename: "test.c", directory: "/tmp")
> > +!2 = !{}
> > +!3 = !{i32 2, !"Dwarf Version", i32 4}
> > +!4 = !{i32 2, !"Debug Info Version", i32 3}
> > +!5 = !{!"clang version x.y.z"}
> > +!6 = distinct !DISubprogram(name: "__stack_chk_fail", scope: !1,
> > +unit: !0)
> > +!7 = distinct !DISubprogram(name: "foo", scope: !1, unit: !0)
> > Index: lib/CodeGen/StackProtector.cpp
> >
> ==========================================================
> =========
> > --- lib/CodeGen/StackProtector.cpp
> > +++ lib/CodeGen/StackProtector.cpp
> > @@ -25,6 +25,7 @@
> > #include "llvm/IR/Attributes.h"
> > #include "llvm/IR/Constants.h"
> > #include "llvm/IR/DataLayout.h"
> > +#include "llvm/IR/DebugInfo.h"
> > #include "llvm/IR/DerivedTypes.h"
> > #include "llvm/IR/Function.h"
> > #include "llvm/IR/GlobalValue.h"
> > @@ -448,6 +449,7 @@
> >   LLVMContext &Context = F->getContext();
> >   BasicBlock *FailBB = BasicBlock::Create(Context, "CallStackCheckFailBlk",
> F);
> >   IRBuilder<> B(FailBB);
> > +  B.SetCurrentDebugLocation(DebugLoc::get(0, 0, F->getSubprogram()));
> >   if (Trip.isOSOpenBSD()) {
> >     Constant *StackChkFail =
> >         M->getOrInsertFunction("__stack_smash_handler",
> >
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > 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>>
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> >
> >
> >
> >
> > <debug_info.patch>