[clang-tools-extra] baeff98 - [clang] When loading preamble from AST file, re-export modules in Sema.

[Adam Czachorowski via cfe-commits]

Author: Adam Czachorowski
Date: 2020-08-20T14:19:52+02:00
New Revision: baeff989b050e0f63412c52c1b8f9d8f3e91f671

URL: https://github.com/llvm/llvm-project/commit/baeff989b050e0f63412c52c1b8f9d8f3e91f671
DIFF: https://github.com/llvm/llvm-project/commit/baeff989b050e0f63412c52c1b8f9d8f3e91f671.diff

LOG: [clang] When loading preamble from AST file, re-export modules in Sema.

This addresses a FIXME in ASTReader.

Modules were already re-exported for Preprocessor, but not for Sema.
The result was that, with -fmodules-local-submodule-visibility, all AST
nodes belonging to a module that was loaded in a premable where not
accesible from the main part of the file and a diagnostic recommending
importing those modules would be generated.

Differential Revision: https://reviews.llvm.org/D86069

Added: 
    clang/test/PCH/preamble-modules.cpp

Modified: 
    clang-tools-extra/clangd/unittests/ModulesTests.cpp
    clang/include/clang/Sema/Sema.h
    clang/lib/Serialization/ASTReader.cpp
    clang/test/PCH/Inputs/modules/Foo.h

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/unittests/ModulesTests.cpp b/clang-tools-extra/clangd/unittests/ModulesTests.cpp
index 0098a15f64bb..a10b9e897a48 100644
--- a/clang-tools-extra/clangd/unittests/ModulesTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ModulesTests.cpp
@@ -26,7 +26,7 @@ TEST(Modules, TextualIncludeInPreamble) {
     void foo() {}
 )cpp");
   TU.ExtraArgs.push_back("-fmodule-name=M");
-  TU.ExtraArgs.push_back("-fmodule-map-file=m.modulemap");
+  TU.ExtraArgs.push_back("-fmodule-map-file=" + testPath("m.modulemap"));
   TU.AdditionalFiles["Textual.h"] = "void foo();";
   TU.AdditionalFiles["m.modulemap"] = R"modulemap(
     module M {
@@ -39,6 +39,31 @@ TEST(Modules, TextualIncludeInPreamble) {
   TU.index();
 }
 
+// Verify that visibility of AST nodes belonging to modules, but loaded from
+// preamble PCH, is restored.
+TEST(Modules, PreambleBuildVisibility) {
+  TestTU TU = TestTU::withCode(R"cpp(
+    #include "module.h"
+
+    foo x;
+)cpp");
+  TU.OverlayRealFileSystemForModules = true;
+  TU.ExtraArgs.push_back("-fmodules");
+  TU.ExtraArgs.push_back("-fmodules-strict-decluse");
+  TU.ExtraArgs.push_back("-Xclang");
+  TU.ExtraArgs.push_back("-fmodules-local-submodule-visibility");
+  TU.ExtraArgs.push_back("-fmodule-map-file=" + testPath("m.modulemap"));
+  TU.AdditionalFiles["module.h"] = R"cpp(
+    typedef int foo;
+)cpp";
+  TU.AdditionalFiles["m.modulemap"] = R"modulemap(
+    module M {
+      header "module.h"
+    }
+)modulemap";
+  EXPECT_TRUE(TU.build().getDiagnostics().empty());
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang

diff  --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 49ab94f9df14..84e66b85208e 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -1912,6 +1912,12 @@ class Sema final {
 
   bool isModuleVisible(const Module *M, bool ModulePrivate = false);
 
+  // When loading a non-modular PCH files, this is used to restore module
+  // visibility.
+  void makeModuleVisible(Module *Mod, SourceLocation ImportLoc) {
+    VisibleModules.setVisible(Mod, ImportLoc);
+  }
+
   /// Determine whether a declaration is visible to name lookup.
   bool isVisible(const NamedDecl *D) {
     return D->isUnconditionallyVisible() || isVisibleSlow(D);

diff  --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 21cdde9e8455..464bbc662230 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -4987,10 +4987,10 @@ void ASTReader::InitializeContext() {
                         /*ImportLoc=*/Import.ImportLoc);
       if (Import.ImportLoc.isValid())
         PP.makeModuleVisible(Imported, Import.ImportLoc);
-      // FIXME: should we tell Sema to make the module visible too?
+      // This updates visibility for Preprocessor only. For Sema, which can be
+      // nullptr here, we do the same later, in UpdateSema().
     }
   }
-  ImportedModules.clear();
 }
 
 void ASTReader::finalizeForWriting() {
@@ -7943,6 +7943,15 @@ void ASTReader::UpdateSema() {
       SemaObj->FpPragmaStack.CurrentPragmaLocation = FpPragmaCurrentLocation;
     }
   }
+
+  // For non-modular AST files, restore visiblity of modules.
+  for (auto &Import : ImportedModules) {
+    if (Import.ImportLoc.isInvalid())
+      continue;
+    if (Module *Imported = getSubmodule(Import.ID)) {
+      SemaObj->makeModuleVisible(Imported, Import.ImportLoc);
+    }
+  }
 }
 
 IdentifierInfo *ASTReader::get(StringRef Name) {

diff  --git a/clang/test/PCH/Inputs/modules/Foo.h b/clang/test/PCH/Inputs/modules/Foo.h
index d661dbccbc33..c93614e0683f 100644
--- a/clang/test/PCH/Inputs/modules/Foo.h
+++ b/clang/test/PCH/Inputs/modules/Foo.h
@@ -1 +1,3 @@
 void make_foo(void);
+
+typedef int MyType;

diff  --git a/clang/test/PCH/preamble-modules.cpp b/clang/test/PCH/preamble-modules.cpp
new file mode 100644
index 000000000000..3dff4a4a3287
--- /dev/null
+++ b/clang/test/PCH/preamble-modules.cpp
@@ -0,0 +1,15 @@
+// Check that modules included in the preamble remain visible to the rest of the
+// file.
+
+// RUN: rm -rf %t.mcp
+// RUN: %clang_cc1 -emit-pch -o %t.pch %s -fmodules -fmodule-map-file=%S/Inputs/modules/module.modulemap -fmodules-local-submodule-visibility -fmodules-cache-path=%t.mcp
+// RUN: %clang_cc1 -include-pch %t.pch %s -fmodules -fmodule-map-file=%S/Inputs/modules/module.modulemap -fmodules-local-submodule-visibility -fmodules-cache-path=%t.mcp
+
+#ifndef MAIN_FILE
+#define MAIN_FILE
+// Premable section.
+#include "Inputs/modules/Foo.h"
+#else
+// Main section.
+MyType foo;
+#endif