[clang][modules] Track included files per submodule

[jansvoboda11]

Summary

When building a module consisting of submodules, the preprocessor keeps a "global" state that for each header file tracks (amongst other things) the number of times it was included/imported. This information is serialized into the PCM file.

When importing anything from such module (either the top-level module or any of its submodules), this number is merged into the state of the importing preprocessor.

This can incorrectly prevent imports of textual headers (see attached tests). This patch fixes this bug by making the number of times a header was included more fine-grained and tracks it on per-submodule basis. This information is lazily deserialized when first importing each (sub)module.

(This patch is an alternative approach to the same issue addressed in D104344.)

Depends on D114095.

[jansvoboda11]

I'm interested in hearing some feedback whether the direction I'm taking here makes sense.

There are a couple of TODOs (mostly on optimizing away unnecessary maps) and a few modules-ts tests are failing.

@rsmith left a suggestion on D104344 to track this information in Preprocessor::SubmoduleState, which has similar semantics to what I'm doing with Preprocessor::IncludeMap (dividing the state during submodule compilation). The issue is that it (Preprocessor::Submodules) is really only enabled when Clang gets invoked with -fmodules-local-submodule-visibility. To keep its current semantics and make it usable for our use-case (which needs to work even without the flag), I think we'd need to always track the submodule state and conditionally merge the outer and local macro states as we now do for IncludeMap in PPLexerChange.cpp. I'm not really sure this is correct/feasible.

Any opinions on this?

[vsapsai]

I'm not going to cover the entire change, some parts I need to consider more carefully.

There can be other reasons to keep IncludeMap out of SubmoduleState but I'm not sure the local submodule visibility is the right reason. I might be reading the code incorrectly but looks like CurSubmoduleState is used when local submodule visibility is disabled. The difference is it's tracking the aggregate state instead of per-submodule state. Need to experiment more but so far tracking includes in SubmoduleState follows the spirit of local submodule visibility. Though it's not guaranteed it'll work perfectly from the technical perspective.

Also I think we'll need to increase granularity to track other HeaderFileInfo attributes, not just NumIncludes. Don't have a test case to illustrate that right now and no need to change that now but something to keep in mind.

clang/lib/Lex/PPLexerChange.cpp

700

How many includes are expected to be here? Are this only immediate includes or also transitive?

Asking to evaluate how expensive iterating through the includes can get.

[jansvoboda11]

Avoid copying data between submodules

[jansvoboda11]

In D112915#3104873, @vsapsai wrote:

There can be other reasons to keep IncludeMap out of SubmoduleState but I'm not sure the local submodule visibility is the right reason. I might be reading the code incorrectly but looks like CurSubmoduleState is used when local submodule visibility is disabled. The difference is it's tracking the aggregate state instead of per-submodule state. Need to experiment more but so far tracking includes in SubmoduleState follows the spirit of local submodule visibility. Though it's not guaranteed it'll work perfectly from the technical perspective.

Yes, CurSubmoduleState is being used unconditionally. However, without local submodule visibility enabled, it always points to NullSubmoduleState. Only with the feature enabled does it point to the current submodule state (stored in Submodules). The change happens in Preprocessor::{Enter,Leave}Submodule.

Also I think we'll need to increase granularity to track other HeaderFileInfo attributes, not just NumIncludes. Don't have a test case to illustrate that right now and no need to change that now but something to keep in mind.

That's interesting. I think HeaderFileInfo::isImport should definitely be tracked in the preprocessor, not in HeaderFileInfo. The fact that the header was #imported is not an intrinsic property of the file itself, but rather a preprocessor state. Can you think of other fields that don't really belong to HeaderFileInfo?

Based on your feedback, I simplified the patch quite a bit. We're no longer copying the include state between submodules. In its current form, this patch essentially moves HeaderFileInfo::NumIncludes into Preprocessor::NumIncludes and still uses it as the source of truth.
However, we're also tracking NumIncludes separately in each submodule and serializing this into the PCM. Instead of merging NumIncludes of the whole module when loading it (which we did before), we can merge NumIncludes only of the modules we actually import.

clang/lib/Lex/Preprocessor.cpp

1328

Iterating over all FileEntries is probably not very efficient, as Volodymyr mentioned. Thinking about how to make this more efficient...

[dexonsmith]

Just a few comments on implementation details. The only high-level piece to call out is that I wonder if NumIncludes could/should be simplified (semantically) to a Boolean in a prep commit.

clang/include/clang/Lex/Preprocessor.h

720–723

I'm not sure about this choice. UIDs are unlikely to be adjacent and in SubmoduleIncludeState, since a given submodule is unlikely to have "most" files. Also, memory usage characteristics could be "weird" if the FileManager is being reused between different compilations.

DenseMap<unsigned, unsigned> will have the same number of allocations (i.e., just 1). If UIDs really are dense and near each other in one of the submodules then that map will be a little bigger than necessary (~2x), but it should be better for the rest of them.

clang/lib/Lex/Preprocessor.cpp

1328

My suggestion above to drop FileEntryMap in favour of a simple DenseMap would help a bit, just iterating through the files actually included by the submodules.

Further, I wonder if "num-includes"/file/submodule (unsigned) is actually useful, vs. "was-included"/file/submodule (bool). The only observer I see is HeaderSearch::PrintStats() but maybe I missed something?

If I'm right and we can switch to bool, then NumIncludes becomes a DenseSet<FileEntry *> IncludedFiles (or DenseSet<unsigned> for UIDs). (BTW, I also wondered if you could rotate the map, using File as the outer key, and then use bitsets for the sbumodules, but I doubt this is better, since most files are only included by a few submodules, right?)

Then you can just do a set union here. Also simplifies bitcode serialization.

(If a bool/set is sufficient, then I'd suggest landing that first/separately, before adding the per-submodule granularity in this patch.)

clang/lib/Serialization/ASTWriter.cpp

2410–2411

A vector of maps would be an improvement, but that'll still be a lot of allocations.

• Since insertion/lookup/deletion aren't intermingled, the simplest way to avoid adding unnecessary overhead is a sorted vector (https://llvm.org/docs/ProgrammersManual.html#dss-sortedvectormap).
• With no lookups (at all), there's no benefit to a tiered data structure (vs flat).

Leading me toward a simple flat vector + sort.

struct IncludeToSerialize { // Probably more straightforward than a std::tuple...
  StringRef Filename;
  unsigned SMID;
  unsigned NumIncludes;

  bool operator<(const IncludeToSerialize &RHS) const {
    if (SMID != RHS.SMID)
      return SMID < RHS.SMID;
    int Diff = Filename.compare(RHS.Filename);
    assert(Diff && "Expected unique SMID / Filename pairs");
    return Diff < 0;
  }
};
SmallVector<IncludeToSerialize> IncludesToSerialize;
// ...
    IncludesToSerialize.push_back({Filename, LocalSMID, NumIncludes});
// ...
llvm::sort(IncludesToSerialize);
for (const IncludeToSerialize &SI : IncludesToSerialize) {
  // emit record
}

(Or if there are duplicates expected to be encountered and ignored, you can remove the assertion and use stable_sort + unique + erase.)

2436

I wonder, will the Filename already be serialized elsewhere? Could an ID from that be reused here, rather than writing the filename again? (Maybe that'd need a bigger refactor of some sort to create a filename table?)

Stepping back, it looks like this is always eagerly loaded.

• Could it be lazily-loaded by submodule?
• Could it be lazily-loaded by filename?

In the former case, seems like a single record per submodule makes sense, with a single blob that can be decoded on-demand.

In the latter case, maybe it should be rotated, and stored a single record per filename as a blob that can be lazily decoded:

<filename-size> <filename> <num-submodules> (<smid> <num-includes>)+

[vsapsai]

In D112915#3106472, @jansvoboda11 wrote:

That's interesting. I think HeaderFileInfo::isImport should definitely be tracked in the preprocessor, not in HeaderFileInfo. The fact that the header was #imported is not an intrinsic property of the file itself, but rather a preprocessor state. Can you think of other fields that don't really belong to HeaderFileInfo?

After checking HeaderFileInfo, looks like isImport is the only other field that should be tracked in the preprocessor. I had in mind a case where a hidden submodule imports a file with x-macros and then a visible submodule includes this header twice with different macros. First include would go through because NumIncludes == 0, and the second one shouldn't because NumIncludes == 1 && isImport == true. The import in the hidden submodule is incorrect but errors in unused headers shouldn't break actually used headers.

[jansvoboda11]

Call getFileInfo in Preprocessor::EnterMainSourceFile. This ensures deserialization of HeaderFileInfo, which seems to be necessary with modules-ts enabled.

[jansvoboda11]

Fix deserialization, improve (sub)module state tracking

[jansvoboda11]

Make loading of (sub)module includes lazy

[jansvoboda11]

Thanks for your feedback @dexonsmith. I reworked the patch to use more sensible data structures as suggested, and made the AST deserialization lazy (on (sub)module import).

I think the only thing to figure out is the exact structure of the serialized information - whether we're fine with serializing all transitively included files in each AST file, or whether we'd like to fetch that information from other AST files instead.

I removed the WIP tag and would be happy to gather more feedback.

clang/include/clang/Lex/Preprocessor.h

720–723

You're right that UIDs of files included in a single submodule are unlikely to have "most" files. Thanks for pointing that out.

In the latest revision, I switched to llvm::DenseMap<const FileEntry *, unsigned>.

clang/lib/Lex/Preprocessor.cpp

1328

For each file, we need to have three distinct states: not included at all, included exactly once (firstTimeLexingFile), included more than once. This means we can't use a single DenseSet.

But we could use a DenseMap<Key, bool>, where "not included at all" can be expressed as being absent from the map, exactly once as having true in the map and more than once as having false in the map. Alternatively, we could use two DenseSet instances to encode the same, but I don't think having two lookups per file to determine stuff is efficient.

I can look into this in a follow-up patch.

clang/lib/Serialization/ASTWriter.cpp

2410–2411

In the latest revision, I ended up sorting just based on Filename, since this is now explicitly stored per submodule.

2436

We already serialize Filename elsewhere, but only for local input files. Here we need transitive closure of all included input files.

I'm still unsure whether it's fine to store transitively included files here or if we should look that information up in the respective AST files.

The current solution looks like it will bloat sizes of the AST files, but I think the transitive closure is already being stored for HeaderFileInfo anyways, so it shouldn't be that big of a deal?

2436

Thinking about it some more, the current implementation will be duplicating a lot of filenames between submodules of the same module. We might need to extract that to some common storage that we can refer to with simple integer offsets...

[jansvoboda11]

Make clang-format happy.

[jansvoboda11]

Store only direct includes in the PCM (compared to transitive stored previously), use InputFile ID (instead of full filesystem path).

Also: add test of transitive includes, fix bug in VisibleModuleSet::setVisible.

[dexonsmith]

Implementation looks a lot cleaner!

I'd still like to drop NumIncludes first if possible because I think it'll be easier to reason about without this extra layer of complexity. Also, that'd mitigate the potential regression in .pcm size.

(Note: I'd be more comfortable with @vsapsai and/or @rsmith reviewing the overall direction; I just jumped in for the implementation details.)

clang/include/clang/Lex/HeaderSearch.h

133–139 ↗

(On Diff #386139)

Looks like this is already dead code? If so, please separate out and commit ahead of time (e.g., now).

clang/include/clang/Serialization/ModuleFile.h

399

Each StringMapEntry is going to have a pretty big allocation here, for a 512B vector. Given that it doesn't need to be after creation, it'd be more efficient to use this pattern:

llvm::StringMap<ArrayRef<uint64_t>> SubmoduleIncludedFiles;
SpecificBumpPtrAlloc<uint64_t> SubmoduleIncludedFilesAlloc;

// later
MutableArrayRef<uint64_t> Files(SubmoduleIncludedFiles.Allocate(Record.size()), Record.size());
llvm::copy(Record, Files.begin());
SubmoduleIncludedFiles[Key] = Files;

That said, I feel like this should just be:

DenseMap<StringRef, StringRef> SubmoduleIncludedFiles;

The key can point at the name of the submodule, which must already exist somewhere without needing a StringMap to create a new copy of it. The value is a lazily deserialized blob.

clang/lib/Lex/Preprocessor.cpp

1328

Seems like a DenseSet could still be used by having HeaderInfo pass back the WasInserted bit from the insertion to the preprocessor, and threading it through to Preprocessor::HandleEndOfFile (the only caller of FirstTimeLexingFile):

bool IsFirst = Set.insert(Key).second;

The threading doesn't seem too hard. Looking at main:

• Preprocessor::HandleHeaderIncludeOrImport calls HeaderInfo::ShouldEnterIncludeFile. This does the ++FI.NumIncludes (going from 0 to 1). Instead, it could be IsFirst = !FI.WasIncluded; FI.WasIncluded = true;, then return IsFirst somehow. (Then your patch can pull IsFirst from the insert().second).
• Preprocessor::HandleHeaderIncludeOrImport calls Preprocessor::EnterSourceFile. This creates a new Lexer for that file. IsFirst can be stored on that Lexer.
• Preprocessor::HandleEndOfFile calls FirstTimeLexingFile. Instead, it can check the new accessor CurLexer->isFirstTimeLexing().

I can look into this in a follow-up patch.

Follow-up might be okay, but it'd be nice to remove an axis of complexity before adding a new one if it's reasonable. E.g., it'll be easier to debug emergent issues from changing it to a simple set since there's less machinery to worry about.

clang/lib/Serialization/ASTReader.cpp

5738–5740

This looks lazy, but a bunch of work was just done to decode the Record from bitcode. To make this actually lazy, you can encode the data in a blob, which doesn't have to be decoded until it's used.

clang/lib/Serialization/ASTWriter.cpp

2173–2177

Why does the count need to be encoded?

The only observer is Preprocessor::HandleEndOfFile. If it gets called again for this file, it'll be after ++NumIncludes.

[vsapsai]

As we've discussed earlier, tracking isImport shouldn't be done per .pcm and here is the test case https://gist.github.com/vsapsai/a2d2bd19c54c24540495fd9b262106aa I'm not sure it is worth adding the second #include as the test fails just with one.

Overall, the change seems more complicated than it has to be. I need to check it carefully to see what can be simplified. And I need to check in debugger how and when AST reading is triggered. Looks like not all of that is lazy but I need to check the compiled code, not my guesses.

[vsapsai]

Didn't go in-depth for serialization/deserialization. When we add tracking isImport on per-submodule basis, do you think AST reading/writing would change significantly?

clang/include/clang/Lex/ExternalPreprocessorSource.h

48

I think it is better for understanding and more convenient to use some using instead of duplicating llvm::DenseMap<const FileEntry *, unsigned> in multiple places.

clang/include/clang/Lex/Preprocessor.h

775–781

I think the interplay between CurSubmoduleIncludeState, IncludedFiles, and CurSubmoduleState is pretty complicated. Recently I've realized that it can be beneficial to distinguish include tracking for the purpose of serializing per submodule and for the purpose of deciding if should enter a file. In D114051 I've tried to illustrate this approach. There are some tests failing but hope the main idea is still understandable.

One of my big concerns is tracking VisibleModules in multiple places. D114051 demonstrates one of the ways to deal with it but I think it is more important for you to know the problem I was trying to solve, not the solution I came up with.

clang/lib/Basic/Module.cpp

653

Was meaning to make this fix for a long time but couldn't test it. Thanks for finally fixing it!

clang/lib/Lex/Preprocessor.cpp

1327

If I drop checking getLocalSubmoduleIncludes, no tests are failing. But it seems like this call is required. How can we test it?

[jansvoboda11]

Rebase on top of extracted patches.

[jansvoboda11]

In D112915#3122340, @dexonsmith wrote:

Implementation looks a lot cleaner!

I'd still like to drop NumIncludes first if possible because I think it'll be easier to reason about without this extra layer of complexity. Also, that'd mitigate the potential regression in .pcm size.

Done in D114096. Thanks for the feedback.

clang/include/clang/Lex/HeaderSearch.h

133–139 ↗

(On Diff #386139)

Done in D114092.

clang/include/clang/Serialization/ModuleFile.h

399

I switched to StringRef value in the latest revision. Unfortunately, had to use std::string as the key instead of StringRef, since getFullModuleName constructs the string on heap. That forced me to use std::map, too. I'll explore using something else entirely as the key.

clang/lib/Lex/Preprocessor.cpp

1328

Extracted into D114093.

[jansvoboda11]

In D112915#3136492, @vsapsai wrote:

Didn't go in-depth for serialization/deserialization. When we add tracking isImport on per-submodule basis, do you think AST reading/writing would change significantly?

I think moving isImport into Preprocessor can be done in a similar way to how we handle the number of includes (or rather "has been included/imported" behavior), so we should be able to reuse some parts of this patch there.

One question that remains to be answered is whether to keep two separate DenseSet<const FileEntry *> instances, or merge them into something like:

struct IncludeInfo {
  bool IsImportedOrIncluded; // currently Preprocessor::IncludedFiles
  bool IsImported;           // currently HeaderFileInfo::isImport
}
llvm::DenseMap<const FileEntry *, IncludeInfo> IncludedFilesInfo;

Thanks a lot for the feedback, I appreciate it.

clang/include/clang/Lex/ExternalPreprocessorSource.h

48

Yeah, spelling the type out everywhere is a bit unwieldy. It's a bit better with llvm::DenseSet<const FileEntry *> in the latest revision, but still not pretty. I wanted to avoid pulling Preprocessor.h everywhere, but will probably reconsider doing that.

clang/include/clang/Lex/Preprocessor.h

775–781

Thanks a ton, this must've taken quite a bit of time to put together. I agree your approach is much simpler. I'll investigate how it behaves on larger projects, but probably will end up adopting it in this patch.

I have done some minor tweaks locally to fix the failing PCH tests, Modules/import-textual-noguard.mm doesn't make much sense to me, I think I'll end up updating that test.

clang/lib/Lex/Preprocessor.cpp

1327

I think this should kick in when importing a submodule from the same module. I'll try to come up with a test case.

[dexonsmith]

clang/include/clang/Serialization/ModuleFile.h

399

Oh, if the key isn't being kept alive elsewhere, you can/should use StringMap<StringRef>, which is strictly better than std::map<std::string, StringRef> (except for non-deterministic iteration order). I suggested otherwise because I assumed the key already existed.

Also, if the map isn't being deleted from (much), might as well bump-ptr-allocate it:

BumpPtrAllocator SubmoduleIncludedFilesAlloc;
StringMap<StringRef, BumpPtrAllocator> SubmoduleIncludedFiles; // iniitalized with teh Alloc above.

I'll explore using something else entirely as the key.

Not necessarily important; just if the key was already (e.g., stored in its Module or something) you might as well use it. Unless, maybe if the Module is known to be constructed already, you could use its address?

399

I switched to StringRef value in the latest revision. Unfortunately, had to use std::string as the key instead of StringRef, since getFullModuleName constructs the string on heap. That forced me to use std::map, too. I'll explore using something else entirely as the key.

[jansvoboda11]

Rebase, add type alias, improve map allocation in ASTReader.

[jansvoboda11]

Apply part of Volodymyr's cleanup.

[jansvoboda11]

Add missed newline.

[vsapsai]

I think AST format for IncludedFiles was discussed here, so I'll continue here though the bulk of implementation is in D114095 now. Have you compared the size of resulting .pcm files when you are using a bitvector compared to a list of included headers? In my quick check (which is not a perfect comparison, to be honest) bitvector approach takes more space. For example, Darwin.pcm is 7320 bytes bigger, UIKit.pcm is 2388 bytes bigger, and entire modules cache is 14KB bigger. I haven't checked the details of the discrepancies, so curious if you have some insights already. For the record, I was testing with

echo '#import <UIKit/UIKit.h>' | path/to/built/bin/clang -fsyntax-only -isysroot "$(xcrun --sdk iphoneos --show-sdk-path)" -target arm64-apple-ios -fmodules -fmodules-cache-path=modules.noindex -x objective-c -

[jansvoboda11]

Rebase

[jansvoboda11]

In D112915#3141417, @vsapsai wrote:

I think AST format for IncludedFiles was discussed here, so I'll continue here though the bulk of implementation is in D114095 now. Have you compared the size of resulting .pcm files when you are using a bitvector compared to a list of included headers? In my quick check (which is not a perfect comparison, to be honest) bitvector approach takes more space. For example, Darwin.pcm is 7320 bytes bigger, UIKit.pcm is 2388 bytes bigger, and entire modules cache is 14KB bigger. I haven't checked the details of the discrepancies, so curious if you have some insights already. For the record, I was testing with

echo '#import <UIKit/UIKit.h>' | path/to/built/bin/clang -fsyntax-only -isysroot "$(xcrun --sdk iphoneos --show-sdk-path)" -target arm64-apple-ios -fmodules -fmodules-cache-path=modules.noindex -x objective-c -

Replied in D114095.

[vsapsai]

Can you please rebase this change after D114095 lands? Overall looks good but I want to take one final look and triggering the pre-merge checks will be useful.

[jansvoboda11]

Rebase, update unnecessary auto, run clang-format

[vsapsai]

clang/include/clang/Lex/Preprocessor.h

1272–1281

Was curious why getNullSubmoduleIncludes isn't getLocalSubmoduleIncludes(nullptr)? If we want to have separate methods and implementations, it might be useful to assert M isn't null in getLocalSubmoduleIncludes.

clang/lib/Serialization/ASTReader.cpp

8626

Can you please check again the returned pointer doesn't end up as a dangling pointer? I don't think we store the pointer anywhere, which is good. My bigger concern is if we can invalidate SubmoduleIncludedFiles iterator while working with the returned pointer. I haven't found any indication of that but would like somebody else to check that.

clang/include/clang/Lex/ExternalPreprocessorSource.h

 //===- ExternalPreprocessorSource.h - Abstract Macro Interface --*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file defines the ExternalPreprocessorSource interface, which enables
 //  construction of macro definitions from some external source.
 //
 //===----------------------------------------------------------------------===//
 #ifndef LLVM_CLANG_LEX_EXTERNALPREPROCESSORSOURCE_H
 #define LLVM_CLANG_LEX_EXTERNALPREPROCESSORSOURCE_H
 
+#include "llvm/ADT/DenseSet.h"
+
 namespace clang {
 
+class FileEntry;
 class IdentifierInfo;
 class Module;
 
 /// Abstract interface for external sources of preprocessor
 /// information.
 ///
 /// This abstract class allows an external sources (such as the \c ASTReader)
 /// to provide additional preprocessing information.
public:
 
   /// Return the identifier associated with the given ID number.
   ///
   /// The ID 0 is associated with the NULL identifier.
   virtual IdentifierInfo *GetIdentifier(unsigned ID) = 0;
 
   /// Map a module ID to a module.
   virtual Module *getModule(unsigned ModuleID) = 0;
+
+  /// Return the files directly included in the given (sub)module.
+  virtual const llvm::DenseSet<const FileEntry *> *
+  getIncludedFiles(Module *M) = 0;
 };
 
 }
 
 #endif

clang/include/clang/Lex/Preprocessor.h

private:
 
   /// For each IdentifierInfo that was associated with a macro, we
   /// keep a mapping to the history of all macro definitions and #undefs in
   /// the reverse order (the latest one is in the head of the list).
   ///
   /// This mapping lives within the \p CurSubmoduleState.
   using MacroMap = llvm::DenseMap<const IdentifierInfo *, MacroState>;
 
   struct SubmoduleState;
+  struct SubmoduleIncludeState;
 
   /// Information about a submodule that we're currently building.
   struct BuildingSubmoduleInfo {
     /// The module that we are building.
     Module *M;
 
     /// The location at which the module was included.
     SourceLocation ImportLoc;
 
     /// Whether we entered this submodule via a pragma.
     bool IsPragma;
 
     /// The previous SubmoduleState.
     SubmoduleState *OuterSubmoduleState;
 
     /// The number of pending module macro names when we started building this.
     unsigned OuterPendingModuleMacroNames;
 
+    /// The previous SubmoduleIncludeState.
+    SubmoduleIncludeState *OuterSubmoduleIncludeState;
+
     BuildingSubmoduleInfo(Module *M, SourceLocation ImportLoc, bool IsPragma,
                           SubmoduleState *OuterSubmoduleState,
-                          unsigned OuterPendingModuleMacroNames)
+                          unsigned OuterPendingModuleMacroNames,
+                          SubmoduleIncludeState *OuterSubmoduleIncludeState)
         : M(M), ImportLoc(ImportLoc), IsPragma(IsPragma),
           OuterSubmoduleState(OuterSubmoduleState),
-          OuterPendingModuleMacroNames(OuterPendingModuleMacroNames) {}
+          OuterPendingModuleMacroNames(OuterPendingModuleMacroNames),
+          OuterSubmoduleIncludeState(OuterSubmoduleIncludeState) {}
   };
   SmallVector<BuildingSubmoduleInfo, 8> BuildingSubmoduleStack;
 
   /// Information about a submodule's preprocessor state.
   struct SubmoduleState {
     /// The macros for the submodule.
     MacroMap Macros;
 
     /// The set of modules that are visible within the submodule.
     VisibleModuleSet VisibleModules;
 
     // FIXME: CounterValue?
     // FIXME: PragmaPushMacroInfo?
   };
   std::map<Module *, SubmoduleState> Submodules;
 
   /// The preprocessor state for preprocessing outside of any submodule.
   SubmoduleState NullSubmoduleState;
 
   /// The current submodule state. Will be \p NullSubmoduleState if we're not
   /// in a submodule.
   SubmoduleState *CurSubmoduleState;
 
+  /// Information about a (sub)module's preprocessor include state.
+  struct SubmoduleIncludeState {
+    /// The files that have been included.
+    llvm::DenseSet<const FileEntry *> IncludedFiles;
+
+    /// The set of modules that are visible within the submodule.
+    VisibleModuleSet VisibleModules;
+  };
+  /// The include state for each (sub)module.
+  std::map<Module *, SubmoduleIncludeState> SubmoduleIncludeStates;
+
+  /// The include state outside of any (sub)module.
+  SubmoduleIncludeState NullSubmoduleIncludeState;
+
+  /// The current include state.
+  SubmoduleIncludeState *CurSubmoduleIncludeState;
+
   /// The files that have been included.
   llvm::DenseSet<const FileEntry *> IncludedFiles;
 
   /// The set of known macros exported from modules.
   llvm::FoldingSet<ModuleMacro> ModuleMacros;
 
   /// The names of potential module macros that we've not yet processed.
   llvm::SmallVector<const IdentifierInfo *, 32> PendingModuleMacroNames;
private:
   };
 
   /// MacroInfos are managed as a chain for easy disposal.  This is the head
   /// of that list.
   MacroInfoChain *MIChainHead = nullptr;
 
   void updateOutOfDateIdentifier(IdentifierInfo &II) const;
 
+  /// Get the external include information for the given (sub)module.
+  const llvm::DenseSet<const FileEntry *> *
+  getExternalSubmoduleIncludes(Module *M) const;
+
 public:
   Preprocessor(std::shared_ptr<PreprocessorOptions> PPOpts,
                DiagnosticsEngine &diags, LangOptions &opts, SourceManager &SM,
                HeaderSearch &Headers, ModuleLoader &TheModuleLoader,
                IdentifierInfoLookup *IILookup = nullptr,
                bool OwnsHeaderSearch = false,
                TranslationUnitKind TUKind = TU_Complete);
 
macros(bool IncludeExternalMacros = true) const {
     return llvm::make_range(begin, end);
   }
 
   /// \}
 
   /// Increment the count for the number of times the specified FileEntry has
   /// been entered. Returns true if this is the first time it file was included.
   bool incrementIncludeCount(const FileEntry *File) {
+    CurSubmoduleIncludeState->IncludedFiles.insert(File);
     return IncludedFiles.insert(File).second;
   }
 
+  /// Increment the include counter for a file transitively included file.
+  void incrementTransitiveIncludeCount(const FileEntry *File);
+
   /// Return true if this header has already been included.
   bool alreadyIncluded(const FileEntry *File) const {
     return IncludedFiles.count(File);
   }
 
-  /// Get the included files.
-  llvm::DenseSet<const FileEntry *> &getIncludedFiles() {
-    return IncludedFiles;
-  }
-  const llvm::DenseSet<const FileEntry *> &getIncludedFiles() const {
-    return IncludedFiles;
+  /// Get the include information outside of any (sub)module.
+  const llvm::DenseSet<const FileEntry *> &getNullSubmoduleIncludes() const {
+    return NullSubmoduleIncludeState.IncludedFiles;
   }
 
+  /// Get the include information for the given local (sub)module.
+  const llvm::DenseSet<const FileEntry *> *
+  getLocalSubmoduleIncludes(Module *M) const;
+
   /// Return the name of the macro defined before \p Loc that has
   /// spelling \p Tokens.  If there are multiple macros with same spelling,
   /// return the last one defined.
   StringRef getLastMacroWithSpelling(SourceLocation Loc,
                                      ArrayRef<TokenValue> Tokens) const;
 
   const std::string &getPredefines() const { return Predefines; }
 
   /// Set the predefines for this Preprocessor.

clang/include/clang/Serialization/ASTBitCodes.h

enum SubmoduleRecordTypes {
 
   /// Specifies some declarations with initializers that must be
   /// emitted to initialize the module.
   SUBMODULE_INITIALIZERS = 16,
 
   /// Specifies the name of the module that will eventually
   /// re-export the entities in this module.
   SUBMODULE_EXPORT_AS = 17,
+
+  /// Specifies files included in this module.
+  SUBMODULE_INCLUDED_FILES = 18,
 };
 
 /// Record types used within a comments block.
 enum CommentRecordTypes { COMMENTS_RAW_COMMENT = 0 };
 
 /// \defgroup ASTAST AST file AST constants
 ///
 /// The constants in this group describe various components of the

clang/include/clang/Serialization/ASTReader.h

struct ImportedSubmodule {
     ImportedSubmodule(serialization::SubmoduleID ID, SourceLocation ImportLoc)
         : ID(ID), ImportLoc(ImportLoc) {}
   };
 
 private:
   /// A list of modules that were imported by precompiled headers or
   /// any other non-module AST file.
   SmallVector<ImportedSubmodule, 2> ImportedModules;
+
+  /// Mapping between a (sub)module and deserialized included files set.
+  llvm::DenseMap<Module *, llvm::DenseSet<const FileEntry *>>
+      SubmoduleIncludedFiles;
   //@}
 
   /// The system include root to be used when loading the
   /// precompiled header.
   std::string isysroot;
 
   /// Whether to disable the normal validation performed on precompiled
   /// headers and module files when they are loaded.
private:
 
   llvm::Error ReadASTBlock(ModuleFile &F, unsigned ClientLoadCapabilities);
   llvm::Error ReadExtensionBlock(ModuleFile &F);
   void ReadModuleOffsetMap(ModuleFile &F) const;
   void ParseLineTable(ModuleFile &F, const RecordData &Record);
   llvm::Error ReadSourceManagerBlock(ModuleFile &F);
   llvm::BitstreamCursor &SLocCursorForID(int ID);
   SourceLocation getImportLocation(ModuleFile *F);
-  void readIncludedFiles(ModuleFile &F, StringRef Blob, Preprocessor &PP);
+  llvm::DenseSet<const FileEntry *> readIncludedFiles(ModuleFile &F,
+                                                      StringRef Blob);
   ASTReadResult ReadModuleMapFileBlock(RecordData &Record, ModuleFile &F,
                                        const ModuleFile *ImportedBy,
                                        unsigned ClientLoadCapabilities);
   llvm::Error ReadSubmoduleBlock(ModuleFile &F,
                                  unsigned ClientLoadCapabilities);
   static bool ParseLanguageOptions(const RecordData &Record, bool Complain,
                                    ASTReaderListener &Listener,
                                    bool AllowCompatibleDifferences);
public:
   /// number.
   serialization::SubmoduleID
   getGlobalSubmoduleID(ModuleFile &M, unsigned LocalID);
 
   /// Retrieve the submodule that corresponds to a global submodule ID.
   ///
   Module *getSubmodule(serialization::SubmoduleID GlobalID);
 
+  /// Return the files directly included in the given (sub)module.
+  const llvm::DenseSet<const FileEntry *> *getIncludedFiles(Module *M) override;
+
   /// Retrieve the module that corresponds to the given module ID.
   ///
   /// Note: overrides method in ExternalASTSource
   Module *getModule(unsigned ID) override;
 
   /// Retrieve the module file with a given local ID within the specified
   /// ModuleFile.
   ModuleFile *getLocalModuleFile(ModuleFile &M, unsigned ID);

clang/include/clang/Serialization/ASTWriter.h

private:
   /// Calculate hash of the pcm content.
   static std::pair<ASTFileSignature, ASTFileSignature>
   createSignature(StringRef AllBytes, StringRef ASTBlockBytes);
 
   void WriteInputFiles(SourceManager &SourceMgr, HeaderSearchOptions &HSOpts,
                        bool Modules);
   void WriteSourceManagerBlock(SourceManager &SourceMgr,
                                const Preprocessor &PP);
-  void writeIncludedFiles(raw_ostream &Out, const Preprocessor &PP);
+  void writeIncludedFiles(raw_ostream &Out,
+                          const llvm::DenseSet<const FileEntry *> &Files);
   void WritePreprocessor(const Preprocessor &PP, bool IsModule);
   void WriteHeaderSearch(const HeaderSearch &HS);
   void WritePreprocessorDetail(PreprocessingRecord &PPRec,
                                uint64_t MacroOffsetsBase);
   void WriteSubmodules(Module *WritingModule);
 
   void WritePragmaDiagnosticMappings(const DiagnosticsEngine &Diag,
                                      bool isModule);

clang/include/clang/Serialization/ModuleFile.h

public:
   unsigned LocalNumSubmodules = 0;
 
   /// Base submodule ID for submodules local to this module.
   serialization::SubmoduleID BaseSubmoduleID = 0;
 
   /// Remapping table for submodule IDs in this module.
   ContinuousRangeMap<uint32_t, int, 2> SubmoduleRemap;
 
+  /// Mapping between (sub)module names and the serialized version of included
+  /// files set.
+  std::map<std::string, StringRef> SubmoduleIncludedFiles;
+
   // === Selectors ===
 
   /// The number of selectors new to this file.
   ///
   /// This is the number of entries in SelectorOffsets.
   unsigned LocalNumSelectors = 0;
 
   /// Offsets into the selector lookup table's data array

clang/lib/Basic/Module.cpp

std::function<void(Visiting)> VisitModule = [&](Visiting V) {
     // Nothing to do for a module that's already visible.
     unsigned ID = V.M->getVisibilityID();
     if (ImportLocs.size() <= ID)
       ImportLocs.resize(ID + 1);
     else if (ImportLocs[ID].isValid())
       return;
 
     ImportLocs[ID] = Loc;
-    Vis(M);
+    Vis(V.M);
 
     // Make any exported modules visible.
     SmallVector<Module *, 16> Exports;
     V.M->getExportedModules(Exports);
     for (Module *E : Exports) {
       // Don't import non-importable modules.
       if (!E->isUnimportable())
         VisitModule({E, &V});

clang/lib/Lex/PPLexerChange.cpp

void Preprocessor::HandleMicrosoftCommentPaste(Token &Tok) {
   // preprocessor directive mode), so just return EOF as our token.
   assert(!FoundLexer && "Lexer should return EOD before EOF in PP mode");
 }
 
 void Preprocessor::EnterSubmodule(Module *M, SourceLocation ImportLoc,
                                   bool ForPragma) {
   if (!getLangOpts().ModulesLocalVisibility) {
     // Just track that we entered this submodule.
-    BuildingSubmoduleStack.push_back(
-        BuildingSubmoduleInfo(M, ImportLoc, ForPragma, CurSubmoduleState,
-                              PendingModuleMacroNames.size()));
+    BuildingSubmoduleStack.push_back(BuildingSubmoduleInfo(
+        M, ImportLoc, ForPragma, CurSubmoduleState,
+        PendingModuleMacroNames.size(), CurSubmoduleIncludeState));
+
+    CurSubmoduleIncludeState = &SubmoduleIncludeStates[M];
+
     if (Callbacks)
       Callbacks->EnteredSubmodule(M, ImportLoc, ForPragma);
     return;
   }
 
   // Resolve as much of the module definition as we can now, before we enter
   // one of its headers.
   // FIXME: Can we enable Complain here?
   // FIXME: Can we do this when local visibility is disabled?
   ModuleMap &ModMap = getHeaderSearchInfo().getModuleMap();
for (auto &Macro : StartingMacros) {
 
       MacroState MS(Macro.second.getLatest());
       MS.setOverriddenMacros(*this, Macro.second.getOverriddenMacros());
       State.Macros.insert(std::make_pair(Macro.first, std::move(MS)));
     }
   }
 
   // Track that we entered this module.
-  BuildingSubmoduleStack.push_back(
-      BuildingSubmoduleInfo(M, ImportLoc, ForPragma, CurSubmoduleState,
-                            PendingModuleMacroNames.size()));
+  BuildingSubmoduleStack.push_back(BuildingSubmoduleInfo(
+      M, ImportLoc, ForPragma, CurSubmoduleState,
+      PendingModuleMacroNames.size(), CurSubmoduleIncludeState));
 
   if (Callbacks)
     Callbacks->EnteredSubmodule(M, ImportLoc, ForPragma);
 
   // Switch to this submodule as the current submodule.
   CurSubmoduleState = &State;
+  CurSubmoduleIncludeState = &SubmoduleIncludeStates[M];
 
   // This module is visible to itself.
   if (FirstTime)
     makeModuleVisible(M, ImportLoc);
 }
 
 bool Preprocessor::needModuleMacros() const {
   // If we're not within a submodule, we never need to create ModuleMacros.
if (BuildingSubmoduleStack.empty() ||
     return nullptr;
   }
 
   auto &Info = BuildingSubmoduleStack.back();
 
   Module *LeavingMod = Info.M;
   SourceLocation ImportLoc = Info.ImportLoc;
 
+  CurSubmoduleIncludeState = Info.OuterSubmoduleIncludeState;
+
   if (!needModuleMacros() ||
       (!getLangOpts().ModulesLocalVisibility &&
        LeavingMod->getTopLevelModuleName() != getLangOpts().CurrentModule)) {
     // If we don't need module macros, or this is not a module for which we
     // are tracking macro visibility, don't build any, and preserve the list
     // of pending names for the surrounding submodule.
     BuildingSubmoduleStack.pop_back();
 

clang/lib/Lex/Preprocessor.cpp

: PPOpts(std::move(PPOpts)), Diags(&diags), LangOpts(opts),
       FileMgr(Headers.getFileMgr()), SourceMgr(SM),
       ScratchBuf(new ScratchBuffer(SourceMgr)), HeaderInfo(Headers),
       TheModuleLoader(TheModuleLoader), ExternalSource(nullptr),
       // As the language options may have not been loaded yet (when
       // deserializing an ASTUnit), adding keywords to the identifier table is
       // deferred to Preprocessor::Initialize().
       Identifiers(IILookup), PragmaHandlers(new PragmaNamespace(StringRef())),
       TUKind(TUKind), SkipMainFilePreamble(0, true),
-      CurSubmoduleState(&NullSubmoduleState) {
+      CurSubmoduleState(&NullSubmoduleState),
+      CurSubmoduleIncludeState(&NullSubmoduleIncludeState) {
   OwnsHeaderSearch = OwnsHeaders;
 
   // Default to discarding comments.
   KeepComments = false;
   KeepMacroComments = false;
   SuppressIncludeNotFoundError = false;
 
   // Macro expansion is enabled.
if (!SourceMgr.isLoadedFileID(MainFileID)) {
     // Tell the header info that the main file was entered.  If the file is later
     // #imported, it won't be re-entered.
     if (const FileEntry *FE = SourceMgr.getFileEntryForID(MainFileID)) {
       HeaderInfo.getFileInfo(FE);
       incrementIncludeCount(FE);
     }
   }
 
+  if (Module *M = getCurrentModule())
+    CurSubmoduleIncludeState = &SubmoduleIncludeStates[M];
+
   // Preprocess Predefines to populate the initial preprocessor state.
   std::unique_ptr<llvm::MemoryBuffer> SB =
     llvm::MemoryBuffer::getMemBufferCopy(Predefines, "<built-in>");
   assert(SB && "Cannot create predefined source buffer");
   FileID FID = SourceMgr.createFileID(std::move(SB));
   assert(FID.isValid() && "Could not create FileID for predefines?");
   setPredefinesFileID(FID);
 
CurSubmoduleState->VisibleModules.setVisible(
         // FIXME: Include the path in the diagnostic.
         // FIXME: Include the import location for the conflicting module.
         Diag(ModuleImportLoc, diag::warn_module_conflict)
             << Path[0]->getFullModuleName()
             << Conflict->getFullModuleName()
             << Message;
       });
 
+  CurSubmoduleIncludeState->VisibleModules.setVisible(
+      M, Loc,
+      [&](Module *M) {
+        const auto *Includes = getLocalSubmoduleIncludes(M);
+        if (!Includes)
+          Includes = getExternalSubmoduleIncludes(M);
+        if (Includes)
+          for (const auto &E : *Includes)
+            incrementTransitiveIncludeCount(E);
+      },
+      [](ArrayRef<Module *>, Module *, StringRef) {});
+
   // Add this module to the imports list of the currently-built submodule.
   if (!BuildingSubmoduleStack.empty() && M != BuildingSubmoduleStack.back().M)
     BuildingSubmoduleStack.back().M->Imports.insert(M);
 }
 
 bool Preprocessor::FinishLexStringLiteral(Token &Result, std::string &String,
                                           const char *DiagnosticTag,
                                           bool AllowMacroExpansion) {

 
 void Preprocessor::createPreprocessingRecord() {
   if (Record)
     return;
 
   Record = new PreprocessingRecord(getSourceManager());
   addPPCallbacks(std::unique_ptr<PPCallbacks>(Record));
 }
+
+void Preprocessor::incrementTransitiveIncludeCount(const FileEntry *File) {
+  // Compared to `incrementIncludeCount`, we're not incrementing the count in
+  // CurSubmoduleIncludeState`. This is to ensure it always only contains
+  // **locally** included files and not transitively included ones.
+  IncludedFiles.insert(File);
+}
+
+const llvm::DenseSet<const FileEntry *> *
+Preprocessor::getLocalSubmoduleIncludes(Module *M) const {
+  auto It = SubmoduleIncludeStates.find(M);
+  if (It != SubmoduleIncludeStates.end())
+    return &It->second.IncludedFiles;
+  return nullptr;
+}
+
+const llvm::DenseSet<const FileEntry *> *
+Preprocessor::getExternalSubmoduleIncludes(Module *M) const {
+  if (ExternalSource)
+    return ExternalSource->getIncludedFiles(M);
+  return nullptr;
+}

clang/lib/Serialization/ASTReader.cpp

case INPUT_FILE_OFFSETS:
           (const llvm::support::unaligned_uint64_t *)Blob.data();
       F.InputFilesLoaded.resize(NumInputs);
       F.NumUserInputFiles = NumUserInputs;
       break;
     }
   }
 }
 
-void ASTReader::readIncludedFiles(ModuleFile &F, StringRef Blob,
-                                  Preprocessor &PP) {
+llvm::DenseSet<const FileEntry *> ASTReader::readIncludedFiles(ModuleFile &F,
+                                                               StringRef Blob) {
   using namespace llvm::support;
 
+  llvm::DenseSet<const FileEntry *> Result;
+
   const unsigned char *D = (const unsigned char *)Blob.data();
   unsigned FileCount = endian::readNext<uint32_t, little, unaligned>(D);
 
   for (unsigned I = 0; I < FileCount; ++D)
     for (unsigned Bit = 0; Bit < 8 && I < FileCount; ++Bit, ++I)
       if (*D & (1 << Bit)) {
         auto InputFileInfo = readInputFileInfo(F, I);
         if (auto File = PP.getFileManager().getFile(InputFileInfo.Filename))
-          PP.getIncludedFiles().insert(*File);
+          Result.insert(*File);
       }
+
+  return Result;
 }
 
 llvm::Error ASTReader::ReadASTBlock(ModuleFile &F,
                                     unsigned ClientLoadCapabilities) {
   BitstreamCursor &Stream = F.Stream;
 
   if (llvm::Error Err = Stream.EnterSubBlock(AST_BLOCK_ID))
     return Err;
case MACRO_OFFSET: {
                          F.BaseMacroID - LocalBaseMacroID));
 
         MacrosLoaded.resize(MacrosLoaded.size() + F.LocalNumMacros);
       }
       break;
     }
 
     case PP_INCLUDED_FILES:
-      readIncludedFiles(F, Blob, PP);
+      if (F.isModule())
+        break;
+      for (const auto &File : readIncludedFiles(F, Blob))
+        PP.incrementTransitiveIncludeCount(File);
       break;
 
     case LATE_PARSED_TEMPLATE:
       LateParsedTemplates.emplace_back(
           std::piecewise_construct, std::forward_as_tuple(&F),
           std::forward_as_tuple(Record.begin(), Record.end()));
       break;
 
case SUBMODULE_INITIALIZERS: {
       ContextObj->addLazyModuleInitializers(CurrentModule, Inits);
       break;
     }
 
     case SUBMODULE_EXPORT_AS:
       CurrentModule->ExportAsModule = Blob.str();
       ModMap.addLinkAsDependency(CurrentModule);
       break;
+
+    case SUBMODULE_INCLUDED_FILES:
+      F.SubmoduleIncludedFiles.insert(
+          {CurrentModule->getFullModuleName(), Blob});
     }
   }
 }
 
 /// Parse the record that corresponds to a LangOptions data
 /// structure.
 ///
 /// This routine parses the language options from the AST file and then gives
ASTReader::getGlobalSubmoduleID(ModuleFile &M, unsigned LocalID) {
   ContinuousRangeMap<uint32_t, int, 2>::iterator I
     = M.SubmoduleRemap.find(LocalID - NUM_PREDEF_SUBMODULE_IDS);
   assert(I != M.SubmoduleRemap.end()
          && "Invalid index into submodule index remap");
 
   return LocalID + I->second;
 }
 
+const llvm::DenseSet<const FileEntry *> *
+ASTReader::getIncludedFiles(Module *M) {
+  ModuleFile *F = getModuleManager().lookup(M->getASTFile());
+  if (!F)
+    return nullptr;
+
+  auto ResultIt = SubmoduleIncludedFiles.insert(
+      {M, llvm::DenseSet<const FileEntry *>{}});
+  auto &Result = ResultIt.first->second;
+  if (!ResultIt.second)
+    return &Result;
+
+  auto It = F->SubmoduleIncludedFiles.find(M->getFullModuleName());
+  if (It == F->SubmoduleIncludedFiles.end())
+    return nullptr;
+  auto &Record = It->second;
+
+  Result = readIncludedFiles(*F, Record);
+  return &Result;
+}
+
 Module *ASTReader::getSubmodule(SubmoduleID GlobalID) {
   if (GlobalID < NUM_PREDEF_SUBMODULE_IDS) {
     assert(GlobalID == 0 && "Unhandled global submodule ID");
     return nullptr;
   }
 
   if (GlobalID > SubmodulesLoaded.size()) {
     Error("submodule ID out of range in AST file");

clang/lib/Serialization/ASTWriter.cpp

if (Loc.isInvalid())
       return true;
     if (PP.getSourceManager().getFileID(Loc) == PP.getPredefinesFileID())
       return true;
   }
 
   return false;
 }
 
-void ASTWriter::writeIncludedFiles(raw_ostream &Out, const Preprocessor &PP) {
+void ASTWriter::writeIncludedFiles(
+    raw_ostream &Out, const llvm::DenseSet<const FileEntry *> &Files) {
   using namespace llvm::support;
 
   std::vector<bool> IncludedInputFiles;
-  for (const auto &File : PP.getIncludedFiles()) {
+  for (const auto &File : Files) {
     auto InputFileIt = InputFileIDs.find(File);
     if (InputFileIt == InputFileIDs.end())
       continue;
     auto InputFileID = InputFileIt->second;
     if (IncludedInputFiles.size() <= InputFileID)
       IncludedInputFiles.resize(InputFileID + 1);
     IncludedInputFiles[InputFileIt->second] = true;
   }
void ASTWriter::WritePreprocessor(const Preprocessor &PP, bool IsModule) {
   {
     auto Abbrev = std::make_shared<BitCodeAbbrev>();
     Abbrev->Add(BitCodeAbbrevOp(PP_INCLUDED_FILES));
     Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob));
     unsigned IncludedFilesAbbrev = Stream.EmitAbbrev(std::move(Abbrev));
 
     SmallString<2048> Buffer;
     raw_svector_ostream Out(Buffer);
-    writeIncludedFiles(Out, PP);
+    writeIncludedFiles(Out, PP.getNullSubmoduleIncludes());
     RecordData::value_type Record[] = {PP_INCLUDED_FILES};
     Stream.EmitRecordWithBlob(IncludedFilesAbbrev, Record, Buffer.data(),
                               Buffer.size());
   }
 }
 
 void ASTWriter::WritePreprocessorDetail(PreprocessingRecord &PPRec,
                                         uint64_t MacroOffsetsBase) {
   if (PPRec.local_begin() == PPRec.local_end())
     return;
 
   SmallVector<PPEntityOffset, 64> PreprocessedEntityOffsets;
 
   // Enter the preprocessor block.
   Stream.EnterSubblock(PREPROCESSOR_DETAIL_BLOCK_ID, 3);
 
   // If the preprocessor has a preprocessing record, emit it.
   unsigned NumPreprocessingRecords = 0;
   using namespace llvm;
 
   // Set up the abbreviation for
   unsigned InclusionAbbrev = 0;
   {
     auto Abbrev = std::make_shared<BitCodeAbbrev>();
     Abbrev->Add(BitCodeAbbrevOp(PPD_INCLUSION_DIRECTIVE));
     Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 32)); // filename length
     Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // in quotes
     Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 2)); // kind
     Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // imported module
     Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob));
     InclusionAbbrev = Stream.EmitAbbrev(std::move(Abbrev));
   }
 
   unsigned FirstPreprocessorEntityID
void ASTWriter::WriteSubmodules(Module *WritingModule) {
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob));    // Message
   unsigned ConflictAbbrev = Stream.EmitAbbrev(std::move(Abbrev));
 
   Abbrev = std::make_shared<BitCodeAbbrev>();
   Abbrev->Add(BitCodeAbbrevOp(SUBMODULE_EXPORT_AS));
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob));    // Macro name
   unsigned ExportAsAbbrev = Stream.EmitAbbrev(std::move(Abbrev));
 
+  Abbrev = std::make_shared<BitCodeAbbrev>();
+  Abbrev->Add(BitCodeAbbrevOp(SUBMODULE_INCLUDED_FILES));
+  Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob));
+  unsigned IncludedFilesAbbrev = Stream.EmitAbbrev(std::move(Abbrev));
+
   // Write the submodule metadata block.
   RecordData::value_type Record[] = {
       getNumberOfModules(WritingModule),
       FirstSubmoduleID - NUM_PREDEF_SUBMODULE_IDS};
   Stream.EmitRecord(SUBMODULE_METADATA, Record);
 
   // Write all of the submodules.
   std::queue<Module *> Q;
if (!Inits.empty())
       Stream.EmitRecord(SUBMODULE_INITIALIZERS, Inits);
 
     // Emit the name of the re-exported module, if any.
     if (!Mod->ExportAsModule.empty()) {
       RecordData::value_type Record[] = {SUBMODULE_EXPORT_AS};
       Stream.EmitRecordWithBlob(ExportAsAbbrev, Record, Mod->ExportAsModule);
     }
 
+    if (const auto *Includes = PP->getLocalSubmoduleIncludes(Mod)) {
+      SmallString<2048> Buffer;
+      raw_svector_ostream Out(Buffer);
+      writeIncludedFiles(Out, *Includes);
+      RecordData::value_type Record[] = {SUBMODULE_INCLUDED_FILES};
+      Stream.EmitRecordWithBlob(IncludedFilesAbbrev, Record, Buffer.data(),
+                                Buffer.size());
+    }
+
     // Queue up the submodules of this module.
     for (auto *M : Mod->submodules())
       Q.push(M);
   }
 
   Stream.ExitBlock();
 
   assert((NextSubmoduleID - FirstSubmoduleID ==

clang/test/Modules/import-submodule-visibility.c

+// This test checks that imports of headers that appeared in a different submodule than
+// what is imported by the current TU don't affect the compilation.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+//--- A.framework/Headers/A.h
+#include "Textual.h"
+//--- A.framework/Modules/module.modulemap
+framework module A { header "A.h" }
+
+//--- B.framework/Headers/B1.h
+#include "Textual.h"
+//--- B.framework/Headers/B2.h
+//--- B.framework/Modules/module.modulemap
+framework module B {
+  module B1 { header "B1.h" }
+  module B2 { header "B2.h" }
+}
+
+//--- C/C.h
+#include "Textual.h"
+//--- C/module.modulemap
+module C { header "C.h" }
+
+//--- D/D1.h
+#include "Textual.h"
+//--- D/D2.h
+//--- D/module.modulemap
+module D {
+  module D1 { header "D1.h" }
+  module D2 { header "D2.h" }
+}
+
+//--- E/E1.h
+#include "E2.h"
+//--- E/E2.h
+#include "Textual.h"
+//--- E/module.modulemap
+module E {
+  module E1 { header "E1.h" }
+  module E2 { header "E2.h" }
+}
+
+//--- Textual.h
+#define MACRO_TEXTUAL 1
+
+//--- test.c
+
+#ifdef A
+//
+#endif
+
+#ifdef B
+#import <B/B2.h>
+#endif
+
+#ifdef C
+//
+#endif
+
+#ifdef D
+#import "D/D2.h"
+#endif
+
+#ifdef E
+#import "E/E1.h"
+#endif
+
+#import "Textual.h"
+
+static int x = MACRO_TEXTUAL;
+
+// Specifying the PCM file on the command line (without actually importing "A") should not
+// prevent "Textual.h" to be included in the TU.
+//
+// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/A.framework/Modules/module.modulemap -fmodule-name=A -o %t/A.pcm
+// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test.c -DA -fmodule-file=%t/A.pcm
+
+// Specifying the PCM file on the command line and importing "B2" in the source does not
+// prevent "Textual.h" to be included in the TU.
+//
+// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/B.framework/Modules/module.modulemap -fmodule-name=B -o %t/B.pcm
+// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test.c -DB -iframework %t -fmodule-file=%t/B.pcm
+
+// Module-only version of the test with framework A.
+//
+// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/C/module.modulemap -fmodule-name=C -o %t/C.pcm
+// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test.c -DC -fmodule-file=%t/C.pcm
+
+// Module-only version of the test with framework B.
+//
+// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/D/module.modulemap -fmodule-name=D -o %t/D.pcm
+// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test.c -DD -fmodule-file=%t/D.pcm
+
+// Transitively imported, but not exported.
+//
+// RUN: %clang_cc1 -fmodules -I %t -emit-module %t/E/module.modulemap -fmodule-name=E -o %t/E.pcm
+// RUN: %clang_cc1 -fmodules -I %t -fsyntax-only %t/test.c -DE -fmodule-file=%t/E.pcm