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