Re: [PATCH] D15208: Patch for inline abort code generation
Makes sense, will add that as well. On Thu, Dec 3, 2015 at 4:10 PM Alexey Samsonov wrote: > samsonov added a comment. > > I agree with Richard here. > > > Repository: > rL LLVM > > http://reviews.llvm.org/D15208 > > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15208: Patch for inline abort code generation
danielaustin updated this revision to Diff 41957.
danielaustin added a comment.
Added a flag controlling whether or not the inlining occurs. Tested with ARM 32
bit (shamu) with AOSP. The patched compiler resulted in correct code generation
and no stability issues. The flag is probably not ideally named, but treating
it as a sanitizer did allow for easy integration into the Android build system.
Repository:
rL LLVM
http://reviews.llvm.org/D15208
Files:
include/clang/Basic/Sanitizers.def
lib/CodeGen/CGExpr.cpp
lib/Driver/ToolChain.cpp
Index: lib/Driver/ToolChain.cpp
===
--- lib/Driver/ToolChain.cpp
+++ lib/Driver/ToolChain.cpp
@@ -657,7 +657,8 @@
// platform dependent.
using namespace SanitizerKind;
SanitizerMask Res = (Undefined & ~Vptr & ~Function) | (CFI & ~CFIICall) |
- CFICastStrict | UnsignedIntegerOverflow | LocalBounds;
+ CFICastStrict | UnsignedIntegerOverflow | LocalBounds |
+ DebugMode;
if (getTriple().getArch() == llvm::Triple::x86 ||
getTriple().getArch() == llvm::Triple::x86_64)
Res |= CFIICall;
Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -27,6 +27,7 @@
#include "llvm/ADT/Hashing.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/IR/DataLayout.h"
+#include "llvm/IR/InlineAsm.h"
#include "llvm/IR/Intrinsics.h"
#include "llvm/IR/LLVMContext.h"
#include "llvm/IR/MDBuilder.h"
@@ -2535,9 +2536,25 @@
void CodeGenFunction::EmitTrapCheck(llvm::Value *Checked) {
llvm::BasicBlock *Cont = createBasicBlock("cont");
- // If we're optimizing, collapse all calls to trap down to just one per
- // function to save on code size.
- if (!CGM.getCodeGenOpts().OptimizationLevel || !TrapBB) {
+ // Collapsing all calls to trap down to one per function makes debugging
+ // these issues much more difficult. Eliminating this optimization
+ // for debugging purposes.
+ // RE: Bug: 25682
+ if(SanOpts.has(SanitizerKind::DebugMode)) {
+ llvm::InlineAsm *EmptyAsm =
llvm::InlineAsm::get(llvm::FunctionType::get(CGM.VoidTy, false),
+ StringRef(""),
StringRef(""), true);
+ TrapBB = createBasicBlock("trap");
+ Builder.CreateCondBr(Checked, Cont, TrapBB);
+ EmitBlock(TrapBB);
+ llvm::CallInst *TrapCall = EmitTrapCall(llvm::Intrinsic::trap);
+ TrapCall->setDoesNotReturn();
+ TrapCall->setDoesNotThrow();
+ Builder.CreateUnreachable();
+ //this stops the trap calls from being merged at the end of the function
+ Builder.CreateCall(EmptyAsm, {});
+ } else if(!CGM.getCodeGenOpts().OptimizationLevel || !TrapBB) {
+// If we're optimizing, collapse all calls to trap down to just one per
+// function to save on code size.
TrapBB = createBasicBlock("trap");
Builder.CreateCondBr(Checked, Cont, TrapBB);
EmitBlock(TrapBB);
@@ -2548,7 +2565,7 @@
} else {
Builder.CreateCondBr(Checked, Cont, TrapBB);
}
-
+
EmitBlock(Cont);
}
Index: include/clang/Basic/Sanitizers.def
===
--- include/clang/Basic/Sanitizers.def
+++ include/clang/Basic/Sanitizers.def
@@ -37,6 +37,8 @@
#define SANITIZER_GROUP(NAME, ID, ALIAS)
#endif
+// Debug mode for sanitizers
+SANITIZER("debug-mode", DebugMode)
// AddressSanitizer
SANITIZER("address", Address)
Index: lib/Driver/ToolChain.cpp
===
--- lib/Driver/ToolChain.cpp
+++ lib/Driver/ToolChain.cpp
@@ -657,7 +657,8 @@
// platform dependent.
using namespace SanitizerKind;
SanitizerMask Res = (Undefined & ~Vptr & ~Function) | (CFI & ~CFIICall) |
- CFICastStrict | UnsignedIntegerOverflow | LocalBounds;
+ CFICastStrict | UnsignedIntegerOverflow | LocalBounds |
+ DebugMode;
if (getTriple().getArch() == llvm::Triple::x86 ||
getTriple().getArch() == llvm::Triple::x86_64)
Res |= CFIICall;
Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -27,6 +27,7 @@
#include "llvm/ADT/Hashing.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/IR/DataLayout.h"
+#include "llvm/IR/InlineAsm.h"
#include "llvm/IR/Intrinsics.h"
#include "llvm/IR/LLVMContext.h"
#include "llvm/IR/MDBuilder.h"
@@ -2535,9 +2536,25 @@
void CodeGenFunction::EmitTrapCheck(llvm::Value *Checked) {
llvm::BasicBlock *Cont = createBasicBlock("cont");
- // If we're optimizing, collapse all calls to trap down to just one per
- // function to save on code size.
- if (!CGM.getCodeGenOpts().OptimizationLevel || !TrapBB) {
+ // Collapsing all calls to trap down to one per function makes debugging
+ // these
Re: [PATCH] D15208: Patch for inline abort code generation
I'm not partial to what its called, or where it is. I'm completely ok with -fsanitize-merge-checks On Tue, Dec 8, 2015 at 1:07 PM Evgeniy Stepanov wrote: > eugenis added a comment. > > Better -fsanitize-merge-checks, and it should apply to non-trap checks as > well (i.e. SIGILL address should uniquely correspond to the failure source > location). > > > Repository: > rL LLVM > > http://reviews.llvm.org/D15208 > > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15208: Patch for inline abort code generation
danielaustin added a comment. I'm not partial to what its called, or where it is. I'm completely ok with -fsanitize-merge-checks Repository: rL LLVM http://reviews.llvm.org/D15208 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15208: Patch for inline abort code generation
danielaustin added a comment. I don't think super-fine grained control would be required, a per-binary setting should be sufficient for debugging purposes. If everyone is ok with it, I'll get a diff up later tonight switching the flag to fsanitize-merge-traps/fnosanitize-merge-traps Repository: rL LLVM http://reviews.llvm.org/D15208 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15208: Patch for inline abort code generation
I don't think super-fine grained control would be required, a per-binary setting should be sufficient for debugging purposes. If everyone is ok with it, I'll get a diff up later tonight switching the flag to fsanitize-merge-traps/fnosanitize-merge-traps On Tue, Dec 8, 2015 at 1:17 PM Evgeniy Stepanov wrote: > eugenis added a comment. > > I misunderstood the meaning of -fsanitize-trap, and now I prefer > -fsanitize-merge-traps for the flag name. > > > Repository: > rL LLVM > > http://reviews.llvm.org/D15208 > > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15208: Patch for inline abort code generation
danielaustin updated this revision to Diff 42328.
danielaustin added a comment.
Patch that removes the debug-mode sanitizer setting and makes the
-fsanitize-merge-traps/-fno-sanitize-merge-traps flags, with the default
setting being to merge traps. Flags tested within the Android build system,
with presence and absence of flags both working as expected.
Repository:
rL LLVM
http://reviews.llvm.org/D15208
Files:
include/clang/Basic/LangOptions.h
include/clang/Driver/Options.td
lib/CodeGen/CGExpr.cpp
lib/Frontend/CompilerInvocation.cpp
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -1569,6 +1569,12 @@
if (Args.hasArg(OPT_fvisibility_inlines_hidden))
Opts.InlineVisibilityHidden = 1;
+ if (Args.hasArg(OPT_fno_sanitize_merge_traps)) {
+Opts.mergeTraps = false;
+ } else {
+Opts.mergeTraps = true;
+ }
+
if (Args.hasArg(OPT_ftrapv)) {
Opts.setSignedOverflowBehavior(LangOptions::SOB_Trapping);
// Set the handler, if one is specified.
Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -27,6 +27,7 @@
#include "llvm/ADT/Hashing.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/IR/DataLayout.h"
+#include "llvm/IR/InlineAsm.h"
#include "llvm/IR/Intrinsics.h"
#include "llvm/IR/LLVMContext.h"
#include "llvm/IR/MDBuilder.h"
@@ -2535,9 +2536,25 @@
void CodeGenFunction::EmitTrapCheck(llvm::Value *Checked) {
llvm::BasicBlock *Cont = createBasicBlock("cont");
- // If we're optimizing, collapse all calls to trap down to just one per
- // function to save on code size.
- if (!CGM.getCodeGenOpts().OptimizationLevel || !TrapBB) {
+ // Collapsing all calls to trap down to one per function makes debugging
+ // these issues much more difficult. Eliminating this optimization
+ // for debugging purposes.
+ // RE: Bug: 25682
+ if(!getLangOpts().mergeTraps) {
+ llvm::InlineAsm *EmptyAsm =
llvm::InlineAsm::get(llvm::FunctionType::get(CGM.VoidTy, false),
+ StringRef(""),
StringRef(""), true);
+ TrapBB = createBasicBlock("trap");
+ Builder.CreateCondBr(Checked, Cont, TrapBB);
+ EmitBlock(TrapBB);
+ llvm::CallInst *TrapCall = EmitTrapCall(llvm::Intrinsic::trap);
+ TrapCall->setDoesNotReturn();
+ TrapCall->setDoesNotThrow();
+ Builder.CreateUnreachable();
+ //this stops the trap calls from being merged at the end of the function
+ Builder.CreateCall(EmptyAsm, {});
+ } else if(!CGM.getCodeGenOpts().OptimizationLevel || !TrapBB) {
+// If we're optimizing, collapse all calls to trap down to just one per
+// function to save on code size.
TrapBB = createBasicBlock("trap");
Builder.CreateCondBr(Checked, Cont, TrapBB);
EmitBlock(TrapBB);
@@ -2548,7 +2565,7 @@
} else {
Builder.CreateCondBr(Checked, Cont, TrapBB);
}
-
+
EmitBlock(Cont);
}
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -607,6 +607,12 @@
: CommaJoined<["-"], "fno-sanitize-recover=">,
Group, Flags<[CoreOption]>,
HelpText<"Disable recovery for specified sanitizers">;
+def fsanitize_merge_traps : Flag<["-"], "fsanitize-merge-traps">,
+Group,
+HelpText<"Merge all traps for sanitizers to one
per function.">;
+def fno_sanitize_merge_traps : Flag<["-"], "fno-sanitize-merge-traps">,
+ Group,
+ HelpText<"Generate traps for sanitizers inline
to aid in debugging.">;
def fsanitize_trap_EQ : CommaJoined<["-"], "fsanitize-trap=">,
Group,
Flags<[CC1Option, CoreOption]>,
HelpText<"Enable trapping for specified sanitizers">;
Index: include/clang/Basic/LangOptions.h
===
--- include/clang/Basic/LangOptions.h
+++ include/clang/Basic/LangOptions.h
@@ -92,6 +92,10 @@
/// If none is specified, abort (GCC-compatible behaviour).
std::string OverflowHandler;
+ /// \brief Flag controlling whether or not trap calls are merged
+ /// at the end of each function.
+ bool mergeTraps;
+
/// \brief The name of the current module.
std::string CurrentModule;
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -1569,6 +1569,12 @@
if (Args.hasArg(OPT_fvisibility_inlines_hidden))
Opts.InlineVisibilityHidden = 1;
+ if (Args.hasArg(OPT_fno_sanitize_merge_traps)) {
+Opts.mergeTraps = false;
+ } else {
Re: [PATCH] D15208: Patch for inline abort code generation
danielaustin added a comment.
Test makes sense, will add it with the revisions.
Comment at: include/clang/Basic/LangOptions.h:95
@@ -94,1 +94,3 @@
+ /// \brief Flag controlling whether or not trap calls are merged
+ /// at the end of each function.
samsonov wrote:
> Why is it a language, not codegen option?
Wasn't aware, will relocate it there, as it makes more sense
Comment at: include/clang/Driver/Options.td:614
@@ +613,3 @@
+def fno_sanitize_merge_traps : Flag<["-"], "fno-sanitize-merge-traps">,
+ Group,
+ HelpText<"Generate traps for sanitizers inline
to aid in debugging.">;
samsonov wrote:
> These should probably be CC1Option as well.
Agree
Comment at: lib/CodeGen/CGExpr.cpp:2543
@@ +2542,3 @@
+ // RE: Bug: 25682
+ if(!getLangOpts().mergeTraps) {
+ llvm::InlineAsm *EmptyAsm =
llvm::InlineAsm::get(llvm::FunctionType::get(CGM.VoidTy, false),
samsonov wrote:
> Note that this will also affect `-ftrapv`, which might be unexpected, as we
> mention "sanitize" in the flag name.
Would it be better in your opinion to remove sanitize from the name, or check
for the presence of the integer sanitizers here?
Comment at: lib/CodeGen/CGExpr.cpp:2549
@@ +2548,3 @@
+ EmitBlock(TrapBB);
+ llvm::CallInst *TrapCall = EmitTrapCall(llvm::Intrinsic::trap);
+ TrapCall->setDoesNotReturn();
samsonov wrote:
> Looks like most of this block (everything except for the empty asm statement)
> is duplicated below.
Yea, I can simplify that.
Repository:
rL LLVM
http://reviews.llvm.org/D15208
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15208: Patch for inline abort code generation
danielaustin updated this revision to Diff 43459.
danielaustin added a comment.
Test case added which looks for the existence of multiple trap basic blocks in
generated LLVM-IR with optimization turned on. Flag renamed
(-fmerge-traps/-fno-merge-traps) and placed in a more sensible location
(CodeGenOptions.def). Trap inlining logic changed to work with most recent
version of LLVM (as opposed to the one packaged with AOSP). Without turning
optimization off in the function containing the trap, there were cases that the
abort calls were merged to the end of the function. Turning off inlining and
optimization for a function which is not to merge traps appears to be working,
but further analysis is in process.
This version results in slightly bigger code (~10% per function) than the
original version, but has the benefit of not breaking basic blocks.
Repository:
rL LLVM
http://reviews.llvm.org/D15208
Files:
include/clang/Driver/Options.td
include/clang/Frontend/CodeGenOptions.def
include/clang/Frontend/CodeGenOptions.h
lib/CodeGen/CGExpr.cpp
lib/Frontend/CompilerInvocation.cpp
test/CodeGen/merge-traps.c
Index: test/CodeGen/merge-traps.c
===
--- test/CodeGen/merge-traps.c
+++ test/CodeGen/merge-traps.c
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu %s -emit-llvm -o - -fsanitize=unsigned-integer-overflow -ftrapv -fmerge-traps -O2 | FileCheck %s --check-prefix=ALL --check-prefix=MERGE
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu %s -emit-llvm -o - -fsanitize=unsigned-integer-overflow -ftrapv -fno-merge-traps -O2 | FileCheck %s --check-prefix=ALL --check-prefix=NOMERGETRAPS
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu %s -emit-llvm -o - -fsanitize=unsigned-integer-overflow -ftrapv -O2| FileCheck %s -check-prefix=ALL --check-prefix=MERGE
+// Verify that -fmerge-traps works as expected with appropriate supporting flags
+
+
+void test_trap_merge() {
+ extern volatile int a, b, c;
+ a = b + c;
+ // MERGE: tail call void @llvm.trap()
+ // NOMERGETRAPS: call void @llvm.trap()
+ b = a + c;
+ // NOMERGETRAPS-DAG: call void @llvm.trap()
+}
+
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -518,6 +518,8 @@
Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
+ Opts.MergeTraps = Args.hasFlag(OPT_fmerge_traps, OPT_fno_merge_traps, true);
+
Opts.PrepareForLTO = Args.hasArg(OPT_flto, OPT_flto_EQ);
const Arg *A = Args.getLastArg(OPT_flto, OPT_flto_EQ);
Opts.EmitFunctionSummary = A && A->containsValue("thin");
Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -27,6 +27,7 @@
#include "llvm/ADT/Hashing.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/IR/DataLayout.h"
+#include "llvm/IR/InlineAsm.h"
#include "llvm/IR/Intrinsics.h"
#include "llvm/IR/LLVMContext.h"
#include "llvm/IR/MDBuilder.h"
@@ -2535,16 +2536,40 @@
void CodeGenFunction::EmitTrapCheck(llvm::Value *Checked) {
llvm::BasicBlock *Cont = createBasicBlock("cont");
- // If we're optimizing, collapse all calls to trap down to just one per
- // function to save on code size.
- if (!CGM.getCodeGenOpts().OptimizationLevel || !TrapBB) {
+ // Collapsing all calls to trap down to one per function makes debugging
+ // these issues much more difficult. Allowing for elimination of this optimization
+ // for debugging purposes.
+ // RE: Bug: 25682
+ if(!CGM.getCodeGenOpts().MergeTraps || !CGM.getCodeGenOpts().OptimizationLevel || !TrapBB) {
+// If we aren't merging traps, set the function to not be optimized as some
+// trap calls appear to be merged to the end of the function even with
+// the call - EmptyAsm - Unreachable pattern at the end of the function.
+if(!CGM.getCodeGenOpts().MergeTraps) {
+ CurFn->addFnAttr(llvm::Attribute::OptimizeNone);
+ CurFn->addFnAttr(llvm::Attribute::NoInline);
+}
+
TrapBB = createBasicBlock("trap");
Builder.CreateCondBr(Checked, Cont, TrapBB);
EmitBlock(TrapBB);
-llvm::CallInst *TrapCall = EmitTrapCall(llvm::Intrinsic::trap);
-TrapCall->setDoesNotReturn();
-TrapCall->setDoesNotThrow();
-Builder.CreateUnreachable();
+if(!CGM.getCodeGenOpts().MergeTraps) {
+ // This closely mirrors what is done to avoid function merging
+ // in the address sanitizer. The trap function is not set
+ // to not return as there is an unreachable instruction
+ // generated at the end of the block.
+ EmitTrapCall(llvm::Intrinsic::trap);
+ llvm::InlineAsm *EmptyAsm = llvm::InlineAsm::get(llvm::FunctionType::get(Builder.getVoidTy(), false),
+ StringRef(""), StringRef(""), true);
+
+ // This stops the trap calls from being merged at the end
