[PATCH] D115610: [C++20] [Modules] Don't create multiple global module fragment

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D115610#3304289 , @urnathan wrote:

> the landed version is good, with Richard's suggested member name change.  
> Can't see a way of post-commit accepting?

Yeah, maybe we can't do post-commit accept formally.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115610

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

[PATCH] D115610: [C++20] [Modules] Don't create multiple global module fragment

[Nathan Sidwell via Phabricator via cfe-commits]

urnathan added a comment.

the landed version is good, with Richard's suggested member name change.  Can't 
see a way of post-commit accepting?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115610

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

[PATCH] D115610: [C++20] [Modules] Don't create multiple global module fragment

[Chuanqi Xu via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG3504937dfb2b: [C++20] [Modules] Dont create multiple 
global module fragment (authored by ChuanqiXu).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115610

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaModule.cpp
  clang/test/CXX/module/module.unit/p7/Inputs/h8.h
  clang/test/CXX/module/module.unit/p7/Inputs/m8.cppm
  clang/test/CXX/module/module.unit/p7/t8.cpp


Index: clang/test/CXX/module/module.unit/p7/t8.cpp
===
--- /dev/null
+++ clang/test/CXX/module/module.unit/p7/t8.cpp
@@ -0,0 +1,7 @@
+// RUN: rm -fr %t
+// RUN: mkdir %t
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %S/Inputs/m8.cppm 
-I%S/Inputs -o %t/m8.pcm
+// RUN: %clang_cc1 -std=c++20 -I%S/Inputs/ -fprebuilt-module-path=%t %s 
-verify -fsyntax-only
+// expected-no-diagnostics
+export module t8;
+import m8;
Index: clang/test/CXX/module/module.unit/p7/Inputs/m8.cppm
===
--- /dev/null
+++ clang/test/CXX/module/module.unit/p7/Inputs/m8.cppm
@@ -0,0 +1,7 @@
+module;
+#include "h8.h"
+export module m8;
+
+extern "C++" {
+void bar();
+}
Index: clang/test/CXX/module/module.unit/p7/Inputs/h8.h
===
--- /dev/null
+++ clang/test/CXX/module/module.unit/p7/Inputs/h8.h
@@ -0,0 +1,4 @@
+#ifndef H8
+#define H8
+void foo();
+#endif
Index: clang/lib/Sema/SemaModule.cpp
===
--- clang/lib/Sema/SemaModule.cpp
+++ clang/lib/Sema/SemaModule.cpp
@@ -720,19 +720,24 @@
 
 Module *Sema::PushGlobalModuleFragment(SourceLocation BeginLoc,
bool IsImplicit) {
-  ModuleMap  = PP.getHeaderSearchInfo().getModuleMap();
-  Module *GlobalModule =
-  Map.createGlobalModuleFragmentForModuleUnit(BeginLoc, 
getCurrentModule());
-  assert(GlobalModule && "module creation should not fail");
+  // We shouldn't create new global module fragment if there is already
+  // one.
+  if (!GlobalModuleFragment) {
+ModuleMap  = PP.getHeaderSearchInfo().getModuleMap();
+GlobalModuleFragment = Map.createGlobalModuleFragmentForModuleUnit(
+BeginLoc, getCurrentModule());
+  }
+
+  assert(GlobalModuleFragment && "module creation should not fail");
 
   // Enter the scope of the global module.
-  ModuleScopes.push_back({BeginLoc, GlobalModule,
+  ModuleScopes.push_back({BeginLoc, GlobalModuleFragment,
   /*ModuleInterface=*/false,
   /*ImplicitGlobalModuleFragment=*/IsImplicit,
-  /*VisibleModuleSet*/{}});
-  VisibleModules.setVisible(GlobalModule, BeginLoc);
+  /*VisibleModuleSet*/ {}});
+  VisibleModules.setVisible(GlobalModuleFragment, BeginLoc);
 
-  return GlobalModule;
+  return GlobalModuleFragment;
 }
 
 void Sema::PopGlobalModuleFragment() {
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -2218,6 +2218,8 @@
   };
   /// The modules we're currently parsing.
   llvm::SmallVector ModuleScopes;
+  /// The global module fragment of the current translation unit.
+  clang::Module *GlobalModuleFragment = nullptr;
 
   /// Namespace definitions that we will export when they finish.
   llvm::SmallPtrSet DeferredExportedNamespaces;


Index: clang/test/CXX/module/module.unit/p7/t8.cpp
===
--- /dev/null
+++ clang/test/CXX/module/module.unit/p7/t8.cpp
@@ -0,0 +1,7 @@
+// RUN: rm -fr %t
+// RUN: mkdir %t
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %S/Inputs/m8.cppm -I%S/Inputs -o %t/m8.pcm
+// RUN: %clang_cc1 -std=c++20 -I%S/Inputs/ -fprebuilt-module-path=%t %s -verify -fsyntax-only
+// expected-no-diagnostics
+export module t8;
+import m8;
Index: clang/test/CXX/module/module.unit/p7/Inputs/m8.cppm
===
--- /dev/null
+++ clang/test/CXX/module/module.unit/p7/Inputs/m8.cppm
@@ -0,0 +1,7 @@
+module;
+#include "h8.h"
+export module m8;
+
+extern "C++" {
+void bar();
+}
Index: clang/test/CXX/module/module.unit/p7/Inputs/h8.h
===
--- /dev/null
+++ clang/test/CXX/module/module.unit/p7/Inputs/h8.h
@@ -0,0 +1,4 @@
+#ifndef H8
+#define H8
+void foo();
+#endif
Index: clang/lib/Sema/SemaModule.cpp
===
--- clang/lib/Sema/SemaModule.cpp
+++ clang/lib/Sema/SemaModule.cpp
@@ -720,19 +720,24 @@
 
 Module 

[PATCH] D115610: [C++20] [Modules] Don't create multiple global module fragment

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

Although this wasn't accepted formally, both @urnathan and @rsmith says this 
looks good to them. So I guess it might not be bad to land this.


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

https://reviews.llvm.org/D115610

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

[PATCH] D115610: [C++20] [Modules] Don't create multiple global module fragment

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 406315.
ChuanqiXu added a comment.

Address comments.


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

https://reviews.llvm.org/D115610

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaModule.cpp
  clang/test/CXX/module/module.unit/p7/Inputs/h8.h
  clang/test/CXX/module/module.unit/p7/Inputs/m8.cppm
  clang/test/CXX/module/module.unit/p7/t8.cpp


Index: clang/test/CXX/module/module.unit/p7/t8.cpp
===
--- /dev/null
+++ clang/test/CXX/module/module.unit/p7/t8.cpp
@@ -0,0 +1,7 @@
+// RUN: rm -fr %t
+// RUN: mkdir %t
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %S/Inputs/m8.cppm 
-I%S/Inputs -o %t/m8.pcm
+// RUN: %clang_cc1 -std=c++20 -I%S/Inputs/ -fprebuilt-module-path=%t %s 
-verify -fsyntax-only
+// expected-no-diagnostics
+export module t8;
+import m8;
Index: clang/test/CXX/module/module.unit/p7/Inputs/m8.cppm
===
--- /dev/null
+++ clang/test/CXX/module/module.unit/p7/Inputs/m8.cppm
@@ -0,0 +1,7 @@
+module;
+#include "h8.h"
+export module m8;
+
+extern "C++" {
+void bar();
+}
Index: clang/test/CXX/module/module.unit/p7/Inputs/h8.h
===
--- /dev/null
+++ clang/test/CXX/module/module.unit/p7/Inputs/h8.h
@@ -0,0 +1,4 @@
+#ifndef H8
+#define H8
+void foo();
+#endif
Index: clang/lib/Sema/SemaModule.cpp
===
--- clang/lib/Sema/SemaModule.cpp
+++ clang/lib/Sema/SemaModule.cpp
@@ -720,19 +720,24 @@
 
 Module *Sema::PushGlobalModuleFragment(SourceLocation BeginLoc,
bool IsImplicit) {
-  ModuleMap  = PP.getHeaderSearchInfo().getModuleMap();
-  Module *GlobalModule =
-  Map.createGlobalModuleFragmentForModuleUnit(BeginLoc, 
getCurrentModule());
-  assert(GlobalModule && "module creation should not fail");
+  // We shouldn't create new global module fragment if there is already
+  // one.
+  if (!GlobalModuleFragment) {
+ModuleMap  = PP.getHeaderSearchInfo().getModuleMap();
+GlobalModuleFragment = Map.createGlobalModuleFragmentForModuleUnit(
+BeginLoc, getCurrentModule());
+  }
+
+  assert(GlobalModuleFragment && "module creation should not fail");
 
   // Enter the scope of the global module.
-  ModuleScopes.push_back({BeginLoc, GlobalModule,
+  ModuleScopes.push_back({BeginLoc, GlobalModuleFragment,
   /*ModuleInterface=*/false,
   /*ImplicitGlobalModuleFragment=*/IsImplicit,
-  /*VisibleModuleSet*/{}});
-  VisibleModules.setVisible(GlobalModule, BeginLoc);
+  /*VisibleModuleSet*/ {}});
+  VisibleModules.setVisible(GlobalModuleFragment, BeginLoc);
 
-  return GlobalModule;
+  return GlobalModuleFragment;
 }
 
 void Sema::PopGlobalModuleFragment() {
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -2218,6 +2218,8 @@
   };
   /// The modules we're currently parsing.
   llvm::SmallVector ModuleScopes;
+  /// The global module fragment of the current translation unit.
+  clang::Module *GlobalModuleFragment = nullptr;
 
   /// Namespace definitions that we will export when they finish.
   llvm::SmallPtrSet DeferredExportedNamespaces;


Index: clang/test/CXX/module/module.unit/p7/t8.cpp
===
--- /dev/null
+++ clang/test/CXX/module/module.unit/p7/t8.cpp
@@ -0,0 +1,7 @@
+// RUN: rm -fr %t
+// RUN: mkdir %t
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %S/Inputs/m8.cppm -I%S/Inputs -o %t/m8.pcm
+// RUN: %clang_cc1 -std=c++20 -I%S/Inputs/ -fprebuilt-module-path=%t %s -verify -fsyntax-only
+// expected-no-diagnostics
+export module t8;
+import m8;
Index: clang/test/CXX/module/module.unit/p7/Inputs/m8.cppm
===
--- /dev/null
+++ clang/test/CXX/module/module.unit/p7/Inputs/m8.cppm
@@ -0,0 +1,7 @@
+module;
+#include "h8.h"
+export module m8;
+
+extern "C++" {
+void bar();
+}
Index: clang/test/CXX/module/module.unit/p7/Inputs/h8.h
===
--- /dev/null
+++ clang/test/CXX/module/module.unit/p7/Inputs/h8.h
@@ -0,0 +1,4 @@
+#ifndef H8
+#define H8
+void foo();
+#endif
Index: clang/lib/Sema/SemaModule.cpp
===
--- clang/lib/Sema/SemaModule.cpp
+++ clang/lib/Sema/SemaModule.cpp
@@ -720,19 +720,24 @@
 
 Module *Sema::PushGlobalModuleFragment(SourceLocation BeginLoc,
bool IsImplicit) {
-  ModuleMap  = PP.getHeaderSearchInfo().getModuleMap();
-  Module *GlobalModule =
-  Map.createGlobalModuleFragmentForModuleUnit(BeginLoc, 

[PATCH] D115610: [C++20] [Modules] Don't create multiple global module fragment

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Thanks, this looks functionally good to me. I'm happy for this to land with the 
`Sema` member renamed.




Comment at: clang/lib/Sema/SemaModule.cpp:727
+ModuleMap  = PP.getHeaderSearchInfo().getModuleMap();
+GlobalModuleFragmentCache = Map.createGlobalModuleFragmentForModuleUnit(
+BeginLoc, getCurrentModule());

If this is supposed to be a cache, we should look for an existing global module 
fragment here rather than always creating a new one. Either this member 
shouldn't be called `...Cache` or we should do a search at this point. (I think 
we do want to support the existence of multiple different global module 
fragments in a single module, for example if we have full information about 
multiple different module interface units for the same module loaded, so I 
think just renaming the member is probably the best way to go).


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

https://reviews.llvm.org/D115610

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

[PATCH] D115610: [C++20] [Modules] Don't create multiple global module fragment

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D115610#3269265 , @urnathan wrote:

> Seems good to me, @iains you've been looking at this?

According to the discussion in 
https://github.com/llvm/llvm-project/issues/51682, it looks like Iains plans to 
implement something like P1184R2 (I found that is your paper!) in clang. So I 
added him before.

(Might you make the status as green?)




Comment at: clang/include/clang/Sema/Sema.h:2217
+  /// The gloabl module fragment of the current tranlation unit.
+  clang::Module *GlobalModuleFragmentCache = nullptr;
 

tbaeder wrote:
> Typos in "gloabl" and "tranlation" in the comment. Any reason to call this a 
> "cache"? This was confusing to me at first.
This is consistent with the current style in Sema. You could find many example 
by searching `Cache;` in Sema.h. I think the semantic is "Cache something to 
avoid search/create again".


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

https://reviews.llvm.org/D115610

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

[PATCH] D115610: [C++20] [Modules] Don't create multiple global module fragment

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 403130.
ChuanqiXu added a comment.

Address comments


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

https://reviews.llvm.org/D115610

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaModule.cpp
  clang/test/CXX/module/module.unit/p7/Inputs/h8.h
  clang/test/CXX/module/module.unit/p7/Inputs/m8.cppm
  clang/test/CXX/module/module.unit/p7/t8.cpp


Index: clang/test/CXX/module/module.unit/p7/t8.cpp
===
--- /dev/null
+++ clang/test/CXX/module/module.unit/p7/t8.cpp
@@ -0,0 +1,7 @@
+// RUN: rm -fr %t
+// RUN: mkdir %t
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %S/Inputs/m8.cppm 
-I%S/Inputs -o %t/m8.pcm
+// RUN: %clang_cc1 -std=c++20 -I%S/Inputs/ -fprebuilt-module-path=%t %s 
-verify -fsyntax-only
+// expected-no-diagnostics
+export module t8;
+import m8;
Index: clang/test/CXX/module/module.unit/p7/Inputs/m8.cppm
===
--- /dev/null
+++ clang/test/CXX/module/module.unit/p7/Inputs/m8.cppm
@@ -0,0 +1,7 @@
+module;
+#include "h8.h"
+export module m8;
+
+extern "C++" {
+void bar();
+}
Index: clang/test/CXX/module/module.unit/p7/Inputs/h8.h
===
--- /dev/null
+++ clang/test/CXX/module/module.unit/p7/Inputs/h8.h
@@ -0,0 +1,4 @@
+#ifndef H8
+#define H8
+void foo();
+#endif
Index: clang/lib/Sema/SemaModule.cpp
===
--- clang/lib/Sema/SemaModule.cpp
+++ clang/lib/Sema/SemaModule.cpp
@@ -720,19 +720,24 @@
 
 Module *Sema::PushGlobalModuleFragment(SourceLocation BeginLoc,
bool IsImplicit) {
-  ModuleMap  = PP.getHeaderSearchInfo().getModuleMap();
-  Module *GlobalModule =
-  Map.createGlobalModuleFragmentForModuleUnit(BeginLoc, 
getCurrentModule());
-  assert(GlobalModule && "module creation should not fail");
+  // We shouldn't create new global module fragment if there is already
+  // one.
+  if (!GlobalModuleFragmentCache) {
+ModuleMap  = PP.getHeaderSearchInfo().getModuleMap();
+GlobalModuleFragmentCache = Map.createGlobalModuleFragmentForModuleUnit(
+BeginLoc, getCurrentModule());
+  }
+
+  assert(GlobalModuleFragmentCache && "module creation should not fail");
 
   // Enter the scope of the global module.
-  ModuleScopes.push_back({BeginLoc, GlobalModule,
+  ModuleScopes.push_back({BeginLoc, GlobalModuleFragmentCache,
   /*ModuleInterface=*/false,
   /*ImplicitGlobalModuleFragment=*/IsImplicit,
-  /*VisibleModuleSet*/{}});
-  VisibleModules.setVisible(GlobalModule, BeginLoc);
+  /*VisibleModuleSet*/ {}});
+  VisibleModules.setVisible(GlobalModuleFragmentCache, BeginLoc);
 
-  return GlobalModule;
+  return GlobalModuleFragmentCache;
 }
 
 void Sema::PopGlobalModuleFragment() {
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -2218,6 +2218,8 @@
   };
   /// The modules we're currently parsing.
   llvm::SmallVector ModuleScopes;
+  /// The global module fragment of the current translation unit.
+  clang::Module *GlobalModuleFragmentCache = nullptr;
 
   /// Namespace definitions that we will export when they finish.
   llvm::SmallPtrSet DeferredExportedNamespaces;


Index: clang/test/CXX/module/module.unit/p7/t8.cpp
===
--- /dev/null
+++ clang/test/CXX/module/module.unit/p7/t8.cpp
@@ -0,0 +1,7 @@
+// RUN: rm -fr %t
+// RUN: mkdir %t
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %S/Inputs/m8.cppm -I%S/Inputs -o %t/m8.pcm
+// RUN: %clang_cc1 -std=c++20 -I%S/Inputs/ -fprebuilt-module-path=%t %s -verify -fsyntax-only
+// expected-no-diagnostics
+export module t8;
+import m8;
Index: clang/test/CXX/module/module.unit/p7/Inputs/m8.cppm
===
--- /dev/null
+++ clang/test/CXX/module/module.unit/p7/Inputs/m8.cppm
@@ -0,0 +1,7 @@
+module;
+#include "h8.h"
+export module m8;
+
+extern "C++" {
+void bar();
+}
Index: clang/test/CXX/module/module.unit/p7/Inputs/h8.h
===
--- /dev/null
+++ clang/test/CXX/module/module.unit/p7/Inputs/h8.h
@@ -0,0 +1,4 @@
+#ifndef H8
+#define H8
+void foo();
+#endif
Index: clang/lib/Sema/SemaModule.cpp
===
--- clang/lib/Sema/SemaModule.cpp
+++ clang/lib/Sema/SemaModule.cpp
@@ -720,19 +720,24 @@
 
 Module *Sema::PushGlobalModuleFragment(SourceLocation BeginLoc,
bool IsImplicit) {
-  ModuleMap  = PP.getHeaderSearchInfo().getModuleMap();
-  Module *GlobalModule =
-  

[PATCH] D115610: [C++20] [Modules] Don't create multiple global module fragment

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:2217
+  /// The gloabl module fragment of the current tranlation unit.
+  clang::Module *GlobalModuleFragmentCache = nullptr;
 

Typos in "gloabl" and "tranlation" in the comment. Any reason to call this a 
"cache"? This was confusing to me at first.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115610

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

[PATCH] D115610: [C++20] [Modules] Don't create multiple global module fragment

[Nathan Sidwell via Phabricator via cfe-commits]

urnathan added a comment.

Seems good to me, @iains you've been looking at this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115610

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

[PATCH] D115610: [C++20] [Modules] Don't create multiple global module fragment

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

@rsmith gentle ping~


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115610

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

[PATCH] D115610: [C++20] [Modules] Don't create multiple global module fragment

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

@rsmith gentle ping~


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115610

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

[PATCH] D115610: [C++20] [Modules] Don't create multiple global module fragment

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

@rsmith gentle ping~


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115610

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

[PATCH] D115610: [C++20] [Modules] Don't create multiple global module fragment

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

@rsmith gentle ping~


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115610

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

[PATCH] D115610: [C++20] [Modules] Don't create multiple global module fragment

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu created this revision.
ChuanqiXu added reviewers: rsmith, aaron.ballman, urnathan, erichkeane, 
hubert.reinterpretcast.
ChuanqiXu added a project: clang.
ChuanqiXu requested review of this revision.
Herald added a subscriber: cfe-commits.

Since the serialization code would recognize modules by names and the name of 
all global module fragment is ``, so that the serialization code would 
complain for the same module.

This patch fixes this by using a global module fragment cache in Sema. Before 
this patch, the compiler would fail on an assertion complaining the duplicated 
modules.

Test Plan: check-all, an internal module library


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D115610

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaModule.cpp
  clang/test/CXX/module/module.unit/p7/Inputs/h8.h
  clang/test/CXX/module/module.unit/p7/Inputs/m8.cppm
  clang/test/CXX/module/module.unit/p7/t8.cpp


Index: clang/test/CXX/module/module.unit/p7/t8.cpp
===
--- /dev/null
+++ clang/test/CXX/module/module.unit/p7/t8.cpp
@@ -0,0 +1,7 @@
+// RUN: rm -fr %t
+// RUN: mkdir %t
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %S/Inputs/m8.cppm 
-I%S/Inputs -o %t/m8.pcm
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface -I%S/Inputs/ 
-fprebuilt-module-path=%t %s -verify
+// expected-no-diagnostics
+export module t8;
+import m8;
Index: clang/test/CXX/module/module.unit/p7/Inputs/m8.cppm
===
--- /dev/null
+++ clang/test/CXX/module/module.unit/p7/Inputs/m8.cppm
@@ -0,0 +1,8 @@
+module;
+#include "h8.h"
+export module m8;
+
+extern "C++" {
+void bar();
+}
+
Index: clang/test/CXX/module/module.unit/p7/Inputs/h8.h
===
--- /dev/null
+++ clang/test/CXX/module/module.unit/p7/Inputs/h8.h
@@ -0,0 +1,4 @@
+#ifndef H8
+#define H8
+void foo();
+#endif
Index: clang/lib/Sema/SemaModule.cpp
===
--- clang/lib/Sema/SemaModule.cpp
+++ clang/lib/Sema/SemaModule.cpp
@@ -704,19 +704,24 @@
 
 Module *Sema::PushGlobalModuleFragment(SourceLocation BeginLoc,
bool IsImplicit) {
-  ModuleMap  = PP.getHeaderSearchInfo().getModuleMap();
-  Module *GlobalModule =
-  Map.createGlobalModuleFragmentForModuleUnit(BeginLoc, 
getCurrentModule());
-  assert(GlobalModule && "module creation should not fail");
+  // We shouldn't create new global module fragment if there is already
+  // one.
+  if (!GlobalModuleFragmentCache) {
+ModuleMap  = PP.getHeaderSearchInfo().getModuleMap();
+GlobalModuleFragmentCache = Map.createGlobalModuleFragmentForModuleUnit(
+BeginLoc, getCurrentModule());
+  }
+
+  assert(GlobalModuleFragmentCache && "module creation should not fail");
 
   // Enter the scope of the global module.
-  ModuleScopes.push_back({BeginLoc, GlobalModule,
+  ModuleScopes.push_back({BeginLoc, GlobalModuleFragmentCache,
   /*ModuleInterface=*/false,
   /*ImplicitGlobalModuleFragment=*/IsImplicit,
-  /*VisibleModuleSet*/{}});
-  VisibleModules.setVisible(GlobalModule, BeginLoc);
+  /*VisibleModuleSet*/ {}});
+  VisibleModules.setVisible(GlobalModuleFragmentCache, BeginLoc);
 
-  return GlobalModule;
+  return GlobalModuleFragmentCache;
 }
 
 void Sema::PopGlobalModuleFragment() {
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -2213,6 +2213,8 @@
   };
   /// The modules we're currently parsing.
   llvm::SmallVector ModuleScopes;
+  /// The gloabl module fragment of the current tranlation unit.
+  clang::Module *GlobalModuleFragmentCache = nullptr;
 
   /// Namespace definitions that we will export when they finish.
   llvm::SmallPtrSet DeferredExportedNamespaces;


Index: clang/test/CXX/module/module.unit/p7/t8.cpp
===
--- /dev/null
+++ clang/test/CXX/module/module.unit/p7/t8.cpp
@@ -0,0 +1,7 @@
+// RUN: rm -fr %t
+// RUN: mkdir %t
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %S/Inputs/m8.cppm -I%S/Inputs -o %t/m8.pcm
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface -I%S/Inputs/ -fprebuilt-module-path=%t %s -verify
+// expected-no-diagnostics
+export module t8;
+import m8;
Index: clang/test/CXX/module/module.unit/p7/Inputs/m8.cppm
===
--- /dev/null
+++ clang/test/CXX/module/module.unit/p7/Inputs/m8.cppm
@@ -0,0 +1,8 @@
+module;
+#include "h8.h"
+export module m8;
+
+extern "C++" {
+void bar();
+}
+
Index: clang/test/CXX/module/module.unit/p7/Inputs/h8.h
===
---