llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Akash Manna (akash-manna-sky) <details> <summary>Changes</summary> Fixes #<!-- -->128985 An `EmbedExpr` in a semantic initializer list can represent many array elements at once, and every consumer of such lists has to expand it. The array `new` emitter, `EmitNewArrayInitializer`, never did: it counted the `EmbedExpr` as one element and emitted it through the scalar path, hitting `assert(E->getDataElementCount() == 1)`. The undercount also made the runtime minimum allocation check too lax and miscomputed the trailing zero-fill size. `Codegen` now emits one store per embed data element, converted to the element type, and counts initializers with `getNumInitsWithEmbedExpanded()` in both places. That helper also learned to look through implicit casts, since Sema wraps a multi-element `EmbedExpr` in a conversion when the element type isn't `int` — the other embed consumers already strip casts before checking. LLM tools were used for this contribution. I've reviewed, built, and tested the change myself before pushing to GitHub. --- Full diff: https://github.com/llvm/llvm-project/pull/218262.diff 4 Files Affected: - (modified) clang/docs/ReleaseNotes.md (+4) - (modified) clang/include/clang/AST/Expr.h (+1-1) - (modified) clang/lib/CodeGen/CGExprCXX.cpp (+26-8) - (added) clang/test/CodeGenCXX/GH128985.cpp (+89) ``````````diff diff --git a/clang/docs/ReleaseNotes.md b/clang/docs/ReleaseNotes.md index 3c6694f510952..a973b143e6e2e 100644 --- a/clang/docs/ReleaseNotes.md +++ b/clang/docs/ReleaseNotes.md @@ -531,6 +531,10 @@ features cannot lower the translation-unit ABI level; parameter that follows a parameter pack (e.g. `template <typename... T> S::S(T..., int = 10) {}`). (#GH216211) +- Fixed an assertion failure when `#embed` was used in the braced initializer + of an array new-expression; codegen now expands the embedded data into the + individual array elements. (#GH128985) + #### Bug Fixes to AST Handling - Fixed a non-deterministic ordering of unused local typedefs that made diff --git a/clang/include/clang/AST/Expr.h b/clang/include/clang/AST/Expr.h index 72762c668f26a..93b5a8aea6614 100644 --- a/clang/include/clang/AST/Expr.h +++ b/clang/include/clang/AST/Expr.h @@ -5365,7 +5365,7 @@ class InitListExpr : public Expr { unsigned getNumInitsWithEmbedExpanded() const { unsigned Sum = InitExprs.size(); for (auto *IE : InitExprs) - if (auto *EE = dyn_cast<EmbedExpr>(IE)) + if (auto *EE = dyn_cast<EmbedExpr>(cast<Expr>(IE)->IgnoreParenImpCasts())) Sum += EE->getDataElementCount() - 1; return Sum; } diff --git a/clang/lib/CodeGen/CGExprCXX.cpp b/clang/lib/CodeGen/CGExprCXX.cpp index e400a5c5a49c5..39c0e94b488f4 100644 --- a/clang/lib/CodeGen/CGExprCXX.cpp +++ b/clang/lib/CodeGen/CGExprCXX.cpp @@ -1103,7 +1103,8 @@ void CodeGenFunction::EmitNewArrayInitializer( ArrayRef<const Expr *> InitExprs = ILE ? ILE->inits() : CPLIE->getInitExprs(); - InitListElements = InitExprs.size(); + InitListElements = + ILE ? ILE->getNumInitsWithEmbedExpanded() : InitExprs.size(); // If this is a multi-dimensional array new, we will initialize multiple // elements with each init list element. @@ -1138,6 +1139,14 @@ void CodeGenFunction::EmitNewArrayInitializer( CharUnits StartAlign = CurPtr.getAlignment(); unsigned i = 0; + auto AdvanceToNextElement = [&]() { + CurPtr = Address(Builder.CreateInBoundsGEP(CurPtr.getElementType(), + CurPtr.emitRawPointer(*this), + Builder.getSize(1), + "array.exp.next"), + CurPtr.getElementType(), + StartAlign.alignmentAtOffset((++i) * ElementSize)); + }; for (const Expr *IE : InitExprs) { // Tell the cleanup that it needs to destroy up to this // element. TODO: some of these stores can be trivially @@ -1145,17 +1154,25 @@ void CodeGenFunction::EmitNewArrayInitializer( if (EndOfInit.isValid()) { Builder.CreateStore(CurPtr.emitRawPointer(*this), EndOfInit); } + // An EmbedExpr can initialize more than one array element. + if (const auto *EmbedS = dyn_cast<EmbedExpr>(IE->IgnoreParenImpCasts())) { + for (const IntegerLiteral *DataElement : + EmbedS->underlying_data_elements()) { + llvm::Value *Val = EmitScalarConversion( + Builder.getInt(DataElement->getValue()), DataElement->getType(), + ElementType, DataElement->getExprLoc()); + EmitStoreOfScalar(Val, MakeAddrLValue(CurPtr, ElementType), + /*isInit=*/true); + AdvanceToNextElement(); + } + continue; + } // FIXME: If the last initializer is an incomplete initializer list for // an array, and we have an array filler, we can fold together the two // initialization loops. StoreAnyExprIntoOneUnit(*this, IE, IE->getType(), CurPtr, AggValueSlot::DoesNotOverlap); - CurPtr = Address(Builder.CreateInBoundsGEP(CurPtr.getElementType(), - CurPtr.emitRawPointer(*this), - Builder.getSize(1), - "array.exp.next"), - CurPtr.getElementType(), - StartAlign.alignmentAtOffset((++i) * ElementSize)); + AdvanceToNextElement(); } // The remaining elements are filled with the array filler expression. @@ -1591,7 +1608,8 @@ llvm::Value *CodeGenFunction::EmitCXXNewExpr(const CXXNewExpr *E) { cast<ConstantArrayType>(Init->getType()->getAsArrayTypeUnsafe()) ->getZExtSize(); } else if (ILE || CPLIE) { - minElements = ILE ? ILE->getNumInits() : CPLIE->getInitExprs().size(); + minElements = ILE ? ILE->getNumInitsWithEmbedExpanded() + : CPLIE->getInitExprs().size(); } } diff --git a/clang/test/CodeGenCXX/GH128985.cpp b/clang/test/CodeGenCXX/GH128985.cpp new file mode 100644 index 0000000000000..7b703af005687 --- /dev/null +++ b/clang/test/CodeGenCXX/GH128985.cpp @@ -0,0 +1,89 @@ +// RUN: %clang_cc1 %s -triple x86_64 -emit-llvm -o - | FileCheck %s + +// GH128985: #embed in the braced initializer of an array new-expression +// asserted in codegen. +// The first four bytes of this file are '/', '/', ' ', 'R' (47, 47, 32, 82). + +// CHECK-LABEL: define {{.*}}void @_Z2f1i( +// CHECK: icmp ult i64 %{{.*}}, 4 +// CHECK: %[[A1:.*]] = call {{.*}}ptr @_Znam(i64 {{.*}}) +// CHECK: store i32 47, ptr %[[A1]] +// CHECK: %[[F1E1:.*]] = getelementptr inbounds i32, ptr %[[A1]], i64 1 +// CHECK: store i32 47, ptr %[[F1E1]] +// CHECK: %[[F1E2:.*]] = getelementptr inbounds i32, ptr %[[F1E1]], i64 1 +// CHECK: store i32 32, ptr %[[F1E2]] +// CHECK: %[[F1E3:.*]] = getelementptr inbounds i32, ptr %[[F1E2]], i64 1 +// CHECK: store i32 82, ptr %[[F1E3]] +// CHECK: %[[F1REST:.*]] = sub i64 %{{.*}}, 16 +// CHECK: call void @llvm.memset.p0.i64(ptr align 4 %{{.*}}, i8 0, i64 %[[F1REST]], i1 false) +void f1(int x) { + int *p = new int[x]{ +#embed __FILE__ limit(4) + }; +} + +// CHECK-LABEL: define {{.*}}void @_Z2f2i( +// CHECK: icmp ult i64 %{{.*}}, 4 +// CHECK: %[[A2:.*]] = call {{.*}}ptr @_Znam(i64 {{.*}}) +// CHECK: store i32 500, ptr %[[A2]] +// CHECK: %[[F2E1:.*]] = getelementptr inbounds i32, ptr %[[A2]], i64 1 +// CHECK: store i32 47, ptr %[[F2E1]] +// CHECK: %[[F2E2:.*]] = getelementptr inbounds i32, ptr %[[F2E1]], i64 1 +// CHECK: store i32 47, ptr %[[F2E2]] +// CHECK: %[[F2E3:.*]] = getelementptr inbounds i32, ptr %[[F2E2]], i64 1 +// CHECK: store i32 600, ptr %[[F2E3]] +// CHECK: %[[F2REST:.*]] = sub i64 %{{.*}}, 16 +// CHECK: call void @llvm.memset.p0.i64(ptr align 4 %{{.*}}, i8 0, i64 %[[F2REST]], i1 false) +void f2(int x) { + int *p = new int[x]{ + 500, +#embed __FILE__ limit(2) suffix(, 600) + }; +} + +// char arrays are initialized from the embed data via the string literal +// initialization path. +// CHECK-LABEL: define {{.*}}void @_Z2f3i( +// CHECK: icmp ult i64 %{{.*}}, 4 +// CHECK: %[[A3:.*]] = call {{.*}}ptr @_Znam(i64 {{.*}}) +// CHECK: call void @llvm.memcpy.p0.p0.i64(ptr align 1 %[[A3]], ptr align 1 @{{.*}}, i64 4, i1 false) +// CHECK: %[[F3END:.*]] = getelementptr inbounds i8, ptr %[[A3]], i64 4 +// CHECK: %[[F3REST:.*]] = sub i64 %{{.*}}, 4 +// CHECK: call void @llvm.memset.p0.i64(ptr align 1 %[[F3END]], i8 0, i64 %[[F3REST]], i1 false) +void f3(int x) { + char *p = new char[x]{ +#embed __FILE__ limit(4) + }; +} + +// CHECK-LABEL: define {{.*}}void @_Z2f4i( +// CHECK: icmp ult i64 %{{.*}}, 2 +// CHECK: %[[A4:.*]] = call {{.*}}ptr @_Znam(i64 {{.*}}) +// CHECK: store i32 900, ptr %[[A4]] +// CHECK: %[[F4E1:.*]] = getelementptr inbounds i32, ptr %[[A4]], i64 1 +// CHECK: store i32 47, ptr %[[F4E1]] +// CHECK: %[[F4REST:.*]] = sub i64 %{{.*}}, 8 +// CHECK: call void @llvm.memset.p0.i64(ptr align 4 %{{.*}}, i8 0, i64 %[[F4REST]], i1 false) +void f4(int x) { + int *p = new int[x]{ +#embed __FILE__ limit(1) prefix(900, ) + }; +} + +// Constant size fully covered by the embed data: no trailing fill. +// CHECK-LABEL: define {{.*}}void @_Z2f5v( +// CHECK: %[[A5:.*]] = call {{.*}}ptr @_Znam(i64 {{.*}}) +// CHECK: store i32 47, ptr %[[A5]] +// CHECK: %[[F5E1:.*]] = getelementptr inbounds i32, ptr %[[A5]], i64 1 +// CHECK: store i32 47, ptr %[[F5E1]] +// CHECK: %[[F5E2:.*]] = getelementptr inbounds i32, ptr %[[F5E1]], i64 1 +// CHECK: store i32 32, ptr %[[F5E2]] +// CHECK: %[[F5E3:.*]] = getelementptr inbounds i32, ptr %[[F5E2]], i64 1 +// CHECK: store i32 82, ptr %[[F5E3]] +// CHECK-NOT: call void @llvm.memset +// CHECK: ret void +void f5() { + int *p = new int[4]{ +#embed __FILE__ limit(4) + }; +} `````````` </details> https://github.com/llvm/llvm-project/pull/218262 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
