Add incremental analysis cache via --cache-dir (M9)

Author: czhao-dev

README.md

@@ -7,12 +7,13 @@ defects with precise source locations.
 
 This is the MVP scoped in [`docs/spec.md`](docs/spec.md) -- three rules,
 text/JSON/SARIF output, YAML config with suppression, and one flagship
-cross-file rule with real graph-algorithm teeth -- plus five items from
+cross-file rule with real graph-algorithm teeth -- plus six items from
 the Phase 2+ backlog: SARIF output, three more rules (`use-after-move`,
 `unbalanced-lock`, `unchecked-null-result`, `expensive-copy`, two of them
-sharing a generic worklist dataflow engine), and parallel translation-unit
-analysis (`--jobs`). See [`docs/architecture.md`](docs/architecture.md)
-for how it's built internally.
+sharing a generic worklist dataflow engine), parallel translation-unit
+analysis (`--jobs`), and an incremental analysis cache (`--cache-dir`).
+See [`docs/architecture.md`](docs/architecture.md) for how it's built
+internally.
 
 ## Rules
 
@@ -49,6 +50,15 @@ means, but -- unlike `lock-order-inversion` -- only tracks explicit
 `lock()`/`unlock()` calls, not RAII `lock_guard`/`unique_lock` (which are
 balanced by construction; nothing to check).
 
+`--cache-dir <dir>` enables an incremental analysis cache: a translation
+unit whose main file, every header it transitively includes, its compile
+command, the enabled rule set, `--header-filter`, and the cpp-sentinel
+version all still match a prior run's cache entry is served straight from
+that entry instead of being re-parsed and re-analyzed. On this project's
+own ~50-TU self-scan, a fully warm cache brings a ~14s run down to
+~0.5s -- with byte-for-byte identical findings, verified by diffing cold
+vs. warm output. Unset (the default) disables caching entirely.
+
 ## Building
 
 Requires CMake 3.20+, a C++20 compiler, and LLVM/Clang dev packages
@@ -95,7 +105,8 @@ units are analyzed), `--header-filter <glob>` (also report diagnostics
 from headers matching this pattern; by default only each TU's main file
 is reported, matching clang-tidy's convention), `--fail-on warning|error`,
 `-j`/`--jobs <N>` (translation units analyzed in parallel; default `0` =
-all available hardware threads).
+all available hardware threads), `--cache-dir <dir>` (enables the
+incremental analysis cache; unset by default).
 
 Exit codes: `0` clean, `1` findings at/above `--fail-on`, `2` usage/config
 error, `4` a translation unit failed to compile.
@@ -177,9 +188,13 @@ data point).
 
 ## Known limitations
 
-- No incremental caching: every TU is re-analyzed from scratch on every
-  run, even with parallel TU analysis (`--jobs`) doing the work
-  concurrently.
+- The incremental cache (`--cache-dir`) has no explicit invalidate/clear
+  command beyond deleting the directory -- fine for the common case
+  (every relevant input is already part of the cache key or dependency
+  set), but there's no `--cache-stats`-style inspection beyond the
+  one-line hit-count summary printed to stderr. A TU that fails to
+  compile is never cached, so it's always retried in full on the next run
+  rather than caching (and repeatedly serving) a failure.
 - `lock-order-inversion`: no alias analysis (two pointers to the same
   mutex are different lock ids unless they resolve to the same
   `VarDecl`/`FieldDecl`), loop bodies walked once rather than to a

docs/architecture.md

@@ -8,13 +8,17 @@ compile_commands.json
 CompilationDatabaseLoader        (JSONCompilationDatabase / loadFromDirectory)
         |
 AnalysisDriver + llvm::DefaultThreadPool
-        |  one task per TU, each with its own fresh rule instances and
-        |  its own ClangTool -- see "Parallel translation-unit analysis"
+        |  one task per TU, each with its own fresh rule instances -- see
+        |  "Parallel translation-unit analysis"
+        |  IncrementalCache hit? -> skip straight to Rule::restoreState()
+        |  + cached diagnostics, no ClangTool at all -- see "Incremental
+        |  analysis cache"
         |
    +----+-----------------------------+
    |                                  |
 RuleFrontendAction -> ASTConsumer     |
-   |  per TU (inside one task):      |
+   |  per TU (inside one task,       |
+   |  only on a cache miss):         |
    |   - MatchFinder dispatches      |
    |     AST-matcher rules           |
    |     (missing-override,          |
@@ -87,8 +91,10 @@ default, means "all available hardware threads"). Each task:
 1. Builds its own fresh set of rule instances via `RuleRegistry` (the
    same rule ids `AnalysisDriver` was constructed with), not the driver's
    own instances.
-2. Runs its own `clang::tooling::ClangTool` against just that one file,
-   with its own local `std::vector<Diagnostic>` sink.
+2. If an incremental cache is configured, checks it first (see below) --
+   a hit skips the rest of this list entirely. Otherwise runs its own
+   `clang::tooling::ClangTool` against just that one file, with its own
+   local `std::vector<Diagnostic>` sink.
 
 Fresh instances per task, not shared ones, because an AST-matcher rule
 stashes its current TU's `TranslationUnitContext` in a member between
@@ -115,6 +121,65 @@ returning -- output would otherwise depend on however the thread pool
 happened to interleave tasks, which is a real, if easy to miss,
 determinism regression once a driver goes from sequential to parallel.
 
+## Incremental analysis cache
+
+`IncrementalCache` (`include/sentinel/core/IncrementalCache.h`,
+`lib/core/IncrementalCache.cpp`) is deliberately Clang-agnostic, in the
+same spirit as `dataflow::runForwardWorklist` and `graph/SCC.h`: it knows
+about a "primary key" string, a list of `{file, contentHash}`
+dependencies, and a `CacheEntry` blob; it has no idea what a TU or an AST
+is. All the Clang-specific glue lives in `AnalysisDriver.cpp`.
+
+Two problems make a single "content hash of the file" not enough:
+
+- A TU's result depends on more than the file's own text -- its compile
+  flags, the enabled rule set, `--header-filter`, and the cpp-sentinel
+  build itself can all change what gets reported without the file
+  changing at all. These fold into the **primary key**
+  (`IncrementalCache::computeKey()`), computable *before* parsing, and
+  changing any of them invalidates every entry that depended on the old
+  value (there's no partial invalidation across a key change -- simplest
+  correct behavior, and a version/flag/rule-set change is rare next to a
+  source edit).
+- The TU's result also depends on every header it transitively includes,
+  and that set isn't knowable until *after* a real parse. So freshness
+  for a given primary key is checked lazily: `AnalysisDriver`
+  (`collectDependencyFiles()` in `RuleFrontendAction.cpp`, wired through
+  an optional out-parameter on `createRuleFrontendAction()`) walks the
+  TU's `SourceManager::fileinfo_begin()/end()` after a real analysis and
+  records every real on-disk file it touched -- the main file plus every
+  header -- each hashed via the same FNV-1a `sentinel::hashContent()`
+  `Fingerprint.cpp` already used (extracted to `ContentHash.h` so both
+  agree on one implementation). `IncrementalCache::lookup()` re-hashes
+  every recorded dependency's *current* content on every call; any
+  mismatch (or a deleted file) is a miss.
+
+The trickiest part is `lock-order-inversion` again: its `finalize()`
+needs the full cross-TU picture, but a cache-hit TU is never re-parsed,
+so there's no fresh `LockOrderFact` list for it that run. `Rule::
+serializeState()`/`restoreState()` close that gap generically -- a rule
+can serialize whatever per-TU state its `finalize()` needs into an opaque
+blob `IncrementalCache` stores alongside a TU's diagnostics, and
+reconstitute it on a later cache hit into a fresh per-task instance,
+which then flows into `Rule::mergeFrom()` exactly like a freshly-computed
+task's state would (see "Parallel translation-unit analysis" above) --
+the cache doesn't need a special code path for this at all, because a
+cache hit and a cache miss both end up producing "a rule instance with
+some state," merged the same way either way. Default no-op, like
+`mergeFrom()`; only `LockOrderInversionRule` overrides it, serializing
+`factStore_.facts()` to a small JSON array.
+`AnalysisDriverCacheTest.CrossFileCycleStillDetectedWithOneCachedAndOneChangedFile`
+is the test that actually proves this: two files form a lock-order cycle,
+one is touched (forcing a miss) and one isn't (a hit), and the cycle must
+still be detected -- exactly the case a cache that only stored
+diagnostics (and not this per-rule state) would silently break.
+
+A TU that fails to compile is never cached (`AnalysisDriver.cpp`): its
+dependency list is likely incomplete (Clang can bail out before touching
+every header a correct parse would), so caching it risks a stale entry
+surviving even after the real problem is fixed -- simplest and safest to
+just retry a compile failure in full on every run.
+
 ## The flagship rule: `lock-order-inversion`
 
 Three moving pieces, in `lib/rules/LockOrderInversionRule.cpp`:

include/sentinel/core/AnalysisDriver.h

@@ -35,11 +35,23 @@ class AnalysisDriver {
   // The returned diagnostics are sorted by (file, line, column, rule id)
   // so output is deterministic regardless of how threads happened to be
   // scheduled.
+  //
+  // `cacheDir`, if non-empty, enables the incremental cache: a TU whose
+  // main file, transitively-included headers, compile command, enabled
+  // rule set, header filter, and tool version all match a prior run's
+  // cache entry is served from that entry instead of being re-parsed and
+  // re-analyzed. Empty (the default) disables caching entirely -- every
+  // TU is always freshly analyzed, exactly as before this existed. If
+  // `cacheHitCount` is non-null, it's set to how many of `sourcePaths`
+  // were served from the cache (always 0 when caching is disabled) --
+  // mainly for tests to observe cache behavior directly rather than
+  // inferring it from timing or side effects.
   std::vector<Diagnostic> run(clang::tooling::CompilationDatabase &compileDb,
                               const std::vector<std::string> &sourcePaths,
                               bool *hadCompileErrors = nullptr,
                               const HeaderFilter &headerFilter = HeaderFilter(),
-                              unsigned jobCount = 0);
+                              unsigned jobCount = 0, const std::string &cacheDir = "",
+                              size_t *cacheHitCount = nullptr);
 
 private:
   std::vector<std::unique_ptr<Rule>> rules_;

include/sentinel/core/ContentHash.h

@@ -0,0 +1,15 @@
+#pragma once
+
+#include <string>
+
+namespace sentinel {
+
+// Hex-encoded FNV-1a 64-bit hash of `data` -- no external dependency,
+// deterministic, good enough avalanche behavior for content-addressing
+// (fingerprints, incremental-cache keys) that only needs to distinguish
+// inputs, not resist adversarial collisions. Shared so every hash in this
+// codebase (diagnostic fingerprints, cache keys, dependency-file content
+// hashes) agrees on one implementation.
+std::string hashContent(const std::string &data);
+
+} // namespace sentinel

include/sentinel/core/HeaderFilter.h

@@ -17,6 +17,12 @@ class HeaderFilter {
 
   bool allows(bool isMainFile, const std::string &file) const;
 
+  // The raw pattern string this filter was constructed with -- used by
+  // AnalysisDriver's incremental-cache key, since changing the header
+  // filter changes which diagnostics a TU would report and so must
+  // invalidate any existing cache entry for it.
+  const std::string &pattern() const { return pattern_; }
+
 private:
   // Stored as a plain pattern string and compiled lazily per call (see
   // PathFilter): llvm::GlobPattern only holds a StringRef into its source

include/sentinel/core/IncrementalCache.h

@@ -0,0 +1,74 @@
+#pragma once
+
+#include "sentinel/core/Diagnostic.h"
+
+#include <optional>
+#include <string>
+#include <unordered_map>
+#include <vector>
+
+namespace sentinel {
+
+// One file this cache entry's result depends on: if its content no longer
+// hashes to `contentHash`, the entry is stale. Includes the TU's own main
+// file alongside every header it transitively included -- both matter
+// equally for whether the cached result is still valid.
+struct CacheDependency {
+  std::string file;
+  std::string contentHash;
+};
+
+// A cached translation unit's analysis result: everything needed to skip
+// re-parsing and re-analyzing it entirely on a later run.
+struct CacheEntry {
+  std::vector<CacheDependency> dependencies;
+  std::vector<Diagnostic> diagnostics;
+  // ruleId -> opaque state blob, from Rule::serializeState() -- only
+  // present for rules that had non-nullopt state when this entry was
+  // written (in practice, only lock-order-inversion today). The cache
+  // never interprets these; only the owning rule's restoreState() does.
+  std::unordered_map<std::string, std::string> ruleState;
+};
+
+// On-disk incremental-analysis cache, one JSON file per translation unit
+// under `cacheDir`, named by a hash of everything determinable *before*
+// parsing (main file path, compile command, enabled rule ids, header
+// filter, tool version -- see computeKey()). A cache hit additionally
+// requires every dependency file recorded in the entry to still hash to
+// the same content: knowing the *full* dependency set (every header
+// transitively included) isn't possible before parsing, so freshness is
+// checked lazily on lookup against whatever was recorded the last time
+// this TU was actually analyzed.
+class IncrementalCache {
+public:
+  explicit IncrementalCache(std::string cacheDir);
+
+  // Everything that could change this TU's analysis result and is known
+  // before parsing: which file, with which flags, under which enabled
+  // rules and header-filter pattern, analyzed by which cpp-sentinel
+  // build. Changing any of these must invalidate old entries, since a
+  // stale hit would silently serve results computed under a different
+  // configuration.
+  static std::string computeKey(const std::string &mainFile,
+                                const std::vector<std::string> &compileCommandArgs,
+                                const std::vector<std::string> &enabledRuleIds,
+                                const std::string &headerFilterPattern,
+                                const std::string &toolVersion);
+
+  // Returns the cached entry for `key` if one exists and every recorded
+  // dependency still hashes to the same content -- std::nullopt on a
+  // miss (no entry, a malformed entry, or any dependency changed or is
+  // no longer readable).
+  std::optional<CacheEntry> lookup(const std::string &key) const;
+
+  // Writes (or overwrites) the cache entry for `key`. Creates `cacheDir`
+  // if it doesn't already exist.
+  void store(const std::string &key, const CacheEntry &entry) const;
+
+private:
+  std::string pathFor(const std::string &key) const;
+
+  std::string cacheDir_;
+};
+
+} // namespace sentinel

include/sentinel/core/Rule.h

@@ -2,6 +2,7 @@
 
 #include <clang/ASTMatchers/ASTMatchFinder.h>
 
+#include <optional>
 #include <string>
 #include <vector>
 
@@ -70,6 +71,22 @@ class Rule : public clang::ast_matchers::MatchFinder::MatchCallback {
   // lock-order-inversion) need to override this; a rule whose diagnostics
   // are already fully reported per-TU has nothing to merge.
   virtual void mergeFrom(const Rule &other) { (void)other; }
+
+  // Serializes any per-TU accumulated state this instance holds that
+  // finalize() needs (e.g. lock-order-inversion's LockFactStore) into an
+  // opaque blob the incremental cache can persist to disk alongside a
+  // TU's cached diagnostics -- std::nullopt means "nothing to
+  // serialize" (the default; most rules report everything directly
+  // per-TU, so finalize() is a no-op for them and there's no state to
+  // save). The blob's format is entirely up to the rule; IncrementalCache
+  // never interprets it, only stores and hands it back.
+  virtual std::optional<std::string> serializeState() const { return std::nullopt; }
+
+  // The inverse of serializeState(): merges a blob previously produced
+  // by it (read back from a TU's cache entry on a later run, when that
+  // TU is a cache hit and so never gets re-parsed) into this instance,
+  // exactly as mergeFrom() merges a sibling instance's in-memory state.
+  virtual void restoreState(const std::string &blob) { (void)blob; }
 };
 
 } // namespace sentinel

include/sentinel/core/RuleFrontendAction.h

@@ -22,9 +22,16 @@ namespace sentinel {
 // way down through RuleAction/RuleConsumer to TranslationUnitContext: the
 // returned FrontendAction outlives this call, so a reference parameter
 // bound to a caller's temporary (e.g. the default argument) would dangle.
+//
+// `dependencyFilesOut`, if non-null, is populated with the path of every
+// real on-disk file the TU touched while parsing (the main file plus
+// every header transitively included) -- used by AnalysisDriver to build
+// an incremental-cache entry's dependency list once a TU is fully
+// analyzed. Left null by callers that don't care (every rule unit test).
 std::unique_ptr<clang::FrontendAction>
 createRuleFrontendAction(std::vector<std::unique_ptr<Rule>> &rules,
                          std::vector<Diagnostic> &diagnostics,
-                         HeaderFilter headerFilter = HeaderFilter());
+                         HeaderFilter headerFilter = HeaderFilter(),
+                         std::vector<std::string> *dependencyFilesOut = nullptr);
 
 } // namespace sentinel

include/sentinel/rules/LockOrderInversionRule.h

@@ -51,6 +51,15 @@ class LockOrderInversionRule : public Rule {
   // spanning TUs handled by different tasks would go undetected.
   void mergeFrom(const Rule &other) override;
 
+  // Serializes factStore_ to a JSON blob and back -- see Rule::
+  // serializeState()/restoreState(). Lets the incremental cache skip
+  // re-parsing a TU whose dependencies haven't changed while still
+  // handing this rule's accumulated facts for that TU back to a later
+  // run's finalize(), so a cross-file cycle spanning a cache-hit TU and
+  // a cache-miss TU is still detected correctly.
+  std::optional<std::string> serializeState() const override;
+  void restoreState(const std::string &blob) override;
+
 private:
   LockFactStore factStore_;
 };

lib/CMakeLists.txt

@@ -24,13 +24,15 @@ add_library(sentinel-lib OBJECT
   core/RuleFrontendAction.cpp
   core/AnalysisDriver.cpp
   core/Fingerprint.cpp
+  core/ContentHash.cpp
   core/SuppressionState.cpp
   core/TranslationUnitContext.cpp
   core/PathFilter.cpp
   core/LockFactStore.cpp
   core/ToolchainDefaults.cpp
   core/HeaderFilter.cpp
   core/LockIdentity.cpp
+  core/IncrementalCache.cpp
   config/Config.cpp
   output/TextFormatter.cpp
   output/JsonFormatter.cpp
@@ -63,4 +65,5 @@ execute_process(
 )
 target_compile_definitions(sentinel-lib PRIVATE
   SENTINEL_CLANG_RESOURCE_DIR="${SENTINEL_CLANG_RESOURCE_DIR}"
+  SENTINEL_VERSION="${PROJECT_VERSION}"
 )