From c6cee9eb23fe17e375163098b882547c9bc0e538 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Thu, 13 Aug 2026 04:00:19 +0000 Subject: [PATCH] fix(tests): the Windows arm cannot compile test_backend_cross_device (#514, #540) FOLLOWING_AGENTS_PROTOCOL `windows-msvc-vulkan` fails on EVERY pull request, and because both Windows jobs are `if: github.event_name == 'pull_request'` (.github/workflows/ci.yml :613, :637) they are skipped on `main` pushes -- so `main` reads green while every PR opened against it reads red. Confirmed on three unrelated PRs (#566, #568, #570), all failing the same pair and nothing else. The job's single compiling translation unit is this file, and it fails TWICE: test_backend_cross_device.cpp(513,20): error C2220 <- C4456 x6, /W4 /WX test_backend_cross_device.cpp(1004,5): error C3861: 'setenv' test_backend_cross_device.cpp(1048,5): error C3861: 'setenv' test_backend_cross_device.cpp(1050,5): error C3861: 'unsetenv' Fixing only the C3861s leaves the lane red on the C2220, so both are repaired here; they are the same compile of the same file. #514 -- MSVC has neither `setenv` nor `unsetenv`. Route the three call sites through file-local `SetEnv`/`UnsetEnv`, mirroring the existing idiom at tests/vllm/test_gguf.cpp:83-92 (`#if defined(_WIN32)` / `_putenv_s`, POSIX otherwise). SET and UNSET stay SEPARATE entry points rather than folding unset into `SetEnv(name, "")`: the restore arm needs absence, and collapsing the two would silently turn "absent" into "empty". The unset semantics are the thing worth proving, not assuming. On POSIX, `::unsetenv` is the byte-identical call the file already made. On MSVC, `_putenv_s(name, "")` is the documented removal form. That documentation is not executable here, so the safety does not rest on it: `vt::FusedTier()` (include/vt/fused_recipe.h:175-178, `e != nullptr && e[0] == '1'`) is the ONLY reader of VT_FUSED_TIER in the tree, and it returns 0 for absent AND for empty -- so even the pathological "left an empty value behind" outcome is observationally identical. Verified by running both properties. #540 -- the unbind-flash-layout CPU-oracle block redeclared six names from its enclosing test (`cpu`, `cq`, `cd`, `ck`, `cv`, `cslots`). Renamed the INNER ones with an `unbind_` prefix. No data, shape, stride, call or assertion moves. Behavior and coverage are unchanged on Linux: 19 test cases, 3 assertions, Status SUCCESS before and after (Release, CUDA/Vulkan OFF, gcc 13.3.0, disk 85% used). Mutating `SetEnv` to a no-op turns it RED -- 18 passed | 1 failed, Status FAILURE -- so the two `REQUIRE(vt::FusedTier() == tier)` assertions do guard the set path; the tree was then restored byte-for-byte (md5 verified). The Windows arm cannot be compiled on this Linux box and is NOT claimed here; CI is the proof. Refs #514, #540. Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code] --- tests/vt/test_backend_cross_device.cpp | 68 ++++++++++++++++++++------ 1 file changed, 53 insertions(+), 15 deletions(-) diff --git a/tests/vt/test_backend_cross_device.cpp b/tests/vt/test_backend_cross_device.cpp index a56116eb4..12410aaee 100644 --- a/tests/vt/test_backend_cross_device.cpp +++ b/tests/vt/test_backend_cross_device.cpp @@ -34,6 +34,12 @@ #include #include +#if defined(_WIN32) +// _putenv_s lives in . is only REQUIRED to declare the +// std:: names, so do not rely on it dragging the MSVC-specific one in (#514). +#include +#endif + #include "vt/backend.h" #include "vt/op_provider.h" #include "vt/ops.h" @@ -152,6 +158,32 @@ Tensor TI64(void* p, Device d, int64_t n) { return Tensor::Contiguous(p, DType::kI64, d, {n}); } +// Environment seam (#514). MSVC has NEITHER setenv NOR unsetenv, so the bare +// POSIX calls this file used broke the native Windows build with C3861 before +// any gate ran. Same file-local shape as tests/vllm/test_gguf.cpp:83-92, but +// SET and UNSET stay SEPARATE entry points on purpose: the tier restore below +// needs "make VT_FUSED_TIER absent again", and folding that into +// SetEnv(name, "") would silently turn absent into empty. On MSVC, passing an +// empty value to _putenv_s is the documented REMOVAL form -- getenv then +// returns nullptr, which is the semantic ::unsetenv gives us on POSIX. Return +// values are ignored to match the pre-existing call sites exactly; adding a +// REQUIRE here would change this file's assertion count. +void SetEnv(const char* name, const char* value) { +#if defined(_WIN32) + ::_putenv_s(name, value); +#else + ::setenv(name, value, /*overwrite=*/1); +#endif +} + +void UnsetEnv(const char* name) { +#if defined(_WIN32) + ::_putenv_s(name, ""); +#else + ::unsetenv(name); +#endif +} + } // namespace // --------------------------------------------------------------------------- @@ -510,15 +542,20 @@ TEST_CASE("ReshapeAndCache scatters into the KV cache BIT-EXACTLY") { } std::vector ref_comb = combined; { - vt::Backend& cpu = vt::GetBackend(DeviceType::kCPU); - Queue cq = cpu.CreateQueue(); - const Device cd{DeviceType::kCPU, 0}; - std::vector ck = knew, cv = vnew, cslots_f; - std::vector cslots = slots; - Tensor tk = Tensor::Contiguous(ck.data(), DType::kF32, cd, {kTokens, kHk, kD}); - Tensor tv = Tensor::Contiguous(cv.data(), DType::kF32, cd, {kTokens, kHk, kD}); - Tensor tcomb = - Tensor::Contiguous(ref_comb.data(), DType::kF32, cd, {kBlocks * 2 * within}); + // Names are prefixed `unbind_` so they do not SHADOW the enclosing test's + // bound-layout oracle locals of the same role (#540): MSVC /W4 /WX turns + // C4456 into an error, and the shadow was genuinely confusing to read. + vt::Backend& unbind_cpu = vt::GetBackend(DeviceType::kCPU); + Queue unbind_cq = unbind_cpu.CreateQueue(); + const Device unbind_cd{DeviceType::kCPU, 0}; + std::vector unbind_ck = knew, unbind_cv = vnew, cslots_f; + std::vector unbind_cslots = slots; + Tensor tk = + Tensor::Contiguous(unbind_ck.data(), DType::kF32, unbind_cd, {kTokens, kHk, kD}); + Tensor tv = + Tensor::Contiguous(unbind_cv.data(), DType::kF32, unbind_cd, {kTokens, kHk, kD}); + Tensor tcomb = Tensor::Contiguous(ref_comb.data(), DType::kF32, unbind_cd, + {kBlocks * 2 * within}); auto slice = [&](int which) { Tensor t = tcomb; t.data = static_cast(t.data) + @@ -534,10 +571,11 @@ TEST_CASE("ReshapeAndCache scatters into the KV cache BIT-EXACTLY") { t.stride[3] = 1; return t; }; - Tensor tsm = Tensor::Contiguous(cslots.data(), DType::kI64, cd, {kTokens}); + Tensor tsm = + Tensor::Contiguous(unbind_cslots.data(), DType::kI64, unbind_cd, {kTokens}); Tensor tkc = slice(0), tvc = slice(1); - vt::ReshapeAndCache(cq, tk, tv, tkc, tvc, tsm); - cpu.DestroyQueue(cq); + vt::ReshapeAndCache(unbind_cq, tk, tv, tkc, tvc, tsm); + unbind_cpu.DestroyQueue(unbind_cq); } for (DeviceType dt : RegisteredDevices()) { @@ -1001,7 +1039,7 @@ TEST_CASE("FusedChain matches the CPU oracle within NMSE <= 5e-4 (both tiers)") for (int tier : {0, 1}) { CAPTURE(tier); - setenv("VT_FUSED_TIER", tier == 0 ? "0" : "1", 1); + SetEnv("VT_FUSED_TIER", tier == 0 ? "0" : "1"); // ASSERT the tier actually took effect rather than trusting the log: doctest // CAPTURE is lazily stringified, so a mis-set environment would silently // run the same path twice and still look like two-tier coverage. @@ -1045,9 +1083,9 @@ TEST_CASE("FusedChain matches the CPU oracle within NMSE <= 5e-4 (both tiers)") } if (had_prev) { - setenv("VT_FUSED_TIER", saved.c_str(), 1); + SetEnv("VT_FUSED_TIER", saved.c_str()); } else { - unsetenv("VT_FUSED_TIER"); + UnsetEnv("VT_FUSED_TIER"); } }