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