<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Aug 29, 2014 at 11:15 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"><span class="">On Thu, Aug 28, 2014 at 5:34 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
> On Thu, Aug 28, 2014 at 9:48 AM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
> wrote:<br>
>><br>
>> Author: aaronballman<br>
>> Date: Thu Aug 28 11:48:44 2014<br>
>> New Revision: 216675<br>
>><br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=216675&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=216675&view=rev</a><br>
>> Log:<br>
>> Throw a std::bad_array_new_length exception when the expression (or<br>
>> constant-expression) passed to operator new[] results in overflow in<br>
>> conformance with [expr.new]p7. Fixes PR11644.<br>
><br>
><br>
> This is certainly an improvement, but we're still not implementing<br>
> [expr.new]p7 correctly.<br>
<br>
</span>Thank you for the feedback! I've attached another patch which<br>
rectifies some of what you've expressed here.<br>
<br>
You're correct -- "in conformance with" was premature. ;-)<br>
<span class=""><br>
> Here are some cases we get wrong:<br>
><br>
> // We convert this to -1 rather than throwing.<br>
> int k = 0x80000000; char *p = new char[k];<br>
<br>
</span>From what I can tell, we handle this properly. That code emits:<br>
<br>
; Function Attrs: nounwind<br>
define void @_Z2f3v() #0 {<br>
entry:<br>
%k = alloca i32, align 4<br>
%p = alloca i8*, align 4<br>
store i32 -<a href="tel:2147483648" value="+12147483648">2147483648</a>, i32* %k, align 4<br>
%0 = load i32* %k, align 4<br>
%1 = icmp slt i32 %0, 0<br>
br i1 %1, label %new.bad_array_new_length, label %new.end<br>
<br>
new.bad_array_new_length: ; preds = %entry<br>
call void @__cxa_throw_bad_array_new_length() #2<br>
unreachable<br>
<br>
new.end: ; preds = %entry<br>
%call = call noalias i8* @_Znaj(i32 %0) #3<br>
store i8* %call, i8** %p, align 4<br>
ret void<br>
<span class="">}<br>
<br>
><br>
> // We convert these to -1 rather than rejecting with an error.<br>
> char *p = new char[-1];<br>
> char *q = new char[1]{1, 2};<br>
<br>
</span>I've fixed both of those in the attached patch.<br>
<span class="">><br>
> // We reject this at compile time even though it is valid.<br>
> int k = 10; char *p = new char[k]{"foobar"};<br>
<br>
</span>I've fixed this as well.<br>
<br>
We needed a bit of extra information within the InitializedEntity for<br>
an array new so that we could have the potential array size<br>
information available when checking the initializer list. Do you think<br>
my approach makes sense?</blockquote><div><br></div><div>My first thought was that this check should live in the new-expression handling rather than in initialization. Is there a reason you need to put it there?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
><br>
>> Modified:<br>
>> cfe/trunk/lib/CodeGen/CGCXXABI.cpp<br>
>> cfe/trunk/lib/CodeGen/CGCXXABI.h<br>
>> cfe/trunk/lib/CodeGen/CGExprCXX.cpp<br>
>> cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp<br>
>> cfe/trunk/test/CodeGenCXX/new-array-init.cpp<br>
>> cfe/trunk/test/CodeGenCXX/operator-new.cpp<br>
>><br>
>> Modified: cfe/trunk/lib/CodeGen/CGCXXABI.cpp<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGCXXABI.cpp?rev=216675&r1=216674&r2=216675&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGCXXABI.cpp?rev=216675&r1=216674&r2=216675&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- cfe/trunk/lib/CodeGen/CGCXXABI.cpp (original)<br>
>> +++ cfe/trunk/lib/CodeGen/CGCXXABI.cpp Thu Aug 28 11:48:44 2014<br>
>> @@ -325,3 +325,12 @@ LValue CGCXXABI::EmitThreadLocalVarDeclL<br>
>> bool CGCXXABI::NeedsVTTParameter(GlobalDecl GD) {<br>
>> return false;<br>
>> }<br>
>> +<br>
>> +llvm::Value *CGCXXABI::EmitNewArrayLengthOverflowCheck(<br>
>> + CodeGenFunction &CGF, bool ConstantOverflow, llvm::Value<br>
>> *DynamicOverflow,<br>
>> + llvm::Value *Size) {<br>
>> + llvm::Value *AllOnes = llvm::Constant::getAllOnesValue(CGF.SizeTy);<br>
>> + if (ConstantOverflow)<br>
>> + return AllOnes;<br>
>> + return CGF.Builder.CreateSelect(DynamicOverflow, AllOnes, Size);<br>
>> +}<br>
>><br>
>> Modified: cfe/trunk/lib/CodeGen/CGCXXABI.h<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGCXXABI.h?rev=216675&r1=216674&r2=216675&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGCXXABI.h?rev=216675&r1=216674&r2=216675&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- cfe/trunk/lib/CodeGen/CGCXXABI.h (original)<br>
>> +++ cfe/trunk/lib/CodeGen/CGCXXABI.h Thu Aug 28 11:48:44 2014<br>
>> @@ -236,6 +236,13 @@ public:<br>
>><br>
>> virtual bool EmitBadCastCall(CodeGenFunction &CGF) = 0;<br>
>><br>
>> + /// Emit the code required to throw a std::bad_array_new_length<br>
>> exception by<br>
>> + /// the ABI, and returns the array length size to allocate.<br>
>> + virtual llvm::Value *<br>
>> + EmitNewArrayLengthOverflowCheck(CodeGenFunction &CGF, bool<br>
>> ConstantOverflow,<br>
>> + llvm::Value *DynamicOverflow,<br>
>> + llvm::Value *Size);<br>
>> +<br>
>> virtual llvm::Value *GetVirtualBaseClassOffset(CodeGenFunction &CGF,<br>
>> llvm::Value *This,<br>
>> const CXXRecordDecl<br>
>> *ClassDecl,<br>
>><br>
>> Modified: cfe/trunk/lib/CodeGen/CGExprCXX.cpp<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExprCXX.cpp?rev=216675&r1=216674&r2=216675&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExprCXX.cpp?rev=216675&r1=216674&r2=216675&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- cfe/trunk/lib/CodeGen/CGExprCXX.cpp (original)<br>
>> +++ cfe/trunk/lib/CodeGen/CGExprCXX.cpp Thu Aug 28 11:48:44 2014<br>
>> @@ -572,11 +572,11 @@ static llvm::Value *EmitCXXNewAllocSize(<br>
>> }<br>
>><br>
>> // On overflow, produce a -1 so operator new will fail.<br>
>> - if (hasAnyOverflow) {<br>
>> - size = llvm::Constant::getAllOnesValue(CGF.SizeTy);<br>
>> - } else {<br>
>> + if (hasAnyOverflow)<br>
>> + size = CGF.CGM.getCXXABI().EmitNewArrayLengthOverflowCheck(<br>
>> + CGF, true, nullptr,<br>
>> llvm::Constant::getAllOnesValue(CGF.SizeTy));<br>
>> + else<br>
>> size = llvm::ConstantInt::get(CGF.SizeTy, allocationSize);<br>
>> - }<br>
>><br>
>> // Otherwise, we might need to use the overflow intrinsics.<br>
>> } else {<br>
>> @@ -714,9 +714,9 @@ static llvm::Value *EmitCXXNewAllocSize(<br>
>> // overwrite 'size' with an all-ones value, which should cause<br>
>> // operator new to throw.<br>
>> if (hasOverflow)<br>
>> - size = CGF.Builder.CreateSelect(hasOverflow,<br>
>> -<br>
>> llvm::Constant::getAllOnesValue(CGF.SizeTy),<br>
>> - size);<br>
>> + size = CGF.CGM.getCXXABI().EmitNewArrayLengthOverflowCheck(CGF,<br>
>> false,<br>
>> +<br>
>> hasOverflow,<br>
>> + size);<br>
>> }<br>
>><br>
>> if (cookieSize == 0)<br>
>><br>
>> Modified: cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp?rev=216675&r1=216674&r2=216675&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp?rev=216675&r1=216674&r2=216675&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp (original)<br>
>> +++ cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp Thu Aug 28 11:48:44 2014<br>
>> @@ -133,6 +133,11 @@ public:<br>
>><br>
>> bool EmitBadCastCall(CodeGenFunction &CGF) override;<br>
>><br>
>> + llvm::Value *EmitNewArrayLengthOverflowCheck(CodeGenFunction &CGF,<br>
>> + bool ConstantOverflow,<br>
>> + llvm::Value<br>
>> *DynamicOverflow,<br>
>> + llvm::Value *Size)<br>
>> override;<br>
>> +<br>
>> llvm::Value *<br>
>> GetVirtualBaseClassOffset(CodeGenFunction &CGF, llvm::Value *This,<br>
>> const CXXRecordDecl *ClassDecl,<br>
>> @@ -1044,6 +1049,36 @@ bool ItaniumCXXABI::EmitBadCastCall(Code<br>
>> return true;<br>
>> }<br>
>><br>
>> +llvm::Value *ItaniumCXXABI::EmitNewArrayLengthOverflowCheck(<br>
>> + CodeGenFunction &CGF, bool ConstantOverflow, llvm::Value<br>
>> *DynamicOverflow,<br>
>> + llvm::Value *Size) {<br>
>> + // In C++11 and later, an overflow results in throwing<br>
>> + // std::bad_array_new_length.<br>
>> + if (!CGF.getLangOpts().CPlusPlus11)<br>
>> + return CGCXXABI::EmitNewArrayLengthOverflowCheck(CGF,<br>
>> ConstantOverflow,<br>
>> + DynamicOverflow,<br>
>> Size);<br>
>> +<br>
>> + llvm::BasicBlock *BadArrayNewLengthBlock =<br>
>> + CGF.createBasicBlock("new.bad_array_new_length");<br>
>> + llvm::BasicBlock *EndBlock = CGF.createBasicBlock("new.end");<br>
>> +<br>
>> + if (!ConstantOverflow) {<br>
>> + assert(DynamicOverflow && "Called for dynamic case, but no overflow<br>
>> value");<br>
>> + CGF.Builder.CreateCondBr(DynamicOverflow, BadArrayNewLengthBlock,<br>
>> EndBlock);<br>
>> + }<br>
>> + CGF.EmitBlock(BadArrayNewLengthBlock);<br>
>> +<br>
>> + // void __cxa_bad_array_new_length();<br>
>> + llvm::FunctionType *FTy = llvm::FunctionType::get(CGF.VoidTy, false);<br>
>> + llvm::Value *Fn =<br>
>> + CGF.CGM.CreateRuntimeFunction(FTy, "__cxa_bad_array_new_length");<br>
><br>
><br>
> This is incorrect; see<br>
> <a href="http://mentorembedded.github.io/cxx-abi/abi.html#array-ctor" target="_blank">http://mentorembedded.github.io/cxx-abi/abi.html#array-ctor</a><br>
><br>
> The function is named __cxa_throw_bad_array_new_length<br>
<br>
</div></div>I found that out a little later; it was originally named<br>
__cxa_bad_array_new_length, but the "throw" part was added later.<br>
Somehow I had a stale doc. Thank you for pointing it out though!<br>
<span class=""><br>
> Have you thought about how we might support this when our C++ runtime<br>
> library doesn't contain this symbol? I expect that will be the case for a<br>
> lot of systems. We could emit a weak definition of this function that calls<br>
> _Zna[lm](-1) then calls std::terminate, for instance.<br>
<br>
</span>That's exactly why this patch was reverted. However, I don't really<br>
understand the approach you are suggesting. Could you explain it a bit<br>
further, or do you have an example somewhere I could learn from or<br>
model after? This is not something I've addressed with the attached<br>
patch, but need to figure out before the patch can be committed.</blockquote><div><br></div><div>We do something similar for the C++14 sized delete operator (we emit a weak definition in each TU that uses it, and the weak definition just forwards to the non-sized delete operator).</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
><br>
>> + CGF.EmitRuntimeCallOrInvoke(Fn).setDoesNotReturn();<br>
>> + CGF.Builder.CreateUnreachable();<br>
>> +<br>
>> + CGF.EmitBlock(EndBlock);<br>
>> + return Size;<br>
>> +}<br>
>> +<br>
>> llvm::Value *<br>
>> ItaniumCXXABI::GetVirtualBaseClassOffset(CodeGenFunction &CGF,<br>
>> llvm::Value *This,<br>
>><br>
>> Modified: cfe/trunk/test/CodeGenCXX/new-array-init.cpp<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/new-array-init.cpp?rev=216675&r1=216674&r2=216675&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/new-array-init.cpp?rev=216675&r1=216674&r2=216675&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- cfe/trunk/test/CodeGenCXX/new-array-init.cpp (original)<br>
>> +++ cfe/trunk/test/CodeGenCXX/new-array-init.cpp Thu Aug 28 11:48:44 2014<br>
>> @@ -14,7 +14,12 @@ void fn(int n) {<br>
>> // CHECK-LABEL: define void @_Z15const_underflowv<br>
>> void const_underflow() {<br>
>> // CHECK-NOT: icmp ult i{{32|64}} %{{[^ ]+}}, 3<br>
>> - // CHECK: call noalias i8* @_Zna{{.}}(i{{32|64}} -1)<br>
>> + // CHECK: br label %[[BAD:.*]]<br>
>> + // CHECK: [[BAD]]:<br>
>> + // CHECK-NEXT: call void @__cxa_bad_array_new_length()<br>
>> + // CHECK-NEXT: unreachable<br>
>> + // CHECK: {{.*}}:<br>
>> + // CHECK: ret void<br>
>> new int[2] { 1, 2, 3 };<br>
>> }<br>
>><br>
>><br>
>> Modified: cfe/trunk/test/CodeGenCXX/operator-new.cpp<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/operator-new.cpp?rev=216675&r1=216674&r2=216675&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/operator-new.cpp?rev=216675&r1=216674&r2=216675&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- cfe/trunk/test/CodeGenCXX/operator-new.cpp (original)<br>
>> +++ cfe/trunk/test/CodeGenCXX/operator-new.cpp Thu Aug 28 11:48:44 2014<br>
>> @@ -1,8 +1,7 @@<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>
>> -<br>
>> +// RUN: %clang_cc1 -triple i686-pc-linux-gnu -std=c++98 -emit-llvm -o -<br>
>> %s | FileCheck -check-prefix SANE98 %s<br>
>> +// RUN: %clang_cc1 -triple i686-pc-linux-gnu -std=c++11 -emit-llvm -o -<br>
>> %s | FileCheck -check-prefix SANE11 %s<br>
>> +// RUN: %clang_cc1 -triple i686-pc-win32-msvc -std=c++11 -emit-llvm -o -<br>
>> %s | FileCheck -check-prefix SANE11MS %s<br>
>> +// RUN: %clang_cc1 -triple i686-pc-linux-gnu -emit-llvm<br>
>> -fno-assume-sane-operator-new -o - %s | FileCheck -check-prefix SANENOT %s<br>
>><br>
>> class teste {<br>
>> int A;<br>
>> @@ -20,10 +19,44 @@ void f1() {<br>
>> // rdar://5739832 - operator new should check for overflow in multiply.<br>
>> void *f2(long N) {<br>
>> return new int[N];<br>
>> -<br>
>> -// SANE: [[UWO:%.*]] = call {{.*}} @llvm.umul.with.overflow<br>
>> -// SANE-NEXT: [[OVER:%.*]] = extractvalue {{.*}} [[UWO]], 1<br>
>> -// SANE-NEXT: [[SUM:%.*]] = extractvalue {{.*}} [[UWO]], 0<br>
>> -// SANE-NEXT: [[RESULT:%.*]] = select i1 [[OVER]], i32 -1, i32 [[SUM]]<br>
>> -// SANE-NEXT: call noalias i8* @_Znaj(i32 [[RESULT]])<br>
>> +<br>
>> +// SANE98: [[UWO:%.*]] = call {{.*}} @llvm.umul.with.overflow<br>
>> +// SANE98-NEXT: [[OVER:%.*]] = extractvalue {{.*}} [[UWO]], 1<br>
>> +// SANE98-NEXT: [[SUM:%.*]] = extractvalue {{.*}} [[UWO]], 0<br>
>> +// SANE98-NEXT: [[RESULT:%.*]] = select i1 [[OVER]], i32 -1, i32 [[SUM]]<br>
>> +// SANE98-NEXT: call noalias i8* @_Znaj(i32 [[RESULT]])<br>
>> +<br>
>> +// SANE11: [[UWO:%.*]] = call {{.*}} @llvm.umul.with.overflow<br>
>> +// SANE11-NEXT: [[OVER:%.*]] = extractvalue {{.*}} [[UWO]], 1<br>
>> +// SANE11-NEXT: [[SUM:%.*]] = extractvalue {{.*}} [[UWO]], 0<br>
>> +// SANE11-NEXT: br i1 [[OVER]], label %[[BAD:.*]], label %[[GOOD:.*]]<br>
>> +// SANE11: [[BAD]]:<br>
>> +// SANE11-NEXT: call void @__cxa_bad_array_new_length()<br>
>> +// SANE11-NEXT: unreachable<br>
>> +// SANE11: [[GOOD]]:<br>
>> +// SANE11-NEXT: call noalias i8* @_Znaj(i32 [[SUM]])<br>
>> +<br>
>> +// FIXME: There should be a call to generate the<br>
>> 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: [[RESULT:%.*]] = select i1 [[OVER]], i32 -1, i32<br>
>> [[SUM]]<br>
>> +// SANE11MS-NEXT: call noalias i8* @"\01??_U@YAPAXI@Z"(i32 [[RESULT]])<br>
>> +}<br>
>> +<br>
>> +#if __cplusplus > 199711L<br>
>> +void *f3() {<br>
>> + return new int[0x7FFFFFFF];<br>
>> +// SANE11: br label %[[BAD:.*]]<br>
>> +// SANE11: [[BAD]]:<br>
>> +// SANE11-NEXT: call void @__cxa_bad_array_new_length()<br>
>> +// SANE11-NEXT: unreachable<br>
>> +// SANE11: {{.*}}:<br>
>> +// SANE11-NEXT: call noalias i8* @_Znaj(i32 -1)<br>
>> +<br>
>> +// FIXME: There should be a call to generate the<br>
>> std::bad_array_new_length<br>
>> +// exception in the Microsoft ABI, however, this is not implemented<br>
>> currently.<br>
>> +// SANE11MS: call noalias i8* @"\01??_U@YAPAXI@Z"(i32 -1)<br>
>> }<br>
>> +#endif<br>
>><br>
>><br>
>> _______________________________________________<br>
>> cfe-commits mailing list<br>
>> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
><br>
><br>
<br>
</div></div>Thanks!<br>
<span class="HOEnZb"><font color="#888888"><br>
~Aaron<br>
</font></span></blockquote></div><br></div></div>