Re: [PATCH] D15208: Patch for inline abort code generation

[Dan Austin via cfe-commits]

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 

Re: [PATCH] D15208: Patch for inline abort code generation

[Dan Austin via cfe-commits]

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

[Dan Austin via cfe-commits]

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

[Dan Austin via cfe-commits]

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

[Dan Austin via cfe-commits]

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

[Dan Austin via cfe-commits]

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

[Dan Austin via cfe-commits]

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
+  //