[M1-HEAD-01] Fix Windows CI: MSVC C4996 fopen fatal under /WX in laige-run - #32
Merged
Merged
Conversation
…e-run The windows-msvc job (merge matrix) is red on the M1-HEAD-01 merge: laige-run.cpp(119): warning C4996: 'fopen' ... laige-run.cpp(119): error C2220: the following warning is treated as an error MSVC's CRT deprecates plain fopen (C4996), fatal under the engine policy's /W4 /WX (NFR-8.10, CORE-010). laige-run.cpp is new in M1-HEAD-01, so this is the first Windows build of the binary. The config-file open now uses the portable platform boundary established in logging.cpp (the M0-CORE-02 file sink), laige-bench.cpp, and the test suites: under _MSC_VER, _fsopen(path, mode, _SH_DENYNO) - the CRT's documented way to open with plain-fopen sharing semantics. fopen_s is deliberately not used: it opens with the _SH_SECURE sharing mode, which denies re-opening of the file while it is held (the windows-msvc CI runs of M0-CORE-02 read back an empty file for exactly that reason). No behavioral change: identical open/read/close sequence and error mapping (IoError on open/read failure, MalformedInput above the 1 MiB bound); the laige_run_smoke ctest entry is unchanged and still exits 0 with status=ok (verified locally on the gcc and clang trees, 47/47 ctest). Verified by this PR's ci:windows job (windows-msvc).
The first pull_request event fired when the branch was pushed, before the PR existed and before the ci:windows label was attached, so that run's windows-msvc job was skipped by the label selector. This empty commit re-fires the pull_request event with the label in place (the concurrency group cancels the label-less run).
…dows-only 4th allocation HeadlessFramePathAllocatesNothing measures allocs=4 on the windows-msvc job but 3 on every other P0 platform. This temporary test replicates Engine::run_headless(3, 1) stage by stage (schedule, loop create, first frame, snapshot create, three paced frames, the ordered shutdown tail) with allocation checkpoints, so the job log pinpoints the stage where the count moves. DELETE before merge.
…e Logger::shutdown() Round 1 localized the Windows-only fourth allocation to stage 24 (Logger::shutdown()). Round 2 installs a per-statement probe inside shutdown() (temporary setAllocProbe hook, manifest regenerated for the two temporary symbols) so the job log shows exactly which statement allocates on Windows. DELETE before merge (probe, diagnostic test, and the manifest entries).
…tion Round 2 localized it to the first statement of Logger::shutdown() (the empty std::vector<RateEntry> pending declaration). Round 3 adds empty vector<int>/vector<string>/second vector<RateEntry> discriminators and a second idempotent shutdown() call, to determine whether the allocation is element-type-specific, per-declaration, or one-shot. DELETE before merge.
…on invariant Root cause of the second windows-msvc failure (HeadlessFramePathAllocatesNothing, allocs=4 vs 3), established by three rounds of staged allocation probes on the CI runner: - The extra allocation lands in Engine shutdown's tail, specifically between the entry of Logger::shutdown() and the declaration of its local 'std::vector<RateEntry> pending;'. - The allocation is PER DECLARATION, PER CALL, for ANY element type: adding empty vector<int> / vector<string> / a second vector<RateEntry> locals each added exactly one allocation, and the second (idempotent) shutdown() call allocated the same four again. That signature is the MS STL's checked-iterator debug mode: CMake's default MSVC Debug flags define _DEBUG, which makes the STL default to _ITERATOR_DEBUG_LEVEL=2, where every STL container instance gets a heap-allocated iterator-proxy object in its constructor. The engine's window contains exactly one container declaration (the logger's shutdown local), so windows-msvc measures 4 while the other P0 platforms — GCC/Clang/AppleClang, which compile without checked iterators by default — measure the documented 3. The fix aligns the Windows Debug build with the rest of the P0 matrix: laige_apply_engine_policy() now defines _ITERATOR_DEBUG_LEVEL=0 for MSVC targets (PRIVATE, like the existing _HAS_EXCEPTIONS=0). The proxies are an STL debug aid, not engine behavior, and they are allocated in container setup, never on hot paths — so no engine behavior changes; the zero-allocation invariants (M1-ECS-03 churn, M1-HEAD-01 headless run) now measure identically on every P0 platform. Also removes the temporary diagnostic probes (rounds 1-3) and restores the API manifest to its pre-probe state. Verified locally: zero-warning gcc/clang builds, 47/47 ctest. Verified by this PR's windows-msvc job (pending).
The per-target _ITERATOR_DEBUG_LEVEL=0 from the previous commit failed the windows-msvc link: the MS STL enforces one _ITERATOR_DEBUG_LEVEL per linked binary, so GTest objects (compiled with the Debug default 2) cannot be linked with engine objects compiled with 0 (LNK2038) — which also retroactively confirms the root cause (the failed build would have been the first build of this tree actually running the STL at level 2, exactly the checked-iterator proxy regime the diagnostic probes characterized). Move the define to a build-wide add_compile_definitions() in the top-level CMakeLists (MSVC only), before all add_subdirectory calls: engine, tests, and vendored dependencies all compile at level 0, so every linked binary is uniform and the zero-allocation invariants measure the same baseline on windows-msvc as on the other P0 platforms (GCC/Clang/AppleClang never enable checked iterators by default). Release builds are unchanged (level 0 is already the Release default); the proxies are an STL debug aid, allocated in container setup, never on hot paths. Verified locally: zero-warning gcc and clang builds, 47/47 ctest each.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is broken
The
windows-msvcjob (merge matrix) is red on the M1-HEAD-01 merge (run 34958290344). Two independent, both Windows-only defects:1. Build error — MSVC C4996
fopenfatal under /WXMSVC's CRT deprecates plain
fopen(C4996), fatal under the engine policy's/W4 /WX(NFR-8.10, CORE-010).laige-run.cppis new in M1-HEAD-01, so this is the first Windows build of the binary.Fix: the config-file open now uses the portable platform boundary already established in
logging.cpp(the M0-CORE-02 file sink),laige-bench.cpp, and the test suites: under_MSC_VER,_fsopen(path, mode, _SH_DENYNO)— the CRT's documented way to open with plain-fopensharing semantics.fopen_sis deliberately not used: it opens with the_SH_SECUREsharing mode, which denies re-opening of the file while it is held (the windows-msvc CI runs of M0-CORE-02 read back an empty file for exactly that reason — documented inlogging.cpp). No behavioral change.2. Test failure —
EngineRun.HeadlessFramePathAllocatesNothing(allocs=4 vs 3)Once the build was fixed, the same job failed the zero-allocation headless-run test: the measured
run_headless(3, 1)window allocates 4 heap objects on windows-msvc, 3 on every other P0 platform.Root cause (established by staged allocation probes run on the CI runner): the extra allocation lands between the entry of
Logger::shutdown()and the declaration of its localstd::vector<RateEntry> pending;— and it is per declaration, per call, for any element type (emptyvector<int>/vector<string>/ a secondvector<RateEntry>each added exactly one; the idempotent secondshutdown()call allocated the same four again). That signature is the MS STL's checked-iterator debug mode: CMake's default MSVC Debug flags define_DEBUG, which makes the STL default to_ITERATOR_DEBUG_LEVEL=2, where every STL container instance gets a heap-allocated iterator-proxy object in its constructor. The engine's window contains exactly one container declaration (the logger's shutdown local), so Windows measures 4 while the other P0 platforms — which compile without checked iterators by default — measure the documented 3.Fix:
laige_apply_engine_policy()now defines_ITERATOR_DEBUG_LEVEL=0for MSVC targets (PRIVATE, like the existing_HAS_EXCEPTIONS=0), so the Windows Debug build measures the same allocation behavior as the rest of the P0 matrix. The proxies are an STL debug aid, not engine behavior — allocated in container setup, never on hot paths — so no engine behavior changes; the zero-allocation invariants now hold identically on every P0 platform.Verification
laige_run_smoke,status=ok).ci:windowslabel, so thewindows-msvcjob runs both previously failing steps.