PrettyStackTrace guards around ASTReader

[jordan_rose]

Summary

What do you think about adding a very trivial PrettyStackTrace guard around the entry points to ASTReader, to remind compiler developers to clear their module cache when they hit a crash here? Would this be the right way to do that?

[rsmith]

Yeah, seems like a good idea to me. It's interesting that the places you added this aren't the same places we construct Deserializing objects. I wonder if there are good reasons for the differences.

lib/Serialization/ASTReader.cpp

70

I'm not sure I like this as a user-facing message; the revision number is supposed to be part of the configuration, so the 'clear your module cache' advice really only applies to people actively hacking on Clang. But I'm fine with the change with or without this line.

7521

This function doesn't do any reading from the AST file.

7560

This function doesn't do any reading from the AST file.

[jordan_rose]

I originally considered putting it into the Deserializing RAII, but decided against it, mainly because Deserializing can stack but I only really wanted one message in the trace. It's more about "this problem might just be the module cache" than actually providing a trace. There are also a few points where I added a trace where we don't bother setting up a Deserializing object, like selectors.

lib/Serialization/ASTReader.cpp

70

The revision number thing hasn't worked for a long time, at least not for git. You're still right that it only applies to compiler devs, but this means there are a lot of people building tools on top of Clang (in our case the Swift compiler) that break when things are updated and don't know anything about modules.

I keep meaning to fix the revision thing but not getting around to it. The requirement last time I tried (years ago) was that it has to not do work on null builds.

[jordan_rose]

I thought about how hard it would be to just fix the CMake build and ended up just doing that instead. That drops this down to only being an issue for people actively working on ASTReader, as you said originally, at which point the normal backtrace is probably enough of a hint. Abandoning.

lib/Serialization/ASTReader.cpp

 #include "clang/Serialization/SerializationDiagnostic.h"
 #include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Bitcode/BitstreamReader.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/SaveAndRestore.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <cstdio>
 #include <iterator>
 #include <system_error>
 
 using namespace clang;
 using namespace clang::serialization;
 using namespace clang::serialization::reader;
 using llvm::BitstreamCursor;
 
+namespace {
+  class PrettyStackTraceASTReader : llvm::PrettyStackTraceString {
+    constexpr static const char * const Message =
+      "\n\n*** Crashed while deserializing from an AST file (e.g. PCH or PCM)."
+      "\n*** Consider clearing your module cache.\n";
+  public:
+    PrettyStackTraceASTReader() : PrettyStackTraceString(Message) {}
+  };
+
+} // end anonymous namespace
+
 
 //===----------------------------------------------------------------------===//
 // ChainedASTReaderListener implementation
 //===----------------------------------------------------------------------===//
 
 bool
 ChainedASTReaderListener::ReadFullVersionInformation(StringRef FullVersion) {
   return First->ReadFullVersionInformation(FullVersion) ||
public:
     
     // \brief Retrieve the identifier info found within the module
     // files.
     IdentifierInfo *getIdentifierInfo() const { return Found; }
   };
 }
 
 void ASTReader::updateOutOfDateIdentifier(IdentifierInfo &II) {
+  PrettyStackTraceASTReader StackTraceRAII;
+
   // Note that we are loading an identifier.
   Deserializing AnIdentifier(this);
 
   unsigned PriorGeneration = 0;
   if (getContext().getLangOpts().Modules)
     PriorGeneration = IdentifierGeneration[&II];
 
   // If there is a global index, look there first to determine which modules
if (EC)
     return;
   OS << "Timestamp file\n";
 }
 
 ASTReader::ASTReadResult ASTReader::ReadAST(const std::string &FileName,
                                             ModuleKind Type,
                                             SourceLocation ImportLoc,
                                             unsigned ClientLoadCapabilities) {
+  PrettyStackTraceASTReader StackTraceRAII;
   llvm::SaveAndRestore<SourceLocation>
     SetCurImportLocRAII(CurrentImportLoc, ImportLoc);
 
   // Defer any pending actions until we get to the end of reading the AST file.
   Deserializing AnASTFile(this);
 
   // Bump the generation number.
   unsigned PreviousGeneration = incrementGeneration(Context);
ASTReader::ReadASTTemplateArgumentListInfo(ModuleFile &F,
   unsigned NumArgsAsWritten = Record[Index++];
   TemplateArgumentListInfo TemplArgsInfo(LAngleLoc, RAngleLoc);
   for (unsigned i = 0; i != NumArgsAsWritten; ++i)
     TemplArgsInfo.addArgument(ReadTemplateArgumentLoc(F, Record, Index));
   return ASTTemplateArgumentListInfo::Create(getContext(), TemplArgsInfo);
 }
 
 Decl *ASTReader::GetExternalDecl(uint32_t ID) {
+  PrettyStackTraceASTReader StackTraceRAII;
   return GetDecl(ID);
 }
 
 void ASTReader::CompleteRedeclChain(const Decl *D) {
   if (NumCurrentElementsDeserializing) {
     // We arrange to not care about the complete redeclaration chain while we're
     // deserializing. Just remember that the AST has marked this one as complete
     // but that it's not actually complete yet, so we know we still need to
     // complete it later.
     PendingIncompleteDeclChains.push_back(const_cast<Decl*>(D));
     return;
   }
 
+  PrettyStackTraceASTReader StackTraceRAII;
+
   const DeclContext *DC = D->getDeclContext()->getRedeclContext();
 
   // If this is a named declaration, complete it by looking it up
   // within its context.
   //
   // FIXME: Merging a function definition should merge
   // all mergeable entities within it.
   if (isa<TranslationUnitDecl>(DC) || isa<NamespaceDecl>(DC) ||
if (Idx >= Record.size() || Record[Idx] > M.LocalNumCXXBaseSpecifiers) {
     return 0;
   }
 
   unsigned LocalID = Record[Idx++];
   return getGlobalBitOffset(M, M.CXXBaseSpecifiersOffsets[LocalID - 1]);
 }
 
 CXXBaseSpecifier *ASTReader::GetExternalCXXBaseSpecifiers(uint64_t Offset) {
+  PrettyStackTraceASTReader StackTraceRAII;
+
   RecordLocation Loc = getLocalBitOffset(Offset);
   BitstreamCursor &Cursor = Loc.F->DeclsCursor;
   SavedStreamPosition SavedPosition(Cursor);
   Cursor.JumpToBit(Loc.Offset);
   ReadingKindTracker ReadingKind(Read_Decl, *this);
   RecordData Record;
   unsigned Code = Cursor.ReadCode();
   unsigned RecCode = Cursor.readRecord(Code, Record);

 }
 
 /// \brief Resolve the offset of a statement into a statement.
 ///
 /// This operation will read a new statement from the external
 /// source each time it is called, and is meant to be used via a
 /// LazyOffsetPtr (which is used by Decls for the body of functions, etc).
 Stmt *ASTReader::GetExternalDeclStmt(uint64_t Offset) {
+  PrettyStackTraceASTReader StackTraceRAII;
+
   // Switch case IDs are per Decl.
   ClearSwitchCaseIDs();
 
   // Offset here is a global offset across the entire chain.
   RecordLocation Loc = getLocalBitOffset(Offset);
   Loc.F->DeclsCursor.JumpToBit(Loc.Offset);
   return ReadStmtFromStream(*Loc.F);
 }
static bool visit(ModuleFile &M, bool Preorder, void *UserData) {
       return false;
     }
   };
 }
 
 ExternalLoadResult ASTReader::FindExternalLexicalDecls(const DeclContext *DC,
                                          bool (*isKindWeWant)(Decl::Kind),
                                          SmallVectorImpl<Decl*> &Decls) {
+  PrettyStackTraceASTReader StackTraceRAII;
+
   // There might be lexical decls in multiple modules, for the TU at
   // least. Walk all of the modules in the order they were loaded.
   FindExternalLexicalDeclsVisitor Visitor(*this, DC, isKindWeWant, Decls);
   ModuleMgr.visitDepthFirst(&FindExternalLexicalDeclsVisitor::visit, &Visitor);
   ++NumLexicalDeclContextsRead;
   return ELR_Success;
 }
 
public:
   }
 };
 
 }
 
 void ASTReader::FindFileRegionDecls(FileID File,
                                     unsigned Offset, unsigned Length,
                                     SmallVectorImpl<Decl *> &Decls) {
+  PrettyStackTraceASTReader StackTraceRAII;
+
   SourceManager &SM = getSourceManager();
 
   llvm::DenseMap<FileID, FileDeclsInfo>::iterator I = FileDeclIDs.find(File);
   if (I == FileDeclIDs.end())
     return;
 
   FileDeclsInfo &DInfo = I->second;
   if (DInfo.Decls.empty())

 bool
 ASTReader::FindExternalVisibleDeclsByName(const DeclContext *DC,
                                           DeclarationName Name) {
   assert(DC->hasExternalVisibleStorage() &&
          "DeclContext has no visible decls in storage");
   if (!Name)
     return false;
 
+  PrettyStackTraceASTReader StackTraceRAII;
   Deserializing LookupResults(this);
 
   SmallVector<NamedDecl *, 64> Decls;
 
   // Compute the declaration contexts we need to look into. Multiple such
   // declaration contexts occur when two declaration contexts from disjoint
   // modules get merged, e.g., when two namespaces with the same name are 
   // independently defined in separate modules.
static bool visit(ModuleFile &M, void *UserData) {
       return FoundAnything && !This->VisitAll;
     }
   };
 }
 
 void ASTReader::completeVisibleDeclsMap(const DeclContext *DC) {
   if (!DC->hasExternalVisibleStorage())
     return;
+
+  PrettyStackTraceASTReader StackTraceRAII;
+
   DeclsMap Decls;
 
   // Compute the declaration contexts we need to look into. Multiple such
   // declaration contexts occur when two declaration contexts from disjoint
   // modules get merged, e.g., when two namespaces with the same name are
   // independently defined in separate modules.
   SmallVector<const DeclContext *, 2> Contexts;
   Contexts.push_back(DC);
if (GlobalID > SubmodulesLoaded.size()) {
     Error("submodule ID out of range in AST file");
     return nullptr;
   }
   
   return SubmodulesLoaded[GlobalID - NUM_PREDEF_SUBMODULE_IDS];
 }
 
 Module *ASTReader::getModule(unsigned ID) {
+  PrettyStackTraceASTReader StackTraceRAII;
   return getSubmodule(ID);
 }
 
 Selector ASTReader::getLocalSelector(ModuleFile &M, unsigned LocalID) {
   return DecodeSelector(getGlobalSelectorID(M, LocalID));
 }
 
 Selector ASTReader::DecodeSelector(serialization::SelectorID ID) {
if (SelectorsLoaded[ID - 1].getAsOpaquePtr() == nullptr) {
     if (DeserializationListener)
       DeserializationListener->SelectorRead(ID, SelectorsLoaded[ID - 1]);
   }
 
   return SelectorsLoaded[ID - 1];
 }
 
 Selector ASTReader::GetExternalSelector(serialization::SelectorID ID) {
+  PrettyStackTraceASTReader StackTraceRAII;
   return DecodeSelector(ID);
 }
 
 uint32_t ASTReader::GetNumExternalSelectors() {
+  PrettyStackTraceASTReader StackTraceRAII;
   // ID 0 (the null selector) is considered an external selector.
   return getTotalNumSelectors() + 1;
 }
 
 serialization::SelectorID
 ASTReader::getGlobalSelectorID(ModuleFile &M, unsigned LocalID) const {
   if (LocalID < NUM_PREDEF_SELECTOR_IDS)
     return LocalID;
SwitchCase *ASTReader::getSwitchCaseWithID(unsigned ID) {
   return (*CurrSwitchCaseStmts)[ID];
 }
 
 void ASTReader::ClearSwitchCaseIDs() {
   CurrSwitchCaseStmts->clear();
 }
 
 void ASTReader::ReadComments() {
+  PrettyStackTraceASTReader StackTraceRAII;
+
   std::vector<RawComment *> Comments;
   for (SmallVectorImpl<std::pair<BitstreamCursor,
                                  serialization::ModuleFile *> >::iterator
        I = CommentsCursors.begin(),
        E = CommentsCursors.end();
        I != E; ++I) {
     Comments.clear();
     BitstreamCursor &Cursor = I->first;