================================================================================ COMPILER_FIXES.TXT Chocolate Doom 2.3.0 - OpenVMS 8.4 Alpha (AXP) port Astr0baby 2026 ================================================================================ Four images are built: DOOM.EXE, HERETIC.EXE, HEXEN.EXE, STRIFE.EXE. See DESCRIP.MMS for the build itself; this file is only about the source changes. Every change is marked in the source with a comment beginning // OpenVMS port: so `SEARCH [.SRC...]*.C "OpenVMS port"` finds all of them. There are 119 such comment blocks across 50 files. Nothing was changed silently. -------------------------------------------------------------------------------- CONTENTS -------------------------------------------------------------------------------- 1. Summary, and the two changes that carry the most risk 2. PACKEDATTR and struct packing (DEC C has no packed attribute) 3. FEATURE_MULTIPLAYER off: d_loop.c (the off path had never been built) 4. FEATURE_MULTIPLAYER off: doom/d_main.c and doom/d_net.c 5. FEATURE_SOUND off: i_sound.c and doom/m_menu.c 6. %GEM-F-ASSERTION: 93 decrement rewrites, and the 33 increments left alone 7. SDL_RESIZABLE removed; the 640x480 window 8. SDL teardown guarded with SDL_WasInit (a fatal X error on DECwindows) 9. Unchecked allocations 10. hexen/mn_menu.c: duplicate definition of demoextend 11. m_config.c: where the .CFG files and savegames live 12. What was NOT changed, and why -------------------------------------------------------------------------------- 1. SUMMARY -------------------------------------------------------------------------------- Change counts, by kind: 93 decrement-in-controlling-expression rewrites (section 6) - DEC C 8.4 AXP internal compiler error; the reason this file exists 13 feature-off guards for FEATURE_MULTIPLAYER / FEATURE_SOUND (3, 4, 5) 7 video/window changes, incl. the 640x480 pin (7) 3 SDL teardown guards (8) 2 allocation checks (9) 1 duplicate-definition fix (10) 1 config-directory branch (11) Of these, exactly TWO classes change generated code in a way that could alter behaviour if I got the reasoning wrong, and they are the two documented most carefully below: * The 93 decrement rewrites (section 6). Each moves a side effect relative to a test. Five distinct transforms are needed, and three of the five are WRONG if applied to the wrong shape - notably the idiom the sibling Hexen port uses, which silently changes behaviour on unsigned counters. Section 6 gives the equivalence argument per class and the harness that checks them. * The FEATURE_MULTIPLAYER=off stand-in variables (section 3). Upstream's "off" path does not compile, and making it compile means defining two variables that no longer have an owning translation unit. Everything else is either a compile-or-link necessity with one possible correct answer, or a deliberate platform policy choice (7, 11) that is called out as such. -------------------------------------------------------------------------------- 2. PACKEDATTR AND STRUCT PACKING -------------------------------------------------------------------------------- NO SOURCE CHANGE WAS NEEDED. This section exists to record why, because "the packed attribute is being ignored" is exactly the kind of thing that looks like a latent file-format bug and would otherwise be re-investigated by the next person. [.SRC]DOOMTYPE.H defines #ifdef __GNUC__ #define PACKEDATTR __attribute__((packed)) #else #define PACKEDATTR #endif DEC C is not GCC, so PACKEDATTR expands to NOTHING and every struct marked with it is laid out at natural alignment. DESCRIP.MMS compiles with /NOMEMBER (do not pack), which is consistent with that - it does not try to emulate the attribute. The structs so marked are the ON-DISK ones - WAD directory entries, patch and texture headers, the PCX header, the demo header - so a layout difference would mean unreadable WADs, not merely wasted bytes. Every field offset was therefore compared, packed versus natural, across all four build sets. Result: every member offset is IDENTICAL under both layouts. That is not luck. Doom's on-disk structs are built from char/byte arrays, 16-bit shorts and 32-bit ints laid out in an order that is already naturally aligned; the attribute was defensive, not load-bearing. The ONLY difference anywhere is TRAILING padding in one struct: pcx_t packed = 129 bytes, natural = 130 bytes and that one is harmless because [.SRC]V_VIDEO.C's V_WritePCXfile does not use sizeof(pcx_t) for the write length. It computes length = pack - (byte *) pcx; i.e. the distance actually written, so the file it produces is byte-identical either way. (Had it used sizeof, every screenshot would have carried one garbage byte in the middle.) -------------------------------------------------------------------------------- 3. FEATURE_MULTIPLAYER OFF - [.SRC]D_LOOP.C -------------------------------------------------------------------------------- Networking is switched off in [.SRC]DOOMFEATURES.H: /* #define FEATURE_MULTIPLAYER 1 */ That macro is upstream's own mechanism, and [.SRC]MAKEFILE.AM has a matching FEATURE_MULTIPLAYER_SOURCE_FILES list, so the intent is clearly that it can be turned off. IT HAD NEVER BEEN BUILT THAT WAY. With it off, upstream does not compile, and then does not link. Five changes in this file, plus section 4. 3.1 net_client.h is included UNCONDITIONALLY (d_loop.c:35) Upstream includes it inside the FEATURE_MULTIPLAYER block along with net_gui.h, net_io.h, net_query.h, net_server.h, net_sdl.h and net_loop.h. It is kept out here because it is declaration-only - it pulls in no SDL_net - and it declares net_client_connected and drone, which the stand-ins in 3.2 must agree with. Including it keeps one declaration in charge of both the real and the stubbed build. The other six stay guarded. 3.2 Stand-in definitions for net_client_connected and drone (d_loop.c:53) THE ONE CHANGE IN THIS PORT MOST WORTH RE-READING BEFORE TRUSTING. With multiplayer off, no net_*.c is compiled, so nothing defines these two variables - yet they are READ from about a dozen places that upstream left outside its own #ifdefs, both in d_loop.c and in [.SRC.DOOM]D_MAIN.C:415. The options were: (a) guard every read site. That means scattering feature tests through the main loop and into game code, in a release where the game code is meant to be untouched. (b) define the variables once, in the file that owns the loop. (b) is what upstream itself does for the analogous "singleplayer" case elsewhere, and it is what is done here. They are deliberately NOT static, because net_client.h declares them extern and other objects read them. The values are the single-player truth, and they are exactly the values the real net code holds in a single-player game: boolean net_client_connected = false; /* no server */ boolean drone = false; /* we are a player, not a spectating drone */ Because they are compile-time-constant false in this build, the client/server branches they guard are dead code that DEC C's optimiser removes; nothing calls into the absent net layer. 3.3 Guarded function bodies (d_loop.c:346, 440) Two function bodies are pure client/server code (they call NET_*), so with no net layer they cannot compile at all. Both are guarded, and both retain a single-player return path so every caller still links and still gets a correct answer. 3.4 Unused-variable cleanup (d_loop.c:503) addr and i are touched only by the multiplayer block, so with it off DEC C reports them unused. Moved inside the guard rather than silenced with a cast-to-void, so the real build is unchanged. -------------------------------------------------------------------------------- 4. FEATURE_MULTIPLAYER OFF - [.SRC.DOOM]D_MAIN.C AND D_NET.C -------------------------------------------------------------------------------- The same class of problem inside the Doom directory: reads of net state that upstream left unguarded, and calls to net entry points that no longer exist. Guarded the same way, keeping the single-player path. Heretic, Hexen and Strife needed NO equivalent changes: their d_main.c / h2_main.c reach the net layer only through d_loop.c's already-guarded API. Doom is the odd one out because it consults net state directly while parsing command-line switches. -------------------------------------------------------------------------------- 5. FEATURE_SOUND OFF - [.SRC]I_SOUND.C AND [.SRC.DOOM]M_MENU.C -------------------------------------------------------------------------------- Sound is switched off in [.SRC]DOOMFEATURES.H: /* #define FEATURE_SOUND 1 */ [.SRC]I_SOUND.C IS STILL COMPILED, and that is the whole trick. It is the backend-dispatch layer, not a backend: with FEATURE_SOUND off, every I_* sound entry point in it degrades to a no-op stub. The four game directories make several hundred S_* / I_Sound* calls, and this is what keeps every one of them compiling and linking without a single #ifdef in the game code. 5.1 Header ordering (i_sound.c:25) doomfeatures.h must be seen BEFORE the SDL_mixer include, or the include is reached before the macro that is meant to suppress it. The two headers were reordered. 5.2 SDL_mixer include and backend table guarded (i_sound.c:73) The sound_modules[] / music_modules[] tables name backends that live in files this build does not compile (i_sdlsound.c, i_sdlmusic.c, i_oplmusic.c, i_pcsound.c). Referencing them would be an undefined symbol at link time, so the table entries are guarded and the tables come out empty - which the surrounding code already handles, since it must cope with "no usable backend" anyway. 5.3 I_InitTimidityConfig guarded (i_sound.c:258) Defined in i_sdlmusic.c, which is not compiled. 5.4 Shutdown path guarded (i_sound.c:497) Upstream guards only part of this; the rest calls into the absent backends. 5.5 I_OPL_DevMessages guarded ([.SRC.DOOM]M_MENU.C:1836) Doom's menu has a developer-only OPL diagnostics screen that calls into i_oplmusic.c. Guarded; the menu entry simply does nothing. -------------------------------------------------------------------------------- 6. %GEM-F-ASSERTION: 93 DECREMENT REWRITES -------------------------------------------------------------------------------- 6.1 THE BUG DEC C on OpenVMS 8.4 Alpha aborts with an INTERNAL COMPILER ERROR %GEM-F-ASSERTION, Compiler internal error - please submit problem report on a DECREMENT used inside a controlling expression - the test of an if, while, or do/while. This is a compiler defect, not a language issue: the code is valid, portable C89 and builds everywhere else. Both sibling OpenVMS ports in /opt/CLAUDE/OPENVMS hit it and document it: [-.HEXEN]COMPILER_FIXES.txt "if (var-- <= 0)" [-.CHOCOLATE_DUKE]COMPILER_FIXES.TXT "if (pixelsAllowed-- > 0)", 18 sites, with GEM_LU_MAIN in the traceback GEM_LU is the loop unroller, which is why the escape hatch in DESCRIP.MMS is /OPTIMIZE=LEVEL=1 (it disables unrolling) rather than /NOOPTIMIZE. 6.2 SCOPE 93 sites, in 42 files: doom 16 heretic 16 hexen 36 strife 25 By the transform each needed: 81 hoisted above the "if" (6.4, class A/B) 7 kept inside a loop (6.5, class D) 5 kept nested under a guard (6.6, class E) Found by preprocessing every file in all four build sets and scanning the token stream, so macro-hidden occurrences could not escape. Re-run after the edits: zero remaining. 6.3 THE FIVE IDIOMS AND THEIR TRANSFORMS Chocolate Doom spells this five ways. Each needs a DIFFERENT rewrite, and three of the five are wrong if you apply another one's transform: A --var K -> --var; if (var K) !--var -> --var; if (var == 0) B var-- K -> old = var; var--; if (old K) !var-- -> old = var; var--; if (old == 0) D while (var-- != 0) { B } -> while (var != 0) { --var; B } do { B } while (--var != 0) -> do { B; --var; } while (var != 0) E (short-circuited or nested) -> decrement stays exactly where it was 6.4 CLASS A AND B - HOISTED ABOVE THE "IF" (81 sites) Class A is prefix: the test already sees the DECREMENTED value, so evaluating the decrement one statement earlier and then testing the variable is the same computation in the same order. Class B is postfix: the test sees the OLD value while the decrement still always happens. The old value must therefore be CAPTURED - reordering alone would change the comparison: // OpenVMS port: "actor->health-- == 0" split; see COMPILER_FIXES.TXT. // Postfix, so the test sees the OLD value while the decrement always // happens - capture it rather than reordering. int oldhealth = actor->health; actor->health--; if (oldhealth == 0) *** DO NOT USE THE SIBLING HEXEN PORT'S IDIOM HERE. *** That port rewrites postfix tests as if (var == 0) ... else var--; which is right for a SIGNED counter and SILENTLY WRONG for an unsigned one, because it discards the wrap. This tree has unsigned cases: Hexen's mobj_t.args[] is `byte`. At args[0] == 0, upstream's "!actor->args[0]--" takes the branch AND leaves 255 behind; the else-form leaves 0. A harness comparing all 256 byte values found them differing at exactly one value, v == 0 - so a bug that shows up only when a counter hits zero, which is precisely when these counters matter. Capturing the old value preserves both the branch and the stored value at every input. The signed/unsigned split across class B: byte (UNSIGNED, wraps 0 -> 255) hexen args[] - a_action.c, p_enemy.c intptr_t (signed) hexen special1.i / special2.i, via the specialval_t union in h2def.h:175 int (signed) everything else 6.5 CLASS D - THE DECREMENT MUST STAY INSIDE THE LOOP (7 sites) [.SRC.HERETIC]AM_MAP.C:1125, 1148, 1164 DrawWuLine [.SRC.HEXEN]AM_MAP.C:1023, 1046, 1062 DrawWuLine [.SRC.STRIFE]M_SAVES.C:400 trailing-slash stripper A LOOP CONDITION IS RE-EVALUATED EVERY ITERATION. Hoisting the decrement above the loop would run it ONCE and turn a counted loop into an endless one. So for the while form the test moves first and the decrement becomes the first statement of the body: // The decrement must stay INSIDE the loop - a loop condition is // re-evaluated every iteration, so hoisting it out would decrement only // once. while (DeltaX != 0) { --DeltaX; X0 += XDir; PUTDOT(X0, Y0, &BaseColor[0], NULL); } That runs the body exactly DeltaX times, as before. The original's failing test also performed one final decrement, leaving DeltaX at -1; that is unobservable here because the body never reads DeltaX and the function returns immediately afterwards. (Both facts were checked, not assumed.) For do/while the decrement precedes the test, so it becomes the LAST statement of the body with the condition testing the variable. M_SAVES.C:400 is the awkward one - the decrement is the RIGHT operand of && in a loop condition, so it can neither be hoisted out (re-evaluated each iteration) nor made unconditional (it must not run when p == str). Stepping the pointer back first and testing p[0] is exactly equivalent: while (p > str) { --p; if (*p != DIR_SEPARATOR) break; *p = 0; } 6.6 CLASS E - THE DECREMENT MUST STAY NESTED (5 sites) [.SRC.DOOM]HU_STUFF.C:460 !--message_counter, inside if (message_counter) [.SRC.DOOM]P_USER.C:338 !--powers[pw_invisibility], likewise [.SRC.STRIFE]HU_STUFF.C:407 !--message_counter, likewise [.SRC.HEXEN]P_ENEMY.C:4478 parent->args[0]-- <= 0, inside an outer test [.SRC.HEXEN]P_ENEMY.C:4606 A_BounceCheck - two nested tests, where args[3] is decremented ONLY when the args[4] test succeeds Here the decrement was already CONDITIONAL - short-circuited by && / ||, or sitting inside an enclosing if. Hoisting it above the outer test would make it run more often and change behaviour. Each of these keeps its decrement exactly where it was, and says so: // The decrement stays inside the outer test, which is where it always was - // it only ever ran when the power was non-zero. if (player->powers[pw_invisibility]) { --player->powers[pw_invisibility]; if (player->powers[pw_invisibility] == 0) player->mo->flags &= ~MF_SHADOW; } THE DISTINCTION THAT MATTERS: if the decrement is the LEFT operand of || or &&, it was ALWAYS evaluated, and hoisting IS safe. [.SRC.STRIFE]WI_STUFF.C:788 ("--cnt || acceleratestage") is that case and is hoisted; the comment there says why. Only the RIGHT operand, or a decrement under an outer test, must stay put. 6.7 EQUIVALENCE TESTING Each class was checked by a harness rather than by inspection alone: class A prefix, over signed -300..300, all six relational operators class B postfix, over signed int AND unsigned byte 0..255 class C the sibling Hexen "if (x == 0) else x--" idiom, to demonstrate the divergence: differs from upstream at exactly 1 of 256 byte values, at v == 0, as predicted in 6.4 class D loop iteration counts for DeltaX/DeltaY over 0..299, both loop forms class E the trailing-slash stripper, over 13 inputs incl. "", "/", "//", "a//" and a string that is nothing but separators All classes agreed with upstream except C, which is the one this port does not use. The new temporaries (oldhealth, oldcount, oldtimer, oldbounce, oldlife) are all declared at the top of a block, so they are C89-legal; verified by recompiling all four build sets with -std=gnu89 -Wdeclaration-after-statement, which reported nothing. 6.8 THE 33 INCREMENT SITES - DELIBERATELY NOT CHANGED The same tree contains 33 INCREMENT-in-comparison sites, e.g. [.SRC]W_MAIN.C while (++p != myargc && myargv[p][0] != '-') [.SRC.DOOM]P_MAP.C if (++hitcount == 3) [.SRC.DOOM]R_DRAW.C if (++fuzzpos == 50) (x2) [.SRC.DOOM]S_SOUND.C if (sfx->usefulness++ < 0) [.SRC.DOOM]P_MOBJ.C if (cycle_counter++ > 1000000) [.SRC.DOOM]R_BSP.C while (next++ != newend) (all four games) [.SRC.HEXEN]P_ENEMY.C if (count++ > 64) [.SRC.HEXEN]SC_MAN.C while (*ScriptPtr++ != '\n') (x2) [.SRC.STRIFE]P_INTER.C if (players[i].sigiltype++ > 4) [.SRC.STRIFE]ST_STUFF.C if (++st_keypage > 2) ... 33 in total, in 27 files THESE ARE LEFT EXACTLY AS UPSTREAM WROTE THEM. The reasoning, so the next person does not have to guess: * Both sibling ports document the crash for DECREMENTS only. The Duke tree contains ZERO increment sites, so its clean build is no evidence either way. The sibling Hexen tree does contain 8 increment sites - but that port's own ALL_FIXES_COMPLETE.txt leaves "[ ] Build completes without errors" UNCHECKED, so it is not evidence that they compile. * Duke's documented crash was on int32_t pixelsAllowed-- > 0, i.e. a SIGNED operand, so the defect is not specific to unsigned arithmetic and one cannot argue increments are safe on that basis either. * Rewriting 33 correct expressions to dodge a bug not yet observed in this direction is itself a risk. Three of them (SC_MAN.C, M_SAVES.C, R_BSP.C) increment a POINTER inside a loop condition, which is the easiest kind to get subtly wrong, and R_DRAW.C is in the renderer's inner loop. * If one does crash the compiler, the cheap fix is per-file flags, not source surgery: DESCRIP.MMS defines $(CFLAGS_OPT1) and $(CFLAGS_NOOPT) for exactly this, and the sibling notes give the same advice ("Only fix if build actually crashes on them"). If it comes to that, work through the list above; the transforms are the mirror image of 6.3, and the same three traps apply (capture the old value for postfix, keep loop conditions inside their loop, do not hoist a short-circuited operand). 6.9 IF A NEW %GEM-F-ASSERTION APPEARS Read the failing PASS NAME in the traceback and change flags before touching the source: GEM_LU_* / PEEL_LOOP loop unroller; runs only above LEVEL=1 -> $(CFLAGS_OPT1) GEM_DF_* / CSE LEVEL=3 dataflow -> $(CFLAGS_OPT1) GEM_CX_* code EXPANDER; runs at EVERY level, so /NOOPTIMIZE can CAUSE it -> try OPT1, not NOOPT GEM_TN_* register allocation -> $(CFLAGS_OPT1) ME_DEBUGGEN debug-symbol emission -> add /NODEBUG/NOTRACEBACK An unjustified /NOOPTIMIZE cost the sibling Kyra port several build rounds. Remember to repeat the file's own /INCLUDE=$(INCS_x) when swapping the macro - the escape-hatch macros carry $(INCS_COMMON), which is wrong for a game file. -------------------------------------------------------------------------------- 7. SDL_RESIZABLE REMOVED; THE 640x480 WINDOW -------------------------------------------------------------------------------- A 640x480 window was the requirement for this port. It needed NO new scaler code - unlike the sibling Hexen port, which had to grow one - because Chocolate Doom already ships a software scaler table and one of its stock entries is exactly right: mode_stretch_2x = SCREENWIDTH*2 x SCREENHEIGHT_4_3*2 = 640 x 480 It is an ASPECT-CORRECTING mode (I_Stretch2x + I_InitStretchTables), which is what turns the 320x200 framebuffer into a 4:3 640x480 window without squashing. This works for all four games automatically: SCREENWIDTH (320) and SCREENHEIGHT_4_3 (240) live in the SHARED header [.SRC]I_VIDEO.H, no game directory overrides them, and I_VIDEO.C is linked into every image. 7.1 Defaults changed (i_video.c:180, 224) fullscreen defaults to 0 and screen_width/height to 640/480, rather than upstream's fullscreen 320x200. 7.2 The settings are PINNED at the top of I_InitGraphics (i_video.c:1992) Defaults alone are not enough, and this is the subtle part: these three are config-file-bound variables, and M_BindIntVariable() has already run by the time I_InitGraphics is reached. A DEFAULT.CFG carried over from another machine - or from a stock Chocolate Doom install, where fullscreen defaults to 1 - would silently win. So they are forced here: fullscreen = 0 windowed aspect_ratio_correct = 1 MUST stay 1 - see below screen_width = 640 screen_height = 480 aspect_ratio_correct is not cosmetic: it selects screen_modes_corrected[], which is the ONLY table containing a 640x480 entry. With it 0, I_FindScreenMode(640,480) returns mode_scale_2x = 640x400 instead, i.e. a letterboxed window. Verified at runtime, against the real tables: corrected I_FindScreenMode(640,480) -> 640x480 uncorrected I_FindScreenMode(640,480) -> 640x400 Pinning here rather than only changing the defaults makes the window geometry a property of the PORT rather than of the user's config file. I_AutoAdjustSettings() runs AFTER the pin, so it was checked that it cannot undo it: it calls AutoAdjustWindowed(), which rewrites screen_width/height only when the best mode DIFFERS from the request. Since 640x480 is an exact mode, the request is a fixpoint and the call is a no-op. Confirmed at runtime as well as by reading. 7.3 SDL_RESIZABLE is NOT set, unconditionally (i_video.c:1833) This is a fixed-size window by design, so advertising a resize grip the game will not honour is worse than not offering one. Resizing never actually worked in this release anyway. The event loop's resize case (I_GetEvent) tests for "SDL_RESIZABLE", which is a video-mode FLAG (0x10) - not an event type. The event SDL 1.2 actually delivers is SDL_VIDEORESIZE, from an unrelated enum. need_resize is therefore never set and ApplyWindowResize() was already unreachable upstream. Dropping the flag only makes the window's behaviour match its appearance. Left unconditional rather than behind an #ifdef VMS so that the Linux pre-validation build exercises the same code path the VMS build gets. 7.4 Hexen's second video path [.SRC]I_VIDEOHR.C is a separate 640x480 4-bit-plane "high resolution" mode used only for Hexen's startup screen (ST_START.C) and its network wait screen. It asks SDL for 640x480 directly and is unaffected by the above. It is in Hexen's object list and no other game's. 7.5 The window title [.SRC]I_VIDEO.C:1131 builds it as M_StringJoin(window_title, " - ", PACKAGE_STRING, NULL) where window_title comes from each game's own I_SetWindowTitle (gamedescription). PACKAGE_STRING is shared by all four images, so the four windows are distinguished by the game's own description, not by the package string. Nothing was changed here; it is noted because it looks like a per-game string and is not. -------------------------------------------------------------------------------- 8. SDL TEARDOWN GUARDED WITH SDL_WasInit -------------------------------------------------------------------------------- Three sites: [.SRC]I_VIDEO.C:526 (I_ShutdownGraphics), [.TEXTSCREEN]TXT_SDL.C:297 (TXT_Shutdown), and the ordering note at [.SRC]I_ENDOOM.C:42. SDL_QuitSubSystem is documented as reference-counted, so a redundant call "should" be a no-op. UNDER DECWINDOWS IT IS NOT SAFE TO RELY ON THAT: a second video teardown reaches Xlib as X_FreeColors against an already-freed colormap, and Xlib's DEFAULT ERROR HANDLER CALLS exit(). The game would die during its own exit path - after the user quit, so the symptom is a spurious error at exit rather than an obvious crash, which makes it exactly the kind of thing that gets mis-diagnosed. This is not hypothetical here: there are TWO video-teardown sites reachable from a single I_Quit() - I_ShutdownGraphics() and TXT_Shutdown() by way of I_Endoom, which draws the ENDOOM lump as the game exits. Both are now guarded: if (SDL_WasInit(SDL_INIT_VIDEO)) { SDL_QuitSubSystem(SDL_INIT_VIDEO); } Both also NULL the display surface afterwards, since it belongs to the subsystem just dropped, so nothing can hand a dangling surface back to SDL. I_ENDOOM.C:42 additionally CHECKS TXT_Init()'s return value, which upstream ignores. Every failure path in TXT_Init leaves the text screen unusable, and the code after it would then draw through a null screendata pointer. It is also the return value section 9 makes meaningful. -------------------------------------------------------------------------------- 9. UNCHECKED ALLOCATIONS -------------------------------------------------------------------------------- Two mallocs whose results were memset through without a check. 9.1 [.SRC]I_VIDEO.C:1146 - the window icon mask The icon is cosmetic, so failure must not take the game down: the function now returns early, and the game runs without a window icon. (The surrounding code also has a related hazard noted at i_video.c:1178 - SDL_WM_SetIcon dereferences the surface immediately, so the surface must be valid at the point of the call.) 9.2 [.TEXTSCREEN]TXT_SDL.C:266 - screendata in TXT_Init Here failure IS reportable: TXT_Init already has a documented int return, so it now returns 0 and the caller backs out. That is what makes the check added in section 8 meaningful. Neither is a VMS-specific problem. They are worth fixing here because this is a platform where a modest process quota (PGFLQUOTA / VIRTUALPAGECNT) makes a failing malloc a REALISTIC outcome rather than a theoretical one - and a null-pointer memset on VMS is an ACCVIO with no useful message. -------------------------------------------------------------------------------- 10. [.SRC.HEXEN]MN_MENU.C - DUPLICATE DEFINITION OF demoextend -------------------------------------------------------------------------------- mn_menu.c:134 contained boolean demoextend; // from h2def.h which is a second DEFINITION of a variable [.SRC.HEXEN]G_GAME.C:100 already defines and h2def.h:636 correctly declares extern. The comment shows the intent was to declare, not define. Changed to rely on the extern declaration. This is an upstream bug that survives only on toolchains which quietly merge duplicate tentative definitions into one - the traditional "common block" behaviour of GCC before -fno-common became the default. A strict linker rejects it outright: multiple definition of `demoextend'; g_game.o: first defined here Worth understanding rather than just silencing: had the two objects each kept their own storage, h2_main.c's "-demoextend" switch and mn_menu.c's two resets could write a DIFFERENT variable from the one g_game.c tests, so the switch would appear to be ignored intermittently. Fixing the definition is what makes the four writers and one reader agree. -------------------------------------------------------------------------------- 11. [.SRC]M_CONFIG.C - WHERE THE .CFG FILES AND SAVEGAMES LIVE -------------------------------------------------------------------------------- GetDefaultConfigDir() gained a VMS branch (m_config.c:2110) that returns "", i.e. the CURRENT directory - the same behaviour as the existing Windows path. DEFAULT.CFG, CHOCOLATE-DOOM.CFG and the savegames therefore live in the directory the game is run from, normally the one holding the IWAD. Not the ~/.chocolate-doom/ scheme, for three reasons: 1. It would create a directory whose name begins with a dot and contains a hyphen. On an ODS-2 volume that is not a legal directory name, and even on ODS-5 it needs escaping. 2. $HOME is not normally defined in a DCL session, so getenv("HOME") returns NULL and the function would fall through to "" anyway - just without saying why. 3. It matches the sibling OpenVMS ports (Hexen, Chocolate Duke, Kyra), which are all run from inside the game data directory. Returning "" is a SUPPORTED upstream mode, not a special case: M_GetSaveGameDir() and M_SetConfigDir() already test for it, and it also suppresses the M_MakeDirectory() calls, so nothing tries to create a directory on VMS at all. Note this is guarded by #if defined(VMS) || defined(__VMS) - the one place in the port that tests for the platform - so the Linux pre-validation build keeps upstream's behaviour. -------------------------------------------------------------------------------- 12. WHAT WAS NOT CHANGED, AND WHY -------------------------------------------------------------------------------- Recorded because each of these looks like an omission: * QUOTED INCLUDES. Not one needed rewriting. DEC C maps a quoted include containing a slash onto an RMS filespec and cannot stack a relative base directory, so "foo/bar.h" and "../bar.h" both fail with %CC-F-NOINCLFILEF - the sibling Duke port had to rewrite 17 of them across 11 files. Chocolate Doom's sources contain ZERO slashed or parent-relative quoted includes, verified across all four build sets. Keep it that way. * HEADER TYPE SELF-SUFFICIENCY. A transitive include-closure sweep found no header using intN_t before something provides it, so the sibling Duke port's %CC-E-TYPEDEFNOTDEF class of failure does not arise here. * NO /POINTER_SIZE, and no pointer-size-related edits. Unlike Duke - whose Build engine stuffs pointers into int32_t and therefore must stay 32-bit - Doom keeps pointers in pointers. The two deliberate pointer/integer aliasing spots are both properly typed: the thinker function_t union in [.SRC.DOOM]P_TICK.C is a union of function POINTERS, and Hexen's specialval_t (h2def.h:175) stores its integer as intptr_t, which is why sv_save.c can round-trip a pointer through special1. * IMPLICIT FUNCTION DECLARATIONS. There are none; every function is declared before use (checked with -Wimplicit-function-declaration over all four build sets). So no header was added for this, and DESCRIP.MMS's warning-disable list has no tag for it. * THE 33 INCREMENT SITES. Section 6.8. * [.SRC]Z_ZONE.C vs Z_NATIVE.C. The port compiles Z_NATIVE.C; the reasoning is in DESCRIP.MMS. Neither file contains a crash idiom, so this is a platform-fit choice, not a compiler fix. * [.SRC]AES_PRNG.C, ICON.C, W_FILE_POSIX.C, W_FILE_WIN32.C and the SDL_mixer/SDL_net files are not compiled at all. See DESCRIP.MMS for each. ================================================================================