[cfe-dev] [PATCH] Integer Sanitizer Initial Patches

[Richard Smith]

--- a/docs/UsersManual.html
+++ b/docs/UsersManual.html
@@ -875,11 +875,12 @@ likely to affect PCH files that reference a
large number of headers.</p>
 <!-- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
- - - - -->
 <dl>
 <dt id="opt_fsanitize"><b>-fsanitize=check1,check2</b>: Turn on runtime checks
-for various forms of undefined behavior.</dt>
+for various forms of undefined or otherwise undesirable behavior.</dt>

I prefer your later use of the word 'suspicious' over 'undesirable',
since unsigned overflow is often desired.


@@ -889,7 +890,10 @@ message is produced at runtime explaining the
problem. The main checks are:
     <a href="ThreadSanitizer.html">ThreadSanitizer</a>, an
<em>experimental</em>
     data race detector.  Not ready for widespread use.</li>
 <li id="opt_fsanitize_undefined"><tt>-fsanitize=undefined</tt>:
-    Enables all the checks listed below.</li>
+    Enables all the checks for undefined behavior.  This includes all of
+    the checks listed below other than unsigned integer overflow.</li>
+<li id="opt_fsanitize_integer"><tt>-fsanitize=integer</tt>:
+    Enables checks for undefined or suspicious integer behavior.</li>

This isn't quite right; -fsanitize=address and -fsanitize=thread both
check for undefined behavior. Possibly something like "Enables a set
of checks for undefined behavior which have no impact on address space
layout or ABI, and small runtime impact."


+<li id="opt_fsanitize_fp-divide-by-zero"><tt>-fsanitize=fp-divide-by-zero</tt>:
+    Floating point division by zero.</li>

I would prefer 'float-divide-by-zero' to match 'float-cast-overflow'.
Also, this list was previously in alphabetical order :-)


@@ -2031,7 +2053,8 @@ Value
*ScalarExprEmitter::EmitOverflowCheckedBinOp(const BinOpInfo &Ops) {
   if (handlerName->empty()) {
     // If the signed-integer-overflow sanitizer is enabled, emit a call to its
     // runtime. Otherwise, this is a -ftrapv check, so just emit a trap.
-    if (CGF.getLangOpts().SanitizeSignedIntegerOverflow)
+    if ((isSigned && CGF.getLangOpts().SanitizeSignedIntegerOverflow) ||
+        CGF.getLangOpts().SanitizeUnsignedIntegerOverflow)
       EmitBinOpCheck(Builder.CreateNot(overflow), Ops);
     else
       CGF.EmitTrapvCheck(Builder.CreateNot(overflow));

It still looks like this will take the EmitBinOpCheck path for
-fsanitize=unsigned-integer-overflow -ftrapv. I think:

  !isSigned || LangOpts.SanitizeSignedIntegerOverflow

... should work fine.

If Eli is happy, this LGTM with the above fixes.

On Fri, Nov 9, 2012 at 4:09 PM, Will Dietz <willdtz at gmail.com> wrote:
> Thank you for your comments.
>
> Updated patches attached!
>
> ~Will
>
> On Thu, Nov 8, 2012 at 5:08 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>> Hi,
>>
>> On Wed, Nov 7, 2012 at 5:15 PM, Will Dietz <willdtz at gmail.com> wrote:
>>> Attached are patches that add a new 'sanitizer' to clang for detecting
>>> and reporting integer overfllows.  Unlike the checks added by
>>> -fcatch-undefined-behavior, these also include non-undefined-behavior
>>> checks.
>>
>> This seems like it could be valuable to me, and I think it's in scope
>> as a -fsanitize= feature (there are some other checks which I'd like
>> to eventually include in -fsanitize=, which check for possible bugs
>> which don't result in undefined behavior). For instance, I could
>> imagine this being something people would turn on when their code is
>> behaving strangely, as part of a "tell me about suspicious things that
>> my program did" mode, but that would depend on making these
>> diagnostics non-fatal (and maybe supporting a suppression system).
>>
>> Assuming we reach consensus that we want this...
>>
>>> The attached clang patch adds:
>>>
>>> -fsanitize=unsigned-integer-overflow
>>> and
>>> -fsanitize=integer
>>>
>>> The first adds support for inserting checks for unsigned integer
>>> overflow, the latter is a new 'sanitizer group' which is used to
>>> enable all integer-related checking.  In the future I'd like to
>>> include value-losing conversions, but for now this includes the
>>> existing checks (signed overflow, divide-by-zero, shifts) as well as
>>> the new unsigned overflow checks.
>>
>> Please split the divide-by-zero check into integer and floating-point
>> cases, and only include the integer case in the -fsanitize=integer
>> group.
>>
>> Please also provide a patch to the user's manual documenting the new arguments.
>>
>>> Also attached is a corresponding patch for compiler-rt that extends
>>> ubsan to include support for reporting unsigned as well as signed
>>> overflows.
>>>
>>> Two issues with this that I'm hoping can be discussed:
>>> * As per PR14247 (http://llvm.org/bugs/show_bug.cgi?id=14247), the
>>> ubsan checks presently aren't recoverable.  This reduces these checks'
>>> utility for quickly getting a new large codebase into shape as
>>> mentioned in that bug, but this is of course even more important to be
>>> made optional when reporting unsigned overflows is enabled as well.
>>
>> I think this is something we should pursue. My only reservation here
>> is a concern about the performance impact of making the checks
>> recoverable, but I have no data there.
>>
>>> * Extending "ubsan" is unfortunate given its name, but these checks
>>> don't seem to merit a separate library either.  Thoughts?
>>
>> I don't think that's a problem. "One Hour Photo" is just the name of
>> the shop, sir. :)
>>
>>
>> Clang patch:
>>
>> --- a/lib/CodeGen/CGExprScalar.cpp
>> +++ b/lib/CodeGen/CGExprScalar.cpp
>> @@ -414,6 +414,11 @@ public:
>>        }
>>      }
>>
>> +    if (Ops.Ty->isUnsignedIntegerType() &&
>> +        CGF.getLangOpts().SanitizeUnsignedIntegerOverflow) {
>> +      return EmitOverflowCheckedBinOp(Ops);
>> +    }
>>
>> No braces here, please (and for this same construct later in the file).
>>
>>
>>    case BO_Mul:
>>    case BO_MulAssign:
>>      OpID = 3;
>>      IID = llvm::Intrinsic::smul_with_overflow;
>> +    IID = isSigned ? llvm::Intrinsic::smul_with_overflow :
>> +                     llvm::Intrinsic::umul_with_overflow;
>>      break;
>>
>> There's a dead store left behind here.
>>
>>
>> @@ -2031,7 +2054,8 @@ Value
>> *ScalarExprEmitter::EmitOverflowCheckedBinOp(const BinOpInfo &Ops) {
>>    if (handlerName->empty()) {
>>      // If the signed-integer-overflow sanitizer is enabled, emit a call to its
>>      // runtime. Otherwise, this is a -ftrapv check, so just emit a trap.
>> -    if (CGF.getLangOpts().SanitizeSignedIntegerOverflow)
>> +    if (CGF.getLangOpts().SanitizeSignedIntegerOverflow ||
>> +        CGF.getLangOpts().SanitizeUnsignedIntegerOverflow)
>>        EmitBinOpCheck(Builder.CreateNot(overflow), Ops);
>>      else
>>        CGF.EmitTrapvCheck(Builder.CreateNot(overflow));
>>
>> This doesn't look right -- building with -ftrapv
>> -fsanitize=unsigned-integer-overflow will use the -fsanitize path for
>> signed overflow, not the -ftrapv path.
>>
>> The Clang patch should include a test -- it's not sufficient for this
>> to be tested just in the compiler-rt tests (they're not run as a part
>> of normal Clang development).
>>
>>
>> compiler-rt patch:
>>
>> --- a/lib/ubsan/ubsan_handlers.cc
>> +++ b/lib/ubsan/ubsan_handlers.cc
>> @@ -55,30 +55,35 @@ void
>> __ubsan::__ubsan_handle_type_mismatch(TypeMismatchData *Data,
>>    Die();
>>  }
>>
>> -/// \brief Common diagnostic emission for various forms of signed overflow.
>> -template<typename T> static void HandleSignedOverflow(OverflowData *Data,
>> +/// \brief Common diagnostic emission for various forms of integer overflow.
>> +template<typename T> static void HandleIntegerOverflow(OverflowData *Data,
>>                                                        ValueHandle LHS,
>>                                                        const char *Operator,
>> -                                                      T RHS) {
>> -  Diag(Data->Loc, "signed integer overflow: "
>> -                  "%0 %1 %2 cannot be represented in type %3")
>> +                                                      T RHS,
>> +                                                      bool isSigned) {
>> +  Diag(Data->Loc, "%0 integer overflow: "
>> +                  "%1 %2 %3 cannot be represented in type %4")
>> +    << (isSigned ? "signed" : "unsigned")
>>
>> Please look at Data->Type.isSignedIntegerTy() in here, rather than
>> passing in an extra flag.