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

[Chad Rosier]

Thanks, Richard.  Revised patch attached:

-------------- next part --------------
A non-text attachment was scrubbed...
Name: fcatch-undefined-behavior.patch
Type: application/octet-stream
Size: 28625 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130129/e325c5e3/attachment.obj>
-------------- next part --------------


  Chad

On Jan 28, 2013, at 4:24 PM, Richard Smith <richard at metafoo.co.uk> wrote:

> 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'?)