[sanitizer] Add -fsanitize-trap-function.

[jmgao]

Summary

Add an -fsanitize-trap-function flag that acts as -ftrap-function
does, except only for traps generated by -fsanitize-trap.

[jmgao]

tools/clang/lib/CodeGen/CodeGenFunction.h

2889 ↗

(On Diff #32649)

FIXME: Add doxygen comments

[samsonov]

Please upload the patch with more context (see http://llvm.org/docs/Phabricator.html).

tools/clang/include/clang/Driver/Options.td

610 ↗

(On Diff #32649)

Fix the help text: Make trapping sanitizers issue a call to specified function rather than a trap instruction.

tools/clang/include/clang/Frontend/CodeGenOptions.h

205 ↗

(On Diff #32649)

Fix the comment here as well.

tools/clang/lib/CodeGen/CGExpr.cpp

2388 ↗

(On Diff #32649)

This is confusing. So, you have the following behavior whenever you need to emit a check for -fsanitize-trap=foo:

1. Emit a call to -fsanitize-trap-function, if it's specified
2. Otherwise, emit a call to -ftrap-function, if it's specified
3. Otherwise, emit a trap instruction.

Do you really want it? Don't you need to skip (2) instead? If you do, you should carefully document it.

2415 ↗

(On Diff #32649)

This function should not be needed.

2422 ↗

(On Diff #32649)

Isn't it easier to just kill this function, and always pass in CGM.getCodeGenOpts().TrapFuncName where applicable?

tools/clang/lib/CodeGen/CGExprScalar.cpp

2386 ↗

(On Diff #32649)

Why? Looks like this check is not emitted as a part of -fsanitize=signed-integer-overflow

tools/clang/lib/Frontend/CompilerInvocation.cpp

507 ↗

(On Diff #32649)

Please parse this argument next to another sanitizer arguments.

[jmgao]

Uploading diff with arcanist.

[rsmith]

Can you please give a brief description of the motivation for this change? When would it be appropriate to use this rather than -ftrap-function?

Please also include an update for the Clang documentation to describe the new flag.

[samsonov]

See https://llvm.org/bugs/show_bug.cgi?id=24443 (it's worth including this reference to change description).

[jmgao]

tools/clang/lib/CodeGen/CGExpr.cpp

2388 ↗

(On Diff #32649)

My interpretation of the current -ftrap-function help text ("Issue call to specified function rather than a trap instruction") is that if -ftrap-function is specified, clang should never emit a trap instruction if defined. Does this seem reasonable?

I added a blurb to UsersManual.rst saying that it falls back to -ftrap-function if not specified.

2422 ↗

(On Diff #32649)

It's used in a few other places (__builtin_trap, __builtin_debugtrap, fall off the end of a function in -O0), so I think it's probably easier (and less error prone) to keep a copy of the function that fetches the TrapFuncName itself.

[jmgao]

Address comments

[jmgao]

Improve comment

[samsonov]

Overall, this looks reasonable to me, but I'd like Richard to confirm he's fine with this change as well.

docs/UsersManual.rst

1121

for sanitizer checks configured to trap.

include/clang/Driver/Options.td

610

80 cols

lib/CodeGen/CodeGenFunction.h

2895

You can probably make this private

2899

And this

[rsmith]

In D12181#229358, @rsmith wrote:

Can you please give a brief description of the motivation for this change? When would it be appropriate to use this rather than -ftrap-function?

I'd still like an answer to this. It's not clear to me what the purpose of this is, and why you'd want a custom runtime hook for sanitizer traps but not other traps. The only time we emit a trap using -ftrap-function where there is no corresponding sanitizer is for __builtin_trap(); is the intention that you still want a normal trap for that builtin?

[jmgao]

In D12181#229467, @rsmith wrote:

In D12181#229358, @rsmith wrote:

Can you please give a brief description of the motivation for this change? When would it be appropriate to use this rather than -ftrap-function?

I'd still like an answer to this. It's not clear to me what the purpose of this is, and why you'd want a custom runtime hook for sanitizer traps but not other traps. The only time we emit a trap using -ftrap-function where there is no corresponding sanitizer is for __builtin_trap(); is the intention that you still want a normal trap for that builtin?

The goal is to be able to give a useful fsanitize-specific error message ("fsanitize trap occurred"), while not lying and saying this for non-sanitize traps.

[jmgao]

Make option fit in 80 cols

[jmgao]

Doc fix, 80 col

[jmgao]

Make methods private

[filcab]

Looks good, thanks for working on this.
I added some code style comments.

lib/CodeGen/CGExpr.cpp

2398

80 columns

2408–2409

80 columns.

2420–2421

80 columns.

2424–2426

Please run clang-format on lines you change.

lib/CodeGen/CodeGenFunction.h

2891

Don't add \brief if the comment is just one sentence (if you want to remove ones that were there previously, don't do it in the same commit, though).
I would probably remove the first comma, too.

2904

Again, don't add \brief.

lib/Frontend/CompilerInvocation.cpp

684

clang-format here.

[jmgao]

clang-format, remove \brief from modified doxygen comments.

[filcab]

It seems you missed some \brief. Other than that, no complaints on this side.

lib/CodeGen/CGExpr.cpp

2303

Did clang-format do this?

lib/CodeGen/CodeGenFunction.h

2900

There's still a \brief here.

2904

I still see \brief.

[jmgao]

Remove more \briefs

[jmgao]

lib/CodeGen/CGExpr.cpp

2303

Yes

[jmgao]

Ping, I think @samsonov was waiting on @rsmith's feedback on the following:

In D12181#229493, @jmgao wrote:

In D12181#229467, @rsmith wrote:

In D12181#229358, @rsmith wrote:

Can you please give a brief description of the motivation for this change? When would it be appropriate to use this rather than -ftrap-function?

I'd still like an answer to this. It's not clear to me what the purpose of this is, and why you'd want a custom runtime hook for sanitizer traps but not other traps. The only time we emit a trap using -ftrap-function where there is no corresponding sanitizer is for __builtin_trap(); is the intention that you still want a normal trap for that builtin?

The goal is to be able to give a useful fsanitize-specific error message ("fsanitize trap occurred"), while not lying and saying this for non-sanitize traps.

[rsmith]

In D12181#234590, @jmgao wrote:

Ping, I think @samsonov was waiting on @rsmith's feedback on the following:

In D12181#229493, @jmgao wrote:

The goal is to be able to give a useful fsanitize-specific error message ("fsanitize trap occurred"), while not lying and saying this for non-sanitize traps.

My original intent was that users who wanted a different response to sanitizer failures would provide an alternative runtime library (defining the __ubsan_handle_* functions to do whatever they wanted). The trap mechanism was added for users who didn't want the code size increase of generating the calls to a handler function. I worry that we're piling on more and more ways of responding to ubsan failures with no underlying principled design.

Without this patch, you have (at least) two options:

1. Use -fsanitize-trap and -ftrap-function=. You'll get calls on sanitizer failures, and also on explicit calls to __builtin_trap(). If we added a sanitizer for reaching __builtin_trap() -- which doesn't seem completely unreasonable -- these would all be sanitizer failures :)
2. Use your own ubsan runtime library. There are about 20 functions you'd need to define, but you can define them all to be aliases of the same handler function. See ubsan_handlers.h in compiler-rt for a list of the symbols you need to provide.

Given the above, do you think your patch adds enough value to be worth our time to maintain?

[jmgao]

With #1, it seems unfortunate to not be able to distinguish between a sanitize inserted __builtin_trap and code manually calling it. (Would there be an -fsanitize-trap=trap? :-)
With #2, we're worried about the generated code being noticeably worse in the unexceptional case than running without the sanitizers.

Compiling the following snippet with -O3 -fsanitize=unsigned-integer-overflow -fomit-frame-pointer and additional arguments generates:

unsigned foo(unsigned a, unsigned b, unsigned c, unsigned d) { return a + b + c + d; }

no additional arguments

foo:
	push	{r4, r5, r6, r7, r8, lr}
	mov	r5, r2
	mov	r2, r1
	mov	r1, r0
	mov	r0, #1
	mov	r8, r3
	mov	r4, #1
	add	r6, r1, r2
	cmp	r6, r1
	movhs	r0, #0
	cmp	r0, #0
	bne	.LBB0_4
.LBB0_1:
	add	r7, r6, r5
	cmp	r7, r6
	movhs	r4, #0
	cmp	r4, #0
	bne	.LBB0_5
.LBB0_2:
	add	r5, r7, r8
	mov	r0, #1
	cmp	r5, r7
	movhs	r0, #0
	cmp	r0, #0
	bne	.LBB0_6
.LBB0_3:
	mov	r0, r5
	pop	{r4, r5, r6, r7, r8, lr}
	bx	lr
.LBB0_4:
        <overflow handling>

-fsanitize-trap=unsigned-integer-overflow

foo:
	add	r1, r0, r1
	mov	r12, #1
	cmp	r1, r0
	mov	r0, #1
	movhs	r0, #0
	cmp	r0, #0
	bne	.LBB0_3
@ BB#1:
	add	r2, r1, r2
	cmp	r2, r1
	movhs	r12, #0
	cmp	r12, #0
	bne	.LBB0_3
@ BB#2:
	add	r0, r2, r3
	mov	r1, #1
	cmp	r0, r2
	movhs	r1, #0
	cmp	r1, #0
	bxeq	lr
.LBB0_3:
	.long	3892305662              @ trap

-fsanitize-trap=unsigned-integer-overflow -fsanitize-trap-function=sanitize_trap

foo:
	push	{r11, lr}      ; Not quite perfect, but still better
	add	r1, r0, r1
	mov	r12, #1
	cmp	r1, r0
	mov	r0, #1
	movhs	r0, #0
	cmp	r0, #0
	bne	.LBB0_3
@ BB#1:
	add	r2, r1, r2
	cmp	r2, r1
	movhs	r12, #0
	cmp	r12, #0
	bne	.LBB0_3
@ BB#2:
	add	r0, r2, r3
	mov	r1, #1
	cmp	r0, r2
	movhs	r1, #0
	cmp	r1, #0
	popeq	{r11, lr}
	bxeq	lr
.LBB0_3:
	bl	sanitize_trap(PLT)

docs/UsersManual.rst

**-f[no-]sanitize-trap=check1,check2,...**
    This flag is only compatible with ``local-bounds``,
    ``unsigned-integer-overflow``, sanitizers in the ``cfi`` group and
    sanitizers in the ``undefined`` group other than ``vptr``. If this flag
    is supplied together with ``-fsanitize=undefined``, the ``vptr`` sanitizer
    will be implicitly disabled.
 
    This flag is enabled by default for sanitizers in the ``cfi`` group.
 
+**-fsanitize-trap-function=[name]**
+
+   Instruct code generator to emit a function call to the specified
+   function name instead of a trap instruction for sanitizer checks configured
+   to trap.
+
+   If this flag is not specified, clang will use the value of
+   ``-ftrap-function`` if specified.
+
 **-f[no-]sanitize-coverage=[type,features,...]**
 
    Enable simple code coverage in addition to certain sanitizers.
    See :doc:`SanitizerCoverage` for more details.
 
 .. option:: -fsanitize-undefined-trap-on-error
 
    Deprecated alias for ``-fsanitize-trap=undefined``.

include/clang/Driver/Options.td

: CommaJoined<["-"], "fno-sanitize-recover=">,
       Group<f_clang_Group>, Flags<[CoreOption]>,
       HelpText<"Disable recovery for specified sanitizers">;
 def fsanitize_trap_EQ : CommaJoined<["-"], "fsanitize-trap=">, Group<f_clang_Group>,
                         Flags<[CC1Option, CoreOption]>,
                         HelpText<"Enable trapping for specified sanitizers">;
 def fno_sanitize_trap_EQ : CommaJoined<["-"], "fno-sanitize-trap=">, Group<f_clang_Group>,
                            Flags<[CoreOption]>,
                            HelpText<"Disable trapping for specified sanitizers">;
+def fsanitize_trap_function_EQ
+    : Joined<["-"], "fsanitize-trap-function=">,
+      Group<f_clang_Group>, Flags<[CC1Option, CoreOption]>,
+      HelpText<"Make trapping sanitizers issue a call to specified function "
+               "rather than a trap instruction">;
 def fsanitize_undefined_trap_on_error : Flag<["-"], "fsanitize-undefined-trap-on-error">,
                                         Group<f_clang_Group>;
 def fno_sanitize_undefined_trap_on_error : Flag<["-"], "fno-sanitize-undefined-trap-on-error">,
                                            Group<f_clang_Group>;
 def fsanitize_link_cxx_runtime : Flag<["-"], "fsanitize-link-c++-runtime">,
                                  Group<f_clang_Group>;
 def funsafe_math_optimizations : Flag<["-"], "funsafe-math-optimizations">,
   Group<f_Group>;

include/clang/Frontend/CodeGenOptions.h

public:
 
   /// Set of sanitizer checks that are non-fatal (i.e. execution should be
   /// continued when possible).
   SanitizerSet SanitizeRecover;
 
   /// Set of sanitizer checks that trap rather than diagnose.
   SanitizerSet SanitizeTrap;
 
+  /// If not an empty string, trapping sanitizers will issue calls to this
+  /// function instead of trap instructions.
+  std::string SanitizeTrapFuncName;
+
 public:
   // Define accessors/mutators for code generation options of enumeration type.
 #define CODEGENOPT(Name, Bits, Default)
 #define ENUM_CODEGENOPT(Name, Type, Bits, Default) \
   Type get##Name() const { return static_cast<Type>(Name); } \
   void set##Name(Type Value) { Name = static_cast<unsigned>(Value); }
 #include "clang/Frontend/CodeGenOptions.def"
 
   CodeGenOptions();
 };
 
 }  // end namespace clang
 
 #endif

lib/CodeGen/CGExpr.cpp

llvm::Value *&Cond =
         CGM.getCodeGenOpts().SanitizeTrap.has(Checked[i].second)
             ? TrapCond
             : CGM.getCodeGenOpts().SanitizeRecover.has(Checked[i].second)
                   ? RecoverableCond
                   : FatalCond;
     Cond = Cond ? Builder.CreateAnd(Cond, Check) : Check;
   }
 
-  if (TrapCond)
-    EmitTrapCheck(TrapCond);
+  if (TrapCond) EmitSanitizeTrapCheck(TrapCond);
   if (!FatalCond && !RecoverableCond)
     return;
 
   llvm::Value *JointCond;
   if (FatalCond && RecoverableCond)
     JointCond = Builder.CreateAnd(FatalCond, RecoverableCond);
   else
     JointCond = FatalCond ? FatalCond : RecoverableCond;
if (!FatalCond || !RecoverableCond) {
     EmitBlock(NonFatalHandlerBB);
     emitCheckHandlerCall(*this, FnType, Args, CheckName, RecoverKind, false,
                          Cont);
   }
 
   EmitBlock(Cont);
 }
 
+void CodeGenFunction::EmitSanitizeTrapCheck(llvm::Value *Checked) {
+  if (!CGM.getCodeGenOpts().SanitizeTrapFuncName.empty()) {
+    return EmitTrapCheck(Checked, CGM.getCodeGenOpts().SanitizeTrapFuncName);
+  }
+  return EmitTrapCheck(Checked);
+}
+
 void CodeGenFunction::EmitTrapCheck(llvm::Value *Checked) {
+  return EmitTrapCheck(Checked, CGM.getCodeGenOpts().TrapFuncName);
+}
+
+llvm::CallInst *CodeGenFunction::EmitTrapCall(llvm::Intrinsic::ID IntrID) {
+  return EmitTrapCall(IntrID, CGM.getCodeGenOpts().TrapFuncName);
+}
+
+void CodeGenFunction::EmitTrapCheck(llvm::Value *Checked,
+                                    const std::string &TrapFuncName) {
   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) {
     TrapBB = createBasicBlock("trap");
     Builder.CreateCondBr(Checked, Cont, TrapBB);
     EmitBlock(TrapBB);
-    llvm::CallInst *TrapCall = EmitTrapCall(llvm::Intrinsic::trap);
+    llvm::CallInst *TrapCall =
+        EmitTrapCall(llvm::Intrinsic::trap, TrapFuncName);
     TrapCall->setDoesNotReturn();
     TrapCall->setDoesNotThrow();
     Builder.CreateUnreachable();
   } else {
     Builder.CreateCondBr(Checked, Cont, TrapBB);
   }
 
   EmitBlock(Cont);
 }
 
-llvm::CallInst *CodeGenFunction::EmitTrapCall(llvm::Intrinsic::ID IntrID) {
+llvm::CallInst *CodeGenFunction::EmitTrapCall(llvm::Intrinsic::ID IntrID,
+                                              const std::string &TrapFuncName) {
   llvm::CallInst *TrapCall = Builder.CreateCall(CGM.getIntrinsic(IntrID));
 
-  if (!CGM.getCodeGenOpts().TrapFuncName.empty())
-    TrapCall->addAttribute(llvm::AttributeSet::FunctionIndex,
-                           "trap-func-name",
-                           CGM.getCodeGenOpts().TrapFuncName);
+  if (!TrapFuncName.empty())
+    TrapCall->addAttribute(llvm::AttributeSet::FunctionIndex, "trap-func-name",
+                           TrapFuncName);
 
   return TrapCall;
 }
 
 /// isSimpleArrayDecayOperand - If the specified expr is a simple decay from an
 /// array to pointer, return the array subexpression.
 static const Expr *isSimpleArrayDecayOperand(const Expr *E) {
   // If this isn't just an array->pointer decay, bail out.

lib/CodeGen/CodeGenFunction.h

public:
 
   /// \brief Create a basic block that will call a handler function in a
   /// sanitizer runtime with the provided arguments, and create a conditional
   /// branch to it.
   void EmitCheck(ArrayRef<std::pair<llvm::Value *, SanitizerMask>> Checked,
                  StringRef CheckName, ArrayRef<llvm::Constant *> StaticArgs,
                  ArrayRef<llvm::Value *> DynamicArgs);
 
+  /// Create a basic block that will call the trap intrinsic and emit a
+  /// conditional branch to it, for the -fsanitize checks.
+  void EmitSanitizeTrapCheck(llvm::Value *Checked);
+
   /// \brief Create a basic block that will call the trap intrinsic, and emit a
   /// conditional branch to it, for the -ftrapv checks.
   void EmitTrapCheck(llvm::Value *Checked);
 
-  /// \brief Emit a call to trap or debugtrap and attach function attribute
-  /// "trap-func-name" if specified.
+  /// Emit a call to trap or debugtrap and attach function attribute
+  /// "trap-func-name" if -ftrap-function is specified.
   llvm::CallInst *EmitTrapCall(llvm::Intrinsic::ID IntrID);
 
+ private:
+  /// Create a basic block that will call the specified trap function, and emit
+  /// a conditional branch to it.
+  void EmitTrapCheck(llvm::Value *Checked, const std::string &TrapFuncName);
+
+  /// Emit a call to trap or debugtrap and attach function attribute
+  /// "trap-func-name" if nonempty.
+  llvm::CallInst *EmitTrapCall(llvm::Intrinsic::ID IntrID,
+                               const std::string &TrapFuncName);
+
+ public:
   /// \brief Create a check for a function parameter that may potentially be
   /// declared as non-null.
   void EmitNonNullArgCheck(RValue RV, QualType ArgType, SourceLocation ArgLoc,
                            const FunctionDecl *FD, unsigned ParmNum);
 
   /// EmitCallArg - Emit a single call argument.
   void EmitCallArg(CallArgList &args, const Expr *E, QualType ArgType);
 

lib/Frontend/CompilerInvocation.cpp

static bool ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, InputKind IK,
   // Parse -fsanitize-recover= arguments.
   // FIXME: Report unrecoverable sanitizers incorrectly specified here.
   parseSanitizerKinds("-fsanitize-recover=",
                       Args.getAllArgValues(OPT_fsanitize_recover_EQ), Diags,
                       Opts.SanitizeRecover);
   parseSanitizerKinds("-fsanitize-trap=",
                       Args.getAllArgValues(OPT_fsanitize_trap_EQ), Diags,
                       Opts.SanitizeTrap);
+  Opts.SanitizeTrapFuncName =
+      Args.getLastArgValue(OPT_fsanitize_trap_function_EQ);
 
   Opts.CudaGpuBinaryFileNames =
       Args.getAllArgValues(OPT_fcuda_include_gpubinary);
 
   return Success;
 }
 
 static void ParseDependencyOutputArgs(DependencyOutputOptions &Opts,

test/CodeGen/sanitize-trap-function.c

+// RUN: %clang_cc1 -emit-llvm -o - %s -fsanitize=integer-divide-by-zero -fsanitize-trap=integer-divide-by-zero | FileCheck %s -check-prefix=NONE -check-prefix=CHECK
+// RUN: %clang_cc1 -emit-llvm -o - %s -fsanitize=integer-divide-by-zero -fsanitize-trap=integer-divide-by-zero -ftrap-function=foo | FileCheck %s -check-prefix=TRAP -check-prefix=CHECK
+// RUN: %clang_cc1 -emit-llvm -o - %s -fsanitize=integer-divide-by-zero -fsanitize-trap=integer-divide-by-zero -fsanitize-trap-function=bar | FileCheck %s -check-prefix=SANITIZE -check-prefix=CHECK
+// RUN: %clang_cc1 -emit-llvm -o - %s -fsanitize=integer-divide-by-zero -fsanitize-trap=integer-divide-by-zero -ftrap-function=foo -fsanitize-trap-function=bar | FileCheck %s -check-prefix=SANITIZE -check-prefix=CHECK
+
+int f(int x, int y) {
+  // CHECK: call void @llvm.trap() #[[N:[0-9]+]],
+  // CHECK-NEXT: unreachable
+
+  // NONE-NOT: trap-func-name
+  // TRAP: attributes #[[N]] {{.+}} "trap-func-name"="foo"
+  // SANITIZE: attributes #[[N]] {{.+}} "trap-func-name"="bar"
+  return x / y;
+}