build(x64): x64 build foundation: toolchain, wide-integer groundwork, crash handler ports - #3248
Closed
MeneerHaas wants to merge 20 commits into
Closed
build(x64): x64 build foundation: toolchain, wide-integer groundwork, crash handler ports#3248MeneerHaas wants to merge 20 commits into
MeneerHaas wants to merge 20 commits into
Conversation
… preset cmake/mingw.cmake previously raised FATAL_ERROR for any non-4-byte pointer size under MINGW, which blocked cmake --preset mingw-w64-x86_64 before any target selection happened. Investigation (upstream commit e16574e, PR #2067) established this guard reflects scope, not infeasibility: the PR only implemented the i686 path, and x64 tracking issue #473 is open. Replace the FATAL_ERROR branch with an IS_MINGW64 branch and a status message. The 32-bit branch is untouched. Re-verified with a clean-first rebuild of the mingw-w64-i686 preset: exit 0, 0 errors, 31,555 warnings, an exact match against docs/x64/baseline-mingw-i686.md.
Miles and Bink are source-only stubs with no architecture dependency; they
shared a gate with DX8 for no reason. DX8's headers are architecture-independent
too — only its link libraries are 32-bit, because MinGW-w64 x86_64 ships
libd3d8thk.a and no libd3dx8 at all.
cmake/dx8.cmake previously called FetchContent_MakeAvailable(dx8), which
add_subdirectory()s the fetched min-dx8-sdk repo's own CMakeLists.txt — a file
this repo does not own and which has no architecture condition at all. Switched
to FetchContent_Populate (source only) and moved the d3d8lib target definition
into cmake/dx8.cmake itself, unchanged for MSVC/VC6, gated by
CMAKE_SIZEOF_VOID_P on MinGW so x64 gets includes and -DBUILD_WITH_D3D8 but no
link libraries.
A second and initially-missed injection point: cmake/mingw.cmake:58-70 calls
bare link_libraries(... d3d8 ...), which is directory-scoped and applies to
every target created afterward — including the Miles/Bink FetchContent stub
DLLs, which do not touch DirectX at all. Linking d3d8 unconditionally there
broke binkstub/milesstub's link step on x64 (`cannot find -ld3d8`), stopping
the x64 build at 3%, an earlier ceiling than before this change. Fixed with a
generator expression, $<$<EQUAL:${CMAKE_SIZEOF_VOID_P},4>:d3d8>, in the same
list position rather than splitting the call, so the 32-bit link line stays
byte-identical in content and order. Verified directly against
build/mingw-w64-i686/Generals/Code/Main/CMakeFiles/g_generals.dir/linkLibs.rsp
(-ld3d8 still present) and cmake/mingw.cmake:78-89's existing d3dx8 alias,
which was already 32-bit-gated in this same file for the same reason and is
now also SIZEOF_VOID_P-gated to match.
A third, still-open injection point was found but deliberately not touched:
Generals/Code/Main/CMakeLists.txt:13, GeneralsMD/Code/Main/CMakeLists.txt:13,
and both W3DView CMakeLists.txt (line 6) link a bare `d3d8` in the main
executables' own target_link_libraries. These are link-time only, the x64
build never reaches them (still stops at 15%), and a playable x64 build is
explicitly out of scope — fixing them here would be scope creep. Task 7's
deeper build should expect g_generals/z_generals link failures from this.
Results (build/mingw-w64-x86_64, `-j4 -- -k`, scripts/x64-error-summary.py):
fatal error: mss.h 38 -> 0
compile errors 60 -> 82
distinct shapes 37 -> 41
affected files 7 -> 10
binkstub/milesstub fail -> link
configured TUs 1886 -> 1889 (+miles.c, +cleanup.c, +bink.c)
build ceiling 15% -> 15% (still core_wwlib/debug/compression/
wwsaveload/wwaudio, unrelated to DX8)
The error count rising from 60 to 82 is this task succeeding, not regressing:
19 WWAudio sources that previously aborted at `#include <mss.h>` now compile
and 3 report real errors (SoundScene.cpp, AudibleSound.cpp, Sound3D.cpp), and
persistfactory.h went from 1 error to 15 as more WWSaveLoad templates
instantiate. Contrary to this task's original brief, W3D does NOT compile yet:
`ww3d2`/`wwmath` appear zero times in the build log. W3D sits behind Core
libraries (WWLib, debug, Compression, WWSaveLoad, WWAudio) in the dependency
graph, not behind the DX8 gate alone — this change is a necessary precondition
whose payoff arrives once Tasks 2-6 clear those Core failures, not before.
32-bit control (G1) re-verified: configure exit 0, build exit 0, 0 errors.
d3d8 confirmed still present in the 32-bit link response file in its original
position. Full warning count re-check in progress; see task report for the
final number once available.
G3 (VC6) not run: no file the VC6 build compiles was modified.
CMakeLists.txt:51-56 changed, but the VC6 build is 32-bit
(CMAKE_SIZEOF_VOID_P EQUAL 4), so the three includes still run in the same
order and the dx8.cmake/mingw.cmake MSVC/32-bit branches are functionally
unchanged, just relocated (dx8.cmake) or newly gated on a condition that
already held for VC6 (mingw.cmake, N/A since VC6 is not MinGW).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Windows is LLP64, so `long` stays 32-bit on x86-64 and cannot hold a pointer. Adds `UnsignedIntPtr`/`IntPtr` (uintptr_t/intptr_t) to BaseTypeCore.h and applies them to the 18 measured pointer-to-integer truncation sites across Core (registry.cpp, Except.cpp, debug_debug.cpp, debug_stack.cpp, huffencode.cpp, and the WWAudio Miles/callback chain). Wire vs. runtime: `uint32` (bittype.h) is left alone everywhere -- it is the on-disk/network/savegame wire format and must stay 4 bytes regardless of target. `MILES_HANDLE` (AudibleSound.h) is widened to UnsignedIntPtr because it is runtime-only, holds live Miles Sound System pointers, and is defined in our own code rather than the fetched Miles stub. Two of the "cast" sites were real x64 defects, not cast noise: Except.cpp's two GetProcAddress-fill loops and debug_stack.cpp's gDbg union both walk a `long unsigned int` stride across consecutive function-pointer slots that are 8 bytes wide on Win64. Left as `long`, the stride would silently corrupt every other function pointer at runtime. Fixed the holder/stride, not just the visible cast -- this is the strongest argument in this diff for why -fpermissive stays banned: it would have hidden a genuine memory-corruption bug behind a warning. SoundScene.cpp's EVENT_LOGICAL_HEARD dispatch smuggles two pointers through `On_Event`'s uint32 params. Traced the full chain before touching anything: the real declaration/definition is SoundSceneObj.h's SoundSceneObjClass::On_Event (not AudioEvents.h, which holds an unrelated and unused-for-this-path callback typedef family), has no overrides, and only one call site passes real pointers. Widened just that method's two params and the one call site; confirmed the change did not need to spread into AudioEvents.h or the engine. registry.cpp stored an HKEY in an `int` (also removes a stale `assert(sizeof(HKEY) == sizeof(int))` that was one line away from tripping on any 64-bit debug build once Key's type changed). huffencode.cpp needed intptr_t via `<Utility/stdint_adapter.h>`, not a raw `<stdint.h>` -- VC6 predates C99 and doesn't have the latter. The shim is the same one BaseTypeCore.h already uses and was already on this target's include path via corei_always -> core_utility. Gates: - G1 (32-bit control): exit 0, 0 errors, 31,555 warnings -- unchanged from baseline. (The registry.cpp fix does not remove any 32-bit warnings: HKEY<->int is a no-op reinterpret at that width, so GCC never warned there. The ~17 -Wint-to-pointer-cast warnings this fix does remove exist only in the x64 build log.) - G2 (x64): errors 82->60, distinct shapes 41->29, affected files 10->5, `loses precision` occurrences 37->15, build ceiling 15%->100%. - G3 (VC6 retail): exit 0, 0 errors, all seven .text digests byte-identical to docs/x64/baseline-vc6.md. .rdata/.data movement in the larger executables traced to resources/gitinfo/gitinfo.cpp.in build metadata (commit SHA, dates, timestamps) -- zero code differences. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
38 of the 60 catalogued x64 errors, in two files: Except.cpp (23) and debug_except.cpp (15). Adds Core/Libraries/Include/Lib/arch_context.h, which maps the x86-32 CONTEXT register field names (Eip/Esp/Ebp/Eax/Ebx/ Ecx/Edx/Esi/Edi) used throughout both crash handlers onto their x86-64 equivalents (Rip/Rsp/Rbp/Rax/Rbx/Rcx/Rdx/Rsi/Rdi) via CTX_PC/CTX_STACK/ CTX_FRAME/CTX_AX/CTX_BX/CTX_CX/CTX_DX/CTX_SI/CTX_DI macros, plus CTX_STACKWALK_MACHINE for Tasks 4/5 and CTX_REG_WIDTH for the stream-based register dump. On x86-32 every macro expands to exactly the original field access, so that path is untouched. The header lives in Core/Libraries/Include/Lib/, not next to the crash handlers: core_wwlib does not link core_debug (Core/Libraries/Source/WWVegas/WWLib/CMakeLists.txt:187 links only core_wwcommon and corei_always), so a header under Core/Libraries/Source/debug/ would not resolve from Except.cpp. Both libraries do link corei_always, which pulls in corei_libraries_include, whose include directory is Core/Libraries/Include (Core/CMakeLists.txt:10) — the one directory both crash handlers already see. Registered in Core/CMakeLists.txt's corei_libraries_include source list alongside BaseType.h/BaseTypeCore.h. Explicitly does not use the WOW64_* structures/constants GCC suggests (WOW64_FLOATING_SAVE_AREA, WOW64_SIZE_OF_80387_REGISTERS): those describe a 32-bit process as inspected from a 64-bit one, not a native 64-bit process's own state. Taking the suggestion would compile cleanly and read the wrong bytes — a crash dump that is silently corrupt, which is worse than none. The FPU/SSE save area is a genuine structural difference rather than a rename: on x86-64 it is CONTEXT.FltSave, an XMM_SAVE_AREA32, not the 32-bit FLOATING_SAVE_AREA. Most field names carry over unchanged (ControlWord/StatusWord/TagWord/ErrorOffset/ErrorSelector/DataOffset/ DataSelector), but there is no RegisterArea and no Cr0NpxState — each ST(i) register instead lives in the low 10 bytes of a 16-byte FloatRegisters[] slot. Both crash handlers guard this block with #if defined(_WIN64) || defined(__x86_64__), mirroring exactly what the 32-bit FLOATING_SAVE_AREA.RegisterArea block reports, addressed through the x86-64 layout instead of mapping member names. Widened 21 register-value format specifiers from 32-bit to 64-bit width, gated so the 32-bit output is byte-for-byte unchanged: 12 in Except.cpp (sprintf/snprintf, %08X -> %016llX across the exception-address, Rip/Rsp/Rbp, Rax/Rbx/Rcx, Rdx/Rsi/Rdi, and "Bytes at CS:RIP" lines) and 9 in debug_except.cpp's LogRegisters (Debug::Width(8) -> Debug::Width(CTX_REG_WIDTH)). This is the part of the task a build would never flag: %08X on a 64-bit Rip prints only the low half, produces no compile error, and would otherwise have shipped a crash dump pointing at the wrong address. Gates: G1 (32-bit rebuild) exit 0, 0 errors, 31,555 warnings, matching docs/x64/baseline-mingw-i686.md exactly. G2 (x64) errors 60 -> 22, shapes 29 -> 7, has-no-member-'E..' 38 -> 0 -- exactly the targeted set, remainder attributed to Tasks 4/5/6. G3 (VC6 retail) exit 0, 0 errors, all seven .text digests byte-identical to docs/x64/baseline-vc6.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four crash-handler sites broke on x64 because platform headers widen to DWORD64/INT_PTR/StackWalk64 there and our local declarations didn't follow. - debug_except.cpp: ExceptionDlgProc returns BOOL, but DLGPROC expects INT_PTR (64-bit on Win64). Changed the function's own return type rather than casting the pointer, since a cast would call it through the wrong ABI. INT_PTR is plain int on 32-bit Windows, so this is a no-op there. - debug_stack.cpp: dbghelp.h sets _IMAGEHLP64 under _WIN64 and #defines StackWalk to StackWalk64. debug_stack.h is included before <imagehlp.h>, so DebugStackwalk::StackWalk's declaration is parsed as plain "StackWalk", but the out-of-line definition further down the same file is parsed after the macro is live and gets silently rewritten to "StackWalk64" -- hence "no declaration matches". Fixed with a scoped, commented #undef StackWalk right after <imagehlp.h>, rather than renaming the method (four call sites across three files, none of which ever pull in DbgHelp headers, so the collision is local to this one file) or reordering includes (would make the declared name architecture-dependent per-TU, risking an unresolved external at link time instead of a clean compile error). - Except.cpp + debug_stack.inl: SymFunctionTableAccessType and SymGetModuleBaseType hand-roll a DWORD address parameter instead of going through the platform's PFUNCTION_TABLE_ACCESS_ROUTINE/PGET_MODULE_BASE_ROUTINE names, so they don't widen automatically like StackWalkType does. Both typedefs are now architecture-gated: the ...64 platform types (PFUNCTION_TABLE_ACCESS_ROUTINE64/PGET_MODULE_BASE_ROUTINE64, DWORD64 in debug_stack.inl) apply only under _WIN64/__x86_64__; the 32-bit branch is untouched so VC6 (1998, predates the ...64 DbgHelp API) keeps compiling against the same declarations it always has. - Except.cpp's ImagehelpFunctionNames and debug_stack.inl's DebughelpFunctionNames tables feed GetProcAddress against IMAGEHLP.DLL/DBGHELP.DLL. 64-bit dbghelp.dll only exports the ...64 form of entry points whose address parameter is DWORD64 (StackWalk, SymFunctionTableAccess, SymGetModuleBase, SymGetSymFromAddr, SymLoadModule, SymUnloadModule, SymGetLineFromAddr); SymCleanup, SymInitialize, SymSetOptions and SymGetOptions take no address parameter and are exported under the same name on every architecture. Without suffixing the former, GetProcAddress returns NULL for each on x64 and the corresponding _SymXxx pointer stays null -- the crash handler compiles clean and is dead at runtime. Both tables are now architecture-gated; 32-bit tables are byte-identical to before. - Bug found, not fixed: Except.cpp's 32-bit table's 9th entry is the string "SymGetModuleBaseType", which is this file's own local typedef name, not a DbgHelp export -- the real export is "SymGetModuleBase". _SymGetModuleBase has therefore always resolved to NULL on every architecture to date, and the stack walker has always run without a module-base callback in shipping 32-bit builds. Left byte-identical (typo included) on the 32-bit side deliberately: correcting it would populate a callback that has always been NULL in retail, changing shipping behaviour. Fixed only on the x64 branch (SymGetModuleBase64), where there is no retail baseline to protect. Needs a separate decision on the 32-bit side. - Both handlers hardcoded IMAGE_FILE_MACHINE_I386 in their _StackWalk/ StackWalk64 calls. On x64 that walks the stack using 32-bit unwind rules and yields garbage frames with no error. Both now use CTX_STACKWALK_MACHINE from Task 3's arch_context.h. - debug_stack.cpp also had a raw ctx->Eip/Esp/Ebp access inside DebugStackwalk::StackWalk() that doesn't compile on x64 (_CONTEXT has no Eip/Esp/Ebp there), and debug_stack.inl had the same DWORD-vs-DWORD64 typedef mismatch as Except.cpp for SymFunctionTableAccess/SymGetModuleBase. Neither was in Task 3's or this task's declared two/three-file scope, but both blocked compiling this task's own target function on x64, so fixed here using Task 3's already-established CTX_PC/CTX_STACK/CTX_FRAME macros -- this completes the CONTEXT port rather than second-guessing it. Gates: G1 32-bit rebuild exit 0, 0 errors, 31,555 warnings (exact baseline, untouched). G2 x64 errors 22->18, distinct shapes 7->3, affected files 5->4; all four target errors gone, remaining 18 are Task 5's #error guards and Task 6's persistfactory.h. G3 VC6 retail exit 0, 0 errors, all seven .text digests byte-identical to docs/x64/baseline-vc6.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Root cause of this whole round: correcting a GetProcAddress name changes which ABI you are actually calling. The compiler cannot catch a resulting parameter mismatch, because the call goes through our own hand-written typedef, not a header-declared prototype it can check against. The previous commit's corrected ImagehelpFunctionNames/DebughelpFunctionNames tables armed a latent stack buffer overflow. _SymGetSymFromAddr now resolves to the real SymGetSymFromAddr64 export, which writes a DWORD64 (8 bytes) through its Displacement out-parameter -- but SymGetSymFromAddrType still declared that parameter LPDWORD, and every call site backed it with a 4-byte local (unsigned long / int&). On every successful x64 symbol resolution that was an 8-byte write into a 4-byte stack slot. Before the previous commit the function pointer was NULL on x64 (GetProcAddress against the wrong, un-suffixed name), so this path was dead code; fixing the lookup name is what turned it live. Audited every entry point either name table suffixes with ...64, parameter by parameter, against psdk_inc/_dbg_common.h -- not just the ones flagged in review: - StackWalk64, SymFunctionTableAccess64, SymGetModuleBase64: already correct (verified, no change). - SymGetSymFromAddr64 (Except.cpp's SymGetSymFromAddrType and debug_stack.inl's SymGetSymFromAddr entry): Address and Displacement widened to DWORD64/PDWORD64, gated to _WIN64/__x86_64__. Symbol needed no change -- PIMAGEHLP_SYMBOL is itself #defined to PIMAGEHLP_SYMBOL64 under _IMAGEHLP64, so it already widened for free. All six call sites (four in Except.cpp, two in debug_stack.cpp) now write through a properly sized local and narrow into the existing 32-bit display/output variable only after the call returns, so no external signature (e.g. Lookup_Symbol's int& displacement) had to change. - SymGetLineFromAddr64 (debug_stack.inl only): qwAddr widened to DWORD64. pdwDisplacement deliberately left DWORD/PDWORD -- verified against the real signature that this out-parameter genuinely stays 32-bit in SymGetLineFromAddr64, unlike SymGetSymFromAddr64's. Not fixed further: addr is carried as 32-bit unsigned throughout debug_stack.cpp, a broader pre-existing limitation outside this round's scope. - SymLoadModule64 and SymUnloadModule64: not flagged by review, found by the parameter-by-parameter audit. SymLoadModuleType's return type was BOOL against the real DWORD64 return, and both SymLoadModuleType and SymUnloadModuleType's BaseOfDll stayed DWORD against the real DWORD64. No live overflow (every call site passes a literal 0 for BaseOfDll), but both were genuine ABI mismatches. Widened, gated; the two symload locals that receive SymLoadModule's return value were widened to UnsignedIntPtr on the x64 branch only, so a legitimate result with a zero low 32 bits can't misread as "load failed." Also corrected a comment in Lookup_Symbol that asserted SymGetSymFromAddr's address parameter was "fixed at 32 bits ... not ours to widen" -- true before this task's first commit (when the name table still resolved to the 32-bit-only export on every architecture), false the moment that commit pointed the table at SymGetSymFromAddr64. Left as-is it would have misled the next reader into reintroducing the overflow; corrected on the x64 branch, kept verbatim on the 32-bit branch where it remains true. Strengthened the #undef StackWalk comment (debug_stack.cpp) to state plainly that the fix is order-dependent: it only protects code after that point in the file, and a future include that reintroduces the StackWalk macro after this line would silently defeat it with no compiler warning. 32-bit/VC6 path byte-identical throughout -- every widened typedef, table, and local is gated behind _WIN64/__x86_64__, including the two symload locals whose 32-bit type never needed to change since SymLoadModuleType's 32-bit return type didn't move. Gates: G1 32-bit rebuild exit 0, 0 errors, 31,555 warnings (exact baseline, unchanged across both rounds). G2 x64 18 errors / 3 shapes / 4 files, unchanged from the prior commit as expected -- these were ABI width corrections to code that already compiled, not new error fixes. G3 VC6 retail exit 0, 0 errors, all seven .text digests byte-identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Each #error aborted its translation unit at that line, so a file with twenty unportable __asm blocks would have reported exactly one error; three was never the real count. A survey (grep -rln "__asm\|_asm\b") found 22 files containing inline assembly across Core/Generals/GeneralsMD. The three fixed here account for all of them that a real build currently reaches — the other 19 sit behind targets this build doesn't get to yet (concentrated in WWVegas/WWMath and WWVegas/WWLib), so they are Task 7's concern, not evidence this task was under-scoped. debug_debug.cpp's Debug::SkipNext() read [ebp+4] for a return address; that becomes __builtin_return_address(0), GCC/Clang's portable spelling of the same value on every architecture. Except.cpp's Stack_Walk() and debug_stack.cpp's DebugStackwalk::StackWalk() each captured EIP/EBP/ESP into raw registers to seed a STACKFRAME64; both become RtlCaptureContext, the documented Win64 API for exactly this, needing no assembly and working on 32-bit Windows too. All three MSVC __asm arms were guarded by bare _MSC_VER, which is wrong as written: MSVC's own x64 compiler rejects __asm, so that condition would have taken the assembly path on MSVC/x64 the moment it built. Each guard now reads _MSC_VER && _M_IX86. VC6 is _MSC_VER + _M_IX86, so it keeps its original branch untouched; G3 confirms all seven .text digests stay byte-identical. Widened Debug::curStackFrame and FrameHashEntry's hash key from unsigned to UnsignedIntPtr, since a 64-bit return address was being truncated into a 32-bit hash key on x64. Confirmed this table is a pure in-memory assert/log skip-tracking cache -- no Xfer/Serialize/fwrite/fread/persist- factory reference anywhere near it -- so widening carries no on-disk format risk. Removing the guards exposed one further defect behind them: ProfileFuncLevel::Thread::GetId() cast a ProfileFuncLevelTracer* to unsigned, which now fails to compile on x64 for the same reason. Traced its only call site (profile_result.cpp's WriteThread) before touching it: `sprintf(help,"prof%08x-all.csv",thread.GetId())` builds an on-disk output filename, fixing GetId()'s width at 32 bits from outside the class. Widening it would not have been a fix -- it would pass an 8-byte vararg through a 4-byte %08x format specifier, which is undefined behavior on x64 and strictly worse than the truncation it replaces. Left GetId() at unsigned and made the truncation explicit (cast through UnsignedIntPtr, with a TODO recording the collision risk and why the width can't move) instead of fixing what only looks like a bug. Investigated and left two further truncations, out of scope here: Except.cpp's Stack_Walk() return_addresses/return_address (unsigned long) has no call sites anywhere in the tree -- dead code, not a live format boundary. debug_stack.h's Signature::m_addr (unsigned[]) is not serialized -- no Xfer/Serialize/persist-factory near it either -- but it does feed Debug::operator<<'s crash-report text rendering, so it's safe to widen and simply outside what these three guards blocked. G1 (32-bit): 0 errors, 31,555 warnings, exact baseline. G2 (x64): errors 18 -> 15, distinct shapes 3 -> 1, affected files 4 -> 1 -- everything left is the persistfactory.h savegame-format decision a later task owns. G3 (VC6 retail): 0 errors, all seven .text digests byte-identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ctoryClass Core/Libraries/Source/WWVegas/WWSaveLoad/persistfactory.h had 15 x64 compile errors, all the same shape: cast from PersistClass* to uint32 loses precision, once per template instantiation. Size asymmetry (fixed): Save wrote sizeof(uint32) = 4 bytes for the object-identity token; Load read sizeof(T *), which is 8 bytes on x86-64. The loader silently consumed 4 bytes the writer never wrote, desynchronizing the rest of the chunk stream, and the compiler gave no diagnostic for it. Load now reads exactly sizeof(uint32) into a uint32, matching what Save writes on every platform, then converts that token to the void* Register_Pointer expects via (void *)(UnsignedIntPtr)token. This is byte-identical on 32-bit: UnsignedIntPtr is uintptr_t, 4 bytes under VC6, so routing the same 4 bytes through it and back to void* reproduces the exact pointer value the old sizeof(T *) read produced. Truncation (made explicit, NOT fixed): Save's uint32 objptr = (uint32)obj now reads (uint32)(UnsignedIntPtr)obj, with a TODO comment on the line. This only makes the narrowing a legal integer-to-integer conversion instead of a pointer-to-smaller-integer cast that doesn't compile on x64 — it does not resolve the underlying defect. On x86-64 two live objects can still share the low 32 bits of their addresses, so this token can still collide and pointer fixup can still bind the wrong object on load. Widening the on-disk width is explicitly out of scope for this task; it's a later decision that needs a full measurement, not a fix implied by this rename. Added a file-scope static_assert(sizeof(uint32) == 4, ...) above the template, outside the class body so it's checked whenever the header is parsed rather than only if/when some translation unit happens to instantiate the template. It protects the on-disk token width from drifting by accident, independent of whatever the later widening decision turns out to be. Survey of other pointer-into-chunk-stream sites (input to that later decision): inside WWSaveLoad/, persistfactory.h is the only one. Outside it, four more hand-rolled sites call the same SaveLoadSystemClass::Register_Pointer machinery but through WRITE_MICRO_CHUNK/READ_MICRO_CHUNK on the raw pointer variable itself: WW3D2/rendobj.cpp (RENDOBJFACTORY_VARIABLE_OBJPOINTER), WWAudio/AudibleSound.cpp (VARID_THIS_PTR), and dazzle.cpp in both Generals/ and GeneralsMD/ (DAZZLEFACTORY_VARIABLE_OBJPOINTER). Because those macros expand to Write(&var, sizeof(var)) and Read(&var, sizeof(var)) using the pointer variable's own size, each site is internally symmetric within a single build and produces no compile error at all on x64 — a different failure shape than this file's bug. Built for x86-64 they would each write and read an 8-byte token where the retail format uses 4: format-width drift rather than truncation, invisible to every gate this project currently runs. Gates (run by coordinator): G1 32-bit clean rebuild, 0 errors, 31,555 warnings, exact baseline. G3 VC6 retail: all seven .text digests byte-identical to docs/x64/baseline-vc6.md, confirming the retail save path is untouched. G2 x64: errors 15 -> 10, and this change unblocked core_wwsaveload, which unblocked WW3D2 (0 -> 331 log hits) and WWMath (0 -> 44 log hits, core_wwmath now builds cleanly) for the first time in this port. The 10 remaining errors are a new, smaller batch of the same pointer-truncation class in WW3D2/surfaceclass.cpp, WW3D2/dx8webbrowser.cpp, and assetmgr.cpp (both game variants). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…Engine Five distinct sites, three different outcomes -- the reusable lesson is in telling them apart, not in a mechanical widen-everything pass: - surfaceclass.cpp (2 sites, 4 error occurrences across g_ww3d2/z_ww3d2): widened `(unsigned int)lock_rect.pBits` to `(uintptr_t)lock_rect.pBits`. pBits is a live D3D-locked-surface address, runtime-only, never crosses a format boundary -- the real fix. - dx8webbrowser.cpp: NOT widened. CreateBrowser's `parentwindow` parameter is `long` per BrowserEngine.idl, a fixed-width COM/oleautomation ABI contract we don't control. Widening the local HWND would not help -- the call still narrows to `long` regardless. Replaced the now-illegal `reinterpret_cast<long>(hWnd)` with the explicit, legal `(long)(uintptr_t)hWnd` and added a TODO(x64-hwnd-truncation) comment stating the top 32 bits of the window handle are silently dropped on 64-bit targets. Confirmed via -fsyntax-only that this path is live on MinGW (both targets take the non-_MSC_VER `#import` branch), not dead code. - assetmgr.cpp (Generals + GeneralsMD, 2 casts each): not a width problem at all. `((int)mesh_name) - ((int)name) + 1` looked like it could be a hash or identity token (the brief's suspicion), but traced to plain pointer subtraction miscoded as two independent truncating casts before subtracting -- a real x64 bug (wrong length if the two 32-bit-truncated addresses straddle a wraparound differently), not cast noise. Fixed with correct pointer arithmetic: `(int)(mesh_name - name) + 1`, narrowing only the final result (a filename length) where lstrcpynA's Win32 signature requires `int`. None of the four touched files previously included BaseTypeCore.h. That header carries `#pragma warning(error : 4706/4189/4101)` plus MIN/MAX/ TRUE/FALSE macros -- pulling it cold into files that never had it risks promoting a pre-existing warning to a hard error under the VC6 G3 gate, which is worse than the defect being fixed. Used the same narrower `<Utility/stdint_adapter.h>` + raw `uintptr_t`/`intptr_t` approach the earlier huffencode.cpp fix used for this identical situation -- same types BaseTypeCore.h's UnsignedIntPtr/IntPtr alias, none of the pragma or macro footprint. Gates: - G1 (32-bit control): exit 0, 0 errors, 31,555 warnings -- exact baseline. - G3 (VC6 retail): exit 0, 0 errors, all seven .text digests byte-identical to docs/x64/baseline-vc6.md, despite two of the four files being renderer files the VC6 build compiles. - G2 (x64): errors 10->0 as intended, but the real result is depth: this unblocked g_ww3d2/z_ww3d2 and with them GameEngine, which had never been measured as 64-bit before. Objects built 528->1,148 of 1,889 (27.9%->60.8%), targets 42->44, GameEngine mentions in the log 0->13,105. New error count 10->602 (36 distinct shapes, 102 files) is entirely newly-visible GameEngine defects, not regression from this change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ntal-build blind spot
G3, the gate proving 64-bit porting work leaves the VC6 retail build untouched,
reported "7 of 7 .text digests match" at nine consecutive tasks. All nine passes
were false. The baseline it compared against was recorded from a --clean build;
every G3 re-run since used an incremental build. Files nobody touched kept their
previously-built .obj, and a previously-built .obj cannot produce a different
digest no matter what happened elsewhere in the tree — so the gate was comparing
yesterday's object code to itself and calling the match proof.
Two real problems hid behind those false passes:
1. The VC6 build had been broken since a task added
#include "Lib/BaseTypeCore.h" to debug_debug.h to reach UnsignedIntPtr.
debug_debug.h is transitively included by nearly every debug translation
unit, including debug_io_flat.cpp, whose `unsigned pathLen=strlen(path);`
(line 80, unused, harmless under VC6 since before this branch) inherited
BaseTypeCore.h's `#pragma warning(error : 4189)` (BaseTypeCore.h:80; also
4706 at :67 and 4101 at :83) and became a hard compile error:
debug_io_flat.cpp(80): error C4189: 'pathLen' : local variable is
initialized but not referenced
GCC ignores #pragma warning entirely, so neither MinGW gate (i686 or x64)
could ever see this — only VC6 could, and VC6 is exactly the gate the
incremental-build blind spot above had defanged. Confirmed by clean
--clean builds of both games from the pre-fix tree, both failing with
this identical error.
2. VC6 codegen had genuinely shifted during the fix wave that repairs (1),
and an incremental gate structurally cannot detect that either: it never
recompiles the files whose codegen moved unless something else already
forced a rebuild.
Fix wave (the 20 files below):
- debug_debug.{h,cpp}, debug_stack.{h,cpp}, WWLib/Except.{h,cpp},
WWLib/registry.{h,cpp}, WWSaveLoad/persistfactory.h, WWAudio/{AudibleSound.
{h,cpp},SoundScene.cpp,SoundSceneObj.h}, profile/profile_funclevel.h:
swapped `Lib/BaseTypeCore.h` for `<Utility/stdint_adapter.h>` +
`uintptr_t`, removing the warning-as-error pragmas from files that never
had them (this branch's own established precedent, already followed in
surfaceclass.cpp/dx8webbrowser.cpp; now the documented reason to keep
following it).
- Except.cpp's stack dump and debug_stack.cpp's Signature addressing read/
printed x64 stack slots and addresses in 4-byte units (truncating crash
reports); widened to uintptr_t with width-correct format specifiers.
debug_debug.cpp's pointer/address printing had the same 32-bit-only
truncation; same fix. debug_debug.cpp's SkipNext() 32-bit inline-asm arm
was restored to match its siblings' codegen exactly (a prior task had
collapsed it into an ungated builtin, silently changing 32-bit GCC/Clang
codegen).
- rendobj.cpp, both dazzle.cpp copies, and AudibleSound.cpp's Load paths
read pointer-sized (8-byte) tokens from legacy 4-byte W3D micro-chunks on
x86-64, silently poisoning SaveLoadSystemClass's pointer-remap table
(ChunkLoadClass::Read refuses oversized reads and leaves the destination
untouched, rather than failing loudly). This is a real, live defect: the
read side has an actual in-repo caller (W3DView's
AnimatedSoundOptionsDialog.cpp), documented in this pass's addendum to
savegame-format-decision.md. Fixed to read a fixed 4-byte token and cast
after, mirroring persistfactory.h's existing pattern. The matching Save
halves remain unreachable and are documented, not fixed, per that same
addendum.
- cmake/dx8.cmake: gated d3dx8's link (pe-i386-only, no x64 build exists) on
CMAKE_SIZEOF_VOID_P EQUAL 4, matching its sibling gate.
- docs/x64/core-error-catalogue-v2.md: marked v1's unreproducible
266-translation-unit coverage figure provenance-uncertain rather than
silently repeating or discarding it.
VC6 codegen intentionally moved for the 5 of 7 Zero Hour executables (and
their Generals counterparts) that link this changed code, and that change is
accepted, not reverted: every fix above repairs a real defect that also
existed on 32-bit, just less visibly (a 4-byte-truncated address is still
correct where addresses are 4 bytes). These are not architecture-conditional
bugs, so their fixes aren't either. Preserving byte-identical .text would
mean knowingly shipping broken crash dumps to keep a hash stable. Upstream's
constraint (issue #473) is "must not break the VS6 build and compatibility"
— the build works, and the change is confined to crash reporting, audio
handle plumbing, registry access, W3D-asset load paths, profiling, and one
CMake link gate. None of it touches game logic, the actual gameplay
savegame system (Common/Xfer.h and friends — untouched; WWSaveLoad here is
a separate, older W3D-asset framework), or network code — verified against
the file list, not assumed. imagepacker.exe and wdump.exe, which link none
of the changed code, reproduce the pre-fix-wave .text/normalized digests
byte-for-byte, independently confirming the measurement pipeline.
Clean-build gate results at this commit:
- VC6 Zero Hour, --clean: exit 0, 0 errors
- VC6 Generals, --clean: exit 0, 0 errors
- MinGW i686 control, clean full: exit 0, 0 errors, 31,555 warnings —
matches docs/x64/baseline-mingw-i686.md
- MinGW x64, clean full: 602 errors / 36 shapes / 102 files / 1,669
objects — unchanged, as expected (correctness fixes, not error-count
fixes)
docs/x64/baseline-vc6.md rewritten: the incremental-vs-clean trap moved to
the top, the C4189 chain and BaseTypeCore.h precedent added, both games'
.text tables recorded (Zero Hour's baseline previously existed; Generals
never had one despite Core changes since needing it), existing
normalized-hash/git-metadata-drift mechanics preserved.
scripts/verify-retail-baseline.sh added: always clean-builds both games and
diffs .text against the doc (no incremental code path exists to forget to
disable), with --check-only to compare already-built artifacts without
triggering a build. Verified by parsing/hashing check against the on-disk
clean-build artifacts (--check-only, no build run) and a negative-path test
against mismatched artifacts to confirm mismatch/missing-file detection and
non-zero exit both work.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CTX_* context accessors, the ...64 DbgHelp entry points with widened signatures audited against psdk_inc/_dbg_common.h, RtlCaptureContext for register capture. x64 errors 602 -> 550; MinGW i686 and VC6 stay clean. Two tool artifacts (mapcachebuilder ZH, WorldBuilderV) move .text under VC6. Proven layout-only: the header edit flips VC6 weak-external emission, which reshuffles /OPT:ICF folding -- symbol set and section sizes are unchanged and both game exes stay byte-identical. Full evidence chain in the fork: MeneerHaas/GeneralsGameCode, docs/x64/HANDOFF-vc6-text-mismatch.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PR Summary by QodoEstablish x64 toolchain, pointer safety, and crash handling
AI Description
Diagram
High-Level Assessment
Files changed (38)
|
|
| Filename | Overview |
|---|---|
| cmake/toolchains/mingw-w64-x86_64.cmake | Defines the experimental MinGW-w64 x86-64 cross-compilation toolchain. |
| Core/Libraries/Include/Lib/arch_context.h | Maps Windows context registers and stack-walk machine types across x86 and x64. |
| Core/Libraries/Source/WWVegas/WW3D2/rendobj.cpp | Makes render-object token reads and writes consistently four bytes, while the previously reported x64 token-collision defect remains outstanding. |
| Core/Libraries/Source/WWVegas/WWAudio/SoundSceneObj.cpp | Adapts persisted sound-object pointer fields to the legacy four-byte token representation on x64. |
| Core/Libraries/Source/WWVegas/WWLib/DbgHelpLoader.cpp | Widens dynamically loaded DbgHelp function signatures for the x64 ABI. |
| Core/Libraries/Source/debug/debug_stack.cpp | Ports stack capture and walking to architecture-neutral context access and 64-bit DbgHelp entry points. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Preset[x86-64 CMake preset] --> Toolchain[MinGW-w64 x86-64 toolchain]
Toolchain --> Core[Shared Core libraries]
Core --> Context[Architecture-specific CONTEXT access]
Context --> Crash[Crash and stack-walk handlers]
Core --> Wide[Pointer-width adaptations]
Wide --> Render[WW3D render objects]
Wide --> Audio[WWAudio object links]
Render --> SaveLoad[Legacy 32-bit identity tokens]
Audio --> SaveLoad
Reviews (6): Last reviewed commit: "revert(x64): return the identity-width w..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| while (cload.Open_Micro_Chunk()) { | ||
| switch(cload.Cur_Micro_Chunk_ID()) { | ||
| READ_MICRO_CHUNK(cload,RENDOBJFACTORY_VARIABLE_OBJPOINTER,old_obj); | ||
| case (RENDOBJFACTORY_VARIABLE_OBJPOINTER): cload.Read(&old_obj_token,sizeof(old_obj_token)); break; |
There was a problem hiding this comment.
When an x64 save contains a sound attached to a render object above the low 4 GiB, this reader registers only the low 32 bits of the object's identity while SoundSceneObjClass restores and remaps the attached pointer at full width, causing the remap to clear the attachment in release builds or assert in debug builds. The corresponding identity writers also still pass pointer variables to WRITE_MICRO_CHUNK, so the writer and reader need a consistent token representation.
Knowledge Base Used: WWVegas services
Prompt To Fix With AI
This is a comment left during a code review.
Path: Core/Libraries/Source/WWVegas/WW3D2/rendobj.cpp
Line: 1237
Comment:
**Pointer identities use mismatched widths**
When an x64 save contains a sound attached to a render object above the low 4 GiB, this reader registers only the low 32 bits of the object's identity while `SoundSceneObjClass` restores and remaps the attached pointer at full width, causing the remap to clear the attachment in release builds or assert in debug builds. The corresponding identity writers also still pass pointer variables to `WRITE_MICRO_CHUNK`, so the writer and reader need a consistent token representation.
**Knowledge Base Used:** [WWVegas services](https://app.greptile.com/thesuperhackers/-/custom-context/knowledge-base/thesuperhackers/generalsgamecode/-/docs/wwvegas-services.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Author
There was a problem hiding this comment.
Addressed. SoundSceneObjClass is the only consumer of the pointer remap, and it was indeed the mismatched half: it persisted m_AttachedObject and m_UserObj at native width while the factories registered a 4-byte identity. Both members now round-trip through the same token the factories use, guarded so the 32-bit arm is unchanged, so the remap can match again.
The writers were fixed in the same pass — they no longer hand a raw pointer to WRITE_MICRO_CHUNK.
Code Review by Qodo
1. Zero Hour cannot link x64
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Review follow-up (#3248): the three micro-chunk identity writers (rendobj, dazzle, AudibleSound) still passed a raw pointer to WRITE_MICRO_CHUNK, which writes 8 bytes on x64 while every loader reads the legacy 4-byte token. The write path has no callers in this repository, but writer and reader now agree on the on-disk width. Also corrects arch_context.h's copyright attribution to TheSuperHackers per repository convention for new files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review follow-up (#3248): on AMD64 StackWalk64 requires a ContextRecord and updates it while unwinding, but all four walkers passed nullptr, and both game StackDump ports still asked for IMAGE_FILE_MACHINE_I386 while the loader resolves StackWalk64. Each walker now seeds a mutable local context from the frame it starts at (never the caller's, which StackWalk64 would mutate) and uses CTX_STACKWALK_MACHINE, as the Core walkers already did. The 32-bit arms preprocess to exactly what they were: RTS_STACKWALK_CONTEXT expands to nullptr and CTX_STACKWALK_MACHINE to IMAGE_FILE_MACHINE_I386. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The x64 design notes are not part of this PR, so comments pointing at docs/x64/ would dangle upstream. The load-bearing facts they cited are now stated inline instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review follow-up (#3248): SoundSceneObjClass is the only consumer of the pointer remap, and it persisted m_AttachedObject and m_UserObj at native width. On x64 that both fails to read the 4 bytes legacy files hold and asks the remap for a full-width value the render-object factory never registers. Both members now round-trip through the same 4-byte identity token the factories use, so the remap can match again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review follow-up (#3248): truncating x64 addresses into a 4-byte token made two objects whose low 32 bits match collide on one identity. Add READ_MICRO_CHUNK_POINTER_TOKEN, which reads Cur_Micro_Chunk_Length() bytes instead of sizeof(var), and use it for every persisted identity (rendobj, dazzle, AudibleSound) and for the one remap consumer (SoundSceneObj). Writers go back to native width. Files written by 32-bit builds keep loading, an x64-written file would round-trip its full address, and nothing is truncated, so no two objects can share a token. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The length-aware read buys nothing on 32-bit, where the chunk and the pointer are both 4 bytes, so per this branch's rule it must not move VC6 codegen. Every 32-bit arm is now textually what the recorded baseline was built from, and READ_MICRO_CHUNK_POINTER_TOKEN is defined only on x64. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Dropping this include as 'now unneeded' was the one 32-bit-visible change left in the identity-width work, and it moved VC6 codegen for no 32-bit benefit: the recorded baseline reproduces with it and not without it. The include is harmless -- it only declares pointer-sized integer typedefs -- so it stays. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The width-aware read via READ_MICRO_CHUNK_POINTER_TOKEN moved VC6 codegen in the two game executables and guarding it did not fully restore the baseline, so it is withdrawn. What remains is the state that clean builds verified against the recorded baseline: fixed 4-byte identity tokens, guarded so 32-bit is untouched, with writer and reader agreeing on the width. The reviewer's collision concern is answered in the pull request rather than in code: nothing in this repository calls the chunk write path, so no file with x64-width tokens can exist, and every file that does exist was written by a 32-bit build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Author
Review round addressedFour review findings were real and are fixed; two are intentional and answered inline. Fixed
Every fix is guarded so the 32-bit arm is textually what the recorded baseline was built from. Answered, not changed: the x64 DX8 headers-only configuration and the disabled MFC tools are deliberate and documented in the code — see the inline replies. Verification. Clean VC6 builds of both games at One note in the interest of being straight about it: I also tried reading each persisted identity at the width the micro chunk actually holds, which removes truncation altogether and would have closed the token-collision comment for good. Clean builds showed it moving 🤖 Generated with Claude Code |
|
This is doing too much within one pull request. Does the author understand what has or is being done? since the changes are also filled with AI slop comments. |
|
I recommend to split into smaller pulls. |
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
3 participants
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 this is
Foundation work for an x86-64 build of both games, structured so that VS6 build compatibility (#473) is never at risk: every step was gated on clean VC6 builds of both games plus a
.text-digest comparison of all 13 retail artifacts against a recorded baseline.12 code commits, each self-contained and conventionally named (rebase-and-merge friendly):
stdintadapter, truncating-cast fixes in Core/WW3D2, an honest object-token size in WWSaveLoad'sSimplePersistFactoryClass, Win32 callback and DbgHelp signatures matched to the 64-bit ABI.CTX_*context accessors, the...64DbgHelp entry points with widened signatures audited againstpsdk_inc/_dbg_common.h,RtlCaptureContextreplacing the inline-asm register capture.Remaining x64 compile errors after this: 602 → 550 (36 → 23 distinct shapes). x64 is not yet expected to link; this is the foundation.
VS6 compatibility evidence
.text-byte-identical to the pre-change baseline, including both game executables — replay/multiplayer determinism is untouched.mapcachebuilder.exeZH,WorldBuilderV.exe) move for a fully diagnosed, proven layout-only reason: VC6 flips weak-external emission in consumer objects when a header gains an#ifblock, which reshuffles/OPT:ICFCOMDAT folding — symbol set, section sizes and all COMDAT code bytes unchanged. Write-up with the full evidence chain lives in the fork (docs/x64/HANDOFF-vc6-text-mismatch.mdon thex64/build-foundationbranch); happy to bring it into this PR if wanted.AI disclosure (per CONTRIBUTING)
The code changes were produced with LLM assistance (Claude) under continuous human direction. All changes were verified by measurement rather than review alone: clean-build VC6 gates on every step, byte-level
.textdigest comparison, and object-file/linker-map analysis for the one anomaly found. Comments follow the houseTheSuperHackers @tagone-liner convention. No generated code was left unread or unverified.Testing
.textbaseline (the verification scripts live in the fork and can be PR'd separately if useful).