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

[ChuanqiXu]

Summary

Since the serialization code would recognize modules by names and the name of all global module fragment is <global>, 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

[ChuanqiXu]

@rsmith gentle ping~

[ChuanqiXu]

@rsmith gentle ping~

[ChuanqiXu]

@rsmith gentle ping~

[ChuanqiXu]

@rsmith gentle ping~

[urnathan]

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

[tbaeder]

clang/include/clang/Sema/Sema.h

2222

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

[ChuanqiXu]

Address comments

[ChuanqiXu]

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?)

clang/include/clang/Sema/Sema.h

2222

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".

[rsmith]

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

clang/lib/Sema/SemaModule.cpp

727

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).

[ChuanqiXu]

Address comments.

[ChuanqiXu]

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.

[urnathan]

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

[ChuanqiXu]

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.

clang/include/clang/Sema/Sema.h

struct ModuleScope {
     SourceLocation BeginLoc;
     clang::Module *Module = nullptr;
     bool ModuleInterface = false;
     bool ImplicitGlobalModuleFragment = false;
     VisibleModuleSet OuterVisibleModules;
   };
   /// The modules we're currently parsing.
   llvm::SmallVector<ModuleScope, 16> 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<const NamespaceDecl*, 8> DeferredExportedNamespaces;
 
   /// Get the module whose scope we are currently within.
   Module *getCurrentModule() const {
     return ModuleScopes.empty() ? nullptr : ModuleScopes.back().Module;
   }

clang/lib/Sema/SemaModule.cpp

if (!D->isInvalidDecl()) {
     }
   }
 
   return D;
 }
 
 Module *Sema::PushGlobalModuleFragment(SourceLocation BeginLoc,
                                        bool IsImplicit) {
+  // We shouldn't create new global module fragment if there is already
+  // one.
+  if (!GlobalModuleFragment) {
     ModuleMap &Map = PP.getHeaderSearchInfo().getModuleMap();
-  Module *GlobalModule =
-      Map.createGlobalModuleFragmentForModuleUnit(BeginLoc, getCurrentModule());
-  assert(GlobalModule && "module creation should not fail");
+    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() {
   assert(!ModuleScopes.empty() && getCurrentModule()->isGlobalModule() &&
          "left the wrong module scope, which is not global module fragment");
   ModuleScopes.pop_back();
 }

clang/test/CXX/module/module.unit/p7/Inputs/h8.h

+#ifndef H8
+#define H8
+void foo();
+#endif

clang/test/CXX/module/module.unit/p7/Inputs/m8.cppm

+module;
+#include "h8.h"
+export module m8;
+
+extern "C++" {
+void bar();
+}

clang/test/CXX/module/module.unit/p7/t8.cpp

+// 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;