Skip to content

Commit 792a9d6

Browse files
committed
Add parallel translation-unit analysis via --jobs
1 parent c667153 commit 792a9d6

10 files changed

Lines changed: 326 additions & 45 deletions

File tree

README.md

Lines changed: 29 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,12 @@ defects with precise source locations.
77

88
This is the MVP scoped in [`docs/spec.md`](docs/spec.md) -- three rules,
99
text/JSON/SARIF output, YAML config with suppression, and one flagship
10-
cross-file rule with real graph-algorithm teeth -- plus four items from
11-
the Phase 2+ backlog: SARIF output and three more rules (`use-after-move`,
12-
`unbalanced-lock`, `unchecked-null-result`, `expensive-copy`), two of them
13-
sharing a generic worklist dataflow engine. See
14-
[`docs/architecture.md`](docs/architecture.md) for how it's built
15-
internally.
10+
cross-file rule with real graph-algorithm teeth -- plus five items from
11+
the Phase 2+ backlog: SARIF output, three more rules (`use-after-move`,
12+
`unbalanced-lock`, `unchecked-null-result`, `expensive-copy`, two of them
13+
sharing a generic worklist dataflow engine), and parallel translation-unit
14+
analysis (`--jobs`). See [`docs/architecture.md`](docs/architecture.md)
15+
for how it's built internally.
1616

1717
## Rules
1818

@@ -93,7 +93,9 @@ Key `analyze` options: `--format text|json|sarif`, `--output <file>`,
9393
`--include`/`--exclude <glob>` (repeatable, controls which translation
9494
units are analyzed), `--header-filter <glob>` (also report diagnostics
9595
from headers matching this pattern; by default only each TU's main file
96-
is reported, matching clang-tidy's convention), `--fail-on warning|error`.
96+
is reported, matching clang-tidy's convention), `--fail-on warning|error`,
97+
`-j`/`--jobs <N>` (translation units analyzed in parallel; default `0` =
98+
all available hardware threads).
9799

98100
Exit codes: `0` clean, `1` findings at/above `--fail-on`, `2` usage/config
99101
error, `4` a translation unit failed to compile.
@@ -151,27 +153,33 @@ One real run against [spdlog](https://github.com/gabime/spdlog) at commit
151153
(v1.17.0) -- a real-world, mutex-heavy C++ logging library, ~27.5k LOC
152154
across `include/` + `src/`, 7 translation units in its compiled-library
153155
configuration (`-DSPDLOG_BUILD_EXAMPLE=OFF -DSPDLOG_BUILD_TESTS=OFF`).
154-
Measured on an Apple M-series machine, Release build, single-threaded
155-
(no parallel scheduler in the MVP), average of 3 runs:
156+
Measured on an 8-core Apple M3, Release build, average of 3 runs each,
157+
`--jobs 1` vs. the default (`--jobs 0`, all available hardware threads):
156158

157-
| Metric | Value |
158-
|---|---|
159-
| Wall clock | 1.76 s |
160-
| Throughput | ~15,650 LOC/s |
161-
| Peak RSS | ~189 MiB (198 MB) |
162-
163-
It also found a real, pre-existing `redundant-move-return` case in
164-
spdlog's own `pattern_formatter-inl.h`.
159+
| Metric | `--jobs 1` | `--jobs 0` (default) |
160+
|---|---|---|
161+
| Wall clock | 2.31 s | 0.68 s |
162+
| Throughput | ~11,950 LOC/s | ~40,300 LOC/s |
163+
| Peak RSS | ~199 MiB (209 MB) | ~680 MiB (712 MB) |
164+
165+
~3.4x wall-clock speedup on 7 translation units and 8 cores -- limited by
166+
there being only 7 TUs to spread across the pool, not 8, plus thread-pool
167+
startup overhead. Peak RSS scales up correspondingly, since parallel runs
168+
keep multiple TUs' ASTs alive in memory at once instead of one at a time.
169+
Both modes report byte-for-byte identical findings (verified via `diff`),
170+
including the same real, pre-existing `redundant-move-return` case in
171+
spdlog's own `pattern_formatter-inl.h` found back in the M4 benchmark.
165172

166173
This is one honest number, not a benchmark suite -- see
167174
[`docs/spec.md`](docs/spec.md) §10/§14 for what's deliberately out of
168-
scope for the MVP (parallel/incremental analysis, a benchmark corpus,
169-
scaling studies).
175+
scope for the MVP (a benchmark corpus, scaling studies beyond this one
176+
data point).
170177

171178
## Known limitations
172179

173-
- Single-threaded, no incremental caching (analyzes every TU from scratch
174-
every run).
180+
- No incremental caching: every TU is re-analyzed from scratch on every
181+
run, even with parallel TU analysis (`--jobs`) doing the work
182+
concurrently.
175183
- `lock-order-inversion`: no alias analysis (two pointers to the same
176184
mutex are different lock ids unless they resolve to the same
177185
`VarDecl`/`FieldDecl`), loop bodies walked once rather than to a

docs/architecture.md

Lines changed: 46 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,14 @@ compile_commands.json
77
|
88
CompilationDatabaseLoader (JSONCompilationDatabase / loadFromDirectory)
99
|
10-
AnalysisDriver + clang::tooling::ClangTool
11-
| (sequential, one TU at a time -- no parallel scheduler in the MVP)
10+
AnalysisDriver + llvm::DefaultThreadPool
11+
| one task per TU, each with its own fresh rule instances and
12+
| its own ClangTool -- see "Parallel translation-unit analysis"
1213
|
1314
+----+-----------------------------+
1415
| |
1516
RuleFrontendAction -> ASTConsumer |
16-
| per TU: |
17+
| per TU (inside one task): |
1718
| - MatchFinder dispatches |
1819
| AST-matcher rules |
1920
| (missing-override, |
@@ -71,11 +72,48 @@ the run has gone through `registerMatchers()`/`runOnTranslationUnit()`.
7172
Project-level rules need this because their real analysis (the lock-order
7273
graph, in this case) only makes sense once the full cross-TU picture is
7374
available -- there's no single `TranslationUnitContext` that spans multiple
74-
TUs. Since a `LockOrderInversionRule` instance persists across every TU
75-
`AnalysisDriver` processes in one run (rules are constructed once up front,
76-
not per-TU), simply accumulating facts into a member of that rule already
77-
gives a project-wide store, without needing a JSON fact file round-trip
78-
in this single-threaded MVP.
75+
TUs. `finalize()` runs on `AnalysisDriver`'s own "template" rule instances
76+
(the ones passed to its constructor), which accumulate facts from every
77+
parallel task via `Rule::mergeFrom()` -- see the next section for why that
78+
indirection exists.
79+
80+
## Parallel translation-unit analysis
81+
82+
`AnalysisDriver::run()` (`lib/core/AnalysisDriver.cpp`) hands each
83+
translation unit to its own task on an `llvm::DefaultThreadPool`
84+
(`llvm::hardware_concurrency(jobCount)`; `jobCount == 0`, the CLI
85+
default, means "all available hardware threads"). Each task:
86+
87+
1. Builds its own fresh set of rule instances via `RuleRegistry` (the
88+
same rule ids `AnalysisDriver` was constructed with), not the driver's
89+
own instances.
90+
2. Runs its own `clang::tooling::ClangTool` against just that one file,
91+
with its own local `std::vector<Diagnostic>` sink.
92+
93+
Fresh instances per task, not shared ones, because an AST-matcher rule
94+
stashes its current TU's `TranslationUnitContext` in a member between
95+
`registerMatchers()` and `run()` (see `Rule.h`) -- sharing one instance
96+
across two threads processing two different TUs at once would race on
97+
that member. `clang::tooling::CompilationDatabase` is the one object
98+
every task reads concurrently; that's safe since compile-command lookup
99+
is read-only (the same pattern clangd relies on for concurrent parses).
100+
101+
Once a task finishes, `AnalysisDriver` merges its diagnostics into the
102+
final vector and calls `Rule::mergeFrom()` on its own template instance
103+
for every rule, passing that task's instance as the sibling to merge
104+
from -- default a no-op, so most rules need nothing here. Only
105+
`lock-order-inversion` overrides it (merging the task's `LockFactStore`
106+
into its own), because it's the only rule whose `finalize()` needs the
107+
*full* cross-TU picture: without the merge, a cross-file cycle spanning
108+
TUs handled by two different tasks would never be reassembled, since
109+
`finalize()` only runs on the template instance, and it would never have
110+
been given the other task's facts (`LockOrderInversionProjectTest.
111+
CrossFileCycleIsDetectedWithMultipleJobs` forces `jobCount=4` over
112+
exactly two files specifically to prove this). Finally, the merged
113+
diagnostics are sorted by `(file, line, column, rule id)` before
114+
returning -- output would otherwise depend on however the thread pool
115+
happened to interleave tasks, which is a real, if easy to miss,
116+
determinism regression once a driver goes from sequential to parallel.
79117

80118
## The flagship rule: `lock-order-inversion`
81119

include/sentinel/core/AnalysisDriver.h

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -12,21 +12,34 @@
1212

1313
namespace sentinel {
1414

15-
// Runs every registered rule over every requested translation unit,
16-
// sequentially (no parallel scheduler in the MVP), and collects the
17-
// resulting diagnostics.
15+
// Runs every registered rule over every requested translation unit and
16+
// collects the resulting diagnostics. Translation units are processed in
17+
// parallel across a thread pool: each parallel task gets its own fresh
18+
// set of rule instances (so per-TU mutable state, e.g. an AST-matcher
19+
// rule's `context_` member, never races across threads), and those
20+
// per-task instances are merged back into this driver's own "template"
21+
// instances (via Rule::mergeFrom()) as each task finishes, before
22+
// finalize() runs on them -- so project-level rules like
23+
// lock-order-inversion still see the full cross-TU picture regardless of
24+
// which task processed which file.
1825
class AnalysisDriver {
1926
public:
2027
explicit AnalysisDriver(std::vector<std::unique_ptr<Rule>> rules);
2128

2229
// If `hadCompileErrors` is non-null, it's set to true when at least one
23-
// translation unit failed to compile (ClangTool::run() returned nonzero).
24-
// `headerFilter` controls whether a diagnostic outside a TU's main file
25-
// is still reported (default: never -- see HeaderFilter).
30+
// translation unit failed to compile. `headerFilter` controls whether a
31+
// diagnostic outside a TU's main file is still reported (default:
32+
// never -- see HeaderFilter). `jobCount` is the number of worker
33+
// threads to use; 0 (the default) means "use all available hardware
34+
// threads", matching llvm::hardware_concurrency()'s own convention.
35+
// The returned diagnostics are sorted by (file, line, column, rule id)
36+
// so output is deterministic regardless of how threads happened to be
37+
// scheduled.
2638
std::vector<Diagnostic> run(clang::tooling::CompilationDatabase &compileDb,
2739
const std::vector<std::string> &sourcePaths,
2840
bool *hadCompileErrors = nullptr,
29-
HeaderFilter headerFilter = HeaderFilter());
41+
const HeaderFilter &headerFilter = HeaderFilter(),
42+
unsigned jobCount = 0);
3043

3144
private:
3245
std::vector<std::unique_ptr<Rule>> rules_;

include/sentinel/core/Rule.h

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,21 @@ class Rule : public clang::ast_matchers::MatchFinder::MatchCallback {
5555
// `diagnostics`. Default no-op so per-TU rules don't need to implement
5656
// it.
5757
virtual void finalize(std::vector<Diagnostic> &diagnostics) { (void)diagnostics; }
58+
59+
// AnalysisDriver processes translation units in parallel: each parallel
60+
// task gets its own fresh instance of every rule (via RuleRegistry), so
61+
// a rule's per-TU mutable state (an AST-matcher rule's `context_`
62+
// member, lock-order-inversion's accumulated LockFactStore) never races
63+
// across threads. Once every task finishes, its rule instances are
64+
// merged back into the driver's own "template" instances via this
65+
// method -- one instance per rule id, called once per finished task --
66+
// before finalize() runs on them exactly as it would have in a
67+
// single-threaded run. `other` is always a sibling instance of the same
68+
// concrete rule type (same id()). Default no-op: only rules that
69+
// accumulate state across TUs for finalize() (currently only
70+
// lock-order-inversion) need to override this; a rule whose diagnostics
71+
// are already fully reported per-TU has nothing to merge.
72+
virtual void mergeFrom(const Rule &other) { (void)other; }
5873
};
5974

6075
} // namespace sentinel

include/sentinel/rules/LockOrderInversionRule.h

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,14 @@ class LockOrderInversionRule : public Rule {
4343
void runOnTranslationUnit(TranslationUnitContext &context) override;
4444
void finalize(std::vector<Diagnostic> &diagnostics) override;
4545

46+
// Merges `other`'s accumulated LockFactStore into this instance's --
47+
// see Rule::mergeFrom(). Needed because AnalysisDriver gives each
48+
// parallel TU-processing task its own fresh LockOrderInversionRule
49+
// instance; without this, facts from TUs processed by other tasks
50+
// would never reach this instance's finalize(), and a cross-file cycle
51+
// spanning TUs handled by different tasks would go undetected.
52+
void mergeFrom(const Rule &other) override;
53+
4654
private:
4755
LockFactStore factStore_;
4856
};

lib/core/AnalysisDriver.cpp

Lines changed: 89 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,15 @@
11
#include "sentinel/core/AnalysisDriver.h"
22
#include "sentinel/core/RuleFrontendAction.h"
3+
#include "sentinel/core/RuleRegistry.h"
34
#include "sentinel/core/ToolchainDefaults.h"
45

56
#include <clang/Tooling/Tooling.h>
67

8+
#include <llvm/Support/ThreadPool.h>
9+
#include <llvm/Support/Threading.h>
10+
11+
#include <algorithm>
12+
#include <memory>
713
#include <utility>
814

915
#ifndef SENTINEL_CLANG_RESOURCE_DIR
@@ -31,6 +37,49 @@ class RuleActionFactory : public clang::tooling::FrontendActionFactory {
3137
HeaderFilter headerFilter_;
3238
};
3339

40+
// Result of processing exactly one translation unit in its own parallel
41+
// task: the rule instances that did the processing (merged back into the
42+
// driver's own "template" instances once every task finishes -- see
43+
// Rule::mergeFrom()), the diagnostics they produced, and whether the TU
44+
// failed to compile.
45+
struct TaskResult {
46+
std::vector<std::unique_ptr<Rule>> rules;
47+
std::vector<Diagnostic> diagnostics;
48+
bool hadCompileError = false;
49+
};
50+
51+
// Returns a shared_ptr rather than TaskResult by value: llvm::ThreadPool's
52+
// async() returns a std::shared_future, whose get() returns a `const T&`
53+
// (it can be called more than once) -- copy-constructing a TaskResult
54+
// from that would require copying the unique_ptr<Rule>s inside it, which
55+
// doesn't compile. A shared_ptr<TaskResult> is itself cheap to copy, so
56+
// shared_future's contract is satisfied without needing TaskResult (or
57+
// Rule ownership) to change shape at all.
58+
std::shared_ptr<TaskResult> analyzeOneTranslationUnit(
59+
clang::tooling::CompilationDatabase &compileDb, const std::string &sourcePath,
60+
const std::vector<std::string> &ruleIds, const HeaderFilter &headerFilter) {
61+
auto result = std::make_shared<TaskResult>();
62+
for (const auto &id : ruleIds)
63+
result->rules.push_back(RuleRegistry::instance().create(id));
64+
65+
clang::tooling::ClangTool tool(compileDb, {sourcePath});
66+
tool.appendArgumentsAdjuster(
67+
makeDefaultToolchainAdjuster(SENTINEL_CLANG_RESOURCE_DIR, detectMacSysroot()));
68+
RuleActionFactory factory(result->rules, result->diagnostics, headerFilter);
69+
result->hadCompileError = tool.run(&factory) != 0;
70+
return result;
71+
}
72+
73+
bool diagnosticLess(const Diagnostic &a, const Diagnostic &b) {
74+
if (a.location.file != b.location.file)
75+
return a.location.file < b.location.file;
76+
if (a.location.line != b.location.line)
77+
return a.location.line < b.location.line;
78+
if (a.location.column != b.location.column)
79+
return a.location.column < b.location.column;
80+
return a.ruleId < b.ruleId;
81+
}
82+
3483
} // namespace
3584

3685
AnalysisDriver::AnalysisDriver(std::vector<std::unique_ptr<Rule>> rules)
@@ -39,19 +88,52 @@ AnalysisDriver::AnalysisDriver(std::vector<std::unique_ptr<Rule>> rules)
3988
std::vector<Diagnostic>
4089
AnalysisDriver::run(clang::tooling::CompilationDatabase &compileDb,
4190
const std::vector<std::string> &sourcePaths, bool *hadCompileErrors,
42-
HeaderFilter headerFilter) {
91+
const HeaderFilter &headerFilter, unsigned jobCount) {
92+
std::vector<std::string> ruleIds;
93+
for (const auto &rule : rules_)
94+
ruleIds.push_back(rule->id());
95+
96+
// Each task gets its own fresh rule instances (see analyzeOneTranslationUnit)
97+
// rather than sharing rules_ directly: an AST-matcher rule stashes its
98+
// current TU's TranslationUnitContext in a member between
99+
// registerMatchers() and run() (see Rule.h), which would race if two
100+
// threads dispatched the same shared instance for two different TUs at
101+
// once.
102+
llvm::DefaultThreadPool pool(llvm::hardware_concurrency(jobCount));
103+
std::vector<std::shared_future<std::shared_ptr<TaskResult>>> futures;
104+
futures.reserve(sourcePaths.size());
105+
for (const auto &sourcePath : sourcePaths) {
106+
futures.push_back(pool.async([&compileDb, sourcePath, &ruleIds, &headerFilter] {
107+
return analyzeOneTranslationUnit(compileDb, sourcePath, ruleIds, headerFilter);
108+
}));
109+
}
110+
43111
std::vector<Diagnostic> diagnostics;
44-
clang::tooling::ClangTool tool(compileDb, sourcePaths);
45-
tool.appendArgumentsAdjuster(
46-
makeDefaultToolchainAdjuster(SENTINEL_CLANG_RESOURCE_DIR, detectMacSysroot()));
47-
RuleActionFactory factory(rules_, diagnostics, std::move(headerFilter));
48-
int result = tool.run(&factory);
112+
bool anyCompileError = false;
113+
for (auto &future : futures) {
114+
std::shared_ptr<TaskResult> result = future.get();
115+
anyCompileError = anyCompileError || result->hadCompileError;
116+
diagnostics.insert(diagnostics.end(),
117+
std::make_move_iterator(result->diagnostics.begin()),
118+
std::make_move_iterator(result->diagnostics.end()));
119+
// result->rules[i] and rules_[i] are the same rule id, in the same
120+
// order, since analyzeOneTranslationUnit built its rules from ruleIds
121+
// (itself built from rules_) in order.
122+
for (size_t i = 0; i < rules_.size(); ++i)
123+
rules_[i]->mergeFrom(*result->rules[i]);
124+
}
125+
49126
if (hadCompileErrors)
50-
*hadCompileErrors = result != 0;
127+
*hadCompileErrors = anyCompileError;
51128

52129
for (auto &rule : rules_)
53130
rule->finalize(diagnostics);
54131

132+
// Deterministic regardless of how the thread pool happened to schedule
133+
// and interleave tasks -- otherwise the same project could report its
134+
// findings in a different order from one run to the next.
135+
std::sort(diagnostics.begin(), diagnostics.end(), diagnosticLess);
136+
55137
return diagnostics;
56138
}
57139

lib/rules/LockOrderInversionRule.cpp

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -230,6 +230,12 @@ void LockOrderInversionRule::runOnTranslationUnit(TranslationUnitContext &contex
230230
walker.walkFunction(function);
231231
}
232232

233+
void LockOrderInversionRule::mergeFrom(const Rule &other) {
234+
const auto &sibling = static_cast<const LockOrderInversionRule &>(other);
235+
for (const auto &fact : sibling.factStore_.facts())
236+
factStore_.addFact(fact);
237+
}
238+
233239
void LockOrderInversionRule::finalize(std::vector<Diagnostic> &diagnostics) {
234240
graph::DirectedGraph graph;
235241
std::unordered_map<std::string, LockOrderFact> representative;

0 commit comments

Comments
 (0)