From 0a962a2e71e2dc3d5b088337e27e2e6a5591f3ab Mon Sep 17 00:00:00 2001 From: Pavel Solodovnikov Date: Sat, 23 Mar 2024 12:02:51 +0300 Subject: [PATCH 1/3] objmem: allow option for `objmemDestroy()` to not check references Signed-off-by: Pavel Solodovnikov --- src/mechanics.cpp | 2 +- src/objmem.cpp | 6 +++--- src/objmem.h | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/mechanics.cpp b/src/mechanics.cpp index 79c76f65f50..d56dae5777b 100644 --- a/src/mechanics.cpp +++ b/src/mechanics.cpp @@ -39,7 +39,7 @@ bool mechanicsShutdown() { for (BASE_OBJECT* psObj : psDestroyedObj) { - objmemDestroy(psObj); + objmemDestroy(psObj, true); } psDestroyedObj.clear(); diff --git a/src/objmem.cpp b/src/objmem.cpp index e491183efbd..12c3457fbf3 100644 --- a/src/objmem.cpp +++ b/src/objmem.cpp @@ -203,7 +203,7 @@ static bool checkReferences(BASE_OBJECT *psVictim) /* Remove an object from the destroyed list, finally freeing its memory * Hopefully by this time, no pointers still refer to it! */ -bool objmemDestroy(BASE_OBJECT *psObj) +bool objmemDestroy(BASE_OBJECT *psObj, bool checkRefs) { switch (psObj->type) { @@ -222,7 +222,7 @@ bool objmemDestroy(BASE_OBJECT *psObj) default: ASSERT(!"unknown object type", "unknown object type in destroyed list at 0x%p", static_cast(psObj)); } - if (!checkReferences(psObj)) + if (checkRefs && !checkReferences(psObj)) { return false; } @@ -259,7 +259,7 @@ void objmemUpdate() { if ((*it)->died <= gameTime - deltaGameTime) { - objmemDestroy(*it); + objmemDestroy(*it, true); it = psDestroyedObj.erase(it); } else diff --git a/src/objmem.h b/src/objmem.h index 924dec133d4..9946c857079 100644 --- a/src/objmem.h +++ b/src/objmem.h @@ -76,7 +76,7 @@ void objmemUpdate(); /* Remove an object from the destroyed list, finally freeing its memory * Hopefully by this time, no pointers still refer to it! */ -bool objmemDestroy(BASE_OBJECT* psObj); +bool objmemDestroy(BASE_OBJECT* psObj, bool checkRefs); /// Generates a new, (hopefully) unique object id. uint32_t generateNewObjectId(); From 95c695f95a1b7b4e26325f91f1c1ee55a63d5554 Mon Sep 17 00:00:00 2001 From: Pavel Solodovnikov Date: Tue, 12 Mar 2024 01:01:11 +0300 Subject: [PATCH 2/3] Switch `STRUCTURE` to use `PagedEntityContainer` as backing storage 1. Split the structure storage into pages containing 256 structs 2. Disable slot reuse to guard against memory-related issues when some object pointers won't get updated properly, e.g. when transitioning between the base and offworld missions. Signed-off-by: Pavel Solodovnikov --- src/display3d.cpp | 9 ++++- src/objmem.cpp | 69 ++++++++++++++++++++++++++++------ src/structure.cpp | 95 ++++++++++++++++++++++++----------------------- src/structure.h | 6 +++ 4 files changed, 119 insertions(+), 60 deletions(-) diff --git a/src/display3d.cpp b/src/display3d.cpp index 6f32364d1c0..b6c01662d9a 100644 --- a/src/display3d.cpp +++ b/src/display3d.cpp @@ -363,7 +363,8 @@ struct Blueprint STRUCTURE *psStruct = buildBlueprint(); ASSERT_OR_RETURN(, psStruct != nullptr, "buildBlueprint returned nullptr"); renderStructure(psStruct, viewMatrix, perspectiveViewMatrix); - delete psStruct; + + objmemDestroy(psStruct, false); } } @@ -692,7 +693,11 @@ STRUCTURE *getTileBlueprintStructure(int mapX, int mapY) Blueprint blueprint = getTileBlueprint(mapX, mapY); if (blueprint.state == SS_BLUEPRINT_PLANNED) { - delete psStruct; // Delete previously returned structure, if any. + if (psStruct) + { + // Delete previously returned structure, if any. + objmemDestroy(psStruct, false); + } psStruct = blueprint.buildBlueprint(); return psStruct; // This blueprint was clicked on. } diff --git a/src/objmem.cpp b/src/objmem.cpp index 12c3457fbf3..0a12bb8b345 100644 --- a/src/objmem.cpp +++ b/src/objmem.cpp @@ -226,14 +226,22 @@ bool objmemDestroy(BASE_OBJECT *psObj, bool checkRefs) { return false; } - // Droids are managed by a separate droid container. if (psObj->type == OBJ_DROID) { + // Droids are managed by a separate droid container. auto& droidContainer = GlobalDroidContainer(); auto it = droidContainer.find(*static_cast(psObj)); ASSERT(it != droidContainer.end(), "Droid not found in the global container!"); droidContainer.erase(it); } + else if (psObj->type == OBJ_STRUCTURE) + { + // Structs are managed by a separate struct container. + auto& structContainer = GlobalStructContainer(); + auto it = structContainer.find(*static_cast(psObj)); + ASSERT(it != structContainer.end(), "Structure not found in the global container!"); + structContainer.erase(it); + } else { delete psObj; @@ -452,16 +460,53 @@ void killDroid(DROID *psDel) destroyObject(apsDroidLists, psDel); } -static void freeAllDroidsImpl(PerPlayerDroidLists& droidLists) +template +struct GlobalEntityContainerTraits; + +template <> +struct GlobalEntityContainerTraits { - auto& droidContainer = GlobalDroidContainer(); - for (auto& list : droidLists) + using StorageType = DroidContainer; + + static DroidContainer& getContainer() + { + return GlobalDroidContainer(); + } + + static const char* entityName() + { + return "Droid"; + } +}; + +template <> +struct GlobalEntityContainerTraits +{ + using StorageType = StructContainer; + + static StructContainer& getContainer() + { + return GlobalStructContainer(); + } + + static const char* entityName() + { + return "Structure"; + } +}; + +template +static void freeAllEntitiesImpl(PerPlayerObjectLists& entityLists) +{ + using Traits = GlobalEntityContainerTraits; + auto& entityContainer = Traits::getContainer(); + for (auto& list : entityLists) { - for (DROID* d : list) + for (auto* ent : list) { - auto it = droidContainer.find(*d); - ASSERT(it != droidContainer.end(), "Droid not found in the global container!"); - droidContainer.erase(it); + auto it = entityContainer.find(*ent); + ASSERT(it != entityContainer.end(), "%s not found in the global container!", Traits::entityName()); + entityContainer.erase(it); } list.clear(); } @@ -470,7 +515,7 @@ static void freeAllDroidsImpl(PerPlayerDroidLists& droidLists) /* Remove all droids */ void freeAllDroids() { - freeAllDroidsImpl(apsDroidLists); + freeAllEntitiesImpl(apsDroidLists); } /*Remove a single Droid from a list*/ @@ -503,13 +548,13 @@ void removeDroid(DROID* psDroidToRemove, PerPlayerDroidLists& pList) /*Removes all droids that may be stored in the mission lists*/ void freeAllMissionDroids() { - freeAllDroidsImpl(mission.apsDroidLists); + freeAllEntitiesImpl(mission.apsDroidLists); } /*Removes all droids that may be stored in the limbo lists*/ void freeAllLimboDroids() { - freeAllDroidsImpl(apsLimboDroids); + freeAllEntitiesImpl(apsLimboDroids); } /************************** STRUCTURE *******************************/ @@ -592,7 +637,7 @@ void killStruct(STRUCTURE *psBuilding) /* Remove heapall structures */ void freeAllStructs() { - releaseAllObjectsInList(apsStructLists); + freeAllEntitiesImpl(apsStructLists); } /*Remove a single Structure from a list*/ diff --git a/src/structure.cpp b/src/structure.cpp index 450176a9da4..ac2ca417264 100644 --- a/src/structure.cpp +++ b/src/structure.cpp @@ -1414,31 +1414,27 @@ STRUCTURE *buildStructureDir(STRUCTURE_STATS *pStructureType, UDWORD x, UDWORD y } } - // allocate memory for and initialize a structure object - psBuilding = new STRUCTURE(id, player); - if (psBuilding == nullptr) - { - return nullptr; - } + // initialize the structure object + STRUCTURE building(id, player); //fill in other details - psBuilding->pStructureType = pStructureType; + building.pStructureType = pStructureType; - psBuilding->pos.x = x; - psBuilding->pos.y = y; - psBuilding->rot.direction = snapDirection(direction); - psBuilding->rot.pitch = 0; - psBuilding->rot.roll = 0; + building.pos.x = x; + building.pos.y = y; + building.rot.direction = snapDirection(direction); + building.rot.pitch = 0; + building.rot.roll = 0; //This needs to be done before the functionality bit... //load into the map data and structure list if not an upgrade Vector2i map = map_coord(Vector2i(x, y)) - size / 2; //set up the imd to use for the display - psBuilding->sDisplay.imd = pStructureType->pIMD[0]; + building.sDisplay.imd = pStructureType->pIMD[0]; - psBuilding->state = SAS_NORMAL; - psBuilding->lastStateTime = gameTime; + building.state = SAS_NORMAL; + building.lastStateTime = gameTime; /* if resource extractor - need to remove oil feature first, but do not do any * consistency checking here - save games do not have any feature to remove @@ -1452,7 +1448,6 @@ STRUCTURE *buildStructureDir(STRUCTURE_STATS *pStructureType, UDWORD x, UDWORD y if (fireOnLocation(psFeature->pos.x, psFeature->pos.y)) { // Can't build on burning oil resource - delete psBuilding; return nullptr; } // remove it from the map @@ -1486,16 +1481,18 @@ STRUCTURE *buildStructureDir(STRUCTURE_STATS *pStructureType, UDWORD x, UDWORD y #if defined(WZ_CC_GNU) && !defined(WZ_CC_INTEL) && !defined(WZ_CC_CLANG) && (7 <= __GNUC__) # pragma GCC diagnostic pop #endif - delete psBuilding; return nullptr; } } } + // Emplace the structure being built in the global storage to obtain stable address. + STRUCTURE& stableBuilding = GlobalStructContainer().emplace(std::move(building)); + psBuilding = &stableBuilding; for (int tileY = map.y; tileY < map.y + size.y; ++tileY) { for (int tileX = map.x; tileX < map.x + size.x; ++tileX) { - // We now know the previous loop didn't return early, so it is safe to save references to psBuilding now. + // We now know the previous loop didn't return early, so it is safe to save references to `stableBuilding` now. MAPTILE *psTile = mapTile(tileX, tileY); psTile->psObject = psBuilding; @@ -1612,7 +1609,7 @@ STRUCTURE *buildStructureDir(STRUCTURE_STATS *pStructureType, UDWORD x, UDWORD y if (!setFunctionality(psBuilding, pStructureType->type)) { removeStructFromMap(psBuilding); - delete psBuilding; + objmemDestroy(psBuilding, false); //better reset these if you couldn't build the structure! if (FromSave && player == selectedPlayer && missionLimboExpand()) { @@ -1834,8 +1831,6 @@ STRUCTURE *buildStructureDir(STRUCTURE_STATS *pStructureType, UDWORD x, UDWORD y STRUCTURE *buildBlueprint(STRUCTURE_STATS const *psStats, Vector3i pos, uint16_t direction, unsigned moduleIndex, STRUCT_STATES state, uint8_t ownerPlayer) { - STRUCTURE *blueprint = nullptr; - ASSERT_OR_RETURN(nullptr, psStats != nullptr, "No blueprint stats"); ASSERT_OR_RETURN(nullptr, psStats->pIMD[0] != nullptr, "No blueprint model for %s", getStatsName(psStats)); ASSERT_OR_RETURN(nullptr, ownerPlayer < MAX_PLAYERS, "invalid ownerPlayer: %" PRIu8 "", ownerPlayer); @@ -1866,55 +1861,57 @@ STRUCTURE *buildBlueprint(STRUCTURE_STATS const *psStats, Vector3i pos, uint16_t } } - blueprint = new STRUCTURE(0, ownerPlayer); + STRUCTURE blueprint(0, ownerPlayer); // construct the fake structure - blueprint->pStructureType = const_cast(psStats); // Couldn't be bothered to fix const correctness everywhere. + blueprint.pStructureType = const_cast(psStats); // Couldn't be bothered to fix const correctness everywhere. if (selectedPlayer < MAX_PLAYERS) { - blueprint->visible[selectedPlayer] = UBYTE_MAX; + blueprint.visible[selectedPlayer] = UBYTE_MAX; } - blueprint->sDisplay.imd = (*pIMD)[std::min(moduleNumber, pIMD->size() - 1)]; - blueprint->pos = pos; - blueprint->rot = rot; - blueprint->selected = false; + blueprint.sDisplay.imd = (*pIMD)[std::min(moduleNumber, pIMD->size() - 1)]; + blueprint.pos = pos; + blueprint.rot = rot; + blueprint.selected = false; - blueprint->numWeaps = 0; - blueprint->asWeaps[0].nStat = 0; + blueprint.numWeaps = 0; + blueprint.asWeaps[0].nStat = 0; // give defensive structures a weapon if (psStats->psWeapStat[0]) { - blueprint->asWeaps[0].nStat = psStats->psWeapStat[0] - asWeaponStats.data(); + blueprint.asWeaps[0].nStat = psStats->psWeapStat[0] - asWeaponStats.data(); } // things with sensors or ecm (or repair facilities) need these set, even if they have no official weapon - blueprint->numWeaps = 0; - blueprint->asWeaps[0].lastFired = 0; - blueprint->asWeaps[0].rot.pitch = 0; - blueprint->asWeaps[0].rot.direction = 0; - blueprint->asWeaps[0].rot.roll = 0; - blueprint->asWeaps[0].prevRot = blueprint->asWeaps[0].rot; + blueprint.numWeaps = 0; + blueprint.asWeaps[0].lastFired = 0; + blueprint.asWeaps[0].rot.pitch = 0; + blueprint.asWeaps[0].rot.direction = 0; + blueprint.asWeaps[0].rot.roll = 0; + blueprint.asWeaps[0].prevRot = blueprint.asWeaps[0].rot; - blueprint->expectedDamage = 0; + blueprint.expectedDamage = 0; // Times must be different, but don't otherwise matter. - blueprint->time = 23; - blueprint->prevTime = 42; + blueprint.time = 23; + blueprint.prevTime = 42; - blueprint->status = state; + blueprint.status = state; // Rotate wall if needed. - if (blueprint->pStructureType->type == REF_WALL || blueprint->pStructureType->type == REF_GATE) + if (blueprint.pStructureType->type == REF_WALL || blueprint.pStructureType->type == REF_GATE) { - WallOrientation scanType = structChooseWallTypeBlueprint(map_coord(blueprint->pos.xy())); + WallOrientation scanType = structChooseWallTypeBlueprint(map_coord(blueprint.pos.xy())); unsigned type = wallType(scanType); if (scanType != WallConnectNone) { - blueprint->rot.direction = wallDir(scanType); - blueprint->sDisplay.imd = blueprint->pStructureType->pIMD[std::min(type, blueprint->pStructureType->pIMD.size() - 1)]; + blueprint.rot.direction = wallDir(scanType); + blueprint.sDisplay.imd = blueprint.pStructureType->pIMD[std::min(type, blueprint.pStructureType->pIMD.size() - 1)]; } } - return blueprint; + auto& stableBlueprint = GlobalStructContainer().emplace(std::move(blueprint)); + + return &stableBlueprint; } static Vector2i defaultAssemblyPointPos(STRUCTURE *psBuilding) @@ -7216,3 +7213,9 @@ LineBuild calcLineBuild(STRUCTURE_STATS const *stats, uint16_t direction, Vector { return calcLineBuild(stats->size(direction), stats->type, pos, pos2); } + +StructContainer& GlobalStructContainer() +{ + static StructContainer instance; + return instance; +} diff --git a/src/structure.h b/src/structure.h index 8056d7491ce..efe34f9bec1 100644 --- a/src/structure.h +++ b/src/structure.h @@ -496,4 +496,10 @@ struct LineBuild LineBuild calcLineBuild(Vector2i size, STRUCTURE_TYPE type, Vector2i pos, Vector2i pos2); LineBuild calcLineBuild(STRUCTURE_STATS const *stats, uint16_t direction, Vector2i pos, Vector2i pos2); +// Split the struct storage into pages containing 256 structs, disable slot reuse +// to guard against memory-related issues when some object pointers won't get +// updated properly, e.g. when transitioning between the base and offworld missions. +using StructContainer = PagedEntityContainer; +StructContainer& GlobalStructContainer(); + #endif // __INCLUDED_SRC_STRUCTURE_H__ From 326aaec5b98a5a358e0987c4283efc1760818539 Mon Sep 17 00:00:00 2001 From: past-due <30942300+past-due@users.noreply.github.com> Date: Sun, 24 Mar 2024 15:04:04 -0400 Subject: [PATCH 3/3] Clear the PagedEntityContainer in objmemShutdown() --- src/objmem.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/objmem.cpp b/src/objmem.cpp index 0a12bb8b345..fc2870b7653 100644 --- a/src/objmem.cpp +++ b/src/objmem.cpp @@ -88,6 +88,8 @@ void objmemShutdown() { auto& droidContainer = GlobalDroidContainer(); droidContainer.clear(); + auto& structContainer = GlobalStructContainer(); + structContainer.clear(); } // Check that psVictim is not referred to by any other object in the game. We can dump out some extra data in debug builds that help track down sources of dangling pointer errors.