[clang] remove SUBMODULE_TOPHEADER

[rmaz]

Summary

We are seeing issues with path serialization of the
SUBMODULE_TOPHEADER field when the inputs are links. While working
on a fix I noticed this field does not appear to be used anywhere
outside of libclang. This diff removes the field entirely and
updates the libclang API to always return empty top headers.

[rmaz]

@akyrtzi it looks like you added this field a while back. Do you know if there are still consumers for the the libclang API, do we need to keep this field around?

The alternative is to refactor the TopHeaders set to use FileEntryRef so that we can keep track of the correct import names for the headers.

[akyrtzi]

This is useful for functionality, like in an IDE, to show the top headers of a module import for navigational purposes.
Is it reasonable to at least have an alternative implementation for it (e.g. from the module headers find the headers that were not included from other headers), not sure if it'd be straightforward or not.

[rmaz]

In D142028#4062798, @akyrtzi wrote:

Is it reasonable to at least have an alternative implementation for it (e.g. from the module headers find the headers that were not included from other headers), not sure if it'd be straightforward or not.

Its feasible to refactor by moving the collectModuleHeaderIncludes function to be a method on Module with a callback. Then we could avoid serializing the top header list and call the method in CIndex as well as FrontendAction. I'll take a look at how much work this is.

[rmaz]

Its going to be less work to refactor TopheaderNames to use FileEntryRef, so went that way instead. First step is here: https://reviews.llvm.org/D142113

clang/include/clang/Basic/Module.h

private:
   /// A mapping from the submodule name to the index into the
   /// \c SubModules vector at which that submodule resides.
   llvm::StringMap<unsigned> SubModuleIndex;
 
   /// The AST file if this is a top-level module which has a
   /// corresponding serialized AST file, or null otherwise.
   OptionalFileEntryRef ASTFile;
 
-  /// The top-level headers associated with this module.
-  llvm::SmallSetVector<const FileEntry *, 2> TopHeaders;
-
-  /// top-level header filenames that aren't resolved to FileEntries yet.
-  std::vector<std::string> TopHeaderNames;
-
   /// Cache of modules visible to lookup in this module.
   mutable llvm::DenseSet<const Module*> VisibleModulesCache;
 
   /// The ID used when referencing this module within a VisibleModuleSet.
   unsigned VisibilityID;
 
 public:
   enum HeaderKind {
public:
   }
 
   /// Determine whether this module has an umbrella directory that is
   /// not based on an umbrella header.
   bool hasUmbrellaDir() const {
     return Umbrella && Umbrella.is<const DirectoryEntry *>();
   }
 
-  /// Add a top-level header associated with this module.
-  void addTopHeader(const FileEntry *File);
-
-  /// Add a top-level header filename associated with this module.
-  void addTopHeaderFilename(StringRef Filename) {
-    TopHeaderNames.push_back(std::string(Filename));
-  }
-
-  /// The top-level headers associated with this module.
-  ArrayRef<const FileEntry *> getTopHeaders(FileManager &FileMgr);
-
   /// Determine whether this module has declared its intention to
   /// directly use another module.
   bool directlyUses(const Module *Requested);
 
   /// Add the given feature requirement to the list of features
   /// required by this module.
   ///
   /// \param Feature The feature that is required by this module (and

clang/include/clang/Serialization/ASTBitCodes.h

enum SubmoduleRecordTypes {
 
   /// Specifies the umbrella header used to create this module,
   /// if any.
   SUBMODULE_UMBRELLA_HEADER = 2,
 
   /// Specifies a header that falls into this (sub)module.
   SUBMODULE_HEADER = 3,
 
-  /// Specifies a top-level header that falls into this (sub)module.
-  SUBMODULE_TOPHEADER = 4,
-
   /// Specifies an umbrella directory.
-  SUBMODULE_UMBRELLA_DIR = 5,
+  SUBMODULE_UMBRELLA_DIR = 4,
 
   /// Specifies the submodules that are imported by this
   /// submodule.
-  SUBMODULE_IMPORTS = 6,
+  SUBMODULE_IMPORTS = 5,
 
   /// Specifies the submodules that are re-exported from this
   /// submodule.
-  SUBMODULE_EXPORTS = 7,
+  SUBMODULE_EXPORTS = 6,
 
   /// Specifies a required feature.
-  SUBMODULE_REQUIRES = 8,
+  SUBMODULE_REQUIRES = 7,
 
   /// Specifies a header that has been explicitly excluded
   /// from this submodule.
-  SUBMODULE_EXCLUDED_HEADER = 9,
+  SUBMODULE_EXCLUDED_HEADER = 8,
 
   /// Specifies a library or framework to link against.
-  SUBMODULE_LINK_LIBRARY = 10,
+  SUBMODULE_LINK_LIBRARY = 9,
 
   /// Specifies a configuration macro for this module.
-  SUBMODULE_CONFIG_MACRO = 11,
+  SUBMODULE_CONFIG_MACRO = 10,
 
   /// Specifies a conflict with another module.
-  SUBMODULE_CONFLICT = 12,
+  SUBMODULE_CONFLICT = 11,
 
   /// Specifies a header that is private to this submodule.
-  SUBMODULE_PRIVATE_HEADER = 13,
+  SUBMODULE_PRIVATE_HEADER = 12,
 
   /// Specifies a header that is part of the module but must be
   /// textually included.
-  SUBMODULE_TEXTUAL_HEADER = 14,
+  SUBMODULE_TEXTUAL_HEADER = 13,
 
   /// Specifies a header that is private to this submodule but
   /// must be textually included.
-  SUBMODULE_PRIVATE_TEXTUAL_HEADER = 15,
+  SUBMODULE_PRIVATE_TEXTUAL_HEADER = 14,
 
   /// Specifies some declarations with initializers that must be
   /// emitted to initialize the module.
-  SUBMODULE_INITIALIZERS = 16,
+  SUBMODULE_INITIALIZERS = 15,
 
   /// Specifies the name of the module that will eventually
   /// re-export the entities in this module.
-  SUBMODULE_EXPORT_AS = 17,
+  SUBMODULE_EXPORT_AS = 16,
 
   /// Specifies affecting modules that were not imported.
-  SUBMODULE_AFFECTING_MODULES = 18,
+  SUBMODULE_AFFECTING_MODULES = 17,
 };
 
 /// 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/lib/Basic/Module.cpp

 Module::DirectoryName Module::getUmbrellaDir() const {
   if (Header U = getUmbrellaHeader())
     return {"", "", U.Entry->getDir()};
 
   return {UmbrellaAsWritten, UmbrellaRelativeToRootModuleDirectory,
           Umbrella.dyn_cast<const DirectoryEntry *>()};
 }
 
-void Module::addTopHeader(const FileEntry *File) {
-  assert(File);
-  TopHeaders.insert(File);
-}
-
-ArrayRef<const FileEntry *> Module::getTopHeaders(FileManager &FileMgr) {
-  if (!TopHeaderNames.empty()) {
-    for (std::vector<std::string>::iterator
-           I = TopHeaderNames.begin(), E = TopHeaderNames.end(); I != E; ++I) {
-      if (auto FE = FileMgr.getFile(*I))
-        TopHeaders.insert(*FE);
-    }
-    TopHeaderNames.clear();
-  }
-
-  return llvm::ArrayRef(TopHeaders.begin(), TopHeaders.end());
-}
-
 bool Module::directlyUses(const Module *Requested) {
   auto *Top = getTopLevelModule();
 
   // A top-level module implicitly uses itself.
   if (Requested->isSubModuleOf(Top))
     return true;
 
   for (auto *Use : Top->DirectUses)

clang/lib/Frontend/FrontendAction.cpp

static void addHeaderInclude(StringRef HeaderName,
   Includes += HeaderName;
 
   Includes += "\"\n";
   if (IsExternC && LangOpts.CPlusPlus)
     Includes += "}\n";
 }
 
 /// Collect the set of header includes needed to construct the given
-/// module and update the TopHeaders file set of the module.
+/// module.
 ///
 /// \param Module The module we're collecting includes from.
 ///
 /// \param Includes Will be augmented with the set of \#includes or \#imports
 /// needed to load all of the named headers.
 static std::error_code collectModuleHeaderIncludes(
     const LangOptions &LangOpts, FileManager &FileMgr, DiagnosticsEngine &Diag,
     ModuleMap &ModMap, clang::Module *Module, SmallVectorImpl<char> &Includes) {
if (!Module->MissingHeaders.empty()) {
     Diag.Report(MissingHeader.FileNameLoc, diag::err_module_header_missing)
       << MissingHeader.IsUmbrella << MissingHeader.FileName;
     return std::error_code();
   }
 
   // Add includes for each of these headers.
   for (auto HK : {Module::HK_Normal, Module::HK_Private}) {
     for (Module::Header &H : Module->Headers[HK]) {
-      Module->addTopHeader(H.Entry);
       // Use the path as specified in the module map file. We'll look for this
       // file relative to the module build directory (the directory containing
       // the module map file) so this will find the same file that we found
       // while parsing the module map.
       addHeaderInclude(H.PathRelativeToRootModuleDirectory, Includes, LangOpts,
                        Module->IsExternC);
     }
   }
-  // Note that Module->PrivateHeaders will not be a TopHeader.
 
   if (Module::Header UmbrellaHeader = Module->getUmbrellaHeader()) {
-    Module->addTopHeader(UmbrellaHeader.Entry);
     if (Module->Parent)
       // Include the umbrella header for submodules.
       addHeaderInclude(UmbrellaHeader.PathRelativeToRootModuleDirectory,
                        Includes, LangOpts, Module->IsExternC);
   } else if (Module::DirectoryName UmbrellaDir = Module->getUmbrellaDir()) {
     // Add all of the headers we find in this subdirectory.
     std::error_code EC;
     SmallString<128> DirNative;
if (Module::Header UmbrellaHeader = Module->getUmbrellaHeader()) {
     if (EC)
       return EC;
 
     // Sort header paths and make the header inclusion order deterministic
     // across different OSs and filesystems.
     llvm::sort(Headers, llvm::less_first());
     for (auto &H : Headers) {
       // Include this header as part of the umbrella directory.
-      Module->addTopHeader(H.second);
       addHeaderInclude(H.first, Includes, LangOpts, Module->IsExternC);
     }
   }
 
   // Recurse into submodules.
   for (clang::Module::submodule_iterator Sub = Module->submodule_begin(),
                                       SubEnd = Module->submodule_end();
        Sub != SubEnd; ++Sub)

clang/lib/Lex/ModuleMap.cpp

if (UmbrellaModule->InferSubmodules) {
       // Infer a submodule with the same name as this header file.
       SmallString<32> NameBuf;
       StringRef Name = sanitizeFilenameAsIdentifier(
                          llvm::sys::path::stem(File->getName()), NameBuf);
       Result = findOrCreateModule(Name, Result, /*IsFramework=*/false,
                                   Explicit).first;
       InferredModuleAllowedBy[Result] = UmbrellaModuleMap;
       Result->IsInferred = true;
-      Result->addTopHeader(File);
 
       // If inferred submodules export everything they import, add a
       // wildcard to the set of exports.
       if (UmbrellaModule->InferExportWildcard && Result->Exports.empty())
         Result->Exports.push_back(Module::ExportDecl(nullptr, true));
     } else {
       // Record each of the directories we stepped through as being part of
       // the module we found, since the umbrella header covers them all.

clang/lib/Serialization/ASTReader.cpp

case SUBMODULE_PRIVATE_HEADER:
       break;
 
     case SUBMODULE_TEXTUAL_HEADER:
     case SUBMODULE_PRIVATE_TEXTUAL_HEADER:
       // FIXME: Textual headers are not marked in the HeaderInfo table. Load
       // them here.
       break;
 
-    case SUBMODULE_TOPHEADER: {
-      std::string HeaderName(Blob);
-      ResolveImportedPath(F, HeaderName);
-      CurrentModule->addTopHeaderFilename(HeaderName);
-      break;
-    }
-
     case SUBMODULE_UMBRELLA_DIR: {
       // See comments in SUBMODULE_UMBRELLA_HEADER
       std::string Dirname = std::string(Blob);
       ResolveImportedPath(F, Dirname);
       if (auto Umbrella = PP.getFileManager().getDirectory(Dirname)) {
         if (!CurrentModule->getUmbrellaDir()) {
           // FIXME: NameAsWritten
           ModMap.setUmbrellaDir(CurrentModule, *Umbrella, Blob, "");

clang/lib/Serialization/ASTWriter.cpp

#define RECORD(X) EmitRecordID(X, #X, Stream, Record)
   RECORD(PP_TOKEN);
 
   // Submodule Block.
   BLOCK(SUBMODULE_BLOCK);
   RECORD(SUBMODULE_METADATA);
   RECORD(SUBMODULE_DEFINITION);
   RECORD(SUBMODULE_UMBRELLA_HEADER);
   RECORD(SUBMODULE_HEADER);
-  RECORD(SUBMODULE_TOPHEADER);
   RECORD(SUBMODULE_UMBRELLA_DIR);
   RECORD(SUBMODULE_IMPORTS);
   RECORD(SUBMODULE_AFFECTING_MODULES);
   RECORD(SUBMODULE_EXPORTS);
   RECORD(SUBMODULE_REQUIRES);
   RECORD(SUBMODULE_EXCLUDED_HEADER);
   RECORD(SUBMODULE_LINK_LIBRARY);
   RECORD(SUBMODULE_CONFIG_MACRO);
void ASTWriter::WriteSubmodules(Module *WritingModule) {
   unsigned UmbrellaAbbrev = Stream.EmitAbbrev(std::move(Abbrev));
 
   Abbrev = std::make_shared<BitCodeAbbrev>();
   Abbrev->Add(BitCodeAbbrevOp(SUBMODULE_HEADER));
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob)); // Name
   unsigned HeaderAbbrev = Stream.EmitAbbrev(std::move(Abbrev));
 
   Abbrev = std::make_shared<BitCodeAbbrev>();
-  Abbrev->Add(BitCodeAbbrevOp(SUBMODULE_TOPHEADER));
-  Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob)); // Name
-  unsigned TopHeaderAbbrev = Stream.EmitAbbrev(std::move(Abbrev));
-
-  Abbrev = std::make_shared<BitCodeAbbrev>();
   Abbrev->Add(BitCodeAbbrevOp(SUBMODULE_UMBRELLA_DIR));
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob)); // Name
   unsigned UmbrellaDirAbbrev = Stream.EmitAbbrev(std::move(Abbrev));
 
   Abbrev = std::make_shared<BitCodeAbbrev>();
   Abbrev->Add(BitCodeAbbrevOp(SUBMODULE_REQUIRES));
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // State
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob));     // Feature
struct {
       {SUBMODULE_EXCLUDED_HEADER, ExcludedHeaderAbbrev, Module::HK_Excluded}
     };
     for (auto &HL : HeaderLists) {
       RecordData::value_type Record[] = {HL.RecordKind};
       for (auto &H : Mod->Headers[HL.HeaderKind])
         Stream.EmitRecordWithBlob(HL.Abbrev, Record, H.NameAsWritten);
     }
 
-    // Emit the top headers.
-    {
-      auto TopHeaders = Mod->getTopHeaders(PP->getFileManager());
-      RecordData::value_type Record[] = {SUBMODULE_TOPHEADER};
-      for (auto *H : TopHeaders) {
-        SmallString<128> HeaderName(H->getName());
-        PreparePathForOutput(HeaderName);
-        Stream.EmitRecordWithBlob(TopHeaderAbbrev, Record, HeaderName);
-      }
-    }
-
     // Emit the imports.
     if (!Mod->Imports.empty()) {
       RecordData Record;
       for (auto *I : Mod->Imports)
         Record.push_back(getSubmoduleID(I));
       Stream.EmitRecord(SUBMODULE_IMPORTS, Record);
     }
 

clang/test/Index/annotate-module.m

 // CHECK-MOD-NEXT: Keyword: "int" [2:1 - 2:4] VarDecl=Module_Sub:2:6
 // CHECK-MOD-NEXT: Punctuation: "*" [2:5 - 2:6] VarDecl=Module_Sub:2:6
 // CHECK-MOD-NEXT: Identifier: "Module_Sub" [2:6 - 2:16] VarDecl=Module_Sub:2:6
 // CHECK-MOD-NEXT: Punctuation: ";" [2:16 - 2:17]
 
 // RUN: c-index-test -cursor-at=%s:3:11 %s -fmodules-cache-path=%t.cache -fmodules -F %S/../Modules/Inputs \
 // RUN:     | FileCheck %s -check-prefix=CHECK-CURSOR
 
-// CHECK-CURSOR:      3:1 ModuleImport=DependsOnModule:3:1 (Definition) Extent=[3:1 - 3:24] Spelling=DependsOnModule ([3:9 - 3:24]) ModuleName=DependsOnModule ({{.*}}DependsOnModule-{{[^.]*}}.pcm) system=0 Headers(2):
-// CHECK-CURSOR-NEXT: {{.*}}other.h
-// CHECK-CURSOR-NEXT: {{.*}}DependsOnModule.h
+// CHECK-CURSOR:      3:1 ModuleImport=DependsOnModule:3:1 (Definition) Extent=[3:1 - 3:24] Spelling=DependsOnModule ([3:9 - 3:24]) ModuleName=DependsOnModule ({{.*}}DependsOnModule-{{[^.]*}}.pcm) system=0 Headers(0):

clang/test/Modules/relative-submodule-topheader.m

 // RUN: cd %S
 // RUN: %clang_cc1 -fmodules -fno-implicit-modules -x objective-c++ -fmodule-name=std -emit-module Inputs/submodules/module.map -o %t/mod.pcm
 // RUN: llvm-bcanalyzer --dump --disable-histogram %t/mod.pcm | FileCheck %s
 
 // CHECK: <SUBMODULE_HEADER abbrevid=6/> blob data = 'vector.h'
-// CHECK: <SUBMODULE_TOPHEADER abbrevid=7/> blob data = 'vector.h'
 // CHECK: <SUBMODULE_HEADER abbrevid=6/> blob data = 'type_traits.h'
-// CHECK: <SUBMODULE_TOPHEADER abbrevid=7/> blob data = 'type_traits.h'
 // CHECK: <SUBMODULE_HEADER abbrevid=6/> blob data = 'hash_map.h'
-// CHECK: <SUBMODULE_TOPHEADER abbrevid=7/> blob data = 'hash_map.h'

clang/tools/libclang/CIndex.cpp

int clang_Module_isSystem(CXModule CXMod) {
   Module *Mod = static_cast<Module *>(CXMod);
   return Mod->IsSystem;
 }
 
 unsigned clang_Module_getNumTopLevelHeaders(CXTranslationUnit TU,
                                             CXModule CXMod) {
   if (isNotUsableTU(TU)) {
     LOG_BAD_TU(TU);
-    return 0;
   }
-  if (!CXMod)
-    return 0;
-  Module *Mod = static_cast<Module *>(CXMod);
-  FileManager &FileMgr = cxtu::getASTUnit(TU)->getFileManager();
-  ArrayRef<const FileEntry *> TopHeaders = Mod->getTopHeaders(FileMgr);
-  return TopHeaders.size();
 }
 
 CXFile clang_Module_getTopLevelHeader(CXTranslationUnit TU, CXModule CXMod,
                                       unsigned Index) {
   if (isNotUsableTU(TU)) {
     LOG_BAD_TU(TU);
-    return nullptr;
   }
-  if (!CXMod)
-    return nullptr;
-  Module *Mod = static_cast<Module *>(CXMod);
-  FileManager &FileMgr = cxtu::getASTUnit(TU)->getFileManager();
-
-  ArrayRef<const FileEntry *> TopHeaders = Mod->getTopHeaders(FileMgr);
-  if (Index < TopHeaders.size())
-    return const_cast<FileEntry *>(TopHeaders[Index]);
-
   return nullptr;
 }
 
 //===----------------------------------------------------------------------===//
 // C++ AST instrospection.
 //===----------------------------------------------------------------------===//
 
 unsigned clang_CXXConstructor_isDefaultConstructor(CXCursor C) {