Skip to content

Commit 729480c

Browse files
andykaylorandykaylor
authored
[CIR] Generalize cxx alloc new size handling (#187790)
The non-constant size handling in `emitCXXNewAllocSize` was making the incorrect assumption that the default behavior of the size value being explicitly cast to size_t would be the only behavior we'd see. This is actually only true with C++14 and later. To properly handle earlier standards, we need the more robust checking that classic codegen does in the equivalent function. This change adds that handling. Assisted-by: Cursor / claude-4.6-opus-high
1 parent 89503bd commit 729480c

File tree

5 files changed

+224
-22
lines changed

5 files changed

+224
-22
lines changed

‎clang/lib/CIR/CodeGen/CIRGenExprCXX.cpp‎

Lines changed: 78 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -534,39 +534,97 @@ static mlir::Value emitCXXNewAllocSize(CIRGenFunction &cgf, const CXXNewExpr *e,
534534
// Create a value for the variable number of elements
535535
numElements = cgf.emitScalarExpr(*e->getArraySize());
536536
auto numElementsType = mlir::cast<cir::IntType>(numElements.getType());
537-
[[maybe_unused]] unsigned numElementsWidth = numElementsType.getWidth();
538-
539-
// We might need check for overflow.
540-
541-
mlir::Value hasOverflow;
542-
// Classic codegen checks for the size variable being signed, having a
543-
// smaller width than size_t, and having a larger width than size_t.
544-
// However, the AST implicitly casts the size variable to size_t so none of
545-
// these conditions will ever be met.
546-
assert(
547-
!(*e->getArraySize())->getType()->isSignedIntegerOrEnumerationType() &&
548-
(numElementsWidth == sizeWidth) &&
549-
(numElements.getType() == cgf.sizeTy) &&
550-
"Expected array size to be implicitly cast to size_t!");
551-
552-
// There are up to three conditions we need to test for:
553-
// 1) if minElements > 0, we need to check whether numElements is smaller
537+
unsigned numElementsWidth = numElementsType.getWidth();
538+
539+
// The number of elements can have an arbitrary integer type;
540+
// essentially, we need to multiply it by a constant factor, add a
541+
// cookie size, and verify that the result is representable as a
542+
// size_t. That's just a gloss, though, and it's wrong in one
543+
// important way: if the count is negative, it's an error even if
544+
// the cookie size would bring the total size >= 0.
545+
bool isSigned =
546+
(*e->getArraySize())->getType()->isSignedIntegerOrEnumerationType();
547+
548+
// There are up to five conditions we need to test for:
549+
// 1) if isSigned, we need to check whether numElements is negative;
550+
// 2) if numElementsWidth > sizeWidth, we need to check whether
551+
// numElements is larger than something representable in size_t;
552+
// 3) if minElements > 0, we need to check whether numElements is smaller
554553
// than that.
555-
// 2) we need to compute
554+
// 4) we need to compute
556555
// sizeWithoutCookie := numElements * typeSizeMultiplier
557556
// and check whether it overflows; and
558-
// 3) if we need a cookie, we need to compute
557+
// 5) if we need a cookie, we need to compute
559558
// size := sizeWithoutCookie + cookieSize
560559
// and check whether it overflows.
561560

561+
mlir::Value hasOverflow;
562+
563+
// If numElementsWidth > sizeWidth, then one way or another, we're
564+
// going to have to do a comparison for (2), and this happens to
565+
// take care of (1), too.
566+
if (numElementsWidth > sizeWidth) {
567+
llvm::APInt threshold =
568+
llvm::APInt::getOneBitSet(numElementsWidth, sizeWidth);
569+
570+
// Use an unsigned comparison regardless of the sign of numElements.
571+
mlir::Value unsignedNumElements = numElements;
572+
if (isSigned)
573+
unsignedNumElements = cgf.getBuilder().createIntCast(
574+
numElements, cgf.getBuilder().getUIntNTy(numElementsWidth));
575+
576+
mlir::Value thresholdV =
577+
cgf.getBuilder().getConstInt(loc, threshold, /*isUnsigned=*/true);
578+
hasOverflow = cgf.getBuilder().createCompare(
579+
loc, cir::CmpOpKind::ge, unsignedNumElements, thresholdV);
580+
numElements = cgf.getBuilder().createIntCast(
581+
unsignedNumElements, mlir::cast<cir::IntType>(cgf.sizeTy));
582+
583+
// Otherwise, if we're signed, we want to sext up to size_t.
584+
} else if (isSigned) {
585+
if (numElementsWidth < sizeWidth)
586+
numElements = cgf.getBuilder().createIntCast(
587+
numElements, cgf.getBuilder().getSIntNTy(sizeWidth));
588+
589+
// If there's a non-1 type size multiplier, then we can do the
590+
// signedness check at the same time as we do the multiply
591+
// because a negative number times anything will cause an
592+
// unsigned overflow. Otherwise, we have to do it here. But at
593+
// least in this case, we can subsume the >= minElements check.
594+
if (typeSizeMultiplier == 1)
595+
hasOverflow = cgf.getBuilder().createCompare(
596+
loc, cir::CmpOpKind::lt, numElements,
597+
cgf.getBuilder().getConstInt(loc, numElements.getType(),
598+
minElements));
599+
600+
numElements = cgf.getBuilder().createIntCast(
601+
numElements, mlir::cast<cir::IntType>(cgf.sizeTy));
602+
603+
// Otherwise, zext up to size_t if necessary.
604+
} else if (numElementsWidth < sizeWidth) {
605+
numElements = cgf.getBuilder().createIntCast(
606+
numElements, mlir::cast<cir::IntType>(cgf.sizeTy));
607+
}
608+
609+
assert(numElements.getType() == cgf.sizeTy);
610+
562611
if (minElements) {
563612
// Don't allow allocation of fewer elements than we have initializers.
564613
if (!hasOverflow) {
565-
// FIXME: Avoid creating this twice. It may happen above.
566614
mlir::Value minElementsV = cgf.getBuilder().getConstInt(
567615
loc, llvm::APInt(sizeWidth, minElements));
568616
hasOverflow = cgf.getBuilder().createCompare(loc, cir::CmpOpKind::lt,
569617
numElements, minElementsV);
618+
} else if (numElementsWidth > sizeWidth) {
619+
// The other existing overflow subsumes this check.
620+
// We do an unsigned comparison, since any signed value < -1 is
621+
// taken care of either above or below.
622+
mlir::Value minElementsV = cgf.getBuilder().getConstInt(
623+
loc, llvm::APInt(sizeWidth, minElements));
624+
hasOverflow = cgf.getBuilder().createOr(
625+
loc, hasOverflow,
626+
cgf.getBuilder().createCompare(loc, cir::CmpOpKind::lt, numElements,
627+
minElementsV));
570628
}
571629
}
572630

‎clang/lib/CIR/CodeGen/CIRGenFunction.cpp‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -344,13 +344,23 @@ void CIRGenFunction::LexicalScope::cleanup() {
344344
return;
345345

346346
// Get rid of any empty block at the end of the scope.
347-
bool entryBlock = builder.getInsertionBlock()->isEntryBlock();
348-
if (!entryBlock && curBlock->empty()) {
347+
bool isEntryBlock = builder.getInsertionBlock()->isEntryBlock();
348+
if (!isEntryBlock && curBlock->empty()) {
349349
curBlock->erase();
350350
for (mlir::Block *retBlock : retBlocks) {
351351
if (retBlock->getUses().empty())
352352
retBlock->erase();
353353
}
354+
// The empty block was created by a terminator (return/break/continue)
355+
// and is now erased. If there are pending cleanup scopes (from variables
356+
// with destructors), we need to pop them and ensure the containing scope
357+
// block gets a proper terminator (e.g. cir.yield). Without this, the
358+
// cleanup-scope-op popping that would otherwise happen in
359+
// ~RunCleanupsScope leaves the scope block without a terminator.
360+
if (hasPendingCleanups()) {
361+
builder.setInsertionPointToEnd(entryBlock);
362+
insertCleanupAndLeave(entryBlock);
363+
}
354364
return;
355365
}
356366

‎clang/lib/CIR/CodeGen/CIRGenFunction.h‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1037,6 +1037,12 @@ class CIRGenFunction : public CIRGenTypeCache {
10371037
performCleanup = false;
10381038
cgf.currentCleanupStackDepth = oldCleanupStackDepth;
10391039
}
1040+
1041+
/// Whether there are any pending cleanups that have been pushed since
1042+
/// this scope was entered.
1043+
bool hasPendingCleanups() const {
1044+
return cgf.ehStack.stable_begin() != cleanupStackDepth;
1045+
}
10401046
};
10411047

10421048
// Cleanup stack depth of the RunCleanupsScope that was pushed most recently.

‎clang/test/CIR/CodeGen/cleanup-scope-return-in-loop.cpp‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fclangir -emit-cir %s -o %t.cir
2+
// RUN: FileCheck --input-file=%t.cir %s -check-prefix=CIR
3+
// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fclangir -emit-llvm %s -o %t.ll
4+
// RUN: FileCheck --input-file=%t.ll %s -check-prefix=LLVM
5+
// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm %s -o %t.ll
6+
// RUN: FileCheck --input-file=%t.ll %s -check-prefix=LLVM
7+
8+
struct Struk {
9+
~Struk();
10+
bool check();
11+
};
12+
13+
// Test that a for-loop body containing a local variable with a destructor
14+
// followed by 'continue' and 'return' produces a properly terminated
15+
// cir.scope. This exercises the interaction between LexicalScope cleanup,
16+
// CleanupScopeOp popping, and empty-block erasure.
17+
18+
int test_cleanup_return_in_loop(int n) {
19+
for (int i = 0; i < n; i++) {
20+
Struk s;
21+
if (s.check())
22+
continue;
23+
return i;
24+
}
25+
return -1;
26+
}
27+
28+
// CIR: cir.func {{.*}} @_Z27test_cleanup_return_in_loopi
29+
// CIR: cir.scope {
30+
// CIR: cir.for : cond {
31+
// CIR: } body {
32+
// CIR: cir.scope {
33+
// CIR: cir.cleanup.scope {
34+
// CIR: cir.continue
35+
// CIR: cir.return
36+
// CIR: } cleanup normal {
37+
// CIR: cir.call @_ZN5StrukD1Ev
38+
// CIR: cir.yield
39+
// CIR: cir.yield
40+
// CIR: } step {
41+
// CIR: cir.return
42+
43+
// LLVM: define dso_local noundef i32 @_Z27test_cleanup_return_in_loopi(i32 noundef %{{.*}})
44+
// LLVM: [[LOOP_COND:.*]]:
45+
// LLVM: %[[ICMP:.*]] = icmp slt i32
46+
// LLVM-NEXT: br i1 %[[ICMP]]
47+
// LLVM: [[LOOP_BODY:.*]]:
48+
// LLVM: %[[CALL:.*]] = call noundef {{.*}} @_ZN5Struk5checkEv(
49+
// LLVM-NEXT: br i1 %[[CALL]]
50+
// LLVM: [[CLEANUP:.*]]:
51+
// LLVM: call void @_ZN5StrukD1Ev(
52+
// LLVM: switch i32 %{{.*}}, label %{{.*}} [
53+
// LLVM: ]
54+
// LLVM: [[LOOP_STEP:.*]]:
55+
// LLVM: add nsw i32 %{{.*}}, 1
56+
// LLVM: [[POST_LOOP:.*]]:
57+
// LLVM: store i32 -1, ptr %{{.*}}, align 4
58+
// LLVM: ret i32

‎clang/test/CIR/CodeGen/new-array-size-conv.cpp‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
// RUN: %clang_cc1 -std=c++11 -triple x86_64-unknown-linux-gnu -fclangir -emit-cir %s -o %t.cir
2+
// RUN: FileCheck --check-prefix=CIR --input-file=%t.cir %s
3+
// RUN: %clang_cc1 -std=c++11 -triple x86_64-unknown-linux-gnu -fclangir -emit-llvm %s -o %t-cir.ll
4+
// RUN: FileCheck --check-prefix=LLVM --input-file=%t-cir.ll %s
5+
// RUN: %clang_cc1 -std=c++11 -triple x86_64-unknown-linux-gnu -emit-llvm %s -o %t.ll
6+
// RUN: FileCheck --check-prefix=OGCG --input-file=%t.ll %s
7+
8+
// Tests for array new expressions where the array size is not implicitly
9+
// converted to size_t by Sema (pre-C++14 behavior). The CIR codegen must
10+
// handle the signed/width conversion itself.
11+
12+
typedef __typeof__(sizeof(int)) size_t;
13+
14+
// Sized non-allocating (placement) new.
15+
void *operator new[](size_t, void *p) noexcept { return p; }
16+
17+
struct S {
18+
int x;
19+
S();
20+
};
21+
22+
// Signed int array size with multi-byte element (typeSizeMultiplier != 1).
23+
// The sign extension is done and the multiply overflow catches negative values.
24+
void t_new_signed_size(int n) {
25+
auto p = new double[n];
26+
}
27+
28+
// CIR-LABEL: cir.func {{.*}} @_Z17t_new_signed_sizei
29+
// CIR: %[[N:.*]] = cir.load{{.*}} %[[ARG_ALLOCA:.*]]
30+
// CIR: %[[N_SEXT:.*]] = cir.cast integral %[[N]] : !s32i -> !s64i
31+
// CIR: %[[N_SIZE_T:.*]] = cir.cast integral %[[N_SEXT]] : !s64i -> !u64i
32+
// CIR: %[[ELEMENT_SIZE:.*]] = cir.const #cir.int<8> : !u64i
33+
// CIR: %[[RESULT:.*]], %[[OVERFLOW:.*]] = cir.mul.overflow %[[N_SIZE_T]], %[[ELEMENT_SIZE]] : !u64i -> !u64i
34+
// CIR: %[[ALL_ONES:.*]] = cir.const #cir.int<18446744073709551615> : !u64i
35+
// CIR: %[[ALLOC_SIZE:.*]] = cir.select if %[[OVERFLOW]] then %[[ALL_ONES]] else %[[RESULT]]
36+
37+
// LLVM-LABEL: define{{.*}} void @_Z17t_new_signed_sizei
38+
// LLVM: %[[N:.*]] = load i32, ptr %{{.+}}
39+
// LLVM: %[[N_SIZE_T:.*]] = sext i32 %[[N]] to i64
40+
// LLVM: %[[MUL_OVERFLOW:.*]] = call { i64, i1 } @llvm.umul.with.overflow.i64(i64 %[[N_SIZE_T]], i64 8)
41+
42+
// OGCG-LABEL: define{{.*}} void @_Z17t_new_signed_sizei
43+
// OGCG: %[[N:.*]] = load i32, ptr %{{.+}}
44+
// OGCG: %[[N_SIZE_T:.*]] = sext i32 %[[N]] to i64
45+
// OGCG: %[[MUL_OVERFLOW:.*]] = call { i64, i1 } @llvm.umul.with.overflow.i64(i64 %[[N_SIZE_T]], i64 8)
46+
47+
// Signed int array size with single-byte element (typeSizeMultiplier == 1).
48+
// A signed comparison catches negative values directly.
49+
void t_new_signed_size_char(int n) {
50+
auto p = new char[n];
51+
}
52+
53+
// CIR-LABEL: cir.func {{.*}} @_Z22t_new_signed_size_chari
54+
// CIR: %[[N:.*]] = cir.load{{.*}} %[[ARG_ALLOCA:.*]]
55+
// CIR: %[[N_SEXT:.*]] = cir.cast integral %[[N]] : !s32i -> !s64i
56+
// CIR: %[[ZERO:.*]] = cir.const #cir.int<0> : !s64i
57+
// CIR: %[[IS_NEG:.*]] = cir.cmp lt %[[N_SEXT]], %[[ZERO]] : !s64i
58+
// CIR: %[[N_SIZE_T:.*]] = cir.cast integral %[[N_SEXT]] : !s64i -> !u64i
59+
// CIR: %[[ALL_ONES:.*]] = cir.const #cir.int<18446744073709551615> : !u64i
60+
// CIR: %[[ALLOC_SIZE:.*]] = cir.select if %[[IS_NEG]] then %[[ALL_ONES]] else %[[N_SIZE_T]]
61+
62+
// LLVM-LABEL: define{{.*}} void @_Z22t_new_signed_size_chari
63+
// LLVM: %[[N:.*]] = load i32, ptr %{{.+}}
64+
// LLVM: %[[N_SIZE_T:.*]] = sext i32 %[[N]] to i64
65+
// LLVM: %[[IS_NEG:.*]] = icmp slt i64 %[[N_SIZE_T]], 0
66+
67+
// OGCG-LABEL: define{{.*}} void @_Z22t_new_signed_size_chari
68+
// OGCG: %[[N:.*]] = load i32, ptr %{{.+}}
69+
// OGCG: %[[N_SIZE_T:.*]] = sext i32 %[[N]] to i64
70+
// OGCG: %[[IS_NEG:.*]] = icmp slt i64 %[[N_SIZE_T]], 0

0 commit comments

Comments
 (0)