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

[Gao, Yunzhong via llvm-commits]

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 --------------
A non-text attachment was scrubbed...
Name: test.patch
Type: text/x-patch
Size: 1048 bytes
Desc: test.patch
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160708/8da084e9/attachment.bin>