<div dir="ltr">lgtm, thanks!<div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Aug 28, 2014 at 6:11 AM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I think this patch addresses your review comments.<br>
<br>
Thanks!<br>
<span class="HOEnZb"><font color="#888888"><br>
~Aaron<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
On Wed, Aug 27, 2014 at 5:46 PM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>> wrote:<br>
> On Wed, Aug 27, 2014 at 5:41 PM, Reid Kleckner <<a href="mailto:rnk@google.com">rnk@google.com</a>> wrote:<br>
>> -// RUN: %clang_cc1 -triple i686-pc-linux-gnu -emit-llvm -o %t-1.ll %s<br>
>> -// RUN: FileCheck -check-prefix SANE --input-file=%t-1.ll %s<br>
>> -// RUN: %clang_cc1 -triple i686-pc-linux-gnu -emit-llvm<br>
>> -fno-assume-sane-operator-new -o %t-2.ll %s<br>
>> -// RUN: FileCheck -check-prefix SANENOT --input-file=%t-2.ll %s<br>
>> +// RUN: %clang_cc1 -triple i686-pc-linux-gnu -std=c++98 -emit-llvm -o<br>
>> %t-1.ll %s<br>
>> +// RUN: FileCheck -check-prefix SANE98 --input-file=%t-1.ll %s<br>
>> +// RUN: %clang_cc1 -triple i686-pc-linux-gnu -std=c++11 -emit-llvm -o<br>
>> %t-2.ll %s<br>
>> +// RUN: FileCheck -check-prefix SANE11 --input-file=%t-2.ll %s<br>
>> +// RUN: %clang_cc1 -triple i686-pc-win32-msvc -std=c++11 -emit-llvm -o<br>
>> %t-3.ll %s<br>
>> +// RUN: FileCheck -check-prefix SANE11MS --input-file=%t-3.ll %s<br>
>> +// RUN: %clang_cc1 -triple i686-pc-linux-gnu -emit-llvm<br>
>> -fno-assume-sane-operator-new -o %t-4.ll %s<br>
>> +// RUN: FileCheck -check-prefix SANENOT --input-file=%t-4.ll %s<br>
>><br>
>> These RUN lines should use pipes. The less tempfiles we need, the better.<br>
><br>
> Can do.<br>
><br>
>><br>
>> +// FIXME: There should be a call to generate the std::bad_array_new_length<br>
>> +// exception in the Microsoft ABI, however, this is not implemented<br>
>> currently.<br>
>> +// SANE11MS:      [[UWO:%.*]] = call {{.*}} @llvm.umul.with.overflow<br>
>> +// SANE11MS-NEXT: [[OVER:%.*]] = extractvalue {{.*}} [[UWO]], 1<br>
>> +// SANE11MS-NEXT: [[SUM:%.*]] = extractvalue {{.*}} [[UWO]], 0<br>
>> +// SANE11MS-NEXT: br i1 [[OVER]], label %[[BAD:.*]], label %[[GOOD:.*]]<br>
>> +// SANE11MS: [[BAD]]:<br>
>> +// SANE11MS-NEXT: br label %[[GOOD]]<br>
>> +// SANE11MS: [[GOOD]]:<br>
>> +// SANE11MS-NEXT: call noalias i8* @"\01??_U@YAPAXI@Z"(i32 [[SUM]])<br>
>><br>
>> I think we should keep passing -1 on overflow on Windows in C++11,<br>
>> especially given that it's our default language mode in clang-cl.<br>
><br>
> Yes, good catch! That was unintentionally changed.<br>
><br>
>><br>
>> +    if (hasOverflow) {<br>
>> +      // In C++11 and later, overflow for a call to operator new[] should<br>
>> throw<br>
>> +      // a std::bad_array_new_length exception.<br>
>> +      if (CGF.getLangOpts().CPlusPlus11) {<br>
>> +        llvm::BasicBlock *BadArrayNewLengthBlock =<br>
>> +          CGF.createBasicBlock("new.bad_array_new_length");<br>
>> +        llvm::BasicBlock *EndBlock = CGF.createBasicBlock("new.end");<br>
>> +<br>
>> +        CGF.Builder.CreateCondBr(hasOverflow, BadArrayNewLengthBlock,<br>
>> EndBlock);<br>
>> +        CGF.EmitBlock(BadArrayNewLengthBlock);<br>
>> +        CGF.CGM.getCXXABI().EmitBadArrayNewLengthCall(CGF);<br>
>> +        CGF.EmitBlock(EndBlock);<br>
>> +      } else<br>
>> +        size = CGF.Builder.CreateSelect(<br>
>> +            hasOverflow, llvm::Constant::getAllOnesValue(CGF.SizeTy),<br>
>> size);<br>
>> +    }<br>
>><br>
>> This is semi-duplicated from above. Here's how I think we can do it with the<br>
>> least duplication. Make the CGCXXABI prototype be something like:<br>
>> Value *EmitNewArrayLengthOverflowCheck(CodeGenFunction &CGF, bool<br>
>> ConstantOverflow, Value *DynamicOverflow, Value *Size);<br>
>><br>
>> Then, provide an implementation in CGCXXABI which implements the old<br>
>> behavior of returning -1 if ConstantOverflow is true and emitting a select<br>
>> i1 on DynamicOverflow with -1.<br>
>><br>
>> In ItaniumCXXABI, we can override the method, and in non-C++11 mode,<br>
>> delegate back to the base class implementation. In C++11 mode, we can emit<br>
>> the basic blocks if we need dynamic overflow checks. MicrosoftCXXABI doesn't<br>
>> need to change at all.<br>
>><br>
>> Sound good?<br>
><br>
> Sounds good, I will come back with an updated patch.<br>
><br>
> Thank you!<br>
><br>
> ~Aaron<br>
</div></div></blockquote></div><br></div>