r216675 - 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.
Aaron Ballman
aaron at aaronballman.com
Fri Oct 10 11:59:24 PDT 2014
On Wed, Sep 24, 2014 at 11:04 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> On Fri, Aug 29, 2014 at 11:15 AM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> On Thu, Aug 28, 2014 at 5:34 PM, Richard Smith <richard at metafoo.co.uk>
>> wrote:
>> > On Thu, Aug 28, 2014 at 9:48 AM, Aaron Ballman <aaron at aaronballman.com>
>> > 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: oper_new_v3.patch
Type: application/octet-stream
Size: 26754 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141010/fc5f575b/attachment.obj>
More information about the cfe-commits
mailing list