Skip to content

Commit 6dbfdc2

Browse files
committed
test(build): pin the schedule policy, and record why the naive graph split fails
policy 的 7 条单测,每条都从两侧钉: * 两种机制**互补而非可互换** —— clang 必须是 two-phase(它以 O_TRUNC 直写最终路径, detach 会把写了一半的 BMI 交给导入者),gcc 必须是 detach(它没有便宜的两阶段, two-phase 等于把所有东西编两遍)。装反是**静默**的。 * 未实测的编译器必须留在 None —— 这里猜错不是构建变慢,是误编译。 * `off` 关得掉,而**同样输入下 `auto` 关不掉** —— 只钉前一条的话, 「永远不启用」的实现也能通过。 * HAZARD 2 编进断言:detach 下给 ninja 的槽必须**多于**编译器上限, two-phase 下必须**相等**。 * 每一条决策都必须带 reason,包括选 None 的那些。 * hostJobs=0 不能变成 -j0 或负数。 `header_line` 现在必须带 schedule tag —— 编译期就抓到了漏改的调用点, 这正是要它必传的原因。测试同时钉住「旧图没有该字段时读出的是空,不是 "none"」: 「这份文件早于该字段」和「这份文件选择了不做」必须能区分。 --- ⚠️ **实测记录:朴素的两边拆分会静默丢掉头文件跟踪。** 拆分后 depfile 挂哪条边,两种挂法都错: depfile 写出于 16.39s,BMI 发布于 2.36s,整条编译 16.55s —— depfile 在 BMI 之后 * 挂 **BMI 边**:该边在 2.36s 就完成,那时 depfile 还不存在,ninja 读到陈旧/缺失依赖。 * 挂 **对象边**:头文件变更会让对象边重跑,但 `bmi-await` 只会看到已存在的 `.rc` 立刻返回 —— **什么都不重编**。 这一类缺陷不会报错,只会让改了头文件的构建悄悄不生效。所以图的形态**未改动**: 默认路径与本 PR 之前完全一致,已落地的是决策、运行期与可观测性。 候选解法(BMI 边改用 P1689 扫描已经产出的 `.ddi.dep` 作依赖来源)需要先证明 扫描的依赖集与编译的一致,尚未验证。
1 parent 9f2d940 commit 6dbfdc2

5 files changed

Lines changed: 145 additions & 29 deletions

File tree

src/build/schedule/detach_codegen.cppm

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -97,9 +97,17 @@ struct CompileRequest {
9797
// Directory of concurrency tokens. Empty disables the cap (hazard 2).
9898
std::filesystem::path semaphore;
9999
int maxCompilers{0};
100-
std::vector<std::string> argv; // compiler and its arguments
101-
// The file `argv` was read from; handed to the supervisor unchanged.
102-
std::filesystem::path argvFile;
100+
// The compiler invocation, as ONE shell command line.
101+
//
102+
// Not a token list: the backend has already joined and quoted the flags for
103+
// ninja, and splitting that string back into argv would need to reimplement
104+
// the shell's rules — the exact assumption ("one flag element == one argv
105+
// token") that has been wrong here before. ninja runs every command through
106+
// a shell already, so going through one costs no portability.
107+
std::string command;
108+
// The file `command` was read from; handed to the supervisor unchanged, so
109+
// there is one representation and no re-quoting.
110+
std::filesystem::path commandFile;
103111
};
104112

105113
// Phase 1 — returns 0 as soon as the BMI is published, leaving code generation
@@ -110,7 +118,7 @@ int compile_release_at_bmi(const CompileRequest& req);
110118
// then records the status. Never invoked directly by a build edge.
111119
int supervise(const std::filesystem::path& slot,
112120
const std::filesystem::path& semaphoreToken,
113-
const std::vector<std::string>& argv);
121+
std::string_view command);
114122

115123
// Phase 2 — blocks until the compiler for `slot` finished, replays what it
116124
// wrote, and propagates its status. `object`, when given, must exist: a
@@ -209,8 +217,9 @@ bool spawn_detached(const std::vector<std::string>& argv) {
209217
return true;
210218
}
211219

212-
int run_to_completion(const std::vector<std::string>& argv,
220+
int run_to_completion(std::string_view command,
213221
const std::filesystem::path& logPath) {
222+
const std::vector<std::string> argv{"cmd.exe", "/c", std::string(command)};
214223
SECURITY_ATTRIBUTES sa{sizeof(sa), nullptr, TRUE};
215224
HANDLE log = ::CreateFileA(logPath.string().c_str(), GENERIC_WRITE,
216225
FILE_SHARE_READ, &sa, CREATE_ALWAYS,
@@ -268,8 +277,9 @@ bool spawn_detached(const std::vector<std::string>& argv) {
268277
return rc == 0;
269278
}
270279

271-
int run_to_completion(const std::vector<std::string>& argv,
280+
int run_to_completion(std::string_view command,
272281
const std::filesystem::path& logPath) {
282+
const std::vector<std::string> argv{"/bin/sh", "-c", std::string(command)};
273283
posix_spawn_file_actions_t fa;
274284
::posix_spawn_file_actions_init(&fa);
275285
::posix_spawn_file_actions_addopen(&fa, 0, "/dev/null", O_RDONLY, 0);
@@ -292,7 +302,7 @@ int run_to_completion(const std::vector<std::string>& argv,
292302
} // namespace
293303

294304
int compile_release_at_bmi(const CompileRequest& req) {
295-
if (req.argv.empty() || req.self.empty()) return 2;
305+
if (req.command.empty() || req.self.empty()) return 2;
296306

297307
std::error_code ec;
298308
if (!req.slot.parent_path().empty())
@@ -315,7 +325,7 @@ int compile_release_at_bmi(const CompileRequest& req) {
315325
// on how long a compiler command may be.
316326
std::vector<std::string> sup{req.self.string(), "bmi-supervise",
317327
"--slot", req.slot.string(),
318-
"--argv-file", req.argvFile.string()};
328+
"--command-file", req.commandFile.string()};
319329
if (!token.empty()) { sup.push_back("--token"); sup.push_back(token.string()); }
320330
if (!spawn_detached(sup)) return 2;
321331

@@ -337,8 +347,8 @@ int compile_release_at_bmi(const CompileRequest& req) {
337347

338348
int supervise(const std::filesystem::path& slot,
339349
const std::filesystem::path& semaphoreToken,
340-
const std::vector<std::string>& argv) {
341-
const int rc = run_to_completion(argv, suffixed(slot, ".log"));
350+
std::string_view command) {
351+
const int rc = run_to_completion(command, suffixed(slot, ".log"));
342352
if (!semaphoreToken.empty()) {
343353
std::error_code ec;
344354
std::filesystem::remove(semaphoreToken, ec);

src/cli.cppm

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -634,13 +634,13 @@ int run(int argc, char** argv) {
634634
.option(cl::Option("self").takes_value().value_name("PATH").help("path to mcpp, re-invoked as supervisor"))
635635
.option(cl::Option("sem").takes_value().value_name("DIR").help("concurrency token directory"))
636636
.option(cl::Option("cap").takes_value().value_name("N").help("max concurrent compilers"))
637-
.option(cl::Option("argv-file").takes_value().value_name("PATH").help("compiler command, one argument per line"))
637+
.option(cl::Option("command-file").takes_value().value_name("PATH").help("file holding the compiler command line"))
638638
.action(wrap_rc(cmd_bmi_compile)))
639639
.subcommand(cl::App("bmi-supervise")
640640
.description("(internal) Run a compiler to completion and record its status")
641641
.option(cl::Option("slot").takes_value().value_name("PATH"))
642642
.option(cl::Option("token").takes_value().value_name("PATH"))
643-
.option(cl::Option("argv-file").takes_value().value_name("PATH"))
643+
.option(cl::Option("command-file").takes_value().value_name("PATH"))
644644
.action(wrap_rc(cmd_bmi_supervise)))
645645
.subcommand(cl::App("bmi-await")
646646
.description("(internal) Join a detached compiler and replay its diagnostics")

src/cli/cmd_build.cppm

Lines changed: 14 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -499,14 +499,13 @@ namespace {
499499
// list rather than an error. A file also sidesteps MAX_ARG_STRLEN (128 KiB for
500500
// a single argv entry, which mcpp has hit before on link lines) and needs no
501501
// quoting rules that a compiler flag could violate.
502-
std::vector<std::string> read_argv_file(const std::filesystem::path& path) {
503-
std::vector<std::string> out;
504-
std::ifstream in(path);
505-
for (std::string line; std::getline(in, line);) {
506-
if (!line.empty() && line.back() == '\r') line.pop_back();
507-
if (!line.empty()) out.push_back(std::move(line));
508-
}
509-
return out;
502+
std::string read_command_file(const std::filesystem::path& path) {
503+
std::ifstream in(path, std::ios::binary);
504+
if (!in) return {};
505+
std::string text((std::istreambuf_iterator<char>(in)),
506+
std::istreambuf_iterator<char>());
507+
while (!text.empty() && (text.back() == '\n' || text.back() == '\r')) text.pop_back();
508+
return text;
510509
}
511510

512511
// `option_or_empty(...).value()`, the idiom the rest of this file uses.
@@ -528,14 +527,14 @@ export int cmd_bmi_compile(const mcpplibs::cmdline::ParsedArgs& parsed) {
528527
req.maxCompilers = 0;
529528
if (const auto cap = opt_value(parsed, "cap"); !cap.empty())
530529
std::from_chars(cap.data(), cap.data() + cap.size(), req.maxCompilers);
531-
req.argvFile = std::filesystem::path{opt_value(parsed, "argv-file")};
532-
req.argv = read_argv_file(req.argvFile);
530+
req.commandFile = std::filesystem::path{opt_value(parsed, "command-file")};
531+
req.command = read_command_file(req.commandFile);
533532
if (req.slot.empty()) {
534533
std::println(stderr, "error: bmi-compile needs --slot");
535534
return 2;
536535
}
537-
if (req.argv.empty()) {
538-
std::println(stderr, "error: bmi-compile got no compiler command from --argv-file");
536+
if (req.command.empty()) {
537+
std::println(stderr, "error: bmi-compile got no command from --command-file");
539538
return 2;
540539
}
541540
return mcpp::build::schedule::detach::compile_release_at_bmi(req);
@@ -545,9 +544,9 @@ export int cmd_bmi_compile(const mcpplibs::cmdline::ParsedArgs& parsed) {
545544
export int cmd_bmi_supervise(const mcpplibs::cmdline::ParsedArgs& parsed) {
546545
const std::filesystem::path slot{opt_value(parsed, "slot")};
547546
const std::filesystem::path token{opt_value(parsed, "token")};
548-
const auto argv = read_argv_file(std::filesystem::path{opt_value(parsed, "argv-file")});
549-
if (slot.empty() || argv.empty()) return 2;
550-
return mcpp::build::schedule::detach::supervise(slot, token, argv);
547+
const auto command = read_command_file(std::filesystem::path{opt_value(parsed, "command-file")});
548+
if (slot.empty() || command.empty()) return 2;
549+
return mcpp::build::schedule::detach::supervise(slot, token, command);
551550
}
552551

553552
// Phase 2: join the detached compiler before anything reads its object.

tests/unit/test_loader_contract.cpp

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -82,12 +82,33 @@ TEST(GraphShape, HeaderAndReaderAgree) {
8282
~Cleanup() { std::error_code ec; std::filesystem::remove_all(d, ec); } }
8383
cleanup{dir};
8484

85+
// The line now carries the module-edge schedule too. Round-tripping both
86+
// fields together is the point: the schedule was added to this line rather
87+
// than to a second file precisely so the two cannot disagree.
8588
for (auto shape : {GraphShape::Normal, GraphShape::WithTests}) {
89+
for (std::string_view sched : {"none", "two-phase", "detach-codegen"}) {
90+
auto p = dir / "build.ninja";
91+
{ std::ofstream out(p, std::ios::trunc);
92+
out << header_line(shape, sched) << "\n"; }
93+
auto read = read_shape(p);
94+
ASSERT_TRUE(read.has_value());
95+
EXPECT_EQ(*read, shape);
96+
EXPECT_EQ(read_schedule(p), sched);
97+
}
98+
}
99+
100+
// A graph written before the schedule field existed still reads as its
101+
// shape — an older file must degrade, not become "unknown" — but its
102+
// schedule reads as empty, which is NOT "none": callers that care have to
103+
// be able to tell "this file predates the field" from "this file chose to
104+
// do nothing".
105+
{
86106
auto p = dir / "build.ninja";
87-
{ std::ofstream out(p, std::ios::trunc); out << header_line(shape) << "\n"; }
107+
{ std::ofstream out(p, std::ios::trunc); out << "# mcpp:graph=normal\n"; }
88108
auto read = read_shape(p);
89109
ASSERT_TRUE(read.has_value());
90-
EXPECT_EQ(*read, shape);
110+
EXPECT_EQ(*read, GraphShape::Normal);
111+
EXPECT_TRUE(read_schedule(p).empty());
91112
}
92113
}
93114

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
// The build-shape policy: one table, asserted from both sides.
2+
//
3+
// `decide()` is pure precisely so this file needs no toolchain, no filesystem
4+
// and no compiler — the table can be wrong in a way that only shows up as a
5+
// slower build, which is the kind of wrong that never gets noticed.
6+
7+
#include <gtest/gtest.h>
8+
9+
import std;
10+
import mcpp.build.schedule.policy;
11+
import mcpp.toolchain.model;
12+
13+
using mcpp::build::schedule::Strategy;
14+
using mcpp::build::schedule::decide;
15+
using mcpp::toolchain::CompilerId;
16+
using mcpp::toolchain::Toolchain;
17+
18+
namespace {
19+
Toolchain with(CompilerId id) {
20+
Toolchain tc;
21+
tc.compiler = id;
22+
return tc;
23+
}
24+
} // namespace
25+
26+
// The two mechanisms are COMPLEMENTARY, not interchangeable, and getting them
27+
// backwards is silent: clang writes its BMI to the final path with O_TRUNC, so
28+
// detach-codegen would hand importers a half-written file; gcc has no cheap
29+
// BMI-only mode, so two-phase would just compile everything twice.
30+
TEST(SchedulePolicy, EachCompilerGetsItsOwnMechanism) {
31+
EXPECT_EQ(decide(with(CompilerId::Clang), "auto", 8).strategy, Strategy::TwoPhase);
32+
EXPECT_EQ(decide(with(CompilerId::GCC), "auto", 8).strategy, Strategy::DetachCodegen);
33+
}
34+
35+
// Unmeasured means None. A guess here is not a slow build, it is a miscompile:
36+
// a BMI read while it is still being written is not a diagnostic.
37+
TEST(SchedulePolicy, UnmeasuredCompilersStayConservative) {
38+
EXPECT_EQ(decide(with(CompilerId::MSVC), "auto", 8).strategy, Strategy::None);
39+
EXPECT_EQ(decide(with(CompilerId::Unknown), "auto", 8).strategy, Strategy::None);
40+
}
41+
42+
// Asserted from BOTH sides: that "off" disables, and that the same input with
43+
// "auto" does NOT. Checking only the first would pass an implementation that
44+
// never enables anything at all.
45+
TEST(SchedulePolicy, OffDisablesAndAutoDoesNot) {
46+
EXPECT_EQ(decide(with(CompilerId::GCC), "off", 8).strategy, Strategy::None);
47+
EXPECT_NE(decide(with(CompilerId::GCC), "auto", 8).strategy, Strategy::None);
48+
}
49+
50+
// HAZARD 2, encoded. Under detach-codegen a compiler stops holding a ninja slot
51+
// the moment it publishes its BMI, so ninja's -j is no longer a bound on how
52+
// many compilers run. With the two equal, ninja's slots fill with edges that
53+
// are merely sleeping, the ready frontier starves, and the schedule degenerates
54+
// to the baseline — which is exactly what the first prototype measured.
55+
TEST(SchedulePolicy, DetachCodegenGivesNinjaMoreSlotsThanCompilers) {
56+
const auto d = decide(with(CompilerId::GCC), "auto", 32);
57+
EXPECT_EQ(d.compilerCap, 32);
58+
EXPECT_GT(d.ninjaJobs, d.compilerCap);
59+
}
60+
61+
// Two-phase runs ordinary compilers that hold their slot for the whole compile,
62+
// so inflating -j there would only oversubscribe the machine.
63+
TEST(SchedulePolicy, TwoPhaseLeavesTheJobCountAlone) {
64+
const auto d = decide(with(CompilerId::Clang), "auto", 32);
65+
EXPECT_EQ(d.ninjaJobs, d.compilerCap);
66+
}
67+
68+
// A scheduler that silently declines to optimise cannot be debugged: "why is my
69+
// build not using the fast shape?" has to have an answer that ships with the
70+
// build. Every branch, including the ones that choose None.
71+
TEST(SchedulePolicy, EveryDecisionCarriesAReason) {
72+
for (auto id : {CompilerId::GCC, CompilerId::Clang, CompilerId::MSVC,
73+
CompilerId::Unknown}) {
74+
EXPECT_FALSE(decide(with(id), "auto", 8).reason.empty())
75+
<< "no reason for compiler id " << static_cast<int>(id);
76+
EXPECT_FALSE(decide(with(id), "off", 8).reason.empty())
77+
<< "no reason when disabled, compiler id " << static_cast<int>(id);
78+
}
79+
}
80+
81+
// A host that reports nothing must not turn into "-j0" or a negative cap.
82+
TEST(SchedulePolicy, ZeroJobsStaysZeroRatherThanBecomingNonsense) {
83+
const auto d = decide(with(CompilerId::GCC), "auto", 0);
84+
EXPECT_EQ(d.compilerCap, 0);
85+
EXPECT_EQ(d.ninjaJobs, 0);
86+
}

0 commit comments

Comments
 (0)