On Wed, Sep 24, 2014 at 11:04 PM, Richard Smith <[email protected]> wrote: > 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?
It was required to support all of the use cases we came up with -- we couldn't capture all of it in the new-expression handling because we don't have all of the information available at that time (such as the initializer list). > >> >> > >> >> 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). I made a very sad attempt at modeling after that, however, this is not production-ready. Mostly, I am looking to see if I'm even close to being on the right track. The things I struggled with and am mostly wondering about are: * I need to gin up a FunctionDecl in order to defer the emission of the weak definition. Is that a reasonable approach? * The ginned-up function should not be mangled, and the way I am handling that is horrible. However, I'm not certain of a better approach. I was thinking about using C language linkage, but that requires being able to safely modify the declaration context. Suggestions obviously welcome. Thanks! ~Aaron
oper_new_v3.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
