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

[Richard Smith]

On Mon, Jan 28, 2013 at 5:01 PM, Will Dietz <wdietz2 at uiuc.edu> wrote:
> 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?

I like the synergy between -fsanitize=undefined-trap and
-fsanitize-undefined-trap-on-error, but if you can suggest an equally
harmonious pair of option names which better explain the purpose,
that'd be great!

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