From c539a44bbb151dce032d5d18d5b68a073d022893 Mon Sep 17 00:00:00 2001 From: ykiko Date: Tue, 31 Mar 2026 22:23:16 +0800 Subject: [PATCH 1/5] feat: integrate PCH into MasterServer build drain and stateless forwarding Add `ensure_pch()` to MasterServer that builds/reuses precompiled headers via stateless workers. PCH staleness is detected by hashing the preamble content (xxh3_64bits) and comparing with the cached hash. The PCH is used in both the build drain loop (stateful compile) and stateless forwarding (completion/signatureHelp). Key changes: - Add `preamble_bound` field to `BuildPCHParams` so the stateless worker truncates remapped content at the preamble boundary - Fix `BuildPCHResult` to return the PCH file path - Add PCH state maps (pch_paths, pch_bounds, pch_hashes) to MasterServer - Clean up PCH state on didClose; invalidate hash on didSave - Add unit tests (PreambleHash, PCHWorker) and integration tests (test_pch) Co-Authored-By: Claude Opus 4.6 --- src/server/master_server.cpp | 76 +++++++++ src/server/master_server.h | 15 ++ src/server/protocol.h | 2 + src/server/stateless_worker.cpp | 10 +- tests/data/pch_test/common.h | 8 + tests/data/pch_test/main.cpp | 11 ++ tests/data/pch_test/no_includes.cpp | 7 + tests/integration/test_pch.py | 104 +++++++++++++ tests/unit/compile/compilation_tests.cpp | 59 +++++++ tests/unit/server/pch_worker_tests.cpp | 153 +++++++++++++++++++ tests/unit/server/stateless_worker_tests.cpp | 2 + 11 files changed, 442 insertions(+), 5 deletions(-) create mode 100644 tests/data/pch_test/common.h create mode 100644 tests/data/pch_test/main.cpp create mode 100644 tests/data/pch_test/no_includes.cpp create mode 100644 tests/integration/test_pch.py create mode 100644 tests/unit/server/pch_worker_tests.cpp diff --git a/src/server/master_server.cpp b/src/server/master_server.cpp index e497908e9..fea743519 100644 --- a/src/server/master_server.cpp +++ b/src/server/master_server.cpp @@ -18,6 +18,8 @@ #include "syntax/dependency_graph.h" #include "syntax/scan.h" +#include "llvm/Support/xxhash.h" + namespace clice { namespace protocol = eventide::ipc::protocol; @@ -181,6 +183,15 @@ et::task<> MasterServer::run_build_drain(std::uint32_t path_id, std::string uri) } } + // Build or reuse PCH for preamble acceleration. + co_await ensure_pch(path_id, params.path, params.text, + params.directory, params.arguments); + + // Populate PCH info if available. + if(auto pch_it = pch_paths.find(path_id); pch_it != pch_paths.end()) { + params.pch = {pch_it->second, pch_bounds[path_id]}; + } + LOG_DEBUG("Sending compile: path={}, args={}, gen={}", params.path, params.arguments.size(), @@ -379,6 +390,51 @@ bool MasterServer::fill_compile_args(llvm::StringRef path, return true; } +et::task MasterServer::ensure_pch(std::uint32_t path_id, llvm::StringRef path, + const std::string& text, + const std::string& directory, + const std::vector& arguments) { + auto bound = compute_preamble_bound(text); + if(bound == 0) { + // No preamble directives — PCH would be empty, skip. + co_return true; + } + + auto preamble_hash = llvm::xxh3_64bits(llvm::StringRef(text).substr(0, bound)); + + // Reuse existing PCH if preamble content hasn't changed. + if(auto it = pch_hashes.find(path_id); it != pch_hashes.end()) { + if(it->second == preamble_hash && pch_paths.contains(path_id)) { + pch_bounds[path_id] = bound; + co_return true; + } + } + + // Build a new PCH via stateless worker. + worker::BuildPCHParams pch_params; + pch_params.file = std::string(path); + pch_params.directory = directory; + pch_params.arguments = arguments; + pch_params.content = text; + pch_params.preamble_bound = bound; + + LOG_DEBUG("Building PCH for {}, bound={}", path, bound); + + auto result = co_await pool.send_stateless(pch_params); + if(!result.has_value() || !result.value().success) { + LOG_WARN("PCH build failed for {}: {}", path, + result.has_value() ? result.value().error : result.error().message); + co_return false; + } + + pch_paths[path_id] = result.value().pch_path; + pch_bounds[path_id] = bound; + pch_hashes[path_id] = preamble_hash; + + LOG_INFO("PCH built for {}: {}", path, result.value().pch_path); + co_return true; +} + et::task MasterServer::ensure_compiled(std::uint32_t path_id, const std::string& uri) { auto doc_it = documents.find(path_id); if(doc_it == documents.end()) @@ -459,6 +515,20 @@ MasterServer::RawResult MasterServer::forward_stateless(const std::string& uri, if(!fill_compile_args(path, wp.directory, wp.arguments)) co_return serde_raw{}; + // Ensure PCH is available for stateless compilation (completion/signatureHelp). + co_await ensure_pch(path_id, path, wp.text, wp.directory, wp.arguments); + if(auto pch_it = pch_paths.find(path_id); pch_it != pch_paths.end()) { + wp.pch = {pch_it->second, pch_bounds[path_id]}; + } + + // Fill available PCM paths for module-aware completion. + for(auto& [pid, pcm_path]: pcm_paths) { + auto mod_it = path_to_module.find(pid); + if(mod_it != path_to_module.end()) { + wp.pcms[mod_it->second] = pcm_path; + } + } + lsp::PositionMapper mapper(doc.text, lsp::PositionEncoding::UTF16); auto offset = mapper.to_offset(position); if(!offset) @@ -675,6 +745,9 @@ void MasterServer::register_handlers() { documents.erase(path_id); debounce_timers.erase(path_id); + pch_paths.erase(path_id); + pch_bounds.erase(path_id); + pch_hashes.erase(path_id); // Clear diagnostics for closed file clear_diagnostics(params.text_document.uri); @@ -711,6 +784,9 @@ void MasterServer::register_handlers() { } } + // Invalidate cached PCH — preamble may have changed after save. + pch_hashes.erase(path_id); + LOG_DEBUG("didSave: {}", params.text_document.uri); }); diff --git a/src/server/master_server.h b/src/server/master_server.h index 24fa1ff95..ef3c6be1d 100644 --- a/src/server/master_server.h +++ b/src/server/master_server.h @@ -72,6 +72,15 @@ class MasterServer { // path_id -> module name (for files that provide a module interface). llvm::DenseMap path_to_module; + // path_id -> built PCH file path. + llvm::DenseMap pch_paths; + + // path_id -> preamble bound (byte offset) used when building the PCH. + llvm::DenseMap pch_bounds; + + // path_id -> hash of preamble content at PCH build time (for staleness detection). + llvm::DenseMap pch_hashes; + // Document state: path_id -> DocumentState llvm::DenseMap documents; @@ -104,6 +113,12 @@ class MasterServer { std::string& directory, std::vector& arguments); + // Build or reuse PCH for a source file. Returns true if PCH is available. + et::task ensure_pch(std::uint32_t path_id, llvm::StringRef path, + const std::string& text, + const std::string& directory, + const std::vector& arguments); + // Forwarding helpers for feature requests (RawValue passthrough) using RawResult = et::task; diff --git a/src/server/protocol.h b/src/server/protocol.h index 1aaff539e..2afe86e3d 100644 --- a/src/server/protocol.h +++ b/src/server/protocol.h @@ -97,11 +97,13 @@ struct BuildPCHParams { std::string directory; std::vector arguments; std::string content; + std::uint32_t preamble_bound = UINT32_MAX; }; struct BuildPCHResult { bool success; std::string error; + std::string pch_path; }; struct BuildPCMParams { diff --git a/src/server/stateless_worker.cpp b/src/server/stateless_worker.cpp index 2fe456f9f..426655d39 100644 --- a/src/server/stateless_worker.cpp +++ b/src/server/stateless_worker.cpp @@ -71,12 +71,12 @@ int run_stateless_worker_mode() { CompilationParams cp; cp.kind = CompilationKind::Preamble; fill_args(cp, params.directory, params.arguments); - cp.add_remapped_file(params.file, params.content); + cp.add_remapped_file(params.file, params.content, params.preamble_bound); auto tmp = fs::createTemporaryFile("clice-pch", "pch"); if(!tmp) { LOG_ERROR("BuildPCH: failed to create temp file"); - return {false, "Failed to create temporary PCH file"}; + return {false, "Failed to create temporary PCH file", ""}; } cp.output_file = *tmp; @@ -84,11 +84,11 @@ int run_stateless_worker_mode() { auto unit = compile(cp, pch_info); if(unit.completed()) { - LOG_INFO("BuildPCH done: file={}, {}ms", params.file, timer.ms()); - return {true, ""}; + LOG_INFO("BuildPCH done: file={}, output={}, {}ms", params.file, cp.output_file, timer.ms()); + return {true, "", std::string(cp.output_file)}; } else { LOG_WARN("BuildPCH failed: file={}, {}ms", params.file, timer.ms()); - return {false, "PCH compilation failed"}; + return {false, "PCH compilation failed", ""}; } }); co_return result.value(); diff --git a/tests/data/pch_test/common.h b/tests/data/pch_test/common.h new file mode 100644 index 000000000..cfb224b1b --- /dev/null +++ b/tests/data/pch_test/common.h @@ -0,0 +1,8 @@ +#pragma once + +struct Point { + int x; + int y; +}; + +int add(int a, int b); diff --git a/tests/data/pch_test/main.cpp b/tests/data/pch_test/main.cpp new file mode 100644 index 000000000..0656bcab8 --- /dev/null +++ b/tests/data/pch_test/main.cpp @@ -0,0 +1,11 @@ +#include "common.h" + +int add(int a, int b) { + return a + b; +} + +int main() { + Point p{1, 2}; + int result = add(p.x, p.y); + return result; +} diff --git a/tests/data/pch_test/no_includes.cpp b/tests/data/pch_test/no_includes.cpp new file mode 100644 index 000000000..f5eab9c6c --- /dev/null +++ b/tests/data/pch_test/no_includes.cpp @@ -0,0 +1,7 @@ +int square(int x) { + return x * x; +} + +int main() { + return square(3); +} diff --git a/tests/integration/test_pch.py b/tests/integration/test_pch.py new file mode 100644 index 000000000..5b9620e90 --- /dev/null +++ b/tests/integration/test_pch.py @@ -0,0 +1,104 @@ +"""Integration tests for PCH (precompiled header) functionality in MasterServer.""" + +import asyncio + +import pytest +from lsprotocol.types import ( + CompletionParams, + DidChangeTextDocumentParams, + DidCloseTextDocumentParams, + HoverParams, + Position, + TextDocumentContentChangeWholeDocument, + TextDocumentIdentifier, + VersionedTextDocumentIdentifier, +) + + +def _doc(uri: str) -> TextDocumentIdentifier: + return TextDocumentIdentifier(uri=uri) + + +@pytest.mark.workspace("pch_test") +async def test_pch_diagnostics_on_open(client, workspace): + """Opening a file with #include should trigger PCH build and return clean diagnostics.""" + uri, _ = await client.open_and_wait(workspace / "main.cpp") + assert uri in client.diagnostics + # main.cpp is well-formed, so diagnostics list should be empty (no errors). + diags = client.diagnostics[uri] + assert len(diags) == 0, f"Expected no diagnostics, got: {diags}" + client.text_document_did_close(DidCloseTextDocumentParams(text_document=_doc(uri))) + + +@pytest.mark.workspace("pch_test") +async def test_pch_body_edit_triggers_recompile(client, workspace): + """Editing only the body (not the preamble) should trigger recompilation.""" + uri, content = await client.open_and_wait(workspace / "main.cpp") + + # Edit only the function body — preamble (#include "common.h") unchanged. + new_content = content.replace("return result;", "return result + 1;") + event = client.wait_for_diagnostics(uri) + client.text_document_did_change( + DidChangeTextDocumentParams( + text_document=VersionedTextDocumentIdentifier(uri=uri, version=1), + content_changes=[TextDocumentContentChangeWholeDocument(text=new_content)], + ) + ) + # The key assertion: recompilation completes (diagnostics event fires). + await asyncio.wait_for(event.wait(), timeout=30.0) + assert uri in client.diagnostics + client.text_document_did_close(DidCloseTextDocumentParams(text_document=_doc(uri))) + + +@pytest.mark.workspace("pch_test") +async def test_no_pch_for_no_includes(client, workspace): + """A file with no #include directives should compile without PCH.""" + uri, _ = await client.open_and_wait(workspace / "no_includes.cpp") + assert uri in client.diagnostics + diags = client.diagnostics[uri] + assert len(diags) == 0, f"Expected no diagnostics, got: {diags}" + client.text_document_did_close(DidCloseTextDocumentParams(text_document=_doc(uri))) + + +@pytest.mark.workspace("pch_test") +async def test_hover_on_local_symbol(client, workspace): + """Hover on a locally defined symbol should work when PCH is active.""" + uri, _ = await client.open_and_wait(workspace / "main.cpp") + + # Hover over "add" on line 2 (0-indexed): "int add(int a, int b) {" + result = await client.text_document_hover_async( + HoverParams(text_document=_doc(uri), position=Position(line=2, character=4)) + ) + assert result is not None + client.text_document_did_close(DidCloseTextDocumentParams(text_document=_doc(uri))) + + +@pytest.mark.workspace("pch_test") +async def test_completion_with_pch(client, workspace): + """Completion should see symbols from PCH headers.""" + uri, content = await client.open_and_wait(workspace / "main.cpp") + + # Add a line that starts typing "Poi" to trigger completion for Point. + new_content = content + "\nPoi" + lines = new_content.split("\n") + last_line = len(lines) - 1 + + event = client.wait_for_diagnostics(uri) + client.text_document_did_change( + DidChangeTextDocumentParams( + text_document=VersionedTextDocumentIdentifier(uri=uri, version=1), + content_changes=[TextDocumentContentChangeWholeDocument(text=new_content)], + ) + ) + # Brief wait for the change to be processed. + await asyncio.sleep(1.0) + + result = await client.text_document_completion_async( + CompletionParams( + text_document=_doc(uri), + position=Position(line=last_line, character=3), + ) + ) + # Completion should return results. + assert result is not None + client.text_document_did_close(DidCloseTextDocumentParams(text_document=_doc(uri))) diff --git a/tests/unit/compile/compilation_tests.cpp b/tests/unit/compile/compilation_tests.cpp index 677a3f13e..af3fce16a 100644 --- a/tests/unit/compile/compilation_tests.cpp +++ b/tests/unit/compile/compilation_tests.cpp @@ -7,6 +7,7 @@ #include "compile/compilation.h" #include "support/filesystem.h" #include "syntax/scan.h" +#include "llvm/Support/xxhash.h" namespace clice::testing { @@ -274,6 +275,64 @@ int bar() { return 3; } }; // TEST_SUITE(Compiler) +TEST_SUITE(PreambleHash) { + +TEST_CASE(StableForBodyChanges) { + // Same preamble (#include lines) but different body → same hash → PCH reusable. + llvm::StringRef v1 = R"cpp( +#include "a.h" +#include "b.h" +int x = 1; +)cpp"; + llvm::StringRef v2 = R"cpp( +#include "a.h" +#include "b.h" +int x = 2; +void foo() {} +)cpp"; + + auto bound1 = compute_preamble_bound(v1); + auto bound2 = compute_preamble_bound(v2); + EXPECT_EQ(bound1, bound2); + + auto hash1 = llvm::xxh3_64bits(v1.substr(0, bound1)); + auto hash2 = llvm::xxh3_64bits(v2.substr(0, bound2)); + EXPECT_EQ(hash1, hash2); +} + +TEST_CASE(ChangesForNewInclude) { + // Different preamble (#include added) → different hash → PCH must rebuild. + llvm::StringRef v1 = R"cpp( +#include "a.h" +int x = 1; +)cpp"; + llvm::StringRef v2 = R"cpp( +#include "a.h" +#include "b.h" +int x = 1; +)cpp"; + + auto bound1 = compute_preamble_bound(v1); + auto bound2 = compute_preamble_bound(v2); + EXPECT_NE(bound1, bound2); + + auto hash1 = llvm::xxh3_64bits(v1.substr(0, bound1)); + auto hash2 = llvm::xxh3_64bits(v2.substr(0, bound2)); + EXPECT_NE(hash1, hash2); +} + +TEST_CASE(ZeroBoundNoPCH) { + // No preprocessor directives → bound is 0 → PCH should be skipped. + llvm::StringRef code = R"cpp( +int main() { return 0; } +)cpp"; + + auto bound = compute_preamble_bound(code); + EXPECT_EQ(bound, 0u); +} + +}; // TEST_SUITE(PreambleHash) + } // namespace } // namespace clice::testing diff --git a/tests/unit/server/pch_worker_tests.cpp b/tests/unit/server/pch_worker_tests.cpp new file mode 100644 index 000000000..a8a119087 --- /dev/null +++ b/tests/unit/server/pch_worker_tests.cpp @@ -0,0 +1,153 @@ +#include +#include + +#include "test/test.h" +#include "server/protocol.h" +#include "server/worker_test_helpers.h" +#include "syntax/scan.h" + +namespace clice::testing { + +namespace { + +namespace et = eventide; + +// ============================================================================ +// End-to-end PCH compilation through real workers: +// 1. Stateless worker builds PCH for preamble headers +// 2. Stateful worker compiles a file using the PCH +// ============================================================================ + +TEST_SUITE(PCHWorker) { + +TEST_CASE(BuildPCHThenCompile) { + TempDir tmp; + + tmp.touch("common.h", R"cpp(struct Point { int x, y; };)cpp" "\n"); + auto header = tmp.path("common.h"); + + std::string main_text = "#include \"common.h\"\nPoint p{1,2};\n"; + tmp.touch("main.cpp", main_text); + auto main_file = tmp.path("main.cpp"); + + auto dir = std::string(tmp.root); + + // --- Phase 1: Build PCH via stateless worker --- + WorkerHandle sl; + ASSERT_TRUE(sl.spawn("stateless-worker")); + + std::string pch_path; + bool phase1_done = false; + + sl.run([&]() -> et::task<> { + worker::BuildPCHParams params; + params.file = main_file; + params.directory = dir; + params.arguments = {"clang++", + "-resource-dir", + std::string(resource_dir()), + "-x", + "c++-header", + "-I", + dir, + main_file}; + params.content = main_text; + + auto result = co_await sl.peer->send_request(params); + CO_ASSERT_TRUE(result.has_value()); + CO_ASSERT_TRUE(result.value().success); + pch_path = result.value().pch_path; + EXPECT_FALSE(pch_path.empty()); + + phase1_done = true; + sl.peer->close_output(); + }); + + ASSERT_TRUE(phase1_done); + ASSERT_FALSE(pch_path.empty()); + + // Verify the PCH file exists on disk. + ASSERT_TRUE(llvm::sys::fs::exists(pch_path)); + + // --- Phase 2: Compile with PCH via stateful worker --- + WorkerHandle sf; + ASSERT_TRUE(sf.spawn("stateful-worker")); + + bool phase2_done = false; + + auto preamble_bound = compute_preamble_bound(main_text); + + sf.run([&]() -> et::task<> { + worker::CompileParams params; + params.path = main_file; + params.version = 1; + params.text = main_text; + params.directory = dir; + params.arguments = {"clang++", + "-resource-dir", + std::string(resource_dir()), + "-fsyntax-only", + "-I", + dir, + main_file}; + params.pch = {pch_path, preamble_bound}; + + auto result = co_await sf.peer->send_request(params); + CO_ASSERT_TRUE(result.has_value()); + EXPECT_EQ(result.value().version, 1); + + phase2_done = true; + sf.peer->close_output(); + }); + + ASSERT_TRUE(phase2_done); + + // Cleanup PCH temp file. + std::remove(pch_path.c_str()); +} + +TEST_CASE(CompileWithoutPCHStillWorks) { + TempDir tmp; + + tmp.touch("common.h", R"cpp(struct Point { int x, y; };)cpp" "\n"); + std::string main_text = "#include \"common.h\"\nPoint p{1,2};\n"; + tmp.touch("main.cpp", main_text); + auto main_file = tmp.path("main.cpp"); + + auto dir = std::string(tmp.root); + + WorkerHandle sf; + ASSERT_TRUE(sf.spawn("stateful-worker")); + + bool compile_done = false; + + sf.run([&]() -> et::task<> { + worker::CompileParams params; + params.path = main_file; + params.version = 1; + params.text = main_text; + params.directory = dir; + params.arguments = {"clang++", + "-resource-dir", + std::string(resource_dir()), + "-fsyntax-only", + "-I", + dir, + main_file}; + // pch left as default (empty path, 0 bound). + + auto result = co_await sf.peer->send_request(params); + CO_ASSERT_TRUE(result.has_value()); + EXPECT_EQ(result.value().version, 1); + + compile_done = true; + sf.peer->close_output(); + }); + + ASSERT_TRUE(compile_done); +} + +}; // TEST_SUITE(PCHWorker) + +} // namespace +} // namespace clice::testing diff --git a/tests/unit/server/stateless_worker_tests.cpp b/tests/unit/server/stateless_worker_tests.cpp index 0edd60721..7751223bf 100644 --- a/tests/unit/server/stateless_worker_tests.cpp +++ b/tests/unit/server/stateless_worker_tests.cpp @@ -102,6 +102,8 @@ TEST_CASE(BuildPCHRequest) { auto result = co_await w.peer->send_request(params); EXPECT_TRUE(result.has_value()); + EXPECT_TRUE(result.value().success); + EXPECT_FALSE(result.value().pch_path.empty()); test_done = true; w.peer->close_output(); }); From e0b4ec85af8a3e5fcfaebcf334069a8322cd7456 Mon Sep 17 00:00:00 2001 From: ykiko Date: Wed, 1 Apr 2026 15:12:59 +0800 Subject: [PATCH 2/5] fix: address CodeRabbit review comments - Prevent duplicate concurrent PCH builds via pch_building guard set - Guard against UB in PCH worker test when result has no value Co-Authored-By: Claude Opus 4.6 --- src/server/master_server.cpp | 31 +++++++++++++++----- src/server/master_server.h | 6 +++- tests/unit/server/stateless_worker_tests.cpp | 4 +++ 3 files changed, 33 insertions(+), 8 deletions(-) diff --git a/src/server/master_server.cpp b/src/server/master_server.cpp index fea743519..7e00c53e9 100644 --- a/src/server/master_server.cpp +++ b/src/server/master_server.cpp @@ -184,8 +184,7 @@ et::task<> MasterServer::run_build_drain(std::uint32_t path_id, std::string uri) } // Build or reuse PCH for preamble acceleration. - co_await ensure_pch(path_id, params.path, params.text, - params.directory, params.arguments); + co_await ensure_pch(path_id, params.path, params.text, params.directory, params.arguments); // Populate PCH info if available. if(auto pch_it = pch_paths.find(path_id); pch_it != pch_paths.end()) { @@ -390,10 +389,11 @@ bool MasterServer::fill_compile_args(llvm::StringRef path, return true; } -et::task MasterServer::ensure_pch(std::uint32_t path_id, llvm::StringRef path, - const std::string& text, - const std::string& directory, - const std::vector& arguments) { +et::task MasterServer::ensure_pch(std::uint32_t path_id, + llvm::StringRef path, + const std::string& text, + const std::string& directory, + const std::vector& arguments) { auto bound = compute_preamble_bound(text); if(bound == 0) { // No preamble directives — PCH would be empty, skip. @@ -410,6 +410,17 @@ et::task MasterServer::ensure_pch(std::uint32_t path_id, llvm::StringRef p } } + // If another coroutine is already building PCH for this file, wait for it. + if(auto it = pch_building.find(path_id); it != pch_building.end()) { + co_await it->second->wait(); + pch_bounds[path_id] = bound; + co_return pch_paths.contains(path_id); + } + + // Register in-flight build so concurrent requests wait on us. + auto completion = std::make_shared(); + pch_building[path_id] = completion; + // Build a new PCH via stateless worker. worker::BuildPCHParams pch_params; pch_params.file = std::string(path); @@ -421,8 +432,14 @@ et::task MasterServer::ensure_pch(std::uint32_t path_id, llvm::StringRef p LOG_DEBUG("Building PCH for {}, bound={}", path, bound); auto result = co_await pool.send_stateless(pch_params); + + // Signal waiters and remove in-flight entry. + pch_building.erase(path_id); + completion->set(); + if(!result.has_value() || !result.value().success) { - LOG_WARN("PCH build failed for {}: {}", path, + LOG_WARN("PCH build failed for {}: {}", + path, result.has_value() ? result.value().error : result.error().message); co_return false; } diff --git a/src/server/master_server.h b/src/server/master_server.h index ef3c6be1d..89f24f004 100644 --- a/src/server/master_server.h +++ b/src/server/master_server.h @@ -81,6 +81,9 @@ class MasterServer { // path_id -> hash of preamble content at PCH build time (for staleness detection). llvm::DenseMap pch_hashes; + // path_id -> in-flight PCH build event (later arrivals co_await the same build). + llvm::DenseMap> pch_building; + // Document state: path_id -> DocumentState llvm::DenseMap documents; @@ -114,7 +117,8 @@ class MasterServer { std::vector& arguments); // Build or reuse PCH for a source file. Returns true if PCH is available. - et::task ensure_pch(std::uint32_t path_id, llvm::StringRef path, + et::task ensure_pch(std::uint32_t path_id, + llvm::StringRef path, const std::string& text, const std::string& directory, const std::vector& arguments); diff --git a/tests/unit/server/stateless_worker_tests.cpp b/tests/unit/server/stateless_worker_tests.cpp index 7751223bf..41abc3c19 100644 --- a/tests/unit/server/stateless_worker_tests.cpp +++ b/tests/unit/server/stateless_worker_tests.cpp @@ -102,6 +102,10 @@ TEST_CASE(BuildPCHRequest) { auto result = co_await w.peer->send_request(params); EXPECT_TRUE(result.has_value()); + if(!result.has_value()) { + w.peer->close_output(); + co_return; + } EXPECT_TRUE(result.value().success); EXPECT_FALSE(result.value().pch_path.empty()); test_done = true; From 00fd47e93c56787783995098c0be0862eed73e2e Mon Sep 17 00:00:00 2001 From: ykiko Date: Wed, 1 Apr 2026 16:52:28 +0800 Subject: [PATCH 3/5] fix: multiple PCH correctness issues found in self-review - Fix ensure_pch race: update pch_paths/bounds/hashes before signaling waiters, so they see the completed state - Remove waiter's bound override that could mismatch the actual PCH - Add self-PCM exclusion in forward_stateless to prevent clang "multiple module declarations" errors - Clean up old PCH temp file when replacing with a new build - Clean up temp file on PCH compilation failure in stateless worker Co-Authored-By: Claude Opus 4.6 --- src/server/master_server.cpp | 19 ++++++++++++++----- src/server/stateless_worker.cpp | 6 +++++- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/src/server/master_server.cpp b/src/server/master_server.cpp index 7e00c53e9..2ffa99426 100644 --- a/src/server/master_server.cpp +++ b/src/server/master_server.cpp @@ -413,7 +413,6 @@ et::task MasterServer::ensure_pch(std::uint32_t path_id, // If another coroutine is already building PCH for this file, wait for it. if(auto it = pch_building.find(path_id); it != pch_building.end()) { co_await it->second->wait(); - pch_bounds[path_id] = bound; co_return pch_paths.contains(path_id); } @@ -433,22 +432,29 @@ et::task MasterServer::ensure_pch(std::uint32_t path_id, auto result = co_await pool.send_stateless(pch_params); - // Signal waiters and remove in-flight entry. - pch_building.erase(path_id); - completion->set(); - if(!result.has_value() || !result.value().success) { LOG_WARN("PCH build failed for {}: {}", path, result.has_value() ? result.value().error : result.error().message); + pch_building.erase(path_id); + completion->set(); co_return false; } + // Delete old PCH temp file before replacing. + if(auto old_it = pch_paths.find(path_id); old_it != pch_paths.end()) { + fs::remove(old_it->second); + } + pch_paths[path_id] = result.value().pch_path; pch_bounds[path_id] = bound; pch_hashes[path_id] = preamble_hash; LOG_INFO("PCH built for {}: {}", path, result.value().pch_path); + + // Signal waiters after state is fully updated, then remove in-flight entry. + pch_building.erase(path_id); + completion->set(); co_return true; } @@ -539,7 +545,10 @@ MasterServer::RawResult MasterServer::forward_stateless(const std::string& uri, } // Fill available PCM paths for module-aware completion. + // Skip the file's own PCM to avoid "multiple module declarations" errors. for(auto& [pid, pcm_path]: pcm_paths) { + if(pid == path_id) + continue; auto mod_it = path_to_module.find(pid); if(mod_it != path_to_module.end()) { wp.pcms[mod_it->second] = pcm_path; diff --git a/src/server/stateless_worker.cpp b/src/server/stateless_worker.cpp index 426655d39..ce2a3263b 100644 --- a/src/server/stateless_worker.cpp +++ b/src/server/stateless_worker.cpp @@ -84,10 +84,14 @@ int run_stateless_worker_mode() { auto unit = compile(cp, pch_info); if(unit.completed()) { - LOG_INFO("BuildPCH done: file={}, output={}, {}ms", params.file, cp.output_file, timer.ms()); + LOG_INFO("BuildPCH done: file={}, output={}, {}ms", + params.file, + cp.output_file, + timer.ms()); return {true, "", std::string(cp.output_file)}; } else { LOG_WARN("BuildPCH failed: file={}, {}ms", params.file, timer.ms()); + fs::remove(cp.output_file); return {false, "PCH compilation failed", ""}; } }); From 8330d0c42faca0d0b02472dc3a7afbfb61fb1e47 Mon Sep 17 00:00:00 2001 From: ykiko Date: Wed, 1 Apr 2026 17:28:00 +0800 Subject: [PATCH 4/5] fix: address new CodeRabbit review findings - Clear stale PCH entries when bound == 0 (all #includes removed) - Use wp.text instead of doc.text after co_await to avoid use-after-free if document is closed during suspension - Invalidate all PCH hashes on save, not just the saved file's, since the saved file may be a header included by other TUs Co-Authored-By: Claude Opus 4.6 --- src/server/master_server.cpp | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/server/master_server.cpp b/src/server/master_server.cpp index 2ffa99426..1187da166 100644 --- a/src/server/master_server.cpp +++ b/src/server/master_server.cpp @@ -396,7 +396,13 @@ et::task MasterServer::ensure_pch(std::uint32_t path_id, const std::vector& arguments) { auto bound = compute_preamble_bound(text); if(bound == 0) { - // No preamble directives — PCH would be empty, skip. + // No preamble directives — PCH would be empty. Clear any stale entry. + if(auto old_it = pch_paths.find(path_id); old_it != pch_paths.end()) { + fs::remove(old_it->second); + } + pch_paths.erase(path_id); + pch_bounds.erase(path_id); + pch_hashes.erase(path_id); co_return true; } @@ -555,7 +561,7 @@ MasterServer::RawResult MasterServer::forward_stateless(const std::string& uri, } } - lsp::PositionMapper mapper(doc.text, lsp::PositionEncoding::UTF16); + lsp::PositionMapper mapper(wp.text, lsp::PositionEncoding::UTF16); auto offset = mapper.to_offset(position); if(!offset) co_return serde_raw{"null"}; @@ -810,8 +816,9 @@ void MasterServer::register_handlers() { } } - // Invalidate cached PCH — preamble may have changed after save. - pch_hashes.erase(path_id); + // Invalidate all cached PCH hashes — the saved file may be a header + // included by other TUs, so we must force rebuild for all open documents. + pch_hashes.clear(); LOG_DEBUG("didSave: {}", params.text_document.uri); }); From f73ca4f293fa9985c79c31c948b166db446ebf53 Mon Sep 17 00:00:00 2001 From: ykiko Date: Wed, 1 Apr 2026 18:10:40 +0800 Subject: [PATCH 5/5] style: format compilation_tests.cpp Co-Authored-By: Claude Opus 4.6 --- tests/unit/compile/compilation_tests.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/unit/compile/compilation_tests.cpp b/tests/unit/compile/compilation_tests.cpp index af3fce16a..79954b546 100644 --- a/tests/unit/compile/compilation_tests.cpp +++ b/tests/unit/compile/compilation_tests.cpp @@ -7,6 +7,7 @@ #include "compile/compilation.h" #include "support/filesystem.h" #include "syntax/scan.h" + #include "llvm/Support/xxhash.h" namespace clice::testing {