[PATCH] Throw std::bad_array_new_length as expected

[Aaron Ballman]

On Thu, Aug 28, 2014 at 12:43 PM, Reid Kleckner <rnk at google.com> wrote:
> lgtm, thanks!

Thanks! Committed in r216675

~Aaron
>
>
>
> On Thu, Aug 28, 2014 at 6:11 AM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> I think this patch addresses your review comments.
>>
>> Thanks!
>>
>> ~Aaron
>>
>> On Wed, Aug 27, 2014 at 5:46 PM, Aaron Ballman <aaron at aaronballman.com>
>> wrote:
>> > On Wed, Aug 27, 2014 at 5:41 PM, Reid Kleckner <rnk at google.com> wrote:
>> >> -// 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
>> >> %t-1.ll %s
>> >> +// RUN: FileCheck -check-prefix SANE98 --input-file=%t-1.ll %s
>> >> +// RUN: %clang_cc1 -triple i686-pc-linux-gnu -std=c++11 -emit-llvm -o
>> >> %t-2.ll %s
>> >> +// RUN: FileCheck -check-prefix SANE11 --input-file=%t-2.ll %s
>> >> +// RUN: %clang_cc1 -triple i686-pc-win32-msvc -std=c++11 -emit-llvm -o
>> >> %t-3.ll %s
>> >> +// RUN: FileCheck -check-prefix SANE11MS --input-file=%t-3.ll %s
>> >> +// RUN: %clang_cc1 -triple i686-pc-linux-gnu -emit-llvm
>> >> -fno-assume-sane-operator-new -o %t-4.ll %s
>> >> +// RUN: FileCheck -check-prefix SANENOT --input-file=%t-4.ll %s
>> >>
>> >> These RUN lines should use pipes. The less tempfiles we need, the
>> >> better.
>> >
>> > Can do.
>> >
>> >>
>> >> +// 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: br i1 [[OVER]], label %[[BAD:.*]], label
>> >> %[[GOOD:.*]]
>> >> +// SANE11MS: [[BAD]]:
>> >> +// SANE11MS-NEXT: br label %[[GOOD]]
>> >> +// SANE11MS: [[GOOD]]:
>> >> +// SANE11MS-NEXT: call noalias i8* @"\01??_U at YAPAXI@Z"(i32 [[SUM]])
>> >>
>> >> I think we should keep passing -1 on overflow on Windows in C++11,
>> >> especially given that it's our default language mode in clang-cl.
>> >
>> > Yes, good catch! That was unintentionally changed.
>> >
>> >>
>> >> +    if (hasOverflow) {
>> >> +      // In C++11 and later, overflow for a call to operator new[]
>> >> should
>> >> throw
>> >> +      // a std::bad_array_new_length exception.
>> >> +      if (CGF.getLangOpts().CPlusPlus11) {
>> >> +        llvm::BasicBlock *BadArrayNewLengthBlock =
>> >> +          CGF.createBasicBlock("new.bad_array_new_length");
>> >> +        llvm::BasicBlock *EndBlock = CGF.createBasicBlock("new.end");
>> >> +
>> >> +        CGF.Builder.CreateCondBr(hasOverflow, BadArrayNewLengthBlock,
>> >> EndBlock);
>> >> +        CGF.EmitBlock(BadArrayNewLengthBlock);
>> >> +        CGF.CGM.getCXXABI().EmitBadArrayNewLengthCall(CGF);
>> >> +        CGF.EmitBlock(EndBlock);
>> >> +      } else
>> >> +        size = CGF.Builder.CreateSelect(
>> >> +            hasOverflow, llvm::Constant::getAllOnesValue(CGF.SizeTy),
>> >> size);
>> >> +    }
>> >>
>> >> This is semi-duplicated from above. Here's how I think we can do it
>> >> with the
>> >> least duplication. Make the CGCXXABI prototype be something like:
>> >> Value *EmitNewArrayLengthOverflowCheck(CodeGenFunction &CGF, bool
>> >> ConstantOverflow, Value *DynamicOverflow, Value *Size);
>> >>
>> >> Then, provide an implementation in CGCXXABI which implements the old
>> >> behavior of returning -1 if ConstantOverflow is true and emitting a
>> >> select
>> >> i1 on DynamicOverflow with -1.
>> >>
>> >> In ItaniumCXXABI, we can override the method, and in non-C++11 mode,
>> >> delegate back to the base class implementation. In C++11 mode, we can
>> >> emit
>> >> the basic blocks if we need dynamic overflow checks. MicrosoftCXXABI
>> >> doesn't
>> >> need to change at all.
>> >>
>> >> Sound good?
>> >
>> > Sounds good, I will come back with an updated patch.
>> >
>> > Thank you!
>> >
>> > ~Aaron
>
>