[PATCH] D21818: Add artificial debug information to avoid compiler crash
Gao, Yunzhong via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 23 14:39:09 PDT 2016
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: debug_info.patch
Type: text/x-patch
Size: 5766 bytes
Desc: debug_info.patch
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160823/8405f7b7/attachment.bin>
More information about the llvm-commits
mailing list