[PATCH] -fcatch-undefined-behavior with trapping implementation

[Richard Smith]

On Mon, Jan 28, 2013 at 3:58 PM, Chad Rosier <mcrosier at apple.com> wrote:
> All,
> The attached patch implements the -fcatch-undefined-behavior flag using a trapping implementation; this is much more inline with the original implementation (i.e., pre-Ubsan) and does not require run-time library support.
>
> I've added a the 'undefined-trap' SANITIZER_GROUP that includes all the sanitizers which have low overhead, no ABI or address space layout implications, only catch undefined behavior, *and* do not require run-time library support.  This group should be used in conjunction with the new -fsanitize-undefined-trap-on-error flag.  Thus, the trapping implementation can be invoked using either '-fcatch-undefined-behavior' or '-fsanitize=undefined-trap -fsanitize-undefined-trap-on-error'.  Per a discussion between Richard, Chandler, and I, we are going to continue deprecating the -fcatch-undefined-behavior flag, but don't expect the flag to be removed until after a few releases.
>
> Please take a look.
>
> Thanks to Richard for a great deal of feedback prior to my submission to the list.

Patch file seems to have DOS line endings?


+// RUN: %clang -target x86_64-linux-gnu -fcatch-undefined-behavior
-fno-sanitize-undefined-trap-on-error %s -### 2>&1 | FileCheck %s
--check-prefix=CHECK-UNDEFINED-NO-TRAP-ERROR
+// CHECK-UNDEFINED-NO-TRAP-ERROR: '-fcatch-undefined-behavior' not
allowed with '-fno-sanitize-undefined-trap-on-error'

OK, so -fcatch-undefined-behavior isn't exactly equivalent to
-fsanitize=undefined-trap -fsanitize-undefined-trap-on-error? (Its
-fsanitize= effect can be reversed by later command-line options, but
its -fsanitize-undefined-trap-on-error effect cannot be.) That's
probably fine, since we don't have a -fno-catch-undefined-behavior,
but might be a little surprising.


+++ include/clang/Basic/Sanitizers.def	(working copy)
@@ -74,15 +74,24 @@
 // IntegerSanitizer
 SANITIZER("unsigned-integer-overflow", UnsignedIntegerOverflow)

-// -fsanitize=undefined (and its alias -fcatch-undefined-behavior). This should
-// include all the sanitizers which have low overhead, no ABI or address space
-// layout implications, and only catch undefined behavior.
+// -fsanitize=undefined include all the sanitizers which have low overhead, no
+// ABI or address space layout implications, and only catch undefined behavior.

Typo, "-fsanitize=undefined include*s* all [...]"


+++ lib/Driver/Tools.cpp	(working copy)
[...]
+  if (UbsanTrapOnError) {
+    // Warn about undefined sanitizer options that require runtime support.
+    if (Kind & Vptr) {
+      if (Args.hasArg(options::OPT_fcatch_undefined_behavior))
+        D.Diag(diag::err_drv_argument_not_allowed_with)
+          << "-fsanitize=undefined|vptr" << "-fcatch-undefined-behavior";
+      if (Args.hasFlag(options::OPT_fsanitize_undefined_trap_on_error,

else if?

+
options::OPT_fno_sanitize_undefined_trap_on_error, false))
+        D.Diag(diag::err_drv_argument_not_allowed_with)
+          << "-fsanitize=undefined|vptr" <<
"-fsanitize-undefined-trap-on-error";
+    }
+  }

Use SanitizerArgs::lastArgumentForKind(..., Vptr) to find which
argument to print instead of using "-fsanitize=undefined|vptr" here.
Also, please add a named symbolic constant for things which are
incompatible with -fsanitize-undefined-trap-on-error, rather than
hardcoding Vptr.


+++ lib/CodeGen/CGExpr.cpp	(working copy)
@@ -1975,6 +1975,13 @@
                                 ArrayRef<llvm::Value *> DynamicArgs,
                                 CheckRecoverableKind RecoverKind) {
   assert(SanOpts != &SanitizerOptions::Disabled);
+
+  if (CGM.getCodeGenOpts().SanitizeUndefinedTrapOnError) {
+    assert (RecoverKind != CRK_AlwaysRecoverable &&
+            "Runtime call required for AlwaysRecoverable kind!");
+    return EmitTrapvCheck(Checked);

Please rename this to something more general (maybe just remove the 'v'?)