Skip to content

feat: add CompileGraph for pull-based module dependency compilation#374

Closed
16bit-ykiko wants to merge 5 commits intomainfrom
feat/compile-graph
Closed

feat: add CompileGraph for pull-based module dependency compilation#374
16bit-ykiko wants to merge 5 commits intomainfrom
feat/compile-graph

Conversation

@16bit-ykiko
Copy link
Copy Markdown
Member

@16bit-ykiko 16bit-ykiko commented Mar 28, 2026

Summary

  • Add CompileGraph class that manages pull-based compilation scheduling for C++20 module dependencies (PCM/PCH)
  • Supports sequential dependency compilation with dedup (wait if already compiling), cancellation propagation via cancellation_source/token, and cascade invalidation via BFS traversal
  • Includes 11 unit tests covering: registration, no-dep/single-dep/chain/diamond compilation, update invalidation/cascade, recompilation after update, unregistered units, dispatch failure, and cancel_all

Test plan

  • All 11 CompileGraph.* tests pass (--test-filter "CompileGraph.")
  • No segfaults when running tests together
  • Code formatted with pixi run format

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Added dependency-aware compilation graph with concurrent builds, dirty-state propagation, cancellation, and cycle detection.
    • Module-aware compilation: automatic module scanning, dependency-driven PCM builds, PCM bookkeeping, and inclusion of transitive PCMs in compile requests.
    • Build responses now include produced PCM file paths.
  • Tests
    • Large new unit, integration, and end-to-end test suites covering module scenarios, compile-graph semantics, invalidation, cancellation, and PCM generation.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 28, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds a new CompileGraph (header + implementation), integrates module-aware PCM discovery and orchestration into MasterServer (resolve/dispatch closures and pcm tracking), extends BuildPCM protocol to include PCM paths, updates stateless worker responses, adds the compile_graph file to CMake, and introduces extensive unit/integration tests for module compilation.

Changes

Cohort / File(s) Summary
Build System
CMakeLists.txt
Added src/server/compile_graph.cpp to clice-core sources.
CompileGraph core
src/server/compile_graph.h, src/server/compile_graph.cpp
New clice::CompileGraph type: per-unit state, lazy dependency resolution via resolve_fn, async orchestration via dispatch_fn, cycle detection, cancellation, update/invalidation propagation, and helpers (compile, compile_impl, update, cancel_all, has_unit, is_dirty, is_compiling).
Master server wiring
src/server/master_server.h, src/server/master_server.cpp
MasterServer now holds std::unique_ptr<CompileGraph> compile_graph, pcm_paths, and path_to_module; workspace load builds module maps, initializes compile_graph with resolve/dispatch closures, orchestrates PCM builds before stateful compilation, and invalidates/erases PCM entries on save.
Protocol & stateless worker
src/server/protocol.h, src/server/stateless_worker.cpp
BuildPCMResult adds std::string pcm_path; stateless worker BuildPCM handler now returns the produced PCM path (or empty string) in responses.
Compile graph tests
tests/unit/server/compile_graph_tests.cpp, tests/unit/server/compile_graph_integration_tests.cpp
New unit and integration tests exercising compile ordering, deduplication, dirty/update semantics, cancellation, cycle detection, failure propagation, resolution re-computation, and PCM generation.
Module & integration tests
tests/integration/test_modules.py, tests/unit/server/module_worker_tests.cpp
Large new integration/e2e tests covering C++20 module scenarios, partitions, GMF, re-exports, worker-based PCM generation, and LSP interactions.
Test infra tweak
tests/conftest.py
Generate POSIX-style paths in compile_commands.json.

Sequence Diagram

sequenceDiagram
    participant Master as MasterServer
    participant CG as CompileGraph
    participant Resolver as resolve_fn (scan/DependencyGraph)
    participant Stateless as StatelessWorker
    participant PCMStore as pcm_paths

    Master->>CG: compile(module_path_id)
    CG->>CG: ensure_resolved(path_id)
    CG->>Resolver: resolve(path_id) (scan imports)
    Resolver-->>CG: dependency path_ids
    CG->>CG: compile_impl(deps...)
    alt dependency needed
        CG->>CG: compile_impl(dep, ancestors)
    end
    CG->>Master: dispatch(path_id) via dispatch_fn
    Master->>Stateless: BuildPCM(params with transitive pcm_paths)
    Stateless-->>Master: BuildPCMResult{success, error, pcm_path}
    Master->>PCMStore: pcm_paths[path_id] = pcm_path
    Master-->>CG: dispatch result (true/false)
    CG->>CG: mark clean / set dirty on failures
    Note right of CG: cancellation and completion events coordinate waiting
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Poem

🐰 I hopped through nodes and tangled threads,
I chased each import where the pathway led.
PCMs collected, cancellations played—
Dependencies built, and stale links swayed.
Hooray — modules compile, and carrots are paid! 🥕✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.91% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat: add CompileGraph for pull-based module dependency compilation' clearly summarizes the main change: introducing a new CompileGraph component for managing module dependency compilation.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/compile-graph

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
src/server/compile_graph.cpp (1)

40-69: Consider adding a visited set for efficiency in complex graphs.

The BFS traversal doesn't track visited nodes. In diamond dependency graphs, the same node could be added to the queue multiple times via different paths. While the if(unit.dirty && !unit.compiling) check on line 55-57 prevents redundant work, nodes still get enqueued and looked up multiple times.

For large dependency graphs, consider adding a DenseSet to track visited nodes:

♻️ Optional optimization with visited set
 void CompileGraph::update(std::uint32_t path_id) {
     llvm::SmallVector<std::uint32_t> queue;
+    llvm::DenseSet<std::uint32_t> visited;
     queue.push_back(path_id);
 
     while(!queue.empty()) {
         auto current = queue.pop_back_val();
+        if(!visited.insert(current).second) {
+            continue;  // Already processed.
+        }
 
         auto it = units.find(current);
         if(it == units.end()) {
             continue;
         }
 
         auto& unit = it->second;
-
-        // Skip if already dirty and not compiling (no work to do).
-        if(unit.dirty && !unit.compiling) {
-            continue;
-        }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/server/compile_graph.cpp` around lines 40 - 69, The update traversal in
CompileGraph::update currently re-enqueues nodes via multiple paths (queue,
dependents) causing redundant lookups; add a visited set (e.g.,
llvm::DenseSet<std::uint32_t> visited) and mark the starting path_id and any
dep_id before pushing them into queue so you only process each unit once; keep
the existing checks on unit.dirty and unit.compiling but use visited to skip
already-seen ids when iterating dependents and before queue.push_back to avoid
repeated work and lookups in units.
tests/unit/server/compile_graph_tests.cpp (1)

105-128: Consider adding a cycle detection test.

Given the dependency chain and diamond tests, it would be valuable to add a test verifying that cycles are handled gracefully (returning false rather than hanging/crashing):

🧪 Suggested test for cycle detection
TEST_CASE(CycleDetection) {
    std::vector<std::uint32_t> compiled;
    run_test(tracking_dispatch(compiled), [this, &compiled](CompileGraph& graph) -> et::task<> {
        // Create a cycle: 1 -> 2 -> 3 -> 1
        graph.register_unit(1, {2});
        graph.register_unit(2, {3});
        graph.register_unit(3, {1});

        auto result = co_await graph.compile(1);
        // Cycle should be detected, compilation should fail.
        EXPECT_FALSE(result);
    });
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/unit/server/compile_graph_tests.cpp` around lines 105 - 128, Add a new
unit test that verifies cycle detection by registering units that form a cycle
(e.g., using CompileGraph::register_unit to create 1->2, 2->3, 3->1) and then
calling CompileGraph::compile(1) via the existing run_test and tracking_dispatch
harness; assert that compile returns false (EXPECT_FALSE(result)) to ensure the
graph detects cycles instead of hanging or crashing. Use the same test structure
as DiamondDependency (std::vector<std::uint32_t> compiled,
tracking_dispatch(compiled), run_test, and the lambda signature CompileGraph&
graph -> et::task<>) so it integrates with the test suite. Ensure no extra
side-effects (no need to inspect compiled list) and keep the test name
descriptive (e.g., TEST_CASE(CycleDetection)).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/server/compile_graph.cpp`:
- Around line 91-99: The cycle-detection stack is never initialized because
CompileGraph::compile calls compile_impl(path_id) with a null stack, so
recursive calls never see a stack; change the public CompileGraph::compile entry
to create a local llvm::SmallVector<std::uint32_t> (or equivalent) and pass its
pointer into CompileGraph::compile_impl(path_id, &stack) so that the initial and
all recursive calls (which already forward the stack) share the same stack for
proper cycle detection; ensure compile_impl continues to push/pop entries on
that same stack.
- Around line 8-18: register_unit currently re-adds reverse edges into
units[dep_id].dependents on every call, causing duplicate dependents and growth;
before assigning new dependencies in CompileGraph::register_unit, remove any
existing reverse edges for this unit by iterating the unit.dependencies (the
previous deps) and removing path_id from each corresponding
units[old_dep].dependents (e.g., erase-remove or remove all occurrences), then
assign the new dependencies and add path_id to each new dep's dependents (or
guard push_back with a check) so update() won't see duplicate dependents and
re-registrations don't leak.

---

Nitpick comments:
In `@src/server/compile_graph.cpp`:
- Around line 40-69: The update traversal in CompileGraph::update currently
re-enqueues nodes via multiple paths (queue, dependents) causing redundant
lookups; add a visited set (e.g., llvm::DenseSet<std::uint32_t> visited) and
mark the starting path_id and any dep_id before pushing them into queue so you
only process each unit once; keep the existing checks on unit.dirty and
unit.compiling but use visited to skip already-seen ids when iterating
dependents and before queue.push_back to avoid repeated work and lookups in
units.

In `@tests/unit/server/compile_graph_tests.cpp`:
- Around line 105-128: Add a new unit test that verifies cycle detection by
registering units that form a cycle (e.g., using CompileGraph::register_unit to
create 1->2, 2->3, 3->1) and then calling CompileGraph::compile(1) via the
existing run_test and tracking_dispatch harness; assert that compile returns
false (EXPECT_FALSE(result)) to ensure the graph detects cycles instead of
hanging or crashing. Use the same test structure as DiamondDependency
(std::vector<std::uint32_t> compiled, tracking_dispatch(compiled), run_test, and
the lambda signature CompileGraph& graph -> et::task<>) so it integrates with
the test suite. Ensure no extra side-effects (no need to inspect compiled list)
and keep the test name descriptive (e.g., TEST_CASE(CycleDetection)).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ef9fb926-9154-4f0b-ae45-15739948e92e

📥 Commits

Reviewing files that changed from the base of the PR and between a536865 and 5528d97.

📒 Files selected for processing (4)
  • CMakeLists.txt
  • src/server/compile_graph.cpp
  • src/server/compile_graph.h
  • tests/unit/server/compile_graph_tests.cpp

…tests

- Fix DenseMap dangling reference in ensure_resolved() with local copy
- Fix cycle detection via by-value ancestors in compile_impl()
- Fix update() cascade with visited set, resolved reset, stale edge cleanup
- Fix self-PCM exclusion in run_build_drain() to prevent "multiple module
  declarations" error when recompiling module interfaces
- Add module implementation unit PCM auto-build in run_build_drain()
- Add 18 CompileGraph unit tests (cycle detection, diamond cascade, etc.)
- Add 24 CompileGraph integration tests with real clang compilation
- Add 3 module worker tests (PCM build, chain, impl unit)
- Add 24 Python integration tests covering full LSP server pipeline

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/server/compile_graph.cpp`:
- Around line 69-70: Pre-capture the cancellation token once into the local
variable token (auto token = it->second.source->token()) and use that captured
token for all subsequent dispatch calls instead of re-reading it via
it->second.source->token(); replace any direct calls to
it->second.source->token() at the dispatch sites with the pre-captured token to
ensure that if update() replaces the source and cancels the original,
cancellation is correctly observed by dispatch. Ensure every place that
currently fetches a fresh token for dispatch (instances that call
it->second.source->token()) uses the local token variable instead, preserving
the original cancellation semantics.

In `@src/server/master_server.cpp`:
- Around line 645-652: The handler currently calls
compile_graph->update(path_id) and only erases PCM paths from pcm_paths for each
dirty_id, but does not refresh in-memory importers so open documents keep stale
AST/diagnostics; after computing the dirtied set, iterate the dirtied IDs and
for any id that is present in documents schedule a rebuild/reparse for that unit
(i.e. enqueue or call the existing build/reparse function used elsewhere) so
open dependents are recompiled and their importers refreshed; keep the existing
pcm_paths erasure but add the scheduling step for dirty_ids found in documents.
- Around line 139-149: The PCM population logic that fills
worker::CompileParams::pcms for stateful compiles must also be applied to the
stateless path used by forward_stateless(); find where forward_stateless()
constructs a worker::CompileParams (or equivalent) and either extract the
for-loop that iterates pcm_paths/path_to_module into a small helper (e.g.,
populate_pcms(params, pcm_paths, path_to_module, path_id)) and call it from both
the stateful compile branch and forward_stateless(), or duplicate the same loop
in forward_stateless() so params.pcms is populated for stateless requests;
reference the existing loop that checks if(pid == path_id) and assigns
params.pcms[mod_it->second] = pcm_path.
- Around line 115-117: The code currently takes the first candidate from
dependency_graph.lookup_module(mod_name) which is multi-valued; instead, change
the resolution logic in the loop over scan_result.modules to pick the provider
that matches the active compile command/context (not lookup_module(...)[0]).
Locate the loop that calls dependency_graph.lookup_module(mod_name) and replace
the blind first-element choice with a selection that iterates the returned
path_id candidates and chooses the one whose target/context matches the current
compile context or active target (e.g., compare against the active compile
command, active_target, or compile_context structure used elsewhere in
master_server.cpp); if no match is found, fall back to a clear error or
deterministic tie-breaker rather than silently using the first entry.
- Line 118: The call to compile_graph->compile(...).catch_cancel() is being
awaited but its result is ignored; capture the result of co_await
compile_graph->compile(mod_ids[0]).catch_cancel() (and the identical call at the
other occurrence) and if it indicates failure or cancellation (e.g., returns
false/empty), stop the drain/return early instead of proceeding to send the
stateful compile with a partial PCM set; update the logic around compile_graph,
compile, catch_cancel and the mod_ids handling to check the result and abort the
current iteration when the dependency build failed or was cancelled.

In `@tests/integration/test_modules.py`:
- Around line 584-627: Update test_save_recompile to exercise the new didSave
logic: after writing new_content to leaf.cppm on disk, call
client.text_document_did_save(DidSaveTextDocumentParams(text_document=TextDocumentIdentifier(uri=leaf_uri)))
instead of closing/reopening the file, then wait for diagnostics for the
importer (use client.wait_for_diagnostics(mid_uri) or leaf_uri as appropriate)
and assert diagnostics are updated/rebuilt with no errors; reference the
test_save_recompile function and the MasterServer didSave invalidation path when
making this change.

In `@tests/unit/server/module_worker_tests.cpp`:
- Around line 86-88: The test currently only checks that send_request returned a
CompileResult with version==1; update the assertion to also validate
result.value().diagnostics contains no errors by parsing the diagnostics field
(e.g., access result.value().diagnostics and assert either diagnostics.empty()
or that no diagnostic entry has severity==Error / is_error() depending on your
Diagnostic type) so the compile was clean; apply the same change to the other
occurrences around the tests that check send_request (the blocks using result,
CO_ASSERT_TRUE and EXPECT_EQ) to ensure they assert no error-level diagnostics
in addition to version==1.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 27e9ea27-1075-42c3-b83c-d6d6eda6216b

📥 Commits

Reviewing files that changed from the base of the PR and between 5528d97 and cc45336.

📒 Files selected for processing (10)
  • src/server/compile_graph.cpp
  • src/server/compile_graph.h
  • src/server/master_server.cpp
  • src/server/master_server.h
  • src/server/protocol.h
  • src/server/stateless_worker.cpp
  • tests/integration/test_modules.py
  • tests/unit/server/compile_graph_integration_tests.cpp
  • tests/unit/server/compile_graph_tests.cpp
  • tests/unit/server/module_worker_tests.cpp

On Windows, str(Path(...)) produces backslash paths but the server's
URI-to-path conversion produces forward-slash paths, causing CDB
lookup failures. Use .as_posix() for all CDB path entries.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
tests/conftest.py (1)

119-127: Make CDB generation idempotent to avoid stale path formats.

On Line 119, generation is skipped if compile_commands.json already exists. That can leave old backslash-formatted entries in place on Windows after this change. Consider rewriting when content differs.

♻️ Proposed adjustment
-    if main_cpp.exists() and not cdb_path.exists():
-        cdb = [
-            {
-                "directory": hw_dir.as_posix(),
-                "file": main_cpp.as_posix(),
-                "arguments": ["clang++", "-std=c++17", "-fsyntax-only", main_cpp.as_posix()],
-            }
-        ]
-        cdb_path.write_text(json.dumps(cdb, indent=2))
+    if main_cpp.exists():
+        cdb = [
+            {
+                "directory": hw_dir.as_posix(),
+                "file": main_cpp.as_posix(),
+                "arguments": ["clang++", "-std=c++17", "-fsyntax-only", main_cpp.as_posix()],
+            }
+        ]
+        new_content = json.dumps(cdb, indent=2)
+        if not cdb_path.exists() or cdb_path.read_text() != new_content:
+            cdb_path.write_text(new_content)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/conftest.py` around lines 119 - 127, The cdb generation should be
idempotent: construct the desired compile_commands.json content (build the cdb
list using hw_dir.as_posix() and main_cpp.as_posix(), dump to JSON with a stable
format e.g., indent=2 and sort_keys=True) and then compare the serialized text
against cdb_path.read_text(); only write the file if it doesn’t exist or the
content differs, so stale backslash-formatted entries are replaced while
avoiding unnecessary writes; update the logic around main_cpp, cdb_path and the
cdb variable to perform this compare-and-write.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tests/conftest.py`:
- Around line 119-127: The cdb generation should be idempotent: construct the
desired compile_commands.json content (build the cdb list using
hw_dir.as_posix() and main_cpp.as_posix(), dump to JSON with a stable format
e.g., indent=2 and sort_keys=True) and then compare the serialized text against
cdb_path.read_text(); only write the file if it doesn’t exist or the content
differs, so stale backslash-formatted entries are replaced while avoiding
unnecessary writes; update the logic around main_cpp, cdb_path and the cdb
variable to perform this compare-and-write.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 2af06d82-3fc5-4050-a109-2a4a214e1430

📥 Commits

Reviewing files that changed from the base of the PR and between cc45336 and b0fae33.

📒 Files selected for processing (2)
  • tests/conftest.py
  • tests/integration/test_modules.py
✅ Files skipped from review due to trivial changes (1)
  • tests/integration/test_modules.py

16bit-ykiko and others added 2 commits March 29, 2026 03:44
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Use pre-captured cancellation token for dispatch (compile_graph.cpp)
  to preserve cancellation correctness when update() replaces the source
  during dependency compilation.
- Check compile_graph->compile() results and skip stateful compile when
  module dependency builds fail or are cancelled.
- Schedule rebuilds for open importers when a module file is saved,
  so cascade invalidation actually refreshes dependent documents.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/server/master_server.cpp`:
- Around line 308-342: The dispatch lambda mutates shared pcm_paths concurrently
causing a data race; change dispatch (and its return type) so it does not write
pcm_paths but instead returns the built PCM path (or optional) to the caller,
then in compile_impl (the et::when_all caller) collect each dispatch result
after et::when_all completes and update the shared pcm_paths from that
single-threaded continuation (or under a mutex) — i.e., modify dispatch to only
read pcm_paths and return result.value().pcm_path on success, run dispatch in
parallel as before, then after when_all iterate results and assign
pcm_paths[path_id] = pcm_path (or skip on failure).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: bb7d7f8a-a794-4c2c-84eb-3f7b8d89c944

📥 Commits

Reviewing files that changed from the base of the PR and between ee35ccb and 5d2d272.

📒 Files selected for processing (2)
  • src/server/compile_graph.cpp
  • src/server/master_server.cpp

Comment on lines +308 to +342
auto dispatch = [this](std::uint32_t path_id) -> et::task<bool> {
auto mod_it = path_to_module.find(path_id);
if(mod_it == path_to_module.end()) {
co_return false;
}

auto file_path = std::string(path_pool.resolve(path_id));
worker::BuildPCMParams pcm_params;
pcm_params.file = file_path;
fill_compile_args(file_path, pcm_params.directory, pcm_params.arguments);
pcm_params.module_name = mod_it->second;

// Clang needs ALL transitive PCM deps, not just direct imports.
for(auto& [pid, pcm_path]: pcm_paths) {
auto dep_mod_it = path_to_module.find(pid);
if(dep_mod_it != path_to_module.end()) {
pcm_params.pcms[dep_mod_it->second] = pcm_path;
}
}

auto result = co_await pool.send_stateless(pcm_params);
if(!result.has_value() || !result.value().success) {
LOG_WARN("BuildPCM failed for module {}: {}",
mod_it->second,
result.has_value() ? result.value().error : result.error().message);
co_return false;
}

pcm_paths[path_id] = result.value().pcm_path;
LOG_INFO("Built PCM for module {}: {}", mod_it->second, result.value().pcm_path);
co_return true;
};

compile_graph = std::make_unique<CompileGraph>(std::move(dispatch), std::move(resolve));
LOG_INFO("CompileGraph initialized with {} module(s)", path_to_module.size());
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if et::when_all allows concurrent task execution
rg -n -C5 'et::when_all' src/server/compile_graph.cpp

# Check if pcm_paths has any synchronization
rg -n 'pcm_paths' src/server/master_server.h src/server/master_server.cpp

Repository: clice-io/clice

Length of output: 895


🏁 Script executed:

# Find where dispatch is used - check CompileGraph initialization and usage
rg -n 'dispatch' src/server/master_server.cpp | head -20

# Check CompileGraph class definition to see how dispatch is invoked
fd -e h -e cpp src/server/compile_graph | xargs rg -l 'class CompileGraph|struct CompileGraph'

# Look for how compile_impl calls into pcm compilation
rg -n -A10 'compile_impl.*path_id' src/server/compile_graph.cpp | head -40

Repository: clice-io/clice

Length of output: 794


🏁 Script executed:

# Check if pcm_paths has any mutex protection
rg -n -B5 -A5 'pcm_paths' src/server/master_server.h | head -40

# Check for synchronization primitives (mutex, lock, etc.) near pcm_paths
rg -n 'std::mutex|std::lock|std::atomic|mutable' src/server/master_server.h

Repository: clice-io/clice

Length of output: 526


🏁 Script executed:

# Verify llvm::DenseMap thread safety - check how it's typically used
rg -n 'DenseMap' src/ | head -10

Repository: clice-io/clice

Length of output: 1140


🏁 Script executed:

# Find where dispatch/resolve callbacks are invoked in CompileGraph
fd -e h -e cpp src/ -path '*/server/*' | xargs rg -l 'CompileGraph'

# Check the CompileGraph implementation for callback invocations
rg -n -A20 'class CompileGraph' src/server/compile_graph.h

# Search for where the dispatch callback is actually called
rg -n 'dispatch\(' src/server/compile_graph.cpp

Repository: clice-io/clice

Length of output: 1569


🏁 Script executed:

# Look at full context of compile_impl to understand the flow
sed -n '39,120p' src/server/compile_graph.cpp

Repository: clice-io/clice

Length of output: 2687


Data race on pcm_paths: concurrent writes and reads without synchronization.

The dispatch lambda reads pcm_paths (lines 321–326) and writes to it (line 336). Since compile_impl uses et::when_all to execute dependencies in parallel (line 79 in compile_graph.cpp), and each parallel task recursively calls compile_impl which eventually invokes dispatch (line 101 in compile_graph.cpp), multiple dispatch calls execute concurrently. llvm::DenseMap is not thread-safe—concurrent reads and writes cause undefined behavior and potential corruption.

Consider one of:

  1. Serialize dispatch calls by removing parallel compilation in compile_impl (sequential for-loop instead of et::when_all)
  2. Protect pcm_paths with a mutex or use a concurrent map
  3. Collect PCM paths after parallel compilation by returning the PCM path from dispatch and updating pcm_paths in the caller after et::when_all completes

Option 3 preserves parallelism while eliminating the race.

🧰 Tools
🪛 Cppcheck (2.20.0)

[error] 342-342: Found an exit path from function with non-void return type that has missing return statement

(missingReturn)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/server/master_server.cpp` around lines 308 - 342, The dispatch lambda
mutates shared pcm_paths concurrently causing a data race; change dispatch (and
its return type) so it does not write pcm_paths but instead returns the built
PCM path (or optional) to the caller, then in compile_impl (the et::when_all
caller) collect each dispatch result after et::when_all completes and update the
shared pcm_paths from that single-threaded continuation (or under a mutex) —
i.e., modify dispatch to only read pcm_paths and return result.value().pcm_path
on success, run dispatch in parallel as before, then after when_all iterate
results and assign pcm_paths[path_id] = pcm_path (or skip on failure).

@16bit-ykiko
Copy link
Copy Markdown
Member Author

Split into two PRs for easier review:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant