From e867977543a890f2bb4dcd36c078daae8d382b53 Mon Sep 17 00:00:00 2001 From: Jan Engelhardt Date: Tue, 3 Sep 2024 11:49:20 +0200 Subject: [PATCH] scripts: resolve strict aliasing violations in level DLLs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit $GIT/scripts/LEVEL15.cpp: In function ‘void aMatCenPuzzleInit()’: $GIT/scripts/LEVEL15.cpp:833:38: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing] 833 | #define MagicMatCenSwitchSequence (*((int *)(&User_vars[17]))) $GIT/scripts/LEVEL15.cpp:834:25: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing] 834 | #define MatCenStateA (*((int *)(&User_vars[0]))) ... $GIT/scripts/Level6.cpp: In function ‘void aPriestKeyEnter(int)’: $GIT/scripts/Level6.cpp:910:47: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing] 910 | #define Var_ThereIsPlayerInPriestKeyPuzzle (*((int *)(&User_vars[7]))) Turn ``User_var`` into an array of std::variant, the latter of which can hold either float or int. Savegames do not carry the necessary type information which variant (float/int) is in use; instead, this is statically decided by the level DLL logic on a per-index basis. This approach is retained for now. A lot of ``Var_something = 0`` is used despite Var_something being logically used as float, so we need to override op= to keep the variant type as-is. --- scripts/DallasFuncs.cpp | 46 +++++++++++++++++++++++++++++------------ scripts/DallasFuncs.h | 32 ++++++++++++++++++++++++++-- scripts/LEVEL15.cpp | 30 ++++++++++++++++++--------- scripts/Level6.cpp | 28 ++++++++++++++----------- 4 files changed, 99 insertions(+), 37 deletions(-) diff --git a/scripts/DallasFuncs.cpp b/scripts/DallasFuncs.cpp index 0abcb7a3..21b5e9f6 100644 --- a/scripts/DallasFuncs.cpp +++ b/scripts/DallasFuncs.cpp @@ -708,6 +708,9 @@ * */ +#include +#include +#include #include #include #include @@ -913,7 +916,7 @@ $$END // Variables // -float User_vars[MAX_USER_VARS]; +std::vector User_vars(MAX_USER_VARS); int Spew_handles[MAX_SPEW_HANDLES]; #define MAX_SOUND_HANDLES 10 // make sure this value matches the USERTYPE definition @@ -955,7 +958,7 @@ $$USERTYPE SavedObjectSlot:19 // // Initialize vars -void dfInit() { +void dfInit(const std::initializer_list &uv_int) { int i; for (i = 0; i < MAX_SPEW_HANDLES; i++) @@ -966,6 +969,8 @@ void dfInit() { for (i = 0; i < MAX_USER_VARS; i++) User_vars[i] = 0.0; + for (auto idx : uv_int) + User_vars[idx].set_type(); for (i = 0; i < MAX_SAVED_OBJECT_HANDLES; i++) Saved_object_handles[i] = OBJECT_HANDLE_NONE; @@ -983,8 +988,12 @@ void dfSave(void *fileptr) { for (i = 0; i < MAX_SOUND_HANDLES; i++) File_WriteInt(Sound_handles[i], fileptr); - for (i = 0; i < MAX_USER_VARS; i++) - File_WriteFloat(User_vars[i], fileptr); + for (i = 0; i < MAX_USER_VARS; i++) { + if (const auto *value = std::get_if(&User_vars[i])) + File_WriteFloat(*value, fileptr); + else + File_WriteInt(std::get(User_vars[i]), fileptr); + } for (i = 0; i < MAX_SAVED_OBJECT_HANDLES; i++) File_WriteInt(Saved_object_handles[i], fileptr); @@ -1018,7 +1027,10 @@ void dfRestore(void *fileptr) { Sound_handles[i] = File_ReadInt(fileptr); for (i = 0; i < MAX_USER_VARS; i++) - User_vars[i] = File_ReadFloat(fileptr); + if (std::get_if(&User_vars[i])) + User_vars[i] = File_ReadInt(fileptr); + else + User_vars[i] = File_ReadFloat(fileptr); for (i = 0; i < MAX_SAVED_OBJECT_HANDLES; i++) Saved_object_handles[i] = File_ReadInt(fileptr); @@ -5514,10 +5526,11 @@ Parameters: $$END */ float qUserVarValue(int varnum) { - if ((varnum >= 0) && (varnum < MAX_USER_VARS)) - return User_vars[varnum]; - else - return 0.0; + if (varnum < 0 || varnum >= MAX_USER_VARS) + return 0; + if (const auto *value = std::get_if(&User_vars[varnum])) + return *value; + return std::get(User_vars[varnum]); } /* @@ -5533,10 +5546,17 @@ Parameters: $$END */ int qUserVarValueInt(int varnum) { - if ((varnum >= 0) && (varnum < MAX_USER_VARS)) - return (User_vars[varnum] + 0.5); - else - return 0.0; + if (varnum < 0 || varnum >= MAX_USER_VARS) + return 0; + if (const auto *value = std::get_if(&User_vars[varnum])) + return *value; + auto value = std::get(User_vars[varnum]); + // Check boundaries first, else float=>int conversion is UB + if (value < std::nexttoward(INT_MIN, 0)) + return INT_MIN; + if (value >= std::nexttoward(INT_MAX, 0)) + return INT_MAX; + return value + 0.5; } /* diff --git a/scripts/DallasFuncs.h b/scripts/DallasFuncs.h index 6f7715e9..ab5e504c 100644 --- a/scripts/DallasFuncs.h +++ b/scripts/DallasFuncs.h @@ -1,7 +1,35 @@ #pragma once +#include +#include +#include + +class user_var : public std::variant { + private: + using VarT = std::variant; + + public: + // For compatibility with existing code, override op= with one that + // retains the active type, requiring instead set_type<> to change it. + template void operator=(T &&v) noexcept { + VarT &self = *this; + if (std::get_if(this)) + self = static_cast(v); + else + self = static_cast(v); + } + template void set_type() noexcept { static_cast(*this) = T{}; } + void operator++(int) noexcept { std::visit([](auto &&uv) { ++uv; }, *this); } + void operator--(int) noexcept { std::visit([](auto &&uv) { ++uv; }, *this); } + template void operator+=(T &&r) noexcept { std::visit([&](auto &&uv) { uv += std::forward(r); }, *this); } + template void operator-=(T &&r) noexcept { std::visit([&](auto &&uv) { uv -= std::forward(r); }, *this); } + template bool operator==(T &&r) noexcept { return std::visit([&](auto &&uv) { return uv == std::forward(r); }, *this); } + template bool operator!=(T &&r) noexcept { return std::visit([&](auto &&uv) { return uv != std::forward(r); }, *this); } + template bool operator<(T &&r) noexcept { return std::visit([&](auto &&uv) { return uv < std::forward(r); }, *this); } + template bool operator>(T &&r) noexcept { return std::visit([&](auto &&uv) { return uv > std::forward(r); }, *this); } +}; #define MAX_USER_VARS 25 // make sure this value matches the USERTYPE definition -extern float User_vars[MAX_USER_VARS]; +extern std::vector User_vars; #define MAX_SPEW_HANDLES 50 // make sure this value matches the USERTYPE definition extern int Spew_handles[MAX_SPEW_HANDLES]; @@ -13,7 +41,7 @@ extern int Saved_object_handles[MAX_SAVED_OBJECT_HANDLES]; // System functions // -extern void dfInit(); +extern void dfInit(const std::initializer_list & = {}); extern void dfSave(void *fileptr); extern void dfRestore(void *fileptr); diff --git a/scripts/LEVEL15.cpp b/scripts/LEVEL15.cpp index b2093b4a..a16fe985 100644 --- a/scripts/LEVEL15.cpp +++ b/scripts/LEVEL15.cpp @@ -828,12 +828,12 @@ const char *GetMessage(const char *name); #define MatCenSwitchAOn 13 #define MatCenSwitchAOff 14 -#define MagicMatCenSwitchSequence (*((int *)(&User_vars[17]))) -#define MatCenStateA (*((int *)(&User_vars[0]))) -#define MatCenStateB (*((int *)(&User_vars[1]))) -#define MatCenStateC (*((int *)(&User_vars[2]))) -#define MatCenStateD (*((int *)(&User_vars[3]))) -#define MatCenStateE (*((int *)(&User_vars[4]))) +#define MagicMatCenSwitchSequence User_vars[17] +#define MatCenStateA User_vars[0] +#define MatCenStateB User_vars[1] +#define MatCenStateC User_vars[2] +#define MatCenStateD User_vars[3] +#define MatCenStateE User_vars[4] struct tMyHandle { const char *name; @@ -1053,7 +1053,11 @@ void aMatCenPuzzleInit(void) { PlayOffAnimation(MatCenSwitchG); MagicMatCenSwitchSequence = 0; - MatCenStateA = MatCenStateB = MatCenStateC = MatCenStateD = MatCenStateE = 1; + MatCenStateA = 1; + MatCenStateB = 1; + MatCenStateC = 1; + MatCenStateD = 1; + MatCenStateE = 1; aObjPlayAnim(GetMyObject(MatCenResetSwitch), 0, 4, 4.000000f, 0); } @@ -1119,7 +1123,11 @@ void aMatCenPuzzleReset(void) { SetMatCenToLevel1(MatCen5); MagicMatCenSwitchSequence = 0; - MatCenStateA = MatCenStateB = MatCenStateC = MatCenStateD = MatCenStateE = 1; + MatCenStateA = 1; + MatCenStateB = 1; + MatCenStateC = 1; + MatCenStateD = 1; + MatCenStateE = 1; aObjPlayAnim(GetMyObject(MatCenResetSwitch), 0, 4, 4.000000f, 0); } @@ -1746,6 +1754,8 @@ const char *Message_strings[NUM_MESSAGE_NAMES]; // =============== // InitializeDLL() // =============== +static constexpr std::initializer_list uservars_as_int = {17, 0, 1, 2, 3, 4}; + char STDCALL InitializeDLL(tOSIRISModuleInit *func_list) { osicommon_Initialize((tOSIRISModuleInit *)func_list); if (func_list->game_checksum != CHECKSUM) { @@ -1755,7 +1765,7 @@ char STDCALL InitializeDLL(tOSIRISModuleInit *func_list) { } ClearGlobalActionCtrs(); - dfInit(); + dfInit(uservars_as_int); InitMessageList(); // Build the filename of the message file @@ -2467,7 +2477,7 @@ int16_t LevelScript_0000::CallEvent(int event, tOSIRISEventInfo *data) { tOSIRISEVTLEVELSTART *event_data = &data->evt_levelstart; ClearGlobalActionCtrs(); - dfInit(); + dfInit(uservars_as_int); // Script 034: Level Start Initialization if (1) { diff --git a/scripts/Level6.cpp b/scripts/Level6.cpp index 0e4c92e2..d3d1735d 100644 --- a/scripts/Level6.cpp +++ b/scripts/Level6.cpp @@ -907,10 +907,10 @@ int PriestClueSound = -1; int DummyObject = -1; int DummyDefaultPos = -1; -#define Var_ThereIsPlayerInPriestKeyPuzzle (*((int *)(&User_vars[7]))) -#define Var_PriestPlayerCurrentRoom (*((int *)(&User_vars[8]))) -#define Var_PriestPlayerCurrNode (*((int *)(&User_vars[9]))) -#define Var_PriestPuzzleCurrSoundNode (*((int *)(&User_vars[10]))) +#define Var_ThereIsPlayerInPriestKeyPuzzle User_vars[7] +#define Var_PriestPlayerCurrentRoom User_vars[8] +#define Var_PriestPlayerCurrNode User_vars[9] +#define Var_PriestPuzzleCurrSoundNode User_vars[10] #define Var_PriestPuzzleCompleted User_vars[11] #define Var_PriestPuzzleGoofed User_vars[12] #define Var_SavedObject Saved_object_handles[1] @@ -1189,11 +1189,12 @@ void aPriestKeyRoomChange(void) { int GoodRoom; int num_BadRooms = 0; int curr_x, curr_y; - curr_x = PriestCorrectPath[Var_PriestPlayerCurrNode].x; - curr_y = PriestCorrectPath[Var_PriestPlayerCurrNode].y; + auto cn = std::get(Var_PriestPlayerCurrNode); + curr_x = PriestCorrectPath[cn].x; + curr_y = PriestCorrectPath[cn].y; - GoodRoom = PriestKeyRoomMap[PriestCorrectPath[Var_PriestPlayerCurrNode + 1].y] - [PriestCorrectPath[Var_PriestPlayerCurrNode + 1].x]; + GoodRoom = PriestKeyRoomMap[PriestCorrectPath[cn + 1].y] + [PriestCorrectPath[cn + 1].x]; // check room to left if (curr_x > 0) { @@ -1261,11 +1262,12 @@ void aPriestKeyRoomChange(void) { // the player moved into a good room Var_PriestPlayerCurrNode += 1; Var_PriestPlayerCurrentRoom = Player_room; - if (Var_PriestPlayerCurrNode == 16) { + cn = std::get(Var_PriestPlayerCurrNode); + if (cn == 16) { PriestPlayerSolvesPuzzle(); } else { // adjust sound Position - PriestPlaySoundAtNode(Var_PriestPlayerCurrNode + 1); + PriestPlaySoundAtNode(cn + 1); } } break; } @@ -1687,6 +1689,8 @@ const char *Message_strings[NUM_MESSAGE_NAMES]; // =============== // InitializeDLL() // =============== +static constexpr std::initializer_list uservars_as_int = {7, 8, 9, 10}; + char STDCALL InitializeDLL(tOSIRISModuleInit *func_list) { osicommon_Initialize((tOSIRISModuleInit *)func_list); if (func_list->game_checksum != CHECKSUM) { @@ -1696,7 +1700,7 @@ char STDCALL InitializeDLL(tOSIRISModuleInit *func_list) { } ClearGlobalActionCtrs(); - dfInit(); + dfInit(uservars_as_int); InitMessageList(); // Build the filename of the message file @@ -2483,7 +2487,7 @@ int16_t LevelScript_0000::CallEvent(int event, tOSIRISEventInfo *data) { tOSIRISEVTLEVELSTART *event_data = &data->evt_levelstart; ClearGlobalActionCtrs(); - dfInit(); + dfInit(uservars_as_int); // Script 006: LevelStart if (1) {