[libclang][index][NFCi] Refactor machinery for skipping already parsed function bodies

[jkorous]

Summary

Refactor machinery for skipping inline function bodies that have already been parsed in other frontend actions.

Mostly just:

• Changed naming with the expectation that this code is to be moved from its original context to libIndex.
• Added couple asserts.
• Added comments.

[gribozavr]

clang/tools/libclang/Indexing.cpp

126

"SharedParsedRegions"? "ThreadSafeParsedRegions"?

134

I think we should lock both the source and destination mutexes (use std::scoped_lock that allows to lock multiple mutexes).

Also, it would be more idiomatic to express this API as a copy constructor and an assignment operator.

138

"addParsedRegions"?

371–372

This pointer seems to be only used in the constructor, however the constructor already has access to skData parameter.

[jkorous]

Hmm, I already landed this - let's transfer the discussion to https://reviews.llvm.org/D66764.
I'll make the changes to that patch. Is that ok?

clang/tools/libclang/Indexing.cpp

126

I'll use the ThreadSafeParsedRegions . Thanks!

134

Sorry, I am not following. You are aware of the argument being a DenseSet, right? This doesn't look like a named copy-ctor to me.

The way this works is that ParsedSrcLocationsTracker takes snapshot of the SharedParsedRegionsStorage in its constructor (using copyTo()), then uses its own local data and synchronizes the local data with SharedParsedRegionsStorage in syncWithStorage() method.

Maybe you just made a case for removing the typedef?

138

That's probably better. Thanks!

371–372

Are you saying that we don't need this member and can just create ParsedSrcLocationsTracker in CreateASTConsumer since we have the skData?

[jkorous]

clang/tools/libclang/Indexing.cpp

371–372

If yes, then it's fixed.

[gribozavr]

clang/tools/libclang/Indexing.cpp

134

Sorry, I am not following. You are aware of the argument being a DenseSet, right? This doesn't look like a named copy-ctor to me.

Sorry -- you're right. I confused PPRegionSetTy with SharedParsedRegionsStorage.

Maybe you just made a case for removing the typedef?

I think that would be an improvement and happy to do that! PTAL at https://reviews.llvm.org/D67077 .

clang/tools/libclang/Indexing.cpp

struct DenseMapInfo<PPRegion> {
     static bool isEqual(const PPRegion &LHS, const PPRegion &RHS) {
       return LHS == RHS;
     }
   };
 }
 
 namespace {
 
-class SessionSkipBodyData {
+/// Keeps track of function bodies that have already been parsed.
+///
+/// Is thread-safe.
+class SharedParsedRegionsStorage {
   std::mutex Mux;
   PPRegionSetTy ParsedRegions;
 
 public:
-  ~SessionSkipBodyData() = default;
+  ~SharedParsedRegionsStorage() = default;
 
   void copyTo(PPRegionSetTy &Set) {
     std::lock_guard<std::mutex> MG(Mux);
     Set = ParsedRegions;
   }
 
-  void update(ArrayRef<PPRegion> Regions) {
+  void merge(ArrayRef<PPRegion> Regions) {
     std::lock_guard<std::mutex> MG(Mux);
     ParsedRegions.insert(Regions.begin(), Regions.end());
   }
 };
 
-class TUSkipBodyControl {
-  SessionSkipBodyData &SessionData;
+/// Provides information whether source locations have already been parsed in
+/// another FrontendAction.
+///
+/// Is NOT thread-safe.
+class ParsedSrcLocationsTracker {
+  SharedParsedRegionsStorage &ParsedRegionsStorage;
   PPConditionalDirectiveRecord &PPRec;
   Preprocessor &PP;
 
+  /// Snapshot of the shared state at the point when this instance was
+  /// constructed.
   PPRegionSetTy ParsedRegions;
+  /// Regions that were queried during this instance lifetime.
   SmallVector<PPRegion, 32> NewParsedRegions;
+
+  /// Caching the last queried region.
   PPRegion LastRegion;
   bool LastIsParsed;
 
 public:
-  TUSkipBodyControl(SessionSkipBodyData &sessionData,
+  /// Creates snapshot of \p ParsedRegionsStorage.
+  ParsedSrcLocationsTracker(SharedParsedRegionsStorage &ParsedRegionsStorage,
                             PPConditionalDirectiveRecord &ppRec,
                             Preprocessor &pp)
-    : SessionData(sessionData), PPRec(ppRec), PP(pp) {
-    SessionData.copyTo(ParsedRegions);
+      : ParsedRegionsStorage(ParsedRegionsStorage), PPRec(ppRec), PP(pp) {
+    ParsedRegionsStorage.copyTo(ParsedRegions);
   }
 
-  bool isParsed(SourceLocation Loc, FileID FID, const FileEntry *FE) {
+  /// \returns true iff \p Loc has already been parsed.
+  ///
+  /// Can provide false-negative in case the location was parsed after this
+  /// instance had been constructed.
+  bool hasAlredyBeenParsed(SourceLocation Loc, FileID FID,
+                           const FileEntry *FE) {
+    assert(FE);
     PPRegion region = getRegion(Loc, FID, FE);
     if (region.isInvalid())
       return false;
 
     // Check common case, consecutive functions in the same region.
     if (LastRegion == region)
       return LastIsParsed;
 
     LastRegion = region;
+    // Source locations can't be revisited during single TU parsing.
+    // That means if we hit the same region again, it's a different location in
+    // the same region and so the "is parsed" value from the snapshot is still
+    // correct.
     LastIsParsed = ParsedRegions.count(region);
     if (!LastIsParsed)
-      NewParsedRegions.push_back(region);
+      NewParsedRegions.emplace_back(std::move(region));
     return LastIsParsed;
   }
 
-  void finished() {
-    SessionData.update(NewParsedRegions);
-  }
+  /// Updates ParsedRegionsStorage with newly parsed regions.
+  void syncWithStorage() { ParsedRegionsStorage.merge(NewParsedRegions); }
 
 private:
   PPRegion getRegion(SourceLocation Loc, FileID FID, const FileEntry *FE) {
-    SourceLocation RegionLoc = PPRec.findConditionalDirectiveRegionLoc(Loc);
-    if (RegionLoc.isInvalid()) {
+    assert(FE);
+    auto Bail = [this, FE]() {
       if (isParsedOnceInclude(FE)) {
         const llvm::sys::fs::UniqueID &ID = FE->getUniqueID();
         return PPRegion(ID, 0, FE->getModificationTime());
       }
       return PPRegion();
-    }
+    };
 
-    const SourceManager &SM = PPRec.getSourceManager();
+    SourceLocation RegionLoc = PPRec.findConditionalDirectiveRegionLoc(Loc);
     assert(RegionLoc.isFileID());
+    if (RegionLoc.isInvalid())
+      return Bail();
+
     FileID RegionFID;
     unsigned RegionOffset;
-    std::tie(RegionFID, RegionOffset) = SM.getDecomposedLoc(RegionLoc);
+    std::tie(RegionFID, RegionOffset) =
+        PPRec.getSourceManager().getDecomposedLoc(RegionLoc);
 
-    if (RegionFID != FID) {
-      if (isParsedOnceInclude(FE)) {
-        const llvm::sys::fs::UniqueID &ID = FE->getUniqueID();
-        return PPRegion(ID, 0, FE->getModificationTime());
-      }
-      return PPRegion();
-    }
+    if (RegionFID != FID)
+      return Bail();
 
     const llvm::sys::fs::UniqueID &ID = FE->getUniqueID();
     return PPRegion(ID, RegionOffset, FE->getModificationTime());
   }
 
   bool isParsedOnceInclude(const FileEntry *FE) {
     return PP.getHeaderSearchInfo().isFileMultipleIncludeGuarded(FE);
   }

 };
 
 //===----------------------------------------------------------------------===//
 // IndexingConsumer
 //===----------------------------------------------------------------------===//
 
 class IndexingConsumer : public ASTConsumer {
   CXIndexDataConsumer &DataConsumer;
-  TUSkipBodyControl *SKCtrl;
+  ParsedSrcLocationsTracker *ParsedLocsTracker;
 
 public:
-  IndexingConsumer(CXIndexDataConsumer &dataConsumer, TUSkipBodyControl *skCtrl)
-    : DataConsumer(dataConsumer), SKCtrl(skCtrl) { }
+  IndexingConsumer(CXIndexDataConsumer &dataConsumer,
+                   ParsedSrcLocationsTracker *parsedLocsTracker)
+      : DataConsumer(dataConsumer), ParsedLocsTracker(parsedLocsTracker) {}
 
   // ASTConsumer Implementation
 
   void Initialize(ASTContext &Context) override {
     DataConsumer.setASTContext(Context);
     DataConsumer.startedTranslationUnit();
   }
 
   void HandleTranslationUnit(ASTContext &Ctx) override {
-    if (SKCtrl)
-      SKCtrl->finished();
+    if (ParsedLocsTracker)
+      ParsedLocsTracker->syncWithStorage();
   }
 
   bool HandleTopLevelDecl(DeclGroupRef DG) override {
     return !DataConsumer.shouldAbort();
   }
 
   bool shouldSkipFunctionBody(Decl *D) override {
-    if (!SKCtrl) {
+    if (!ParsedLocsTracker) {
       // Always skip bodies.
       return true;
     }
 
     const SourceManager &SM = DataConsumer.getASTContext().getSourceManager();
     SourceLocation Loc = D->getLocation();
     if (Loc.isMacroID())
       return false;
     if (SM.isInSystemHeader(Loc))
       return true; // always skip bodies from system headers.
 
     FileID FID;
     unsigned Offset;
     std::tie(FID, Offset) = SM.getDecomposedLoc(Loc);
     // Don't skip bodies from main files; this may be revisited.
     if (SM.getMainFileID() == FID)
       return false;
     const FileEntry *FE = SM.getFileEntryForID(FID);
     if (!FE)
       return false;
 
-    return SKCtrl->isParsed(Loc, FID, FE);
+    return ParsedLocsTracker->hasAlredyBeenParsed(Loc, FID, FE);
   }
 };
 
 //===----------------------------------------------------------------------===//
 // CaptureDiagnosticConsumer
 //===----------------------------------------------------------------------===//
 
 class CaptureDiagnosticConsumer : public DiagnosticConsumer {

 
 //===----------------------------------------------------------------------===//
 // IndexingFrontendAction
 //===----------------------------------------------------------------------===//
 
 class IndexingFrontendAction : public ASTFrontendAction {
   std::shared_ptr<CXIndexDataConsumer> DataConsumer;
 
-  SessionSkipBodyData *SKData;
-  std::unique_ptr<TUSkipBodyControl> SKCtrl;
+  SharedParsedRegionsStorage *SKData;
+  std::unique_ptr<ParsedSrcLocationsTracker> ParsedLocsTracker;
 
 public:
   IndexingFrontendAction(std::shared_ptr<CXIndexDataConsumer> dataConsumer,
-                         SessionSkipBodyData *skData)
+                         SharedParsedRegionsStorage *skData)
       : DataConsumer(std::move(dataConsumer)), SKData(skData) {}
 
   std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
                                                  StringRef InFile) override {
     PreprocessorOptions &PPOpts = CI.getPreprocessorOpts();
 
     if (!PPOpts.ImplicitPCHInclude.empty()) {
       auto File = CI.getFileManager().getFile(PPOpts.ImplicitPCHInclude);
       if (File)
         DataConsumer->importedPCH(*File);
     }
 
     DataConsumer->setASTContext(CI.getASTContext());
     Preprocessor &PP = CI.getPreprocessor();
     PP.addPPCallbacks(std::make_unique<IndexPPCallbacks>(PP, *DataConsumer));
     DataConsumer->setPreprocessor(CI.getPreprocessorPtr());
 
     if (SKData) {
       auto *PPRec = new PPConditionalDirectiveRecord(PP.getSourceManager());
       PP.addPPCallbacks(std::unique_ptr<PPCallbacks>(PPRec));
-      SKCtrl = std::make_unique<TUSkipBodyControl>(*SKData, *PPRec, PP);
+      ParsedLocsTracker =
+          std::make_unique<ParsedSrcLocationsTracker>(*SKData, *PPRec, PP);
     }
 
-    return std::make_unique<IndexingConsumer>(*DataConsumer, SKCtrl.get());
+    return std::make_unique<IndexingConsumer>(*DataConsumer,
+                                              ParsedLocsTracker.get());
   }
 
   TranslationUnitKind getTranslationUnitKind() override {
     if (DataConsumer->shouldIndexImplicitTemplateInsts())
       return TU_Complete;
     else
       return TU_Prefix;
   }
if (index_options & CXIndexOpt_IndexFunctionLocalSymbols)
     IdxOpts.IndexFunctionLocals = true;
   if (index_options & CXIndexOpt_IndexImplicitTemplateInstantiations)
     IdxOpts.IndexImplicitInstantiation = true;
   return IdxOpts;
 }
 
 struct IndexSessionData {
   CXIndex CIdx;
-  std::unique_ptr<SessionSkipBodyData> SkipBodyData;
+  std::unique_ptr<SharedParsedRegionsStorage> SkipBodyData;
 
   explicit IndexSessionData(CXIndex cIdx)
-    : CIdx(cIdx), SkipBodyData(new SessionSkipBodyData) {}
+      : CIdx(cIdx), SkipBodyData(new SharedParsedRegionsStorage) {}
 };
 
 } // anonymous namespace
 
 static CXErrorCode clang_indexSourceFile_Impl(
     CXIndexAction cxIdxAction, CXClientData client_data,
     IndexerCallbacks *client_index_callbacks, unsigned index_callbacks_size,
     unsigned index_options, const char *source_filename,