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

[Richard Smith]

-    NeedsUbsanRt = Undefined ^ Bounds
+    NeedsUbsanRt = (Undefined ^ Bounds) | Integer

Would you mind changing this to "(Undefined & ~Bounds) | Integer" or
similar, in passing? Otherwise, LGTM.

On Sat, Nov 24, 2012 at 12:16 PM, Will Dietz <willdtz at gmail.com> wrote:

> Ping!
>
> Updated patches attached (previous set conflicted with
> -fsanitize=bounds change).
>
> Thanks!
>
> ~Will
>
> On Mon, Nov 19, 2012 at 2: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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20121126/a5a5a3ad/attachment.html>