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

Tue Aug 23 14:42:17 PDT 2016

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:reviews%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.com<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>