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

[Will Dietz]

Glad this is going in, although I would prefer to see compiler-rt
become more widely used (shipped by default, etc) instead.  However,
that's not the case yet and it's good to make these checks available
to users that either don't want to or can't use it (kernel work, no
compiler-rt readily available, etc).

One thought: is there anything to be done to better clarify the
different roles played by -f[no-]sanitize-recover and
-f[no]-sanitize-undefined-trap-on-error? This seems like it could be
awfully confusing.  Would -fsanitize-undefined-standalone or similar
fit this and better identify the reasons one might use the
-fsanitize-undefined-trap-on-error flag?

~Will

On Mon, Jan 28, 2013 at 6: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'?)
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits