[PATCH] operator new[] generating a std::bad_array_new_length exception
Aaron Ballman
aaron at aaronballman.com
Thu Sep 11 10:59:38 PDT 2014
Pinging this under a new, not-related-to-a-reverted-commit thread.
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?
>
>> 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.
>
>> + 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 --------------
A non-text attachment was scrubbed...
Name: oper_new_v2.patch
Type: application/octet-stream
Size: 21902 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140911/2a4d6782/attachment.obj>
More information about the cfe-commits
mailing list