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

[Adam Czachorowski via Phabricator via cfe-commits]

adamcz updated this revision to Diff 286561.
adamcz added a comment.

Added comment in place of a FIXME


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86069

Files:
  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
  clang/test/PCH/preamble-modules.cpp


Index: clang/test/PCH/preamble-modules.cpp
===================================================================
--- /dev/null
+++ 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
Index: clang/test/PCH/Inputs/modules/Foo.h
===================================================================
--- clang/test/PCH/Inputs/modules/Foo.h
+++ clang/test/PCH/Inputs/modules/Foo.h
@@ -1 +1,3 @@
 void make_foo(void);
+
+typedef int MyType;
Index: clang/lib/Serialization/ASTReader.cpp
===================================================================
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -4987,10 +4987,10 @@
                         /*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() {
@@ -7942,6 +7942,15 @@
       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) {
Index: clang/include/clang/Sema/Sema.h
===================================================================
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -1912,6 +1912,12 @@
 
   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);
Index: clang-tools-extra/clangd/unittests/ModulesTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/ModulesTests.cpp
+++ clang-tools-extra/clangd/unittests/ModulesTests.cpp
@@ -26,7 +26,7 @@
     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 @@
   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

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