[PATCH] D148515: [Modules] Do not rewrite existing decls when deserializing class fields

[Ilya Biryukov via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGccf719171937: [Modules] Do not rewrite existing decls when 
deserializing class fields (authored by ilya-biryukov).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148515/new/

https://reviews.llvm.org/D148515

Files:
  clang/lib/AST/Decl.cpp
  clang/test/Modules/pr61065_2.cppm


Index: clang/test/Modules/pr61065_2.cppm
===
--- /dev/null
+++ clang/test/Modules/pr61065_2.cppm
@@ -0,0 +1,71 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/a.cppm -emit-module-interface -o %t/a.pcm
+// RUN: %clang_cc1 -std=c++20 %t/b.cppm -emit-module-interface -o %t/b.pcm \
+// RUN: -fprebuilt-module-path=%t
+// RUN: %clang_cc1 -std=c++20 %t/c.cppm -emit-module-interface -o %t/c.pcm \
+// RUN: -fprebuilt-module-path=%t
+// RUN: %clang_cc1 -std=c++20 %t/d.cppm -emit-module-interface -o %t/d.pcm \
+// RUN: -fprebuilt-module-path=%t
+// RUN: %clang_cc1 -std=c++20 %t/e.cpp -fsyntax-only -verify 
-fprebuilt-module-path=%t
+
+//--- a.cppm
+export module a;
+
+struct WithCtor {
+  WithCtor();
+};
+
+export template 
+struct Getter {
+  union {
+WithCtor container;
+  };
+};
+
+//--- b.cppm
+export module b;
+
+import a;
+
+export template 
+class AnySpan {
+ public:
+  AnySpan();
+  AnySpan(Getter getter)
+  : getter_(getter) {}
+
+ private:
+  Getter getter_;
+};
+
+//--- c.cppm
+export module c;
+import b;
+
+export inline void RegisterInt322(
+   AnySpan sibling_field_nums) {
+  sibling_field_nums = sibling_field_nums;
+}
+
+//--- d.cppm
+// expected-no-diagnostics
+export module d;
+import c;
+import b;
+
+export inline void RegisterInt32(
+   AnySpan sibling_field_nums = {}) {
+  sibling_field_nums = sibling_field_nums;
+}
+
+//--- e.cpp
+import d;
+import b;
+
+// expected-no-diagnostics
+void foo(AnySpan s) {
+  s = AnySpan(s);
+}
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -4886,8 +4886,13 @@
   if (Decls.empty())
 return;
 
-  std::tie(FirstDecl, LastDecl) = BuildDeclChain(Decls,
- 
/*FieldsAlreadyLoaded=*/false);
+  auto [ExternalFirst, ExternalLast] =
+  BuildDeclChain(Decls,
+ /*FieldsAlreadyLoaded=*/false);
+  ExternalLast->NextInContextAndBits.setPointer(FirstDecl);
+  FirstDecl = ExternalFirst;
+  if (!LastDecl)
+LastDecl = ExternalLast;
 }
 
 bool RecordDecl::mayInsertExtraPadding(bool EmitRemark) const {


Index: clang/test/Modules/pr61065_2.cppm
===
--- /dev/null
+++ clang/test/Modules/pr61065_2.cppm
@@ -0,0 +1,71 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/a.cppm -emit-module-interface -o %t/a.pcm
+// RUN: %clang_cc1 -std=c++20 %t/b.cppm -emit-module-interface -o %t/b.pcm \
+// RUN: -fprebuilt-module-path=%t
+// RUN: %clang_cc1 -std=c++20 %t/c.cppm -emit-module-interface -o %t/c.pcm \
+// RUN: -fprebuilt-module-path=%t
+// RUN: %clang_cc1 -std=c++20 %t/d.cppm -emit-module-interface -o %t/d.pcm \
+// RUN: -fprebuilt-module-path=%t
+// RUN: %clang_cc1 -std=c++20 %t/e.cpp -fsyntax-only -verify -fprebuilt-module-path=%t
+
+//--- a.cppm
+export module a;
+
+struct WithCtor {
+  WithCtor();
+};
+
+export template 
+struct Getter {
+  union {
+WithCtor container;
+  };
+};
+
+//--- b.cppm
+export module b;
+
+import a;
+
+export template 
+class AnySpan {
+ public:
+  AnySpan();
+  AnySpan(Getter getter)
+  : getter_(getter) {}
+
+ private:
+  Getter getter_;
+};
+
+//--- c.cppm
+export module c;
+import b;
+
+export inline void RegisterInt322(
+   AnySpan sibling_field_nums) {
+  sibling_field_nums = sibling_field_nums;
+}
+
+//--- d.cppm
+// expected-no-diagnostics
+export module d;
+import c;
+import b;
+
+export inline void RegisterInt32(
+   AnySpan sibling_field_nums = {}) {
+  sibling_field_nums = sibling_field_nums;
+}
+
+//--- e.cpp
+import d;
+import b;
+
+// expected-no-diagnostics
+void foo(AnySpan s) {
+  s = AnySpan(s);
+}
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -4886,8 +4886,13 @@
   if (Decls.empty())
 return;
 
-  std::tie(FirstDecl, LastDecl) = BuildDeclChain(Decls,
- /*FieldsAlreadyLoaded=*/false);
+  auto [ExternalFirst, ExternalLast] =
+  BuildDeclChain(Decls,
+ /*FieldsAlreadyLoaded=*/false);
+  ExternalLast->NextInContextAndBits.setPointer(FirstDecl);
+  FirstDecl = ExternalFirst;
+  if (!LastDecl)
+LastDecl = ExternalLast;
 }
 
 bool 

[PATCH] D148515: [Modules] Do not rewrite existing decls when deserializing class fields

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 514579.
ilya-biryukov added a comment.

- clang-format the code


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148515/new/

https://reviews.llvm.org/D148515

Files:
  clang/lib/AST/Decl.cpp
  clang/test/Modules/pr61065_2.cppm


Index: clang/test/Modules/pr61065_2.cppm
===
--- /dev/null
+++ clang/test/Modules/pr61065_2.cppm
@@ -0,0 +1,71 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/a.cppm -emit-module-interface -o %t/a.pcm
+// RUN: %clang_cc1 -std=c++20 %t/b.cppm -emit-module-interface -o %t/b.pcm \
+// RUN: -fprebuilt-module-path=%t
+// RUN: %clang_cc1 -std=c++20 %t/c.cppm -emit-module-interface -o %t/c.pcm \
+// RUN: -fprebuilt-module-path=%t
+// RUN: %clang_cc1 -std=c++20 %t/d.cppm -emit-module-interface -o %t/d.pcm \
+// RUN: -fprebuilt-module-path=%t
+// RUN: %clang_cc1 -std=c++20 %t/e.cpp -fsyntax-only -verify 
-fprebuilt-module-path=%t
+
+//--- a.cppm
+export module a;
+
+struct WithCtor {
+  WithCtor();
+};
+
+export template 
+struct Getter {
+  union {
+WithCtor container;
+  };
+};
+
+//--- b.cppm
+export module b;
+
+import a;
+
+export template 
+class AnySpan {
+ public:
+  AnySpan();
+  AnySpan(Getter getter)
+  : getter_(getter) {}
+
+ private:
+  Getter getter_;
+};
+
+//--- c.cppm
+export module c;
+import b;
+
+export inline void RegisterInt322(
+   AnySpan sibling_field_nums) {
+  sibling_field_nums = sibling_field_nums;
+}
+
+//--- d.cppm
+// expected-no-diagnostics
+export module d;
+import c;
+import b;
+
+export inline void RegisterInt32(
+   AnySpan sibling_field_nums = {}) {
+  sibling_field_nums = sibling_field_nums;
+}
+
+//--- e.cpp
+import d;
+import b;
+
+// expected-no-diagnostics
+void foo(AnySpan s) {
+  s = AnySpan(s);
+}
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -4886,8 +4886,13 @@
   if (Decls.empty())
 return;
 
-  std::tie(FirstDecl, LastDecl) = BuildDeclChain(Decls,
- 
/*FieldsAlreadyLoaded=*/false);
+  auto [ExternalFirst, ExternalLast] =
+  BuildDeclChain(Decls,
+ /*FieldsAlreadyLoaded=*/false);
+  ExternalLast->NextInContextAndBits.setPointer(FirstDecl);
+  FirstDecl = ExternalFirst;
+  if (!LastDecl)
+LastDecl = ExternalLast;
 }
 
 bool RecordDecl::mayInsertExtraPadding(bool EmitRemark) const {


Index: clang/test/Modules/pr61065_2.cppm
===
--- /dev/null
+++ clang/test/Modules/pr61065_2.cppm
@@ -0,0 +1,71 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/a.cppm -emit-module-interface -o %t/a.pcm
+// RUN: %clang_cc1 -std=c++20 %t/b.cppm -emit-module-interface -o %t/b.pcm \
+// RUN: -fprebuilt-module-path=%t
+// RUN: %clang_cc1 -std=c++20 %t/c.cppm -emit-module-interface -o %t/c.pcm \
+// RUN: -fprebuilt-module-path=%t
+// RUN: %clang_cc1 -std=c++20 %t/d.cppm -emit-module-interface -o %t/d.pcm \
+// RUN: -fprebuilt-module-path=%t
+// RUN: %clang_cc1 -std=c++20 %t/e.cpp -fsyntax-only -verify -fprebuilt-module-path=%t
+
+//--- a.cppm
+export module a;
+
+struct WithCtor {
+  WithCtor();
+};
+
+export template 
+struct Getter {
+  union {
+WithCtor container;
+  };
+};
+
+//--- b.cppm
+export module b;
+
+import a;
+
+export template 
+class AnySpan {
+ public:
+  AnySpan();
+  AnySpan(Getter getter)
+  : getter_(getter) {}
+
+ private:
+  Getter getter_;
+};
+
+//--- c.cppm
+export module c;
+import b;
+
+export inline void RegisterInt322(
+   AnySpan sibling_field_nums) {
+  sibling_field_nums = sibling_field_nums;
+}
+
+//--- d.cppm
+// expected-no-diagnostics
+export module d;
+import c;
+import b;
+
+export inline void RegisterInt32(
+   AnySpan sibling_field_nums = {}) {
+  sibling_field_nums = sibling_field_nums;
+}
+
+//--- e.cpp
+import d;
+import b;
+
+// expected-no-diagnostics
+void foo(AnySpan s) {
+  s = AnySpan(s);
+}
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -4886,8 +4886,13 @@
   if (Decls.empty())
 return;
 
-  std::tie(FirstDecl, LastDecl) = BuildDeclChain(Decls,
- /*FieldsAlreadyLoaded=*/false);
+  auto [ExternalFirst, ExternalLast] =
+  BuildDeclChain(Decls,
+ /*FieldsAlreadyLoaded=*/false);
+  ExternalLast->NextInContextAndBits.setPointer(FirstDecl);
+  FirstDecl = ExternalFirst;
+  if (!LastDecl)
+LastDecl = ExternalLast;
 }
 
 bool RecordDecl::mayInsertExtraPadding(bool EmitRemark) const {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D148515: [Modules] Do not rewrite existing decls when deserializing class fields

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu accepted this revision.
ChuanqiXu added a comment.
This revision is now accepted and ready to land.

Thank you very much!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148515/new/

https://reviews.llvm.org/D148515

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148515: [Modules] Do not rewrite existing decls when deserializing class fields

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

I plan to land this together with a reland of 
bc95f27337c7ed77c28e713c855272848f01802a 
 and 
finally close GH61065 .
I have checked this works on previously broken targets. @ChuanqiXu sorry for 
taking so long to track it down, reproducing module-related errors is quite 
hard in our setup.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148515/new/

https://reviews.llvm.org/D148515

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148515: [Modules] Do not rewrite existing decls when deserializing class fields

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov created this revision.
ilya-biryukov added a reviewer: ChuanqiXu.
Herald added a project: All.
ilya-biryukov requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Classes can have implicit members that were added before fields were
deserialized. These members were previously silently removed from
`decls()` when fields were deserialized after them.

This was the root cause of a compilation error exposed in
bc95f27337c7ed77c28e713c855272848f01802a 
, added a 
test for it.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D148515

Files:
  clang/lib/AST/Decl.cpp
  clang/test/Modules/pr61065_2.cppm


Index: clang/test/Modules/pr61065_2.cppm
===
--- /dev/null
+++ clang/test/Modules/pr61065_2.cppm
@@ -0,0 +1,71 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/a.cppm -emit-module-interface -o %t/a.pcm
+// RUN: %clang_cc1 -std=c++20 %t/b.cppm -emit-module-interface -o %t/b.pcm \
+// RUN: -fprebuilt-module-path=%t
+// RUN: %clang_cc1 -std=c++20 %t/c.cppm -emit-module-interface -o %t/c.pcm \
+// RUN: -fprebuilt-module-path=%t
+// RUN: %clang_cc1 -std=c++20 %t/d.cppm -emit-module-interface -o %t/d.pcm \
+// RUN: -fprebuilt-module-path=%t
+// RUN: %clang_cc1 -std=c++20 %t/e.cpp -fsyntax-only -verify 
-fprebuilt-module-path=%t
+
+//--- a.cppm
+export module a;
+
+struct WithCtor {
+  WithCtor();
+};
+
+export template 
+struct Getter {
+  union {
+WithCtor container;
+  };
+};
+
+//--- b.cppm
+export module b;
+
+import a;
+
+export template 
+class AnySpan {
+ public:
+  AnySpan();
+  AnySpan(Getter getter)
+  : getter_(getter) {}
+
+ private:
+  Getter getter_;
+};
+
+//--- c.cppm
+export module c;
+import b;
+
+export inline void RegisterInt322(
+   AnySpan sibling_field_nums) {
+  sibling_field_nums = sibling_field_nums;
+}
+
+//--- d.cppm
+// expected-no-diagnostics
+export module d;
+import c;
+import b;
+
+export inline void RegisterInt32(
+   AnySpan sibling_field_nums = {}) {
+  sibling_field_nums = sibling_field_nums;
+}
+
+//--- e.cpp
+import d;
+import b;
+
+// expected-no-diagnostics
+void foo(AnySpan s) {
+  s = AnySpan(s);
+}
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -4886,8 +4886,12 @@
   if (Decls.empty())
 return;
 
-  std::tie(FirstDecl, LastDecl) = BuildDeclChain(Decls,
+  auto [ExternalFirst, ExternalLast] = BuildDeclChain(Decls,
  
/*FieldsAlreadyLoaded=*/false);
+  ExternalLast->NextInContextAndBits.setPointer(FirstDecl);
+  FirstDecl = ExternalFirst;
+  if (!LastDecl)
+LastDecl = ExternalLast;
 }
 
 bool RecordDecl::mayInsertExtraPadding(bool EmitRemark) const {


Index: clang/test/Modules/pr61065_2.cppm
===
--- /dev/null
+++ clang/test/Modules/pr61065_2.cppm
@@ -0,0 +1,71 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/a.cppm -emit-module-interface -o %t/a.pcm
+// RUN: %clang_cc1 -std=c++20 %t/b.cppm -emit-module-interface -o %t/b.pcm \
+// RUN: -fprebuilt-module-path=%t
+// RUN: %clang_cc1 -std=c++20 %t/c.cppm -emit-module-interface -o %t/c.pcm \
+// RUN: -fprebuilt-module-path=%t
+// RUN: %clang_cc1 -std=c++20 %t/d.cppm -emit-module-interface -o %t/d.pcm \
+// RUN: -fprebuilt-module-path=%t
+// RUN: %clang_cc1 -std=c++20 %t/e.cpp -fsyntax-only -verify -fprebuilt-module-path=%t
+
+//--- a.cppm
+export module a;
+
+struct WithCtor {
+  WithCtor();
+};
+
+export template 
+struct Getter {
+  union {
+WithCtor container;
+  };
+};
+
+//--- b.cppm
+export module b;
+
+import a;
+
+export template 
+class AnySpan {
+ public:
+  AnySpan();
+  AnySpan(Getter getter)
+  : getter_(getter) {}
+
+ private:
+  Getter getter_;
+};
+
+//--- c.cppm
+export module c;
+import b;
+
+export inline void RegisterInt322(
+   AnySpan sibling_field_nums) {
+  sibling_field_nums = sibling_field_nums;
+}
+
+//--- d.cppm
+// expected-no-diagnostics
+export module d;
+import c;
+import b;
+
+export inline void RegisterInt32(
+   AnySpan sibling_field_nums = {}) {
+  sibling_field_nums = sibling_field_nums;
+}
+
+//--- e.cpp
+import d;
+import b;
+
+// expected-no-diagnostics
+void foo(AnySpan s) {
+  s = AnySpan(s);
+}
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -4886,8 +4886,12 @@
   if (Decls.empty())
 return;
 
-  std::tie(FirstDecl, LastDecl) = BuildDeclChain(Decls,
+  auto [ExternalFirst, ExternalLast] = BuildDeclChain(Decls,
  /*FieldsAlreadyLoaded=*/false);
+