Brutal-OXCE 8.3.4

[Meridian]

I noticed that other deconstructors, like Pathfinding, for example, also don't contain any code. I haven't kept up with how C++ changed over the years. My assumption is that manually deleting stuff inside of deconstructors isn't really necessary with modern compilers.

That is unfortunately a very wrong assumption.

And since making a false claim is the best way to learn something on the internet because it will trigger someone to come out and correct me, I'll just claim that this is the case and therefore just deleting the contents of the destructor was the right way to fix the issue.

It's not the right way to fix the issue.

Now you are leaking (a lot of) memory and eventually will spend the entire memory (and start crashing on out-of-memory).

[Xilmi]

I've tested 8.0.2 32 and 64 bit today. No crashes and 64 bit is noticeable faster. Good shit.

Unfortunately I was able to crash 8.0.2 and have a save from which it was reproducibly happening every time. And since my "fix" in 8.0.3 was deemed not viable by everyone who looked at it, I'm still trying to find a real solution that fixes the root-cause and not just treats the symptoms while creating arguably worse side-effects.

[Xilmi]

That is unfortunately a very wrong assumption.

It's not the right way to fix the issue.

Now you are leaking (a lot of) memory and eventually will spend the entire memory (and start crashing on out-of-memory).

Yeah, I thought so. But I honestly don't know how to find and fix the cause of something that isn't a simple segmentation-fault.

My current hypothesis would be that somehow inside of the AIModule-object, which itself is an object created for BattleUnits, there still is a reference to pathfinding and that the deletion of the Pathfinding-object causes a problem. But the way this problem expresses itself is what doesn't make sense. If my hypothesis would be correct, I'd be expecting a simple segmentation fault trying to access the pathfinding-object inside of the AI-code. And also the AI shouldn't be running anymore when the mission ends.
But maybe this connection causes issues with the pathfinding-object when the BattleUnits and thus the AIModule is deleted.
It's also not clear how my changes in 8.0.0 should cause this and it wouldn't happen before because what I did there is very similar to something that already existed.

Well, at least I have a save from which the issue is reproducible. Not having a way to reliably reproduce it was an even more horrible situation to be in, when it comes to debugging it.

Can someone help me understand what's going on with the _nodes-Member in Pathfinding.cpp?
In the constructor it reserves memory for those and then fills it up with one node for every tile of the map.
The destructor does nothing.
If the memory is indeed not automatically managed, shouldn't it somehow free up those nodes?

Edit: According to ChatGPT since _nodes is a std::vector, deconstruction isn't necessary as vectors handle the deletion of their elements automatically.

[Meridian]

Edit: According to ChatGPT since _nodes is a std::vector, deconstruction isn't necessary as vectors handle the deletion of their elements automatically.

It's about what the elements are.

Code: [Select]

std::vector<PathfindingNode> _nodes;

will be cleaned up automatically (by the vector deconstructor)

Code: [Select]

std::vector<PathfindingNode*> _nodes;

would not be cleaned up automatically, you would need to first delete the content of all the pointers manually... and then just the pointers themselves would be cleaned up by the vector deconstructor.

https://stackoverflow.com/questions/12068950/c-destructors-with-vectors-pointers

[Alpha Centauri Bear]

Did you say this is reproducible? Please point me to the whole info (version, save, reproduction, etc.). I will try to pinpoint the problem if this is what you are looking for.

[Alpha Centauri Bear]

It's about what the elements are.

Code: [Select]

std::vector<PathfindingNode> _nodes;

will be cleaned up automatically (by the vector deconstructor)

Code: [Select]

std::vector<PathfindingNode*> _nodes;

would not be cleaned up automatically, you would need to first delete the content of all the pointers manually... and then just the pointers themselves would be cleaned up by the vector deconstructor.

https://stackoverflow.com/questions/12068950/c-destructors-with-vectors-pointers

You are right
In simpler words, collection with elements will create elements on the fly, and, therefore, own them, and, subsequently, destroy them itself.
Collection with pointers will borrow these pointers from existing elements somewhere, which were created by somebody else. This mysterious creator should take care of their destruction.

[Xilmi]

Did you say this is reproducible? Please point me to the whole info (version, save, reproduction, etc.). I will try to pinpoint the problem if this is what you are looking for.

You'll have to update to the last check-in of my repo, where I put back the contents of ~SavedBattleGame.

Then use the save-game I attached.

Enabled auto-play (ctrl + a).

The Aliens should kill the 3 remaining-soldiers. Once their turn is complete the crash happens.

The place it seems to happen is the vector that should clean itself. It's basically crashing within the depths of the vector's destructor.

[Alpha Centauri Bear]

Is this error stack trace? Are you able to run it from VS in debugger?

[Xilmi]

Is this error stack trace? Are you able to run it from VS in debugger?

Just tried. Apparently I need to set some /bigobs-flag to even compile in debug-mode.

Here's the complete Call stack:

    ucrtbase.dll!_invoke_watson()   Unknown
    ucrtbase.dll!_invalid_parameter_internal()   Unknown
    ucrtbase.dll!_invalid_parameter()   Unknown
    ucrtbase.dll!_invalid_parameter_noinfo_noreturn()   Unknown
>   [Inline Frame] OpenXcom.exe!std::_Adjust_manually_vector_aligned(void * &) Line 164   C++
    [Inline Frame] OpenXcom.exe!std::_Deallocate(void * _Ptr, unsigned __int64 _Bytes) Line 252   C++
    [Inline Frame] OpenXcom.exe!std::allocator<OpenXcom::PathfindingNode>::deallocate(OpenXcom::PathfindingNode * const) Line 946   C++
    [Inline Frame] OpenXcom.exe!std::vector<OpenXcom::PathfindingNode,std::allocator<OpenXcom::PathfindingNode>>::_Tidy() Line 2045   C++
    OpenXcom.exe!std::vector<OpenXcom::PathfindingNode,std::allocator<OpenXcom::PathfindingNode>>::~vector<OpenXcom::PathfindingNode,std::allocator<OpenXcom::PathfindingNode>>() Line 765   C++
    OpenXcom.exe!OpenXcom::Pathfinding::~Pathfinding() Line 67   C++
    OpenXcom.exe!OpenXcom::SavedBattleGame::~SavedBattleGame() Line 122   C++
    OpenXcom.exe!OpenXcom::SavedGame::setBattleGame(OpenXcom::SavedBattleGame * battleGame) Line 1467   C++
    OpenXcom.exe!OpenXcom::DebriefingState::init() Line 804   C++
    OpenXcom.exe!OpenXcom::Game::run() Line 169   C++
    OpenXcom.exe!SDL_main(int argc, char * * argv) Line 127   C++
    [External Code]   

[Alpha Centauri Bear]

And this is xcom1, right?

[Alpha Centauri Bear]

And I need to match your mod set, of course. The save complains I am missing some.

[Alpha Centauri Bear]

Where do you see stack trace? I don't see it in stderr.txt or in openxcom.log.

[Xilmi]

Yes, xcom1.
The missing mod-set shouldn't really have an impact. It should load anyways.
I see that right in Visual Studio. The logs don't contain info about the crash.

[Yankes]

@Xilmi
Problem could be call to `~SavedBattleGame` it self. If you double delete then you make hell break loose, as its our favorite UB and effect is unpredicted.
On some compilers work "fine" on other crash instalty. or random and after some time whole game get corrupted and break in unrelated place.
See code:

Code: [Select]

void SavedGame::setBattleGame(SavedBattleGame *battleGame)
{
delete _battleGame;
_battleGame = battleGame;
}

What if `_battleGame` and `battleGame` point same objects? or you call `setBattleGame(x); setBattleGame(x); setBattleGame(x)`?
You need check what exactly is pointed by `_battleGame`, is this valid object? or some garbage?

Overall this is one of OXC few faults, manual memory management. (mainly as it used C++98 standard), in OXCE we use C++17 that have lot of
tools that fix problem like this making ownership of pointer more explicit (like `std::unique_ptr`, use `std::optional` or dropping `*` and use `std::move`).

[Xilmi]

@Xilmi
Problem could be call to `~SavedBattleGame` it self. If you double delete then you make hell break loose, as its our favorite UB and effect is unpredicted.
On some compilers work "fine" on other crash instalty. or random and after some time whole game get corrupted and break in unrelated place.
See code:

Code: [Select]

void SavedGame::setBattleGame(SavedBattleGame *battleGame)
{
delete _battleGame;
_battleGame = battleGame;
}

What if `_battleGame` and `battleGame` point same objects? or you call `setBattleGame(x); setBattleGame(x); setBattleGame(x)`?
You need check what exactly is pointed by `_battleGame`, is this valid object? or some garbage?

Overall this is one of OXC few faults, manual memory management. (mainly as it used C++98 standard), in OXCE we use C++17 that have lot of
tools that fix problem like this making ownership of pointer more explicit (like `std::unique_ptr`, use `std::optional` or dropping `*` and use `std::move`).

The _battleGame that is being deleted in SavedGame::setBattleGame looks like a valid object. It has members like _missionType = "STR_TERROR_MISSION", which is what the mission is. And the parameter when it's called from DebriefingState::init() is just a null-pointer. So it tries to delete the current valid _battleGame-object and then set the _battleGame-pointer to nullptr.

The issue really seems to be deleting the _nodes and _altNodes-vectors. (std::vector<PathfindingNode> _nodes, _altNodes;)

According to ChatGPT it could be problematic that the PathfindingNode-object has a pointer to another PandfindingNode via PathfindingNode* _prevNode;

"The PathfindingNode class seems to contain pointers (PathfindingNode* _prevNode) that reference other PathfindingNode instances, forming a linked structure. When you're using std::vector to manage a collection of PathfindingNode instances and they contain interconnections via pointers (_prevNode), deleting these nodes within a std::vector can potentially lead to issues.

If nodes are connected in a complex manner (such as forming a linked list or a graph), deleting them within a std::vector could break those connections and cause issues when pointers are no longer valid after deletion."