[PATCH] D104344: [modules] Track how headers are included by different submodules.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

That's a good point. Let me check how we track macros, I haven't thought about that approach. And I haven't considered using `Preprocessor::SubmoduleState`, was too excited `HeaderSearch::ShouldEnterIncludeFile` works correctly with the updated data.



================
Comment at: clang/include/clang/Lex/HeaderSearch.h:126
+  /// Bool represents if the header was included (false) or imported (true).
+  llvm::DenseMap<const Module *, bool> ModuleIncludersMap;
+
----------------
rsmith wrote:
> This seems like a very expensive thing to include in `HeaderFileInfo`, which we may have many thousands of in some compilations. (I have similar concerns about `Aliases` FWIW.)
My fault. The fact that the struct is tightly packed, should have been a hint.


================
Comment at: clang/include/clang/Serialization/ASTBitCodes.h:44
 /// AST files at this time.
-const unsigned VERSION_MAJOR = 15;
+const unsigned VERSION_MAJOR = 16;
 
----------------
rsmith wrote:
> Do we really need to bump the version number? (Does this affect something we read before we read the Clang version number and reject on a version mismatch?)
Nope, there is no logic for VERSION_MAJOR. Just wanted to avoid old clang writing HeaderFileInfoTrait in one format and new clang reading it in the new format. But if that is resolved by Clang version number, no need for VERSION_MAJOR bump.


================
Comment at: clang/lib/Serialization/ASTReader.cpp:1912-1916
+    // Update isImport and NumIncludes based on including module.
+    if (Mod->NameVisibility == Module::NameVisibilityKind::AllVisible) {
+      HFI.isImport |= IsImport;
+      ++HFI.NumIncludes;
+    }
----------------
rsmith wrote:
> This doesn't do the right thing in local submodule visibility mode. We need to compute visibility locally from within `HeaderSearch` based on the currently visible modules at the point of the query instead.
Is it wrong from general local submodule visibility approach or do you have a test case in mind? Don't need a specific case, general direction can help to come up with a test case.

In practice it was working because we were reading this part of PCM file only when HeaderFileInfo was requested and that was at the point when modules had correct visibility. Let me try with multiple unmodularized headers, that can be broken and it can be a good test case on its own.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104344