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

[David Blaikie via llvm-commits]

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> 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 [aprantl at apple.com]
> Sent: Friday, July 08, 2016 2:47 PM
> To: David Blaikie
> Cc: reviews+D21818+public+b40f0be86340e0e5 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>> 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>> 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/> = !{!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>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160711/8b9777ce/attachment.html>