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
Wed Sep 24 20:04:39 PDT 2014
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?
> >
> >> 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 at 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 at YAPAXI@Z"(i32 -1)
> >> }
> >> +#endif
> >>
> >>
> >> _______________________________________________
> >> cfe-commits mailing list
> >> cfe-commits at cs.uiuc.edu
> >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >
> >
>
> Thanks!
>
> ~Aaron
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140924/dd75ce63/attachment.html>
More information about the cfe-commits
mailing list