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

[Alexey Samsonov via cfe-commits]

samsonov added a comment.

Sorry for delay.



Comment at: lib/CodeGen/CGExpr.cpp:2543
@@ +2542,3 @@
+  // 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

This condition is somewhat hard to parse. Consider inverting it:
  // Collapse all trap calls to one per function if we're optimizing, and 
  if (TrapBB && CGM.getCodeGenOpts().OptimizationLevel && 
CGM.getCodeGenOpts().MergeTraps) {
Builder.CreateCondBr(Checked, Cont, TrapBB);
  } else {
 //...
  }


Comment at: lib/CodeGen/CGExpr.cpp:2557
@@ +2556,3 @@
+  // 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

Nit: AddressSanitizer, or ASan.


Comment at: lib/CodeGen/CGExpr.cpp:2558
@@ +2557,3 @@
+  // 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.

What's wrong with marking trap function as noreturn anyway? unreachable 
instruction is, well, supposed to be unreachable.


Comment at: lib/CodeGen/CGExpr.cpp:2560
@@ +2559,3 @@
+  // generated at the end of the block.
+  EmitTrapCall(llvm::Intrinsic::trap);
+  llvm::InlineAsm *EmptyAsm = 
llvm::InlineAsm::get(llvm::FunctionType::get(Builder.getVoidTy(), false),

This can also be hoisted from branches.


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

[Alexey Samsonov via cfe-commits]

samsonov added inline comments.


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), 

danielaustin wrote:
> 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?
Probably former - we already have `-ftrap-function` which changes the behavior 
of both sanitizer-generated and regular traps - this flag is no different.


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

[Alexey Samsonov via cfe-commits]

samsonov added a comment.

Please consider adding a test case.



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.

Why is it a language, not codegen option?


Comment at: include/clang/Basic/LangOptions.h:97
@@ +96,3 @@
+  /// at the end of each function.
+  bool mergeTraps;
+

MergeTraps


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.">; 

These should probably be CC1Option as well.


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), 

Note that this will also affect `-ftrapv`, which might be unexpected, as we 
mention "sanitize" in the flag name.


Comment at: lib/CodeGen/CGExpr.cpp:2549
@@ +2548,3 @@
+  EmitBlock(TrapBB);
+  llvm::CallInst *TrapCall = EmitTrapCall(llvm::Intrinsic::trap);
+  TrapCall->setDoesNotReturn();

Looks like most of this block (everything except for the empty asm statement) 
is duplicated below.


Comment at: lib/Frontend/CompilerInvocation.cpp:1572
@@ -1571,1 +1571,3 @@
 
+  if (Args.hasArg(OPT_fno_sanitize_merge_traps)) {
+Opts.mergeTraps = false;

  Args.hasFlag(OPT_fsanitize_merge_traps, OPT_fno_sanitize_merge_traps, true);


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.

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

[Alexey Samsonov via cfe-commits]

samsonov added a comment.

Sorry for the late response. Ugh, flag naming is hard, and it's far too 
complicated for sanitizers already.

However, I'm opposed to passing this down as `-fsanitize=` option =/. So far 
we're trying to make values of `-fsanitize=` correspond *only* to different 
kinds of checks, not configuration modes. For instance, we've pushed back 
against somewhat similar feature request for CFI: original plan for enabling 
"debug mode" in CFI was to pass "-fsanitize=cfi-diag", but we instead changed 
it to be "-fsanitize=cfi -fno-sanitize-trap=cfi".

And we have three possible configurations: recoverable checks, 
fatal/unrecoverable checks, trapping checks. You need, essentially 
"trapping-but-distinguishable checks". I'm afraid adding fourth configuration 
will make things incomprehensible. `-fsanitize-merge-traps` (true by default)?


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

[Evgeniy Stepanov via cfe-commits]

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

[Alexey Samsonov via cfe-commits]

samsonov added a comment.

That is, I presume it's highly unlikely user will need to have fine-grained 
setup for deciding which UBSan checks should be merged, and which should not. 
So, probably having a single `-fsanitize-merge-traps`  / 
`-fno-sanitize-merge-traps` option would be enough. I'd like to hear other 
opinions, though.


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

[Alexey Samsonov via cfe-commits]

samsonov added a comment.

That is, I presume it's highly unlikely user will need to have fine-grained 
setup for deciding which UBSan checks should be merged, and which should not. 
So, probably having a single `-fsanitize-merge-traps`  / 
`-fno-sanitize-merge-traps` option would be enough. I'd like to hear other 
opinions, though.

In http://reviews.llvm.org/D15208#305199, @eugenis wrote:

> 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).


Just to make sure I understand it, you suggest to eventually pass it down to 
ASan/MSan instrumentation passes?


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

[Evgeniy Stepanov via cfe-commits]

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

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

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

[Richard Smith via cfe-commits]

rsmith added a comment.

I would expect that people who are using this for hardening would be upset 
about a 5% binary size increase. I'm OK with having this behind a flag, though.


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

[Alexey Samsonov via cfe-commits]

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