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.

Richard Smith richard at metafoo.co.uk
Fri Oct 24 12:11:51 PDT 2014


On Fri, Oct 10, 2014 at 11:59 AM, Aaron Ballman <aaron at aaronballman.com>
wrote:

> 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).


Hmm, you can't determine the size because of brace elision, you mean? I was
imagining this being a check you would perform after you build the
initializer for the array new; at that point, you should be able to both
determine the bound of the initializer and know whether the bound is a
constant.

>
> >>
> >> >
> >> >> 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.


I don't like this approach; on reflection, I think it would be better to
directly emit IR for the weak definition of __cxa_throw_bad_array_new
rather than trying to fit it into the existing IR emission scheme. (You'll
need to delay until the end of the TU in case you're building the file that
defines that function.)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141024/9268e3a6/attachment.html>


More information about the cfe-commits mailing list