Skip to content

Commit 1b21b00

Browse files
committed
fix(cdb): arguments 是 argv,不是 shell 词
消费者(clangd)逐字 exec `arguments`,不经 shell。所以一个仍带着 shell 引号的 token 不是 flag,而是一个不存在的文件名。 用户报的是 Windows 上 `-fmodule-file=` 带多余双引号、每次构建后要手工删。核实后比 报告更严重,而且在 Linux 上就能复现 —— 带空格的项目路径 + llvm: '-fmodule-file=std=/tmp/.../my project/.../std.pcm <- 闭合引号没了 '-fprebuilt-module-path=/tmp/.../my project/.../pcm.cache' 第一条不只是多个引号:token 在引号**内部**的空格处被切断,一个参数变成两个,其中一个 带着永不闭合的开引号。 机制是四步各自合理的叠加:`shell_quote_arg` 的触发集含反斜杠(Windows 路径必含,所以 那里每个带路径的 flag 都被引号包住)→ 加引号对 ninja 是对的 → `split_flags` 撤销 ninja 转义并重新分词 → 但它不认引号。 `split_flags` 原来的注释把原则写对了 ——「消费者逐字 exec,所以转义必须撤销」—— 只实现 了一半,漏掉同一理由下的 shell 引号。现在三件事都做,且**顺序**是要点:ninja 的 `$ ` 反转义必须发生在引号**内**,否则被引号包住的空格先把 token 切断,那正是原来的 bug。 中间的引号是数据不是引用:`-DGREETING="hi"` 保留内部引号,否则宏的含义就变了。 判据写成「任何 token 不以引号开头/结尾,且带空格的路径是一个 token」,不是「clangd 能 用了」—— 后者在不含空格的路径上恒真,正是它至今没被发现的原因:所有 e2e 的项目路径都 不含空格。 `split_flags` 由匿名 namespace 提升为导出:它就是那份契约,该由测试钉住而不是靠通读 生成文档来推断。 tests/unit/test_compile_commands.cpp +5(9 条);端到端复验:带引号 token 归零
1 parent 10967c5 commit 1b21b00

3 files changed

Lines changed: 239 additions & 20 deletions

File tree

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
# 机器可读输出协议 —— 拆分实施计划
2+
3+
> 设计:`2026-08-08-machine-readable-output-protocol-design.md`(对 RFC #379 的核对与修正)
4+
> 已定:未知格式走 **stderr + rc=2**;`pack --format` **声明为例外**
5+
6+
## 顺序与理由
7+
8+
设计文档 §4.5 把 CDB 引号缺陷排在阶段 1 之前,理由是「协议做得再好,也救不了一个内容
9+
本身就坏掉的出口」。照此排:
10+
11+
```
12+
W0 CDB 引号(§4.5) —— 今天就在坏,且与协议正交,先修
13+
W1 stdout 归属(阶段 0) —— 未知选项/未知值统一 stderr + rc=2
14+
W2 mcpp.wire 模块(阶段 1) —— envelope + destructive + --protocol-version
15+
W3 --format 归一(阶段 2) —— --json 永久别名,入口一处归一
16+
W4 接入(阶段 3a) —— self env / xpkg parse / cache list
17+
```
18+
19+
W4 之后的 `metadata` / `--configure-only`(阶段 3b/3c)**不在本次范围** —— 它们是新增
20+
能力而非契约统一,且依赖 #372 的拆分结论。
21+
22+
## W0 —— CDB 的 `arguments` 带 shell 引号
23+
24+
**File:** `src/build/compile_commands.cppm`
25+
26+
- [ ] **Step 1** 红:带空格路径 + llvm 的单测,断言
27+
① 任何 token 不以引号开头/结尾;② 带空格的路径是**一个** token
28+
- [ ] **Step 2** 跑,确认 FAIL(今天两条都不满足)
29+
- [ ] **Step 3**`split_flags` 认引号。**顺序陷阱**:ninja`$ ` 反转义必须发生在
30+
引号****,否则被引号包住的空格先把 token 切断 —— 那正是现在的 bug
31+
- [ ] **Step 4** 绿
32+
- [ ] **Step 5** Commit
33+
34+
判据不能写成「clangd 能用了」——在不含空格的路径上恒真,正是它至今没被发现的原因。
35+
36+
## W1 —— stdout 归属
37+
38+
**Files:** `src/cli.cppm``src/main.cpp`(+ 依赖 `mcpplibs.cmdline` 的处置)
39+
40+
- [ ] **Step 1**:e2e 断言未知**选项**与未知****在通道(stderr)与退出码(2)上一致
41+
- [ ] **Step 2** 跑,FAIL(今天:未知选项 → stdout/rc=1;未知值 → stderr/rc=2)
42+
- [ ] **Step 3** 实现。`mcpplibs.cmdline:127``std::println` 写 stdout,在依赖里 ——
43+
**本次不改依赖**,改为 mcpp 侧接管 `ParseResult`:不走 `App::run()` 的内建错误
44+
打印,自己输出到 stderr 并返回 2
45+
- [ ] **Step 4** 绿
46+
- [ ] **Step 5** Commit
47+
48+
## W2 —— `mcpp.wire`(独立模块)
49+
50+
**New:** `src/wire.cppm`
51+
52+
- [ ] **Step 1** 单测:envelope 形状(`schemaVersion` / `kind` / `destructive` /
53+
`mcpp.version` / `mcpp.protocol{min,max}` / `data` / `diagnostics`)
54+
- [ ] **Step 2** 实现。`Diagnostic/Position/Range/Severity` 与 envelope 构造从 #372
55+
`src/ide/model.cppm``src/ide/snapshot.cppm` 提升(设计文档 §5-B)
56+
- [ ] **Step 3** `mcpp --protocol-version`:输出 `{min,max}` **加命令 → destructive 静态表**
57+
(设计 §2.3 —— untrusted 门要在执行前知道)
58+
- [ ] **Step 4** golden fixture,且**反向验证过**(改字段名要变红,设计 §4)
59+
- [ ] **Step 5** Commit
60+
61+
## W3 —— `--format` 归一
62+
63+
**Files:** `src/cli.cppm`(入口归一)、各 `cmd_*.cppm`
64+
65+
- [ ] **Step 1** 单测:`--json``--format json` 产出**逐字节相同**
66+
- [ ] **Step 2/3** 入口一处 `--json``--format json`;核心只见 `--format`
67+
- [ ] **Step 4** 绿;`--json` 永久保留、**不打 deprecation 警告**
68+
- [ ] **Step 5** Commit
69+
70+
`pack --format tar|dir` 不动(已定为例外),文档写明。
71+
72+
## W4 —— 接入 envelope
73+
74+
**Files:** `cmd_self.cppm`(新增 `self env --format json`)、`cmd_xpkg.cppm``cmd_cache.cppm`
75+
76+
- [ ] **Step 1** 每个 `kind` 一个 golden fixture(反向验证过)
77+
- [ ] **Step 2** `self env` 覆盖 mcpp-vscode#8 §2.4:MCPP_HOME / registry / xlings home /
78+
index repos / default toolchain
79+
- [ ] **Step 3** `xpkg parse` / `cache list` 包进 envelope(`schemaVersion` 随之而来)
80+
- [ ] **Step 4** 绿
81+
- [ ] **Step 5** Commit
82+
83+
## 平台特化的去处
84+
85+
按要求:平台差异进 `src/platform/`,协议本体独立 `.cppm`
86+
87+
- 协议本体 → **`src/wire.cppm`**(新模块,不依赖任何命令)
88+
- 路径/引号的平台差异 → 已在 `src/platform/`(`env::path_list_separator` 等);W0 的
89+
引号识别是**平台无关**的(引号本身两平台都要剥),不新增平台分支
90+
91+
## 交付
92+
93+
- 全部并入 **PR #385**(与 docs 一起)
94+
- 版本已是 2026.8.8.3(刚发布)⇒ 本次**跳过版本 bump**,除非 CI 要求
95+
- xlings pin 已是 2026.8.8.1(索引最新)⇒ 无需再抬

src/build/compile_commands.cppm

Lines changed: 64 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,13 @@ import mcpp.libs.json;
2121

2222
export namespace mcpp::build {
2323

24+
// Split one flag string into CDB `arguments` tokens.
25+
//
26+
// Exported because it IS the contract: what a consumer receives in
27+
// `arguments` is decided here, and that contract needs pinning by test
28+
// rather than by inspection of a whole generated document.
29+
std::vector<std::string> split_flags(std::string_view s);
30+
2431
// Generate compile_commands.json content as a string.
2532
std::string emit_compile_commands(const BuildPlan& plan, const CompileFlags& flags);
2633

@@ -54,50 +61,87 @@ bool is_c_source(const std::filesystem::path& src) {
5461
return ext == ".c" || ext == ".m";
5562
}
5663

57-
// Split a flag string into individual tokens AND un-escape ninja-style
58-
// path escapes (`$ ` → space, `$:` → `:`, `$$` → `$`).
64+
} // namespace
65+
66+
// Split one flag string into CDB `arguments` tokens.
67+
//
68+
// Three things happen here, and the ORDER between them is the whole point.
69+
//
70+
// 1. Ninja escapes are undone (`$ ` -> space, `$:` -> `:`, `$$` -> `$`).
71+
// `flags.cppm::escape_path` adds them so a path survives embedding in a
72+
// ninja rule string. A CDB consumer execs `arguments` literally -- no
73+
// ninja -- so `C$:\Users\...` would be a path that does not exist.
74+
//
75+
// 2. Shell quoting is removed. `flags.cppm::shell_quote_arg` wraps a token
76+
// whose characters would split or alter a word in `sh -c` / cmd.exe --
77+
// and its trigger set contains the BACKSLASH, so on Windows every
78+
// path-bearing flag is quoted. That quoting is correct for ninja and
79+
// wrong for a consumer that never invokes a shell: the quotes arrive as
80+
// part of the filename.
81+
//
82+
// 3. Tokens split on spaces -- but not on a space that came from `$ `, and
83+
// not on one inside quotes.
84+
//
85+
// Getting (3) wrong is what shipped: quoting was ignored entirely, so a
86+
// quoted path containing a space was cut in half at that space. Measured on
87+
// a real build under `/tmp/.../my project`:
88+
//
89+
// '-fmodule-file=std=/tmp/.../my project/.../std.pcm <- closing quote gone
90+
// '-fprebuilt-module-path=/tmp/.../my project/.../pcm.cache'
5991
//
60-
// `flags.cppm::escape_path` ninja-escapes path arguments so they survive
61-
// embedding in ninja rule strings. Those escaped strings are then captured
62-
// into `f.cxx` / `f.cc` which is what we receive here. CDB consumers like
63-
// clangd exec the `arguments` array literally — no ninja involved — so
64-
// escaped chars must be undone or paths like `C:\Users\...` come through
65-
// as `C$:\Users\...` and break clangd's path resolution on Windows. (The
66-
// same issue would silently affect any POSIX path containing a space or
67-
// `$` — those just happen to be rare.)
92+
// One argument became two, one of them opening a quote that never closes.
93+
// The comment that used to live here stated the right principle -- consumers
94+
// exec literally, so escapes must be undone -- and implemented half of it.
6895
//
69-
// Splitting and un-escaping in one pass is correct: a literal space inside
70-
// a path appears as `$ ` in the input, which we must NOT treat as a token
71-
// separator.
96+
// A quote in the MIDDLE of a token is data: `-DGREETING="hi"` keeps its
97+
// inner quotes, or the define changes meaning. Only a quote that OPENS a
98+
// region is quoting.
7299
std::vector<std::string> split_flags(std::string_view s) {
73100
std::vector<std::string> out;
74101
std::string token;
102+
bool started = false; // token has begun (so "" is a real empty arg)
103+
char quote = 0; // active quote char, 0 when outside
104+
75105
auto flush = [&] {
76-
if (!token.empty()) {
77-
out.push_back(std::move(token));
78-
token.clear();
79-
}
106+
if (started) { out.push_back(std::move(token)); token.clear(); }
107+
started = false;
80108
};
109+
81110
for (std::size_t i = 0; i < s.size(); ++i) {
82111
char c = s[i];
112+
113+
// Ninja escapes first, and INSIDE quotes too -- a quoted path's space
114+
// arrives as `$ ` because flags.cppm escapes before it quotes.
83115
if (c == '$' && i + 1 < s.size()) {
84116
char nc = s[i + 1];
85117
if (nc == ' ' || nc == ':' || nc == '$') {
86118
token.push_back(nc);
119+
started = true;
87120
++i;
88121
continue;
89122
}
90123
}
91-
if (c == ' ') {
92-
flush();
93-
} else {
124+
125+
if (quote) {
126+
if (c == quote) { quote = 0; continue; } // closes the region
94127
token.push_back(c);
128+
continue;
95129
}
130+
131+
// Opens a region only at a token boundary; elsewhere it is data.
132+
if ((c == '"' || c == '\'') && !started) { quote = c; started = true; continue; }
133+
134+
if (c == ' ') { flush(); continue; }
135+
136+
token.push_back(c);
137+
started = true;
96138
}
97139
flush();
98140
return out;
99141
}
100142

143+
namespace {
144+
101145
std::vector<std::string> local_include_args(const CompileUnit& cu) {
102146
std::vector<std::string> args;
103147
args.reserve(cu.localIncludeDirs.size());

tests/unit/test_compile_commands.cpp

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,3 +92,83 @@ TEST(CompileCommandsMerge, MalformedExistingFallsBackToFresh) {
9292
EXPECT_NE(merged.find("src/main.cpp"), std::string::npos) << merged;
9393
EXPECT_NE(merged.find("-O2"), std::string::npos) << merged;
9494
}
95+
96+
// ── CDB arguments must be argv, not shell words ─────────────────────────────
97+
//
98+
// A consumer (clangd) execs `arguments` LITERALLY — no shell. So a token that
99+
// still carries the quoting a shell would have removed is not a flag, it is a
100+
// filename that does not exist.
101+
//
102+
// This is reachable on every platform, and was found on Windows first only
103+
// because `shell_quote_arg`'s trigger set contains the backslash: every
104+
// Windows path has one, so every path-bearing flag there gets quoted. On
105+
// POSIX it takes a space in the path — which is why no e2e caught it: none of
106+
// their project paths have one.
107+
//
108+
// Measured before the fix, from a real build under `/tmp/.../my project`:
109+
//
110+
// '-fmodule-file=std=/tmp/.../my project/.../std.pcm <- closing quote gone
111+
// '-fprebuilt-module-path=/tmp/.../my project/.../pcm.cache'
112+
//
113+
// The first is worse than a stray quote: the token was split at the space
114+
// INSIDE the quotes, so one argument became two, one of them opening a quote
115+
// that never closes.
116+
namespace {
117+
118+
bool is_quoted(std::string_view s) {
119+
if (s.size() < 2) return false;
120+
return (s.front() == '"' && s.back() == '"')
121+
|| (s.front() == '\'' && s.back() == '\'');
122+
}
123+
bool has_edge_quote(std::string_view s) {
124+
return !s.empty()
125+
&& (s.front() == '"' || s.front() == '\''
126+
|| s.back() == '"' || s.back() == '\'');
127+
}
128+
129+
} // namespace
130+
131+
TEST(CompileCommandsArgs, StripsPosixQuotingFromAToken) {
132+
auto out = mcpp::build::split_flags("-O2 '-fprebuilt-module-path=/a/b' -g");
133+
ASSERT_EQ(out.size(), 3u);
134+
EXPECT_EQ(out[1], "-fprebuilt-module-path=/a/b");
135+
for (auto const& t : out) EXPECT_FALSE(has_edge_quote(t)) << t;
136+
}
137+
138+
TEST(CompileCommandsArgs, StripsWindowsQuotingFromAToken) {
139+
auto out = mcpp::build::split_flags(R"(-O2 "-IC:\Users\x\inc" -g)");
140+
ASSERT_EQ(out.size(), 3u);
141+
EXPECT_EQ(out[1], R"(-IC:\Users\x\inc)");
142+
for (auto const& t : out) EXPECT_FALSE(has_edge_quote(t)) << t;
143+
}
144+
145+
// The one that bit: a quoted path containing a space is ONE argument. The
146+
// space arrives ninja-escaped (`$ `) because flags.cppm escapes before it
147+
// quotes, so the un-escape has to happen INSIDE the quotes — do it in the
148+
// wrong order and the token splits exactly here.
149+
TEST(CompileCommandsArgs, AQuotedPathWithASpaceStaysOneToken) {
150+
auto out = mcpp::build::split_flags(
151+
"-O2 '-fmodule-file=std=/tmp/my$ project/std.pcm' -g");
152+
ASSERT_EQ(out.size(), 3u) << "the quoted path was split";
153+
EXPECT_EQ(out[1], "-fmodule-file=std=/tmp/my project/std.pcm");
154+
for (auto const& t : out) EXPECT_FALSE(has_edge_quote(t)) << t;
155+
}
156+
157+
// Unquoted input must keep behaving exactly as before: ninja escapes undone,
158+
// split on spaces.
159+
TEST(CompileCommandsArgs, UnquotedInputIsUnchanged) {
160+
auto out = mcpp::build::split_flags("-IC$:/x -DA=1 -Idir$ with$ space");
161+
ASSERT_EQ(out.size(), 3u);
162+
EXPECT_EQ(out[0], "-IC:/x");
163+
EXPECT_EQ(out[1], "-DA=1");
164+
EXPECT_EQ(out[2], "-Idir with space");
165+
}
166+
167+
// A quote in the MIDDLE of a token is data, not quoting: `-DA="x"` must keep
168+
// its inner quotes or the define changes meaning.
169+
TEST(CompileCommandsArgs, InnerQuotesAreNotStripped) {
170+
auto out = mcpp::build::split_flags(R"(-DGREETING="hi")");
171+
ASSERT_EQ(out.size(), 1u);
172+
EXPECT_EQ(out[0], R"(-DGREETING="hi")");
173+
EXPECT_FALSE(is_quoted(out[0]));
174+
}

0 commit comments

Comments
 (0)