[C++2a] [Module] Support extern C/C++ semantics

[ChuanqiXu]

Summary

According to [[ https://eel.is/c++draft/module.unit#7.2.3 | [module.unit]p7.2.3]], a declaration within a linkage-specification should be attached to the global module.
This let user to forward declare types across modules. Here is an example: https://godbolt.org/z/vEWjdrdq9

Test Plan: check-all, https://godbolt.org/z/vEWjdrdq9

[ChuanqiXu]

Add test.

[ChuanqiXu]

Update test.

[mizvekov]

Hello.

Just some minor nibbles, as I am a bit out of the loop on modules.

clang/include/clang/Basic/DiagnosticSemaKinds.td

10842–10845

def **err_module_language_linkage_no_global** : Error <
  "The declaration %0**, which** appears within a linkage-specification**, is not "
  "attached to **the** global module.">;

My two cents since I think the last sentence did not seem to explain the problem very well.

clang/lib/Sema/SemaDecl.cpp

9351

16435

This block is of code is identical to the one above. Can you put this into a function?

clang/lib/Sema/SemaModule.cpp

71

clang/test/CXX/module/module.linkage_specification/Inputs/h1.h

2

Please if possible make your editor set new lines at EOF.

[aaron.ballman]

Adding some more reviewers and subscribing the mailing lists.

clang/include/clang/Basic/DiagnosticSemaKinds.td

10842

typo, should be: err_module_language_linkage_no_global

10842–10845

Diagnostics in Clang are intentionally ungrammatical, so they should not have leading capitalization or most terminating punctuation. I agree that the diagnostic wording here doesn't really explain what's going wrong.

clang/lib/Sema/SemaDecl.cpp

9349

I'm a bit confused here. [module.unit]p7 is describing what module a declaration attached to under which circumstances. I don't see a constraint there which should result in a diagnostic. My reading of https://eel.is/c++draft/module.unit#6 is that the global module always exists, so we should be able to attach declarations to it at any point. Am I misunderstanding?

clang/test/CXX/module/module.linkage_specification/Inputs/h1.h

2

+1 -- please add a newline to the end of each of these files that.

[ChuanqiXu]

Address the comments.

[ChuanqiXu]

clang/lib/Sema/SemaDecl.cpp

9349

Yeah, after looking into this, I think you are right. We should create the global module when we need it but failed to find it.

[ChuanqiXu]

Format.

[rsmith]

Instead of moving functions and classes/enums into the global module after creating them, it would be better to push a module scope for the global module when entering a C or C++ language linkage specification and pop the module scope when leaving the linkage specification. There are lots of other kinds of declaration that can appear inside a linkage specification (for example, variables), and doing it that way would properly handle all of them, not only functions and classes.

clang/include/clang/Sema/Sema.h

2177

I think this should live on the ModuleScope instead of being separate. If we process multiple modules in a single compilation, each needs to have its own global module fragment so that we can restrict the visibility of the global module fragment to the module unit in which it was declared.

[ChuanqiXu]

Address the comments from @rsmith. It looks much better now.

[ChuanqiXu]

@rsmith @aaron.ballman gentle ping~

[ChuanqiXu]

@rsmith gentle ping~

[aaron.ballman]

clang/lib/Sema/SemaDeclCXX.cpp

16155 ↗

(On Diff #379907)

Module * instead of auto * (the type is not spelled out in the initialization).

clang/lib/Sema/SemaModule.cpp

82

Module *

717–718

Please spell these out as well.

722–725

Can we use emplace_back() and construct in place rather than constructing piecemeal?

732–733

FWIW, this seems to be failing to compile according to the precommit CI.

FAILED: tools/clang/lib/Sema/CMakeFiles/obj.clangSema.dir/SemaModule.cpp.o
CCACHE_CPP2=yes CCACHE_HASHDIR=yes /usr/bin/ccache /usr/bin/clang++ -DBUILD_EXAMPLES -DCLANG_ROUND_TRIP_CC1_ARGS=ON -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/clang/lib/Sema -I/var/lib/buildkite-agent/builds/llvm-project/clang/lib/Sema -I/var/lib/buildkite-agent/builds/llvm-project/clang/include -Itools/clang/include -Iinclude -I/var/lib/buildkite-agent/builds/llvm-project/llvm/include -gmlt -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -Wno-nested-anon-types -O3 -DNDEBUG  -fno-exceptions -fno-rtti -UNDEBUG -std=c++14 -MD -MT tools/clang/lib/Sema/CMakeFiles/obj.clangSema.dir/SemaModule.cpp.o -MF tools/clang/lib/Sema/CMakeFiles/obj.clangSema.dir/SemaModule.cpp.o.d -o tools/clang/lib/Sema/CMakeFiles/obj.clangSema.dir/SemaModule.cpp.o -c /var/lib/buildkite-agent/builds/llvm-project/clang/lib/Sema/SemaModule.cpp
/var/lib/buildkite-agent/builds/llvm-project/clang/lib/Sema/SemaModule.cpp:722:53: error: member reference type 'clang::Module *' is a pointer; did you mean to use '->'?
  assert(!ModuleScopes.empty() && getCurrentModule().isGlobalModule() &&
                                  ~~~~~~~~~~~~~~~~~~^
                                                    ->
/usr/include/assert.h:93:27: note: expanded from macro 'assert'
     (static_cast <bool> (expr)                                         \
                          ^~~~
1 error generated.

clang/test/CXX/module/module.linkage_specification/p1.cpp

2

Might as well switch all these to use -std=c++20.

[ChuanqiXu]

Address comments.

[ChuanqiXu]

clang/lib/Sema/SemaModule.cpp

732–733

Oh, my bad. I forgot to run tests under debug locally.

[ChuanqiXu]

@rsmith @aaron.ballman gentle ping~

[ChuanqiXu]

@rsmith ping~

[ChuanqiXu]

Change includes:

• Add parent for newly created global module fragment.
• Add a test.
• Rename test directory. Suddenly I found that the names of the tests in CXX directory are corresponds to the standard one by one. So my name before is not good.

@aaron.ballman @urnathan It looks like @rsmith is busy now. Could you please help to review this one? Many Thanks.

[ChuanqiXu]

Format codes.

[aaron.ballman]

In D110215#3139589, @ChuanqiXu wrote:

Change includes:

• Add parent for newly created global module fragment.
• Add a test.
• Rename test directory. Suddenly I found that the names of the tests in CXX directory are corresponds to the standard one by one. So my name before is not good.

@aaron.ballman @urnathan It looks like @rsmith is busy now. Could you please help to review this one? Many Thanks.

@ChuanqiXu -- I'm happy to take a look (all of your modules patches are still on my review queue), but I've been in WG14 meetings all week this week and will be on vacation all week next week. Given the complexity and importance of the patches, it may be a bit before I can give this a thorough review. (If you get approval from other trusted reviewers before I get to anything, please don't let me hold any of the patches up!)

[ChuanqiXu]

In D110215#3139809, @aaron.ballman wrote:

In D110215#3139589, @ChuanqiXu wrote:

Change includes:

• Add parent for newly created global module fragment.
• Add a test.
• Rename test directory. Suddenly I found that the names of the tests in CXX directory are corresponds to the standard one by one. So my name before is not good.

@aaron.ballman @urnathan It looks like @rsmith is busy now. Could you please help to review this one? Many Thanks.

@ChuanqiXu -- I'm happy to take a look (all of your modules patches are still on my review queue), but I've been in WG14 meetings all week this week and will be on vacation all week next week. Given the complexity and importance of the patches, it may be a bit before I can give this a thorough review. (If you get approval from other trusted reviewers before I get to anything, please don't let me hold any of the patches up!)

Got it. Many thanks! It is great enough to know the review process is going on. I completely understand that we should be very careful about these patches.

[ChuanqiXu]

@aaron.ballman @urnathan @rsmith ping. Might you take a look?

[rsmith]

Thanks, this looks great. A couple of minor suggestions.

clang/include/clang/Lex/ModuleMap.h

542–543 ↗

(On Diff #388151)

We generally refer to sections of the C++ standard by name not by URL.

clang/lib/Sema/SemaDeclCXX.cpp

16158 ↗

(On Diff #388151)

I think this should be ModulePrivate -- such a global module fragment should not be visible to importers of this module. (I think it probably doesn't matter in practice since visibility is generally monotonically increasing while compiling a module unit, but ModulePrivate seems more correct in principle.)

[ChuanqiXu]

Address comments.

[vitalybuka]

llvm-project/clang/lib/Sema/SemaModule.cpp:715:70: warning: missing field 'OuterVisibleModules' initializer [-Wmissing-field-initializers]
                                                                    ^

[ChuanqiXu]

In D110215#3178575, @vitalybuka wrote:

llvm-project/clang/lib/Sema/SemaModule.cpp:715:70: warning: missing field 'OuterVisibleModules' initializer [-Wmissing-field-initializers]
                                                                    ^

I had tried to fix this in a NFC commit: 4168efe1b. Hope this works.
(My compiler locally didn't warn for it... I need to update it...)

clang/include/clang/Basic/DiagnosticSemaKinds.td

   "missing 'export' specifier in module declaration while "
   "building module interface">;
 def err_current_module_name_mismatch : Error<
   "module name '%0' specified on command line does not match name of module">;
 def err_module_redefinition : Error<
   "redefinition of module '%0'">;
 def note_prev_module_definition : Note<"previously defined here">;
 def note_prev_module_definition_from_ast_file : Note<"module loaded from '%0'">;
+def err_module_langugae_linkage_no_global : Error <
+  "The declaration %0 appears within a linkage-specification should be "
+  "attached to global module. But the compiler failed to search the global "
+  "module.">;
 def err_module_not_defined : Error<
   "definition of module '%0' is not available; use -fmodule-file= to specify "
   "path to precompiled module interface">;
 def err_module_redeclaration : Error<
   "translation unit contains multiple module declarations">;
 def note_prev_module_declaration : Note<"previous module declaration is here">;
 def err_module_declaration_missing : Error<
   "missing 'export module' declaration in module interface unit">;
 def err_module_declaration_missing_after_global_module_introducer : Error<
   "missing 'module' declaration at end of global module fragment "

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;
+  Module *GlobalModule = 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;
   }
 
+  Module *getGlobalModule() const { return GlobalModule; }
+
   VisibleModuleSet VisibleModules;
 
 public:
   /// Get the module owning an entity.
   Module *getOwningModule(const Decl *Entity) {
     return Entity->getOwningModule();
   }
 

clang/lib/Sema/SemaDecl.cpp

if (D.getDeclSpec().isModulePrivateSpecified()) {
           << FixItHint::CreateRemoval(ModulePrivateLoc);
       } else {
         NewFD->setModulePrivate();
         if (FunctionTemplate)
           FunctionTemplate->setModulePrivate();
       }
     }
 
+    // C++ [module.unit]p7.2
+    // Otherwise, if the declaration
+    //  - ...
+    //  - ...
+    //  - appears within a linkage-specification
+    // it is attached to the global module.
+    if (Module *M = NewFD->getOwningModule())
+      if (M->Kind == Module::ModuleInterfaceUnit &&
+          (NewFD->isExternCContext() || NewFD->isExternCXXContext())) {
+        if (!GlobalModule)
+          Diag(NewFD->getLocation(),
+               diag::err_module_langugae_linkage_no_global)
+              << NewFD->getName();
+        NewFD->setLocalOwningModule(GlobalModule);
+        NewFD->setModuleOwnershipKind(Decl::ModuleOwnershipKind::Unowned);
+      }
+
     if (isFriend) {
       if (FunctionTemplate) {
         FunctionTemplate->setObjectOfFriendDecl();
         FunctionTemplate->setAccess(AS_public);
       }
       NewFD->setObjectOfFriendDecl();
       NewFD->setAccess(AS_public);
     }
if (isMemberSpecialization)
         << FixItHint::CreateRemoval(ModulePrivateLoc);
     // __module_private__ does not apply to local classes. However, we only
     // diagnose this as an error when the declaration specifiers are
     // freestanding. Here, we just ignore the __module_private__.
     else if (!SearchDC->isFunctionOrMethod())
       New->setModulePrivate();
   }
 
+  // C++ [module.unit]p7.2
+  // Otherwise, if the declaration
+  //  - ...
+  //  - ...
+  //  - appears within a linkage-specification
+  // it is attached to the global module.
+  if (Module *M = New->getOwningModule())
+    if (M->Kind == Module::ModuleInterfaceUnit &&
+        (New->isExternCContext() || New->isExternCXXContext())) {
+      if (!GlobalModule)
+        Diag(New->getLocation(), diag::err_module_langugae_linkage_no_global)
+            << New->getName();
+      New->setLocalOwningModule(GlobalModule);
+      New->setModuleOwnershipKind(Decl::ModuleOwnershipKind::Unowned);
+    }
+
   // If this is a specialization of a member class (of a class template),
   // check the specialization.
   if (isMemberSpecialization && CheckMemberSpecialization(New, Previous))
     Invalid = true;
 
   // If we're declaring or defining a tag in function prototype scope in C,
   // note that this type can only be used within the function and add it to
   // the list of decls to inject into the function definition scope.

clang/lib/Sema/SemaModule.cpp

if (!ModuleScopes.empty() &&
     // Under -std=c++2a -fmodules-ts, we can find an explicit 'module;' after
     // already implicitly entering the global module fragment. That's OK.
     assert(getLangOpts().CPlusPlusModules && getLangOpts().ModulesTS &&
            "unexpectedly encountered multiple global module fragment decls");
     ModuleScopes.back().BeginLoc = ModuleLoc;
     return nullptr;
   }
 
+  assert(
+      !GlobalModule &&
+      "Global module already created but didn't covered by the check above.");
   // We start in the global module; all those declarations are implicitly
   // module-private (though they do not have module linkage).
   auto &Map = PP.getHeaderSearchInfo().getModuleMap();
-  auto *GlobalModule = Map.createGlobalModuleFragmentForModuleUnit(ModuleLoc);
+  GlobalModule = Map.createGlobalModuleFragmentForModuleUnit(ModuleLoc);
   assert(GlobalModule && "module creation should not fail");
 
   // Enter the scope of the global module.
   ModuleScopes.push_back({});
   ModuleScopes.back().BeginLoc = ModuleLoc;
   ModuleScopes.back().Module = GlobalModule;
   VisibleModules.setVisible(GlobalModule, ModuleLoc);
 
   // All declarations created from now on are owned by the global module.
   auto *TU = Context.getTranslationUnitDecl();
   TU->setModuleOwnershipKind(Decl::ModuleOwnershipKind::Visible);
   TU->setLocalOwningModule(GlobalModule);
 
   // FIXME: Consider creating an explicit representation of this declaration.
   return nullptr;
for (auto *Child : ED->decls()) {
         // no declarations (transitively), in which case it's ill-formed.
         diagExportedUnnamedDecl(*this, UnnamedDeclKind::Context, Child,
                                 BlockStart);
       }
     }
   }
 
   return D;
 }

clang/test/CXX/module/module.linkage_specification/Inputs/h1.h

+extern "C" void foo();
 No newline at end of file

clang/test/CXX/module/module.linkage_specification/Inputs/h2.h

+extern "C++" class CPP;
 No newline at end of file

clang/test/CXX/module/module.linkage_specification/Inputs/h4.h

+extern "C" struct C;
 No newline at end of file

clang/test/CXX/module/module.linkage_specification/p1.cpp

+// RUN: %clang_cc1 -std=c++2a %s -verify
+// expected-no-diagnostics
+module;
+
+#include "Inputs/h1.h"
+
+export module x;
+
+extern "C" void foo() {
+  return;
+}

clang/test/CXX/module/module.linkage_specification/p2.cpp

+// RUN: %clang_cc1 -std=c++2a %s -verify
+// expected-no-diagnostics
+module;
+
+#include "Inputs/h2.h"
+
+export module x;
+
+extern "C++" class CPP {};

clang/test/CXX/module/module.linkage_specification/p3.cpp

+// RUN: %clang_cc1 -std=c++2a %s -verify
+export module x;
+
+extern "C" void foo();     // expected-error {{The declaration foo appears within a linkage-specification should be attached to global module. But the compiler failed to search the global module.}}
+extern "C++" class CPP {}; // expected-error {{The declaration CPP appears within a linkage-specification should be attached to global module. But the compiler failed to search the global module.}}

clang/test/CXX/module/module.linkage_specification/p4.cpp

+// RUN: %clang_cc1 -std=c++2a %s -verify
+// expected-no-diagnostics
+module;
+
+#include "Inputs/h4.h"
+
+export module x;
+
+extern "C" struct C {
+  int a;
+  int b;
+  double d;
+};
 No newline at end of file