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

[adamcz]

Summary

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.

[sammccall]

clang/test/PCH/preamble-modules.cpp

9

what are these ifdefs about? you never seem to define this symbol

(Possible you're copying from a test that is doing something tricky like being run in two modes or including itself? Or did you mean to give different flags for the include vs emit pch?)

15

This seems like currently it's never getting parsed as MAIN_FILE is never defined.

[adamcz]

clang/test/PCH/preamble-modules.cpp

9

Oh, I stole this trick from Kadir. You compile this file twice. First one is to generate a PCH for preamble. MAIN_FILE is not set, so it only reads the preamble section. The second one has -include-pch. That means MAIN_FILE is now set (from the preamble PCH file), so we only compile the main section.

It's a creative solution to a problem I wish I didn't have ;-)
Is there a better way to do this, without splitting the file in two? preamble_bytes seems fragile.

15

It's parsed. Without this fix, there's a warning generated here. See comment above for explanation.

[sammccall]

Sorry, I was just not reading the test clearly. LGTM

clang/lib/Serialization/ASTReader.cpp

4990

maybe replace this with a comment pointing at UpdateSema() for the sema part

[adamcz]

Added comment in place of a FIXME

clang-tools-extra/clangd/unittests/ModulesTests.cpp

 
 TEST(Modules, TextualIncludeInPreamble) {
   TestTU TU = TestTU::withCode(R"cpp(
     #include "Textual.h"
 
     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 {
       module Textual {
         textual header "Textual.h"
       }
     }
 )modulemap";
   // Test that we do not crash.
   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

clang/include/clang/Sema/Sema.h

public:
   }
 
   /// Make a merged definition of an existing hidden definition \p ND
   /// visible at the specified location.
   void makeMergedDefinitionVisible(NamedDecl *ND);
 
   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);
   }
 
   /// Determine whether any declaration of an entity is visible.
   bool
   hasVisibleDeclaration(const NamedDecl *D,

clang/lib/Serialization/ASTReader.cpp

void ASTReader::InitializeContext() {
   // Re-export any modules that were imported by a non-module AST file.
   // FIXME: This does not make macro-only imports visible again.
   for (auto &Import : ImportedModules) {
     if (Module *Imported = getSubmodule(Import.ID)) {
       makeModuleVisible(Imported, Module::AllVisible,
                         /*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() {
   // Nothing to do for now.
 }
 
 /// Reads and return the signature record from \p PCH's control block, or
 /// else returns 0.
if (FpPragmaCurrentLocation.isInvalid()) {
       assert(*FpPragmaCurrentValue == SemaObj->FpPragmaStack.DefaultValue &&
              "Expected a default pragma float_control value");
       // Keep the current values.
     } else {
       SemaObj->FpPragmaStack.CurrentValue = *FpPragmaCurrentValue;
       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) {
   // Note that we are loading an identifier.
   Deserializing AnIdentifier(this);
 
   IdentifierLookupVisitor Visitor(Name, /*PriorGeneration=*/0,
                                   NumIdentifierLookups,

clang/test/PCH/Inputs/modules/Foo.h

 void make_foo(void);
+
+typedef int MyType;

clang/test/PCH/preamble-modules.cpp

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