Reapply "[Clang][Sema] placement new initializes typedef array with correct size (#83124)" #89036
Conversation
…ze (llvm#83124) When in-place new-ing a local variable of an array of trivial type, the generated code calls 'memset' with the correct size of the array, earlier it was generating size (squared of the typedef array + size). The cause: `typedef TYPE TArray[8]; TArray x;` The type of declarator is Tarray[8] and in `SemaExprCXX.cpp::BuildCXXNew` we check if it's of typedef and of constant size then we get the original type and it works fine for non-dependent cases. But in case of template we do `TreeTransform.h:TransformCXXNEWExpr` and there we again check the allocated type which is TArray[8] and it stays that way, so ArraySize=(Tarray[8] type, alloc Tarray[8*type]) so the squared size allocation. ArraySize gets calculated earlier in `TreeTransform.h` so that `if(!ArraySize)` condition was failing. fix: I changed that condition to `if(ArraySize)`. Fixes llvm#41441
…ze (llvm#88902) Build Failure Fix Fixes build failures due to llvm#83124
This just replaces an '#include<new>' with a declaration of array placement new.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clang Author: None (mahtohappy) ChangesWhen in-place new-ing a local variable of an array of trivial type, the generated code calls 'memset' with the correct size of the array, earlier it was generating size (squared of the typedef array + size). The cause: typedef TYPE TArray[8]; TArray x; The type of declarator is Tarray[8] and in SemaExprCXX.cpp::BuildCXXNew we check if it's of typedef and of constant size then we get the original type and it works fine for non-dependent cases. ArraySize gets calculated earlier in TreeTransform.h so that if(!ArraySize) condition was failing. Full diff: https://github.com/llvm/llvm-project/pull/89036.diff 3 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 96ad92b540b47f..636d76f1836759 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -534,6 +534,8 @@ Bug Fixes to C++ Support
Fixes (#GH70604), (#GH79754), (#GH84163), (#GH84425), (#GH86054), (#GH86398), and (#GH86399).
- Fix a crash when deducing ``auto`` from an invalid dereference (#GH88329).
- Fix a crash in requires expression with templated base class member function. Fixes (#GH84020).
+- placement new initializes typedef array with correct size
+ (`#GH41441 <https://github.com/llvm/llvm-project/issues/41441>`_)
Bug Fixes to AST Handling
^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
index eb05783a6219dc..0c7fdb357235e1 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -12864,6 +12864,19 @@ TreeTransform<Derived>::TransformCXXNewExpr(CXXNewExpr *E) {
ArraySize = NewArraySize.get();
}
+ // Per C++0x [expr.new]p5, the type being constructed may be a
+ // typedef of an array type.
+ QualType AllocType = AllocTypeInfo->getType();
+ if (ArraySize) {
+ if (const ConstantArrayType *Array =
+ SemaRef.Context.getAsConstantArrayType(AllocType)) {
+ ArraySize = IntegerLiteral::Create(SemaRef.Context, Array->getSize(),
+ SemaRef.Context.getSizeType(),
+ E->getBeginLoc());
+ AllocType = Array->getElementType();
+ }
+ }
+
// Transform the placement arguments (if any).
bool ArgumentChanged = false;
SmallVector<Expr*, 8> PlacementArgs;
@@ -12925,7 +12938,6 @@ TreeTransform<Derived>::TransformCXXNewExpr(CXXNewExpr *E) {
return E;
}
- QualType AllocType = AllocTypeInfo->getType();
if (!ArraySize) {
// If no array size was specified, but the new expression was
// instantiated with an array type (e.g., "new T" where T is
diff --git a/clang/test/SemaCXX/PR41441.cpp b/clang/test/SemaCXX/PR41441.cpp
new file mode 100644
index 00000000000000..d0f2917e52f211
--- /dev/null
+++ b/clang/test/SemaCXX/PR41441.cpp
@@ -0,0 +1,23 @@
+// RUN: %clang --target=x86_64-pc-linux -S -fno-discard-value-names -emit-llvm -o - %s | FileCheck %s
+
+namespace std {
+ using size_t = decltype(sizeof(int));
+};
+void* operator new[](std::size_t, void*) noexcept;
+
+// CHECK: call void @llvm.memset.p0.i64(ptr align 1 %x, i8 0, i64 8, i1 false)
+// CHECK: call void @llvm.memset.p0.i64(ptr align 16 %x, i8 0, i64 32, i1 false)
+template <typename TYPE>
+void f()
+{
+ typedef TYPE TArray[8];
+
+ TArray x;
+ new(&x) TArray();
+}
+
+int main()
+{
+ f<char>();
+ f<int>();
+}
|
Fixing test documentation build failure
Sirraide
changed the title
…orrect size fixing the regression test failure
Repro was provided further down: #83124 (comment) Please include test coverage and a fix before approval/recommit. |
|
Yes it's fixed now. |
Please include details of the fix I the patch description |
|
Check is for dependent types and earlier I was not checking that with the condition being only |
| // Per C++0x [expr.new]p5, the type being constructed may be a | ||
| // typedef of an array type. | ||
| QualType AllocType = AllocTypeInfo->getType(); | ||
| if (ArraySize && E->isTypeDependent()) { |
There was a problem hiding this comment.
does Value dependence matter?
There was a problem hiding this comment.
Yes, it does. Both the original testcase from @slackito and simpler version of @dwblaikie are working now.
normal arrays with new initializer also were passing the check and their type was correct earlier so need for modification for them. typedef TYPE TArray[8]; TArray x; here type of array was Tarray which should be Type* so the check only for type dependent case.
There was a problem hiding this comment.
Note I asked about VALUE dependence, not TYPE dependence. I see why type dependence would matter, but expressions can be both type and value dependent.
There was a problem hiding this comment.
No, it doesn't have value dependence.
| @@ -0,0 +1,23 @@ | |||
| // RUN: %clang --target=x86_64-pc-linux -S -fno-discard-value-names -emit-llvm -o - %s | FileCheck %s | |||
|
|
|||
| namespace std { | |||
There was a problem hiding this comment.
Tests STILL haven't changed from the original patch. Please add the regression to this test as well.
|
Hi, the build is failing for windows but there's not test failures and no errors in the log. What should I do from here? |
|
Hi @cor3ntin Please merge this. |
|
Hi @erichkeane Please merge this change. |
|
I think we're seeing some build failures after this patch, and it isn't clear to me that this is a bug in the source, so I'd appreciate it if you could take a look. The type in question is defined as Here's a reproducer from -gen-reproducer=always`, though it isn't reduced. Would you mind taking a look, since this doesn't appear to be a source issue (at least not from my quick reading). |
|
Hi @ilovepi Sure. Looking at this. |
|
@mahtohappy can we please revert this in the meantime while you look for a fix here. Feel free to land it again with the fix. |
He's likely outside of work hours, but feel free to submit a revert. |
|
It looks like the author of the patch have either diverted to other activities or is unable to fix it. Could we, perhaps, just diagnose the situation and gracefully fail with something like "typedefed array blah-blah is not supported in blah-blah" instead of generating segfaulting code? |
The [last attempt](#89036) to fix #41441 has been reverted immediately. Here I'm trying the simplest idea I've been able to come with: skip handling dependent case in `BuildCXXNew`. The original test (borrowed form #89036) passes. Also I've created and added to the tests a minimal repro of the code #89036 fails on. This (obviously) also passes.
The [last attempt](llvm#89036) to fix llvm#41441 has been reverted immediately. Here I'm trying the simplest idea I've been able to come with: skip handling dependent case in `BuildCXXNew`. The original test (borrowed form llvm#89036) passes. Also I've created and added to the tests a minimal repro of the code llvm#89036 fails on. This (obviously) also passes.
When in-place new-ing a local variable of an array of trivial type, the generated code calls 'memset' with the correct size of the array, earlier it was generating size (squared of the typedef array + size).
The cause: typedef TYPE TArray[8]; TArray x; The type of declarator is Tarray[8] and in SemaExprCXX.cpp::BuildCXXNew we check if it's of typedef and of constant size then we get the original type and it works fine for non-dependent cases.
But in case of template we do TreeTransform.h:TransformCXXNEWExpr and there we again check the allocated type which is TArray[8] and it stays that way, so ArraySize=(Tarray[8] type, alloc Tarray[8*type]) so the squared size allocation.
ArraySize gets calculated earlier in TreeTransform.h so that if(!ArraySize) condition was failing.
fix: I changed that condition to if(ArraySize).
fixes #41441