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

[Will Dietz]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-unsigned-integer-overflow-sanitizer.patch
Type: application/octet-stream
Size: 27004 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20121109/51906556/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-support-for-unsigned-overflow-diagnostic-reporti.patch
Type: application/octet-stream
Size: 10163 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20121109/51906556/attachment-0001.obj>