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

[Will Dietz]

Committed as r168700 and r168701, thanks for all the feedback and review! :)

~Will

On Tue, Nov 27, 2012 at 12:29 AM, Eli Friedman <eli.friedman at gmail.com> wrote:
> Sorry about the late reply.  Yes, I think my concerns have been
> addressed; thanks.
>
> -Eli
>
> On Mon, Nov 19, 2012 at 12:38 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>> Eli: have your concerns been suitably addressed here?
>>
>> 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.