[clang][p2719] Module deserialization does not restore allocator flags #137102
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
When serializing and deserializing a FunctionDecl we don't recover whether or not the decl was a type aware allocator or destroying delete, because in the final PR that information was placed in a side table in ASTContext. In principle it should be possible to re-do the semantic checks to determine what these flags should be when deserializing, but it seems like the most robust path is simply recording the flags directly in the serialized AST.
ojhunt
force-pushed
the
users/ojhunt/module-deserialize-destroying-and-type-aware-allocators
branch
from
fdb5037 to
dec6509
Compare
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-clang-modules Author: Oliver Hunt (ojhunt) ChangesWhen serializing and deserializing a FunctionDecl we don't recover whether or not the decl was a type aware allocator or destroying delete, because in the final PR that information was placed in a side table in ASTContext. In principle it should be possible to re-do the semantic checks to determine what these flags should be when deserializing, but it seems like the most robust path is simply recording the flags directly in the serialized AST. Full diff: https://github.com/llvm/llvm-project/pull/137102.diff 7 Files Affected:
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index 5545cbc8d608c..0f54aa5c5e062 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -1076,6 +1076,8 @@ void ASTDeclReader::VisitFunctionDecl(FunctionDecl *FD) {
FD->setFriendConstraintRefersToEnclosingTemplate(
FunctionDeclBits.getNextBit());
FD->setUsesSEHTry(FunctionDeclBits.getNextBit());
+ FD->setIsDestroyingOperatorDelete(FunctionDeclBits.getNextBit());
+ FD->setIsTypeAwareOperatorNewOrDelete(FunctionDeclBits.getNextBit());
FD->EndRangeLoc = readSourceLocation();
if (FD->isExplicitlyDefaulted())
diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp
index 3a7a23481ea98..d1f92cea4dfea 100644
--- a/clang/lib/Serialization/ASTWriterDecl.cpp
+++ b/clang/lib/Serialization/ASTWriterDecl.cpp
@@ -847,6 +847,8 @@ void ASTDeclWriter::VisitFunctionDecl(FunctionDecl *D) {
FunctionDeclBits.addBit(D->isInstantiatedFromMemberTemplate());
FunctionDeclBits.addBit(D->FriendConstraintRefersToEnclosingTemplate());
FunctionDeclBits.addBit(D->usesSEHTry());
+ FunctionDeclBits.addBit(D->isDestroyingOperatorDelete());
+ FunctionDeclBits.addBit(D->isTypeAwareOperatorNewOrDelete());
Record.push_back(FunctionDeclBits);
Record.AddSourceLocation(D->getEndLoc());
diff --git a/clang/test/Modules/Inputs/PR137102/module.modulemap b/clang/test/Modules/Inputs/PR137102/module.modulemap
new file mode 100644
index 0000000000000..337aff5821e7f
--- /dev/null
+++ b/clang/test/Modules/Inputs/PR137102/module.modulemap
@@ -0,0 +1 @@
+module type_aware_destroying_new_delete { header "type_aware_destroying_new_delete.h" export * }
diff --git a/clang/test/Modules/Inputs/PR137102/type_aware_destroying_new_delete.h b/clang/test/Modules/Inputs/PR137102/type_aware_destroying_new_delete.h
new file mode 100644
index 0000000000000..f96a9ea0c8a41
--- /dev/null
+++ b/clang/test/Modules/Inputs/PR137102/type_aware_destroying_new_delete.h
@@ -0,0 +1,52 @@
+
+namespace std {
+ struct destroying_delete_t { };
+ template <class T> struct type_identity {
+ using type = T;
+ };
+ typedef __SIZE_TYPE__ size_t;
+ enum class align_val_t : size_t;
+};
+
+struct A {
+ A();
+ void *operator new(std::size_t);
+ void operator delete(A*, std::destroying_delete_t);
+};
+
+struct B {
+ B();
+ void *operator new(std::type_identity<B>, std::size_t, std::align_val_t);
+ void operator delete(std::type_identity<B>, void*, std::size_t, std::align_val_t);
+};
+
+struct C {
+ C();
+ template <class T> void *operator new(std::type_identity<T>, std::size_t, std::align_val_t);
+ template <class T> void operator delete(std::type_identity<T>, void*, std::size_t, std::align_val_t);
+};
+
+struct D {
+ D();
+};
+void *operator new(std::type_identity<D>, std::size_t, std::align_val_t);
+void operator delete(std::type_identity<D>, void*, std::size_t, std::align_val_t);
+
+struct E {
+ E();
+};
+template <class T> void *operator new(std::type_identity<T>, std::size_t, std::align_val_t);
+template <class T> void operator delete(std::type_identity<T>, void*, std::size_t, std::align_val_t);
+
+void in_module_tests() {
+ A* a = new A;
+ delete a;
+ B *b = new B;
+ delete b;
+ C *c = new C;
+ delete c;
+ D *d = new D;
+ delete d;
+ E *e = new E;
+ delete e;
+}
diff --git a/clang/test/Modules/type-aware-destroying-new-and-delete-modules.cpp b/clang/test/Modules/type-aware-destroying-new-and-delete-modules.cpp
new file mode 100644
index 0000000000000..e88f8a8791147
--- /dev/null
+++ b/clang/test/Modules/type-aware-destroying-new-and-delete-modules.cpp
@@ -0,0 +1,23 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -x c++ -std=c++26 -fmodules-cache-path=%t -I %S/Inputs/PR137102 -emit-llvm-only %s
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -x c++ -std=c++26 -fmodules-cache-path=%t -I %S/Inputs/PR137102 -emit-llvm-only %s -triple i686-windows
+
+#include "type_aware_destroying_new_delete.h"
+
+
+static void call_in_module_function(void) {
+ in_module_tests();
+}
+
+void out_of_module_tests() {
+ A* a = new A;
+ delete a;
+ B *b = new B;
+ delete b;
+ C *c = new C;
+ delete c;
+ D *d = new D;
+ delete d;
+ E *e = new E;
+ delete e;
+}
diff --git a/clang/test/PCH/Inputs/type_aware_destroying_new_delete.h b/clang/test/PCH/Inputs/type_aware_destroying_new_delete.h
new file mode 100644
index 0000000000000..42d609c0f5c26
--- /dev/null
+++ b/clang/test/PCH/Inputs/type_aware_destroying_new_delete.h
@@ -0,0 +1,52 @@
+
+namespace std {
+ struct destroying_delete_t { };
+ template <class T> struct type_identity {
+ using type = T;
+ };
+ typedef __SIZE_TYPE__ size_t;
+ enum class align_val_t : size_t;
+};
+
+struct A {
+ A();
+ void *operator new(std::size_t);
+ void operator delete(A*, std::destroying_delete_t);
+};
+
+struct B {
+ B();
+ void *operator new(std::type_identity<B>, std::size_t, std::align_val_t);
+ void operator delete(std::type_identity<B>, void*, std::size_t, std::align_val_t);
+};
+
+struct C {
+ C();
+ template <class T> void *operator new(std::type_identity<T>, std::size_t, std::align_val_t);
+ template <class T> void operator delete(std::type_identity<T>, void*, std::size_t, std::align_val_t);
+};
+
+struct D {
+ D();
+};
+void *operator new(std::type_identity<D>, std::size_t, std::align_val_t);
+void operator delete(std::type_identity<D>, void*, std::size_t, std::align_val_t);
+
+struct E {
+ E();
+};
+template <class T> void *operator new(std::type_identity<T>, std::size_t, std::align_val_t);
+template <class T> void operator delete(std::type_identity<T>, void*, std::size_t, std::align_val_t);
+
+void in_pch_tests() {
+ A* a = new A;
+ delete a;
+ B *b = new B;
+ delete b;
+ C *c = new C;
+ delete c;
+ D *d = new D;
+ delete d;
+ E *e = new E;
+ delete e;
+}
diff --git a/clang/test/PCH/type-aware-destroying-new-and-delete-pch.cpp b/clang/test/PCH/type-aware-destroying-new-and-delete-pch.cpp
new file mode 100644
index 0000000000000..d8f7f5dd50c78
--- /dev/null
+++ b/clang/test/PCH/type-aware-destroying-new-and-delete-pch.cpp
@@ -0,0 +1,27 @@
+// Test this without pch.
+// RUN: %clang_cc1 -x c++ -std=c++26 -include %S/Inputs/type_aware_destroying_new_delete.h -emit-llvm -o - %s
+
+// Test with pch.
+// RUN: %clang_cc1 -x c++ -std=c++26 -emit-pch -o %t %S/Inputs/type_aware_destroying_new_delete.h
+// RUN: %clang_cc1 -x c++ -std=c++26 -include-pch %t -emit-llvm -o - %s
+
+// RUN: %clang_cc1 -x c++ -std=c++11 -emit-pch -fpch-instantiate-templates -o %t %S/Inputs/type_aware_destroying_new_delete.h
+// RUN: %clang_cc1 -x c++ -std=c++11 -include-pch %t -emit-llvm -o - %s
+
+
+static void call_in_pch_function(void) {
+ in_pch_tests();
+}
+
+void out_of_pch_tests() {
+ A* a = new A;
+ delete a;
+ B *b = new B;
+ delete b;
+ C *c = new C;
+ delete c;
+ D *d = new D;
+ delete d;
+ E *e = new E;
+ delete e;
+}
|
Member
|
@llvm/pr-subscribers-clang Author: Oliver Hunt (ojhunt) ChangesWhen serializing and deserializing a FunctionDecl we don't recover whether or not the decl was a type aware allocator or destroying delete, because in the final PR that information was placed in a side table in ASTContext. In principle it should be possible to re-do the semantic checks to determine what these flags should be when deserializing, but it seems like the most robust path is simply recording the flags directly in the serialized AST. Full diff: https://github.com/llvm/llvm-project/pull/137102.diff 7 Files Affected:
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index 5545cbc8d608c..0f54aa5c5e062 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -1076,6 +1076,8 @@ void ASTDeclReader::VisitFunctionDecl(FunctionDecl *FD) {
FD->setFriendConstraintRefersToEnclosingTemplate(
FunctionDeclBits.getNextBit());
FD->setUsesSEHTry(FunctionDeclBits.getNextBit());
+ FD->setIsDestroyingOperatorDelete(FunctionDeclBits.getNextBit());
+ FD->setIsTypeAwareOperatorNewOrDelete(FunctionDeclBits.getNextBit());
FD->EndRangeLoc = readSourceLocation();
if (FD->isExplicitlyDefaulted())
diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp
index 3a7a23481ea98..d1f92cea4dfea 100644
--- a/clang/lib/Serialization/ASTWriterDecl.cpp
+++ b/clang/lib/Serialization/ASTWriterDecl.cpp
@@ -847,6 +847,8 @@ void ASTDeclWriter::VisitFunctionDecl(FunctionDecl *D) {
FunctionDeclBits.addBit(D->isInstantiatedFromMemberTemplate());
FunctionDeclBits.addBit(D->FriendConstraintRefersToEnclosingTemplate());
FunctionDeclBits.addBit(D->usesSEHTry());
+ FunctionDeclBits.addBit(D->isDestroyingOperatorDelete());
+ FunctionDeclBits.addBit(D->isTypeAwareOperatorNewOrDelete());
Record.push_back(FunctionDeclBits);
Record.AddSourceLocation(D->getEndLoc());
diff --git a/clang/test/Modules/Inputs/PR137102/module.modulemap b/clang/test/Modules/Inputs/PR137102/module.modulemap
new file mode 100644
index 0000000000000..337aff5821e7f
--- /dev/null
+++ b/clang/test/Modules/Inputs/PR137102/module.modulemap
@@ -0,0 +1 @@
+module type_aware_destroying_new_delete { header "type_aware_destroying_new_delete.h" export * }
diff --git a/clang/test/Modules/Inputs/PR137102/type_aware_destroying_new_delete.h b/clang/test/Modules/Inputs/PR137102/type_aware_destroying_new_delete.h
new file mode 100644
index 0000000000000..f96a9ea0c8a41
--- /dev/null
+++ b/clang/test/Modules/Inputs/PR137102/type_aware_destroying_new_delete.h
@@ -0,0 +1,52 @@
+
+namespace std {
+ struct destroying_delete_t { };
+ template <class T> struct type_identity {
+ using type = T;
+ };
+ typedef __SIZE_TYPE__ size_t;
+ enum class align_val_t : size_t;
+};
+
+struct A {
+ A();
+ void *operator new(std::size_t);
+ void operator delete(A*, std::destroying_delete_t);
+};
+
+struct B {
+ B();
+ void *operator new(std::type_identity<B>, std::size_t, std::align_val_t);
+ void operator delete(std::type_identity<B>, void*, std::size_t, std::align_val_t);
+};
+
+struct C {
+ C();
+ template <class T> void *operator new(std::type_identity<T>, std::size_t, std::align_val_t);
+ template <class T> void operator delete(std::type_identity<T>, void*, std::size_t, std::align_val_t);
+};
+
+struct D {
+ D();
+};
+void *operator new(std::type_identity<D>, std::size_t, std::align_val_t);
+void operator delete(std::type_identity<D>, void*, std::size_t, std::align_val_t);
+
+struct E {
+ E();
+};
+template <class T> void *operator new(std::type_identity<T>, std::size_t, std::align_val_t);
+template <class T> void operator delete(std::type_identity<T>, void*, std::size_t, std::align_val_t);
+
+void in_module_tests() {
+ A* a = new A;
+ delete a;
+ B *b = new B;
+ delete b;
+ C *c = new C;
+ delete c;
+ D *d = new D;
+ delete d;
+ E *e = new E;
+ delete e;
+}
diff --git a/clang/test/Modules/type-aware-destroying-new-and-delete-modules.cpp b/clang/test/Modules/type-aware-destroying-new-and-delete-modules.cpp
new file mode 100644
index 0000000000000..e88f8a8791147
--- /dev/null
+++ b/clang/test/Modules/type-aware-destroying-new-and-delete-modules.cpp
@@ -0,0 +1,23 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -x c++ -std=c++26 -fmodules-cache-path=%t -I %S/Inputs/PR137102 -emit-llvm-only %s
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -x c++ -std=c++26 -fmodules-cache-path=%t -I %S/Inputs/PR137102 -emit-llvm-only %s -triple i686-windows
+
+#include "type_aware_destroying_new_delete.h"
+
+
+static void call_in_module_function(void) {
+ in_module_tests();
+}
+
+void out_of_module_tests() {
+ A* a = new A;
+ delete a;
+ B *b = new B;
+ delete b;
+ C *c = new C;
+ delete c;
+ D *d = new D;
+ delete d;
+ E *e = new E;
+ delete e;
+}
diff --git a/clang/test/PCH/Inputs/type_aware_destroying_new_delete.h b/clang/test/PCH/Inputs/type_aware_destroying_new_delete.h
new file mode 100644
index 0000000000000..42d609c0f5c26
--- /dev/null
+++ b/clang/test/PCH/Inputs/type_aware_destroying_new_delete.h
@@ -0,0 +1,52 @@
+
+namespace std {
+ struct destroying_delete_t { };
+ template <class T> struct type_identity {
+ using type = T;
+ };
+ typedef __SIZE_TYPE__ size_t;
+ enum class align_val_t : size_t;
+};
+
+struct A {
+ A();
+ void *operator new(std::size_t);
+ void operator delete(A*, std::destroying_delete_t);
+};
+
+struct B {
+ B();
+ void *operator new(std::type_identity<B>, std::size_t, std::align_val_t);
+ void operator delete(std::type_identity<B>, void*, std::size_t, std::align_val_t);
+};
+
+struct C {
+ C();
+ template <class T> void *operator new(std::type_identity<T>, std::size_t, std::align_val_t);
+ template <class T> void operator delete(std::type_identity<T>, void*, std::size_t, std::align_val_t);
+};
+
+struct D {
+ D();
+};
+void *operator new(std::type_identity<D>, std::size_t, std::align_val_t);
+void operator delete(std::type_identity<D>, void*, std::size_t, std::align_val_t);
+
+struct E {
+ E();
+};
+template <class T> void *operator new(std::type_identity<T>, std::size_t, std::align_val_t);
+template <class T> void operator delete(std::type_identity<T>, void*, std::size_t, std::align_val_t);
+
+void in_pch_tests() {
+ A* a = new A;
+ delete a;
+ B *b = new B;
+ delete b;
+ C *c = new C;
+ delete c;
+ D *d = new D;
+ delete d;
+ E *e = new E;
+ delete e;
+}
diff --git a/clang/test/PCH/type-aware-destroying-new-and-delete-pch.cpp b/clang/test/PCH/type-aware-destroying-new-and-delete-pch.cpp
new file mode 100644
index 0000000000000..d8f7f5dd50c78
--- /dev/null
+++ b/clang/test/PCH/type-aware-destroying-new-and-delete-pch.cpp
@@ -0,0 +1,27 @@
+// Test this without pch.
+// RUN: %clang_cc1 -x c++ -std=c++26 -include %S/Inputs/type_aware_destroying_new_delete.h -emit-llvm -o - %s
+
+// Test with pch.
+// RUN: %clang_cc1 -x c++ -std=c++26 -emit-pch -o %t %S/Inputs/type_aware_destroying_new_delete.h
+// RUN: %clang_cc1 -x c++ -std=c++26 -include-pch %t -emit-llvm -o - %s
+
+// RUN: %clang_cc1 -x c++ -std=c++11 -emit-pch -fpch-instantiate-templates -o %t %S/Inputs/type_aware_destroying_new_delete.h
+// RUN: %clang_cc1 -x c++ -std=c++11 -include-pch %t -emit-llvm -o - %s
+
+
+static void call_in_pch_function(void) {
+ in_pch_tests();
+}
+
+void out_of_pch_tests() {
+ A* a = new A;
+ delete a;
+ B *b = new B;
+ delete b;
+ C *c = new C;
+ delete c;
+ D *d = new D;
+ delete d;
+ E *e = new E;
+ delete e;
+}
|
Contributor
Author
|
Ok, added PCH and module tests - verified both tests fail without this fix to the serialization, and they pass now. |
Contributor
|
I hope you won't mind if I merge the fix to get us unblocked. It looks rather uncontroversial and feedback (if any) can be addressed post-commit. |
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
llvm#137102) When serializing and deserializing a FunctionDecl we don't recover whether or not the decl was a type aware allocator or destroying delete, because in the final PR that information was placed in a side table in ASTContext. In principle it should be possible to re-do the semantic checks to determine what these flags should be when deserializing, but it seems like the most robust path is simply recording the flags directly in the serialized AST.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When serializing and deserializing a FunctionDecl we don't recover whether or not the decl was a type aware allocator or destroying delete, because in the final PR that information was placed in a side table in ASTContext.
In principle it should be possible to re-do the semantic checks to determine what these flags should be when deserializing, but it seems like the most robust path is simply recording the flags directly in the serialized AST.