diff --git a/.agents/roadmap_v1.md b/.agents/roadmap_v1.md index cb1e35106..f0cc882d5 100644 --- a/.agents/roadmap_v1.md +++ b/.agents/roadmap_v1.md @@ -158,6 +158,7 @@ issue is not yet placed. Keyed record: update in place, never append. | [#615](https://github.com/mudler/vllm.cpp/issues/615) | `GATE-PR-SIZE-BINARY` | `check-pr-size` fail-closes on every binary path with no exemption route, so no golden-bearing PR can merge: it blocks #431 and post-dates the golden precedent it rejects, spec [`gate-pr-size-binary.md`](specs/gate-pr-size-binary.md) | bug | | [#670](https://github.com/mudler/vllm.cpp/issues/670) | `BACKEND-TENSTORRENT-MISTRAL` | Tenstorrent: allowlist `MistralForCausalLM` and gate it on-device; goldens are `transformers`-teacher-forced because vLLM has no TT backend, spec [`tenstorrent-mistral.md`](specs/tenstorrent-mistral.md) | feature | | [#773](https://github.com/mudler/vllm.cpp/issues/773) | `GATE-FORK-ANCESTRY` | `check-pr-size` and `check-commit-trailers` abort on every fork PR because the base SHA stops being an ancestor once main moves, so CI has never validated an outside contributor's trailers, spec [`gate-fork-ancestry.md`](specs/gate-fork-ancestry.md) | bug | +| [#774](https://github.com/mudler/vllm.cpp/issues/774) | `ENG-RELEASE-WINDOWS` | `check-windows-portability.py:1710` asserted the MSVC warning policy with `token in warnings`, and `"/WX" in "/WX-"` is `True` — `/WX-` is MSVC's spelling for DISABLE warnings-as-errors, so the gate was blind to its own inversion. Measured on PR #640 commit `74ba3823f`, which shipped `/WX-` on the CXX arm while the only bare `/WX` left was on `$` — Objective-C++, the Metal backend, which never compiles under MSVC. Two further blindnesses fell out of the same `in`: `/W44996` answers for `/W4`, and `CMakeLists.txt:30`'s `#` comment satisfies the whole policy on its own. Repaired to a token-boundary match over the flags that reach the C/C++ compile, plus the negating spellings `/WX-` `/W0` `/w`; spec [`windows-msvc-warning-policy-tokens.md`](specs/windows-msvc-warning-policy-tokens.md) | bug | | [#238](https://github.com/mudler/vllm.cpp/issues/238) | `SAMPLE-LOGPROB-TOKEN-IDS` | `logprobs_mode`: three of four modes are runtime-refused stubs | bug | | [#264](https://github.com/mudler/vllm.cpp/issues/264) | `SAMPLE-LOGPROB-TOKEN-IDS` | `logprob_token_ids`: generative scoring over an explicit token set is unported | feature | | [#365](https://github.com/mudler/vllm.cpp/issues/365) | `PERF-27B-DENSE-MARLIN-GATEUP` | 27B gap decomposed vs vLLM's own decode profile; dense W4A16 MLP bypassed the fused gate_up seam (spec `specs/perf-27b-dense-marlin-gateup.md`) | bug | diff --git a/.agents/specs/windows-msvc-warning-policy-tokens.md b/.agents/specs/windows-msvc-warning-policy-tokens.md new file mode 100644 index 000000000..08d96c7c9 --- /dev/null +++ b/.agents/specs/windows-msvc-warning-policy-tokens.md @@ -0,0 +1,222 @@ +# MSVC `/W4 /WX` policy — assert the TOKEN on the C/C++ compile, not a substring anywhere in the file + +Identity: `ENG-RELEASE-WINDOWS` + +Issue: [#774](https://github.com/mudler/vllm.cpp/issues/774) + +Parent specification: [windows-binary-release.md](windows-binary-release.md) + +Related repair: [windows-msvc-strict-build.md](windows-msvc-strict-build.md), +which established the strict warning policy this checker line exists to hold. + +Status: `ACTIVE`. Base `af026e5241e16e1d2a89da001b978ffcb3e0f634`. + +## Scope + +Replace the substring assertion at `scripts/check-windows-portability.py:1710` +with a token-boundary assertion evaluated over the flags that actually reach an +MSVC **C/C++** compile. + +In scope: + +1. Token boundaries for `/W4` and `/WX`, so a longer flag that merely *contains* + the token no longer satisfies the policy. +2. Language scoping, so an occurrence confined to a non-C/C++ `COMPILE_LANGUAGE` + generator expression (OBJCXX, CUDA, ...) cannot satisfy the policy. +3. Comment stripping, so the policy cannot be satisfied by prose. +4. Rejecting the *negating spellings of the two tokens the policy names* when + they reach the C/C++ compile: `/WX-`, `/W0`, `/w`. + +Explicitly excluded, argued in **Sibling evasions** below: the +`COMPILE_WARNING_AS_ERROR` target/cache property, and per-warning `/wd####` +suppressions. + +Nothing outside `scripts/check-windows-portability.py`, +`tests/scripts/test_check_windows_portability.py`, this spec and the roadmap +issue table changes. `cmake/CompilerWarnings.cmake` is **not** edited: the tree +already satisfies the repaired policy, and a checker repair that also moves the +thing it checks cannot show that it holds. + +## Observed baseline and root cause + +`scripts/check-windows-portability.py:1710` at base: + +```python +if not all(token in warnings for token in ("/W4", "/WX")): + errors.append("CMakeLists.txt: MSVC /W4 /WX policy is required") +``` + +`warnings` is the concatenation of `CMakeLists.txt` and +`cmake/CompilerWarnings.cmake`, unparsed. Three independent blindnesses follow +from `in`: + +- **Inversion.** `"/WX" in "/WX-"` is `True`. `/WX-` is MSVC's spelling for + *disable* warnings-as-errors — the exact inversion of the policy. + `"/W4" in "/W44996"` is `True` too, and `/W44996` sets warning C4996 to level + 4 rather than raising the warning level. +- **Language.** The test asks only whether the bytes occur *somewhere*. A `/WX` + reachable only through `$<$:...>` is inert under + MSVC — Objective-C++ is the Metal backend and never compiles on Windows — yet + satisfies the policy. +- **Prose.** `CMakeLists.txt:30` contains the literal text `/W4 /WX` inside a + `#` comment. That comment alone satisfies the base assertion, so the policy + survives deleting every real flag in the tree. + +The measured demonstration is PR +[#640](https://github.com/mudler/vllm.cpp/pull/640) commit `74ba3823f`, whose +`cmake/CompilerWarnings.cmake` used all three at once (reverted in `15aa963a6` +after review): + +```cmake + if(MSVC) + set_property(TARGET ${target} PROPERTY COMPILE_WARNING_AS_ERROR OFF) + target_compile_options(${target} PRIVATE + $<$:/W4> + $<$:/WX-> + $<$:/utf-8> + $<$:/wd4324> + $<$:/wd4458> + $<$:/W4> + $<$:/WX> + $<$:-Werror=all-warnings>) + endif() +``` + +The only bare `/WX` left is on OBJCXX. The gate passes. + +## Design + +Two helpers next to the existing CMake-text helpers +(`without_set_source_properties`, `source_properties`), then a rewritten +assertion. + +`msvc_cxx_flag_text(text)` returns the flag text that can reach an MSVC C/C++ +translation unit: + +- `#` comments removed, using the same `re.sub(r"(?m)#.*$", "", ...)` idiom + `_has_central_msvc_nominmax` already uses. +- Every generator expression whose `COMPILE_LANGUAGE` mentions name only + languages outside `{C, CXX}` blanked. Spans come from a `$<` / `>` stack scan, + so `$<$:/WX>` is blanked whole, `$<$:/W4 /WX>` + is kept whole, and a genex naming no language at all (`$<$:/WX>`) + is kept, because it does reach C/C++. Blanking preserves offsets, matching + `without_set_source_properties`. + +`has_msvc_flag(text, flag)` matches the flag with boundaries on both sides: +`(?` is handled by + collecting *every* `COMPILE_LANGUAGE` mention inside a span; a span with no + mention is conservatively kept, so the failure direction is "counts as + reaching C/C++", never "silently excluded". +- **Comment stripping.** `#` inside a CMake string would be stripped early. The + only consequence is a false RED on the warning policy, which is visible, and + the repository has no such line. + +## Tests + +`tests/scripts/test_check_windows_portability.py`, all through the existing +`assert_rejected` / `run_checker` harness so they exercise the real checker +process: + +1. `/WX-` on the CXX arm is rejected (the #774 headline). +2. The #640 `74ba3823f` shape — `/WX-` on CXX with the only bare `/WX` on + OBJCXX — is rejected. +3. `/W44996` and `/WXsomething` do not satisfy the policy. +4. `/W4 /WX` present but cancelled by a later `/w` is rejected. +5. Flags present only inside a `#` comment are rejected. +6. Inverse pins: the honest `$<$:/W4 /WX>` genex, the + bare `add_compile_options(/W4 /WX)` fixture, and a `$<$:...>` + genex all pass. +7. The real tree's `cmake/CompilerWarnings.cmake` satisfies the repaired policy + with the OBJCXX arm excluded and no negation present. + +Every one of 1-5 and 7 must FAIL against the base checker; that is the +red-before evidence, and it is also what `check-pr-size.py` re-executes for a +`governance_checker` change. + +## Gates + +- `scripts/agent-preflight.sh` +- `python3 -m pytest tests/scripts/ --ignore=tests/scripts/test_cpu_kernel_bench.py` +- `python3 scripts/check-windows-portability.py` against the real tree (green + before and after — this repair does not change the verdict on `main`). + +No build, no GPU, no benchmark: this is a source-contract checker. + +## Evidence + +Base checker, run over the real `CMakeLists.txt` plus a substituted +`cmake/CompilerWarnings.cmake`, evaluating the assertion exactly as line 1710 +spells it: + +``` +PASS BASE (main: honest /W4 /WX on CXX) +PASS PR#640@74ba3823f (/WX- on CXX; bare /WX only on OBJCXX) +PASS decoy: /WX- appended on CXX after the real /WX +PASS decoy: /w (all warnings off) appended on CXX +PASS substring-only tokens: /W44996 and /WXsomething +``` + +Four of those five are policy inversions and the gate cannot see any of them. +The per-test red-before and green-after runs are recorded in the PR body. + +## Stop conditions + +- If closing the language scope turns the real tree RED, stop and report: the + tree, not the checker, would then be the finding. +- If a shape outside `$<$:...>` is needed to decide + reachability, stop and return `NEEDS_DECISION` rather than growing a CMake + generator-expression evaluator inside a portability checker. + +## Now + +`ENG-RELEASE-WINDOWS` does not change lifecycle state. This is a checker repair +inside an already-`ACTIVE` row; no `STATUS`/`BENCHMARKS` projection is owed. diff --git a/scripts/check-windows-portability.py b/scripts/check-windows-portability.py index 6eb92607d..557c1d3ad 100644 --- a/scripts/check-windows-portability.py +++ b/scripts/check-windows-portability.py @@ -276,6 +276,72 @@ def windows_possible_lines(text: str): yield number, line +C_FAMILY_COMPILE_LANGUAGES = {"C", "CXX"} + + +def _generator_expression_spans(text: str) -> list[tuple[int, int]]: + """Return (start, end) for every balanced `$<...>` generator expression.""" + spans: list[tuple[int, int]] = [] + stack: list[int] = [] + index = 0 + while index < len(text): + if text.startswith("$<", index): + stack.append(index) + index += 2 + continue + if text[index] == ">" and stack: + spans.append((stack.pop(), index + 1)) + index += 1 + return spans + + +def msvc_cxx_flag_text(cmake_text: str) -> str: + """Return the flag text that can reach an MSVC C/C++ translation unit. + + Two things are removed, both of which satisfied the old substring test + without compiling anything (#774): + + * `#` comments. `CMakeLists.txt` says `/W4 /WX` in prose, and a policy + cannot be satisfied by prose. + * Generator expressions whose `COMPILE_LANGUAGE` names only languages + outside C/C++. `$<$:/WX>` is the Metal backend, + which never compiles under MSVC, so it answers for nothing here. A + generator expression naming no language at all is KEPT: it does reach + C/C++. + + Spans are blanked rather than cut so reported offsets stay meaningful, + matching `without_set_source_properties`. + """ + active = re.sub(r"(?m)#.*$", "", cmake_text) + out = list(active) + for start, end in _generator_expression_spans(active): + languages: set[str] = set() + for mention in re.finditer( + r"COMPILE_LANGUAGE\s*:\s*([^>]*)", active[start:end] + ): + languages |= { + name.strip().upper() + for name in mention.group(1).split(",") + if name.strip() + } + if languages and not (languages & C_FAMILY_COMPILE_LANGUAGES): + out[start:end] = " " * (end - start) + return "".join(out) + + +def has_msvc_flag(text: str, flag: str) -> bool: + """Return whether `flag` appears as a WHOLE token, not as a substring. + + `"/WX" in "/WX-"` is True and `/WX-` DISABLES warnings-as-errors; `"/W4" in + "/W44996"` is True and `/W44996` sets one warning to level 4 rather than + raising the level. Both satisfied the old test (#774). Matching stays + case-sensitive because `cl` is: `/w` and `/W4` are different flags. + """ + return re.search( + rf"(? str: """Remove balanced set_source_files_properties commands.""" lowered = text.lower() @@ -1708,8 +1774,30 @@ def check(root: Path, build_dir: Path | None = None, global_options = without_set_source_properties(cmake) if re.search(r"(?i)/arch\s*:\s*AVX2", global_options): errors.append("CMakeLists.txt: global /arch:AVX2 contaminates the portable baseline") - if not all(token in warnings for token in ("/W4", "/WX")): - errors.append("CMakeLists.txt: MSVC /W4 /WX policy is required") + # Asserted on the flags that reach the C/C++ compile, by TOKEN. A substring + # test passed a tree whose CXX arm said `/WX-` because the only bare `/WX` + # left was on `$` (#774). + cxx_warning_flags = msvc_cxx_flag_text(warnings) + missing = [ + flag for flag in ("/W4", "/WX") + if not has_msvc_flag(cxx_warning_flags, flag) + ] + if missing: + errors.append( + "CMakeLists.txt: MSVC /W4 /WX policy is required on the C/C++ " + f"compile; missing {' '.join(missing)}" + ) + # The disable spellings of the SAME two flags. Present-and-cancelled is a + # different repair from absent, so it is a different error. + negated = [ + flag for flag in ("/WX-", "/W0", "/w") + if has_msvc_flag(cxx_warning_flags, flag) + ] + if negated: + errors.append( + "CMakeLists.txt: MSVC /W4 /WX policy is negated on the C/C++ " + f"compile by {' '.join(negated)}" + ) if re.search(r"__attribute__\s*\(\(\s*target\s*\(\s*\"f16c\"", cpu_baseline): errors.append("cpu_matmul_elem.cpp: F16C must be isolated in a dedicated translation unit") f16c_properties = source_properties(cmake, "src/vt/cpu/cpu_matmul_elem_f16c.cpp") diff --git a/tests/scripts/test_check_windows_portability.py b/tests/scripts/test_check_windows_portability.py index a5202a936..fc4902a2d 100644 --- a/tests/scripts/test_check_windows_portability.py +++ b/tests/scripts/test_check_windows_portability.py @@ -33,6 +33,11 @@ ) +# The single MSVC warning arm of the safe fixture, replaced wholesale by the +# #774 warning-policy tests. Anchored by count, never by "it is in there". +MSVC_WARNING_ARM = "add_compile_options(/fp:strict /W4 /WX)" + + SAFE_FILES = { "CMakeLists.txt": """ set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreaded$<$:Debug>") @@ -315,6 +320,99 @@ def test_real_tree_declares_strict_msvc_platform_contract(self) -> None: with self.subTest(token=token): self.assertIn(token, contract) + def msvc_warning_arm(self, replacement: str) -> str: + """Return the safe CMakeLists with its MSVC warning arm replaced.""" + cmake = textwrap.dedent(SAFE_FILES["CMakeLists.txt"]) + if cmake.count(MSVC_WARNING_ARM) != 1: + raise AssertionError("expected exactly one MSVC warning arm") + return cmake.replace(MSVC_WARNING_ARM, replacement) + + def test_real_tree_msvc_warning_policy_reaches_the_cxx_compile(self) -> None: + """#774: the tokens must survive the reader the checker actually uses.""" + cmake = (REPO / "CMakeLists.txt").read_text(encoding="utf-8") + warnings = (REPO / "cmake/CompilerWarnings.cmake").read_text( + encoding="utf-8" + ) + reaching = checker.msvc_cxx_flag_text(cmake + "\n" + warnings) + for flag in ("/W4", "/WX"): + with self.subTest(required=flag): + self.assertTrue(checker.has_msvc_flag(reaching, flag)) + for flag in ("/WX-", "/W0", "/w"): + with self.subTest(negation=flag): + self.assertFalse(checker.has_msvc_flag(reaching, flag)) + # The prose in CMakeLists.txt is NOT what answers for the policy. + self.assertIn("/W4 /WX remains unchanged", cmake) + self.assertFalse( + checker.has_msvc_flag(checker.msvc_cxx_flag_text(cmake), "/WX") + ) + + def test_rejects_warnings_as_errors_disabled_by_wx_minus(self) -> None: + """`"/WX" in "/WX-"` is True, and /WX- is the INVERSE policy (#774).""" + disabled = self.msvc_warning_arm( + "add_compile_options(/fp:strict /W4 /WX-)" + ) + self.assert_rejected("CMakeLists.txt", disabled, "negated on the C/C++") + self.assert_rejected("CMakeLists.txt", disabled, "missing /WX") + + def test_rejects_policy_satisfied_only_on_objcxx(self) -> None: + """The measured PR #640 shape at commit 74ba3823f (#774). + + Objective-C++ is the Metal backend and never compiles under MSVC, so a + bare /WX confined to it answers for nothing on Windows. + """ + objcxx_only = self.msvc_warning_arm( + "set_property(TARGET vllm PROPERTY COMPILE_WARNING_AS_ERROR OFF)\n" + "add_compile_options(/fp:strict\n" + " $<$:/W4>\n" + " $<$:/WX->\n" + " $<$:/W4>\n" + " $<$:/WX>)" + ) + self.assert_rejected("CMakeLists.txt", objcxx_only, "missing /WX") + self.assert_rejected("CMakeLists.txt", objcxx_only, "negated on the C/C++") + + def test_rejects_prefix_lookalike_warning_flags(self) -> None: + """/W44996 sets ONE warning to level 4; /WXsomething is not /WX.""" + lookalikes = self.msvc_warning_arm( + "add_compile_options(/fp:strict /W44996 /WXsomething)" + ) + self.assert_rejected("CMakeLists.txt", lookalikes, "missing /W4 /WX") + + def test_rejects_warning_level_disabled_alongside_the_policy(self) -> None: + """/w is the disable spelling of /W4 and wins as the later flag.""" + self.assert_rejected( + "CMakeLists.txt", + self.msvc_warning_arm("add_compile_options(/fp:strict /W4 /WX /w)"), + "negated on the C/C++ compile by /w", + ) + + def test_rejects_policy_declared_only_in_a_comment(self) -> None: + self.assert_rejected( + "CMakeLists.txt", + self.msvc_warning_arm("# the MSVC policy is /W4 /WX"), + "missing /W4 /WX", + ) + + def test_accepts_the_policy_on_every_shape_that_reaches_cxx(self) -> None: + """The inverse pin: repaired does not mean stricter about everything.""" + for label, arm in ( + ("cxx genex", "add_compile_options($<$:/W4 /WX>)"), + ("c and cxx", "add_compile_options($<$:/W4 /WX>)"), + ("config genex", + "add_compile_options($<$:/W4> $<$:/WX>)"), + # Targeted suppressions are a DELIBERATE non-goal of #774: they + # narrow what /W4 reports, they do not invert /W4 or /WX. + ("targeted suppression", + "add_compile_options(/fp:strict /W4 /WX /wd4324)"), + ): + with self.subTest(shape=label): + result = self.run_checker( + self.make_tree({"CMakeLists.txt": self.msvc_warning_arm(arm)}) + ) + self.assertEqual( + result.returncode, 0, result.stdout + result.stderr + ) + def test_real_tree_has_no_unguarded_local_nominmax_redefinitions(self) -> None: cmake = (REPO / "CMakeLists.txt").read_text(encoding="utf-8") self.assertRegex( @@ -1780,9 +1878,21 @@ def test_real_unsupported_tier_helper_is_structurally_scoped(self) -> None: ), "exact unsupported-tier probe body", )) + # The checker reads `$calls.Add(` inside the `$good` scriptblock of + # Invoke-UnsupportedTierContractTests (`good_runner`), and NOWHERE + # else. `script.replace(..., 1)` mutates the first occurrence in the + # file, which since #583 added Invoke-CheckedContractTests (#512) is a + # DIFFERENT function the checker never looks at -- so the checker + # stayed green and this case has been failing on main ever since + # (#680). Anchor the mutation to the governed occurrence, and assert + # that occurrence is unique rather than assuming it. + governed_at = script.index("function Invoke-UnsupportedTierContractTests") + self.assertEqual(script[governed_at:].count("$calls.Add("), 1) mutations.extend(( ( - script.replace("$calls.Add(", "$calls.Append(", 1), + script[:governed_at] + script[governed_at:].replace( + "$calls.Add(", "$calls.Append(", 1 + ), "fake runner call recording", ), (