clang/Modules: Split loop in ReadAST between failable and not

[dexonsmith]

Summary

Split a loop in ReadAST that visits the just-loaded module chain,
between an initial loop that reads further from the ASTs (and can fail)
and a second loop that does some preloading (and cannot fail). This
makes it less likely for a reading failure to affect the AST.

This is not fixing a known bug and the behaviour change may not be
observable, it's just part of an audit to look at all of the error
handling in the ASTReader.

[aprantl]

clang/lib/Serialization/ASTReader.cpp

4211

and this?

4236

Could this be a range-based for?

[dexonsmith]

clang/lib/Serialization/ASTReader.cpp

4236

Yes, probably. If so, I'll do that to the existing one as a prep patch (before pushing this change), and follow the style for the new one.

[dexonsmith]

Committed in bfd58fc60ff4b0c081b5b489119c3798d3e2b53c and 01782c3e4df1830d7991e9edfee9119ed41e4c27.

clang/lib/Serialization/ASTReader.cpp

ASTReader::ASTReadResult ASTReader::ReadAST(StringRef FileName,
   case HadErrors:
     return removeModulesAndReturn(ReadResult);
   case Success:
     break;
   }
 
   // Here comes stuff that we only do once the entire chain is loaded.
 
-  // Load the AST blocks of all of the modules that we loaded.
+  // Load the AST blocks of all of the modules that we loaded.  We can still
+  // hit errors parsing the ASTs at this point.
   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);
 
     // 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.
     F.GlobalBitOffset = TotalModulesSizeInBits;
     TotalModulesSizeInBits += F.SizeInBits;
     GlobalBitOffsetsMap.insert(std::make_pair(F.GlobalBitOffset, &F));
+  }
 
+  // Preload source locations and interesting indentifiers.
+  for (SmallVectorImpl<ImportedModule>::iterator M = Loaded.begin(),
+                                              MEnd = Loaded.end();
+       M != MEnd; ++M) {
+    ModuleFile &F = *M->Mod;
     // Preload SLocEntries.
     for (unsigned I = 0, N = F.PreloadSLocEntries.size(); I != N; ++I) {
       int Index = int(F.PreloadSLocEntries[I] - 1) + F.SLocEntryBaseID;
       // Load it through the SourceManager and don't call ReadSLocEntry()
       // directly because the entry may have already been loaded in which case
       // calling ReadSLocEntry() directly would trigger an assertion in
       // SourceManager.
       SourceMgr.getLoadedSLocEntryByID(Index);