[Clang][Sema] placement new initializes typedef array with correct size #83124
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
|
@llvm/pr-subscribers-backend-x86 @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. Full diff: https://github.com/llvm/llvm-project/pull/83124.diff 2 Files Affected:
diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
index 7389a48fe56fcc..0a46deda194826 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -12731,7 +12731,7 @@ TreeTransform<Derived>::TransformCXXNewExpr(CXXNewExpr *E) {
}
QualType AllocType = AllocTypeInfo->getType();
- if (!ArraySize) {
+ 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
// instantiated with "int[4]"), extract the outer bound from the
diff --git a/clang/test/CodeGen/instantiate-new-placement-size.cpp b/clang/test/CodeGen/instantiate-new-placement-size.cpp
new file mode 100644
index 00000000000000..eaf013a57a6176
--- /dev/null
+++ b/clang/test/CodeGen/instantiate-new-placement-size.cpp
@@ -0,0 +1,19 @@
+// RUN: %clang -S -emit-llvm -o - %s | FileCheck %s
+#include <new>
+
+// 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>();
+}
|
|
It fixes this issue #41441 |
mahtohappy
force-pushed
the
temp-placement-new
branch
2 times, most recently
from
a4c9d01 to
4661010
Compare
|
|
||
| // 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> |
There was a problem hiding this comment.
We need more test cases to make sure we are not breaking other cases. Non-dependent Vs dependent, constant Vs dynamically sized, VLA etc
|
Thank you for this fix. Can you put more details in your summary. The approach I like to take is 1. what the problem is 2. what is the approach of the fix 3. any other important details. |
|
I definitely agree with both of Shafik's comments! The fix itself concerns me, the logic in the block that is having its condition inverted is specifically made for 'if no array size was specified' (see comment), so that makes me think this is an incorrect patch. It doesn't seem to follow the suggestion given in the original bug either. |
|
this is the same case. In |
|
Can you explain which cases the |
|
@shafik I'm not aware of which cases it was catching before, I was hoping testing will reveal those testcases. I agree with your thinking that we might be breaking something and and don't have test coverage for it. I'll leave this condition as it is and add a separate check for this issue. It'll be more robust solution. |
mahtohappy
force-pushed
the
temp-placement-new
branch
3 times, most recently
from
7de2ca7 to
991ca33
Compare
|
Hi @shafik @cor3ntin @AaronBallman @erichkeane @Endilll Please review this. Thank you. |
| @@ -12669,6 +12669,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. | |||
There was a problem hiding this comment.
This comment doesn't really match what is happening below? the 'if' is still based on 'ArraySize' (presumably you mean this to be a proxy for E->isArray?). But there is no standardeeze quoted here about why you'd pick up the size/alloc type from it like this, rather than in the E->isArray above.
Also, does this also handle cases where this is going to still be dependent?
There was a problem hiding this comment.
Yes, you're right. It's proxy for E->isArray(), but the condition just before checks if it's Array and then calculates the ArraySize. I didn't want to check again and used the ArraySize calculated before. If the earlier condition E->isArray() fails then ArraySize won't be calculated and if(ArraySize) would also fail. In Typedef case Tarray x; the alloctype is Tarray for x, that's why using Tarray Element Type which is int, and using that as allocType. Yes, this is for dependent cases only.
There was a problem hiding this comment.
So it seems this should probably be inside of that E->isArray() block instead then?
There was a problem hiding this comment.
Yes, it can be inside that block, but in the non-dependent it was outside the block to check for typedef so I followed the same thing here as well. It's in Sema::SemaExprCXX::BuildNEWCXX.
|
Hi Please Review this. @Endilll @cor3ntin @AaronBallman |
|
Can you:
thanks! |
mahtohappy
force-pushed
the
temp-placement-new
branch
from
991ca33 to
517134e
Compare
|
@mahtohappy the test you added seems to be failing on quite a few buildbots including the documentation build and some x86 ones:
|
|
hello, instantiate-new-placement-size.cpp also fails here : |
|
#88902 pushed a fix. Will update after build is done. |
|
Please either revert or get the fix committed. This test has been failing on bots for around 6 hours at this point. |
|
I merged the fix, it looks good enough, and I trust @mahtohappy that it passed locally. I DID have 1 comment on the test. It is unfortunate that our infrastructure CI takes so long, this was likely to have to wait another ~4-5 hrs for Windows CI. |
This just replaces an '#include<new>' with a declaration of array placement new.
|
Unless the original test was subtly broken, the forward fix in 0a789ea seems erroneous. The forward fix changes the test to have a different declaration of We are experiencing errors that look like this: |
|
The problem is still present. I have reverted this chain of commits. |
…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.
That forward-fix was just a test change, doing a #include in our tests isn't something we can do, as it makes them fail with "file not found". As far as your errors, it shouldn't be because of those, but could definitely be a problem wiht the originnal patch, hopefully @mahtohappy can get a chance to look at this. |
|
@Sterling-Augustine please provide a reproducer script for your errors. |
|
The following program fails to build after this change: you get similar errors with both libstdc++: and libc++ |
|
Simplified a bit more: https://godbolt.org/z/MhaYbvefG - yeah seems pretty clear this is a regression. Compiles with GCC, fails with clang, and the non-dependent context passes with either. |
…orrect size (#83124)" (#89036) 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 --------- Co-authored-by: erichkeane <ekeane@nvidia.com>
|
This commit is still problematic. Minimal reproducer (https://godbolt.org/z/hE7M8EfT1). We should get this reverted again. |
We get error messages like above with this commit. This compiles fine without this commit or with gcc. |
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 inSemaExprCXX.cpp::BuildCXXNewwe 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:TransformCXXNEWExprand 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.hso thatif(!ArraySize)condition was failing.fix: I changed that condition to
if(ArraySize).fixes #41441