clang/Modules: Error if ReadASTBlock does not find the main module

[dexonsmith]

Summary

If ReadASTBlock does not find the main module, there's something wrong
the with the PCM. Error in that case, to avoid hitting problems further
from the source.

Note that the Swift compiler sometimes finds in
CompilerInstance::loadModule that the main module mysteriously does not
have Module::IsFromModuleFile set. That will emit a confusing
warn_missing_submodule, which was never intended for a top-level module.
The recent audit of error-handling in ReadAST may have rooted out the
real problem. If not, this commit will help to clarify the real
problem, and replace a confusing warning with an error pointing at the
malformed PCM file.

[dexonsmith]

Updated to use a new diagnostic (err_module_file_missing_definition) that includes the filename of the PCM.

[aprantl]

Do we want to bother with a testcase for this?

clang/include/clang/Basic/DiagnosticSerializationKinds.td

78

Should this be AST file like in the above error message?

[bruno]

clang/include/clang/Basic/DiagnosticSerializationKinds.td

78

+1

clang/lib/Serialization/ASTReader.cpp

5503

Is this enough? Because this is done at the end of SUBMODULE_DEFINITION, could it be that only some of the submodules were read (top level or not) and this is still going to be true? I wonder if marking this true at the end of successful SUBMODULE_BLOCK instead wouldn't be a better option.

[dexonsmith]

clang/include/clang/Basic/DiagnosticSerializationKinds.td

78

I think this should be module file, because it only applies to modules. Notice that err_module_file_not_found and err_module_file_out_of_date use %select{PCH|module|AST} file; this is just a constant-folding of that.

clang/lib/Serialization/ASTReader.cpp

5503

I see three cases to distinguish between:

1. An error is encountered somewhere in the submodule block.
2. The submodule block is read without error, but the main module is not seen/added.
3. The submodule block is read without error, and the main module is seen/added.

Setting a flag to true at the end of a successful SUBMODULE_BLOCK would help to distinguish between (1) and (2 or 3), but the return status already tells us that, and ReadAST already deletes all the just-loaded modules in that case.

This patch as-is helps to distinguish between (2) and (3) once the early return for (1) has been avoided.

Does that make sense? Or maybe I didn't follow your suggestion.

[bruno]

LGTM with some minor comment below!

clang/include/clang/Basic/DiagnosticSerializationKinds.td

78

Makes sense!

clang/lib/Serialization/ASTReader.cpp

5503

Oh, I see now! Thanks for the explanation, perhaps add this to the commit message?

[dexonsmith]

Committed 83dcb34b6bf4c175040b18d3e8c3c468418009fc. Thanks for the reviews!

clang/lib/Serialization/ASTReader.cpp

5503

Added a comment there, and also reworded variables / diagnostics from talking about "main module" to talking about "top-level submodule", since I think that might be more consistent (and maybe more clear).

clang/include/clang/Basic/DiagnosticSerializationKinds.td

def err_module_file_out_of_date : Error<
   "%select{PCH|module|AST}0 file '%1' is out of date and "
   "needs to be rebuilt%select{|: %3}2">, DefaultFatal;
 def err_module_file_invalid : Error<
   "file '%1' is not a valid precompiled %select{PCH|module|AST}0 file">, DefaultFatal;
 def note_module_file_imported_by : Note<
   "imported by %select{|module '%2' in }1'%0'">;
 def err_module_file_not_module : Error<
   "AST file '%0' was not built as a module">, DefaultFatal;
+def err_module_file_missing_definition : Error<
+  "module file '%0' is missing the main module's definition">, DefaultFatal;
 
 def remark_module_import : Remark<
   "importing module '%0'%select{| into '%3'}2 from '%1'">,
   InGroup<ModuleImport>;
 
 def err_imported_module_not_found : Error<
     "module '%0' in AST file '%1' %select{(imported by AST file '%2') |}4"
     "is not defined in any loaded module map file; "

clang/include/clang/Serialization/Module.h

public:
   bool RelocatablePCH = false;
 
   /// Whether timestamps are included in this module file.
   bool HasTimestamps = false;
 
   /// Whether the PCH has a corresponding object file.
   bool PCHHasObjectFile = false;
 
+  /// Whether the main module has been read from the AST file.
+  bool DidReadMainModule = false;
+
   /// The file entry for the module file.
   const FileEntry *File = nullptr;
 
   /// The signature of the module file, which may be used instead of the size
   /// and modification time to identify this particular file.
   ASTFileSignature Signature;
 
   /// Whether this module has been directly imported by the

clang/lib/Serialization/ASTReader.cpp

for (SmallVectorImpl<ImportedModule>::iterator M = Loaded.begin(),
                                               MEnd = Loaded.end();
        M != MEnd; ++M) {
     ModuleFile &F = *M->Mod;
 
     // Read the AST block.
     if (ASTReadResult Result = ReadASTBlock(F, ClientLoadCapabilities))
       return removeModulesAndReturn(Result);
 
+    // The AST block should always have a definition for the main module.
+    if (F.isModule() && !F.DidReadMainModule) {
+      Error(diag::err_module_file_missing_definition, F.FileName);
+      return removeModulesAndReturn(Failure);
+    }
+
     // Read the extension blocks.
     while (!SkipCursorToBlock(F.Stream, EXTENSION_BLOCK_ID)) {
       if (ASTReadResult Result = ReadExtensionBlock(F))
         return removeModulesAndReturn(Result);
     }
 
     // Once read, set the ModuleFile bit base offset and update the size in
     // bits of all files we've seen.
case SUBMODULE_DEFINITION: {
               CurFile != F.File) {
             Error(diag::err_module_file_conflict,
                   CurrentModule->getTopLevelModuleName(), CurFile->getName(),
                   F.File->getName());
             return Failure;
           }
         }
 
+        F.DidReadMainModule = true;
         CurrentModule->setASTFile(F.File);
         CurrentModule->PresumedModuleMapFile = F.ModuleMapPath;
       }
 
       CurrentModule->Kind = Kind;
       CurrentModule->Signature = F.Signature;
       CurrentModule->IsFromModuleFile = true;
       CurrentModule->IsSystem = IsSystem || CurrentModule->IsSystem;