Battlescape progress and questions

I am not familiar with ways to debug memory leaks in C. Is there anywhere I can read about how to set this up on Windows?

Is there no way to get the profiler to tell you what actually leaked?

RGBImage is used by me in the interface to make up stuff like bars, for example, psi bars, or ammo display. I copied the code from research where it's used to draw a progress bar.

By leaks in the framework I meant things like this in my travis log:

Code:

[ 26%] Built target OpenApoc_Framework

Scanning dependencies of target OpenApoc_Forms

[ 27%] Generating gamestate_serialize_generated.h, gamestate_serialize_generated.cpp

/usr/include/boost/any.hpp:243:16: runtime error: downcast of address 0x60200000e7b0 which does not point to an object of type 'any::holder<const basic_string<char> >'

0x60200000e7b0: note: object is of type 'boost::any::holder<std::string>'

 01 00 80 4d  b0 35 07 5f 28 2b 00 00  98 ad 00 00 b0 60 00 00  03 00 00 00 00 00 00 04  10 00 00 00

              ^~~~~~~~~~~~~~~~~~~~~~~

              vptr for 'boost::any::holder<std::string>'

SUMMARY: AddressSanitizer: undefined-behavior /usr/include/boost/any.hpp:243:16 in

/usr/include/boost/any.hpp:243:73: runtime error: member access within address 0x60200000e7b0 which does not point to an object of type 'any::holder<const basic_string<char> >'

0x60200000e7b0: note: object is of type 'boost::any::holder<std::string>'

 01 00 80 4d  b0 35 07 5f 28 2b 00 00  98 ad 00 00 b0 60 00 00  03 00 00 00 00 00 00 04  10 00 00 00

              ^~~~~~~~~~~~~~~~~~~~~~~

              vptr for 'boost::any::holder<std::string>'

SUMMARY: AddressSanitizer: undefined-behavior /usr/include/boost/any.hpp:243:73 in

[ 27%] Building CXX object forms/CMakeFiles/OpenApoc_Forms.dir/checkbox.cpp.o

clang: warning: argument unused during compilation: '-I /home/travis/build/Istrebitel/OpenApoc'

clang: warning: argument unused during compilation: '-I /usr/include/SDL2'

clang: warning: argument unused during compilation: '-I /home/travis/build/Istrebitel/OpenApoc/dependencies/glm'

=================================================================

==11295==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 128 byte(s) in 4 object(s) allocated from:

    #0 0x507d6d in realloc (/home/travis/build/Istrebitel/OpenApoc/bin/OpenApoc_GamestateSerializeGen+0x507d6d)

    #1 0x2b285f0e16c3  (/usr/lib/x86_64-linux-gnu/libstdc++.so.6+0x6c6c3)

SUMMARY: AddressSanitizer: 128 byte(s) leaked in 4 allocation(s).

 

Those "any::holder<>" issues are a bug in boost - https://svn.boost.org/trac/boost/ticket/11632 - that's still not been fixed since it was reported nearly 2 years ago.... I kinda forgot about it as I locally patched my boost and never thought of it again.

I'm not really sure what the 'best' way to debug memory leaks on window is, but you can see the leaked objects in a dump from a linux/sanitize build (attached). It might be useful? It'll at least show the loop that's leaking. (The run was 'start game - novice - skirmish - the first ship (Assault ship) - defaults - realtime - exit battle - abandon game - quit from main menu.

EDIT: Added a log of the same thing running through valgrind (it might give better stacktraces etc. for each allocation)

Also, it seems test_serialize does /not/ leak, so it's not "just" beginBattle()/enterBattle() - maybe Tile related?

 

Okay. I see. I will google this and find out how to debug it. If it fails even when we correctly exit game then it's definitely something wrong with my code.
EDIT: Maybe some of the tools you use are multi-platform? I tried to do it on windows. Deleaker, a tool that supposedly does this, and costs $100, crashes when I try to use it with OpenApoc, and using native VS 2015 tools crashes after it's over. It helps with just loading city and going back to main menu, but fails if I try to enter battle and leave, it either says it encountered a "failfast" or it just shows gibberish - like a heap of 8 gigs, and no deallocations, and total time of 100 000 seconds.

The question of inventory still remains. In which state is agent inventory currently? It would be awesome to finally release something playable for our followers. Inventory (well, and fixing this memory leak) is what holds us back. Maybe if you don't have any time to finish it in the foreseeable future you could give the code over to me and quickly explain what's done already so I could finish it?

 

On topic of memory leaks.

Finally native VS 2015 tool didn't fail. I did a comparison between heap before opening skirmish screen and after returning to city screen. Here's what I found (will keep updating). I am not sure if it's "leak" because VS 2015 tool just shows you what was allocated between the snapshots. So what I know is that something was allocated and was not deallocated, I do not know wether it is a leak.

1) Most of the stuff is images. Images loaded when forms are loaded, when unit image packs are loaded etc. I believe this is because framework caches loaded images and doesn't unload them. I believe this should not be a leak?

2) BattleHazard.expand leaks a total of 4 kbs of something in "external code". Object type is void and sizes are 64 and 32 bytes. I'm not certain of the meaning and I'm not certain how can it leak but it must be a leak because it should not allocate anything that is no de-allocated before returning to city.
https://github.com/Istrebitel/OpenApoc/blob/master/game/state/battle/battlehazard.cpp at 58 is the beginning of the method.
I do not know what can leak there. I don't see anything suspicious other than creating other hazards but that's done in battle's method so supposedly this wouldn't say "External code" it would say it's leaking in "placehazard" code, right?
EDIT: Maybe it's the const array that I use in the method? Maybe this is what is allocated and not deallocated?

3) TileObject.setPosition is leaking objects with size of 8 bytes and TileObject_BattleUnit.setPosition is leaking objects with size of 40 bytes. Again, says "external code" so I dunno what exactly leaks them and what they are. Again, looking at the code, nothing there could leak ?

4) And finally, yes, BattleUnits for some reason stay allocated after battle is destroyed. Will investigate further.

 

And btw, for some reason travis stopped working or me. Here's the log: https://travis-ci.org/Istrebitel/OpenApoc/jobs/234371908
EDIT: For some reason, works again... huh?

 

Okay, basically, I've spent some time and it seems to be there are no tools suitable for debugging leaks in OpenApoc on Windows. Here's the facts.

1) LeakSanitizer from clang and others is not available for windows for some reason. Only linux.

2) Native memory diagnostics in VS 2015 fail to work with OpenApoc.

Its about 50% chance to work if you snapshot only twice (one run of battle), meaning you snapshot when in main menu and when back to main menu, or when in city and when back to city. 50% of the time results will be botched (no mem graph, 17gb heap with only 8 gb memory on my pc, bogus values everywhere). 50% time they will be correct. However, most leaks seem to come from images, and framework caches them, so maybe they aren't leaks actually? Aside from images, most newly allocated and not deallocated stuff comes from functions that have static vars in them (yes, this tool doesn't actually show leaks, it can only show what was allocated and wasn't deallocated). I assume that's intended (they are alloced when first accessed and will be dealocced on program exit).

So you really need to run game, enter - exit battle once, snapshot, then enter-exit again and snapshot again.

This would give clean snapshot of what's leaking because no new images are to be loaded and no new statics are to be alloced on a second run.

But it doesn't work this way, it fails 10% of the time like above, 90% of the time just "fast fail program will exit" when trying to open diagnostic file.

This tool works fine with smaller programs though.

3) DrMemory is insanely slow in release (like 20 minutes to enter-exit battle once) and seems to require deubg because it shows ??? everywhere which i assume means it doesn't understand what code line it is. I'll try it in debug but I assume it will be horrendeously slow (several hours for a run, unpractical)

4) The only untried solution is Deleaker which costs $100 for full and demo won't show where it leaked. Couldn't find a pirated version which works with 2015. Not willing to pay $100 especially not knowing whether it'll work

So, erm... Have I missed some other option?
...

Is there a way to test this using travis? Like, make it run a test with sanitize so that it gives the output like that which you uploaded here?

EDIT: It seems that another way of using native tools in VS 2015 kinda works. It shows the same "diff" between snapshots, so I can confirm between first and second battle, what was allocated and wasn't deallocated. It shows a lot of "void" but otherwise it properly shows 12 new agents and 100 new equipment (makes sense, as that's 12 new agents created by skirmish mode and their gear), and doesn't show any battleunits or something that shouldn't be there. I'll be using this meanwhile

 

Also, about destroy() method for staterefs... is it to be manually called before erasing a stateref object from staterefmap?

If yes, maybe we should have a certralised method of removing stateref objects from staterefmaps? Because it's hardly ever called.

If no, then how exactly does it work?

 

I have pushed to GitHub changes that, at least using the limited clunky tools I have, seem to have fixed the situation with mem leaks. Now when I compare memory between first and second run of battle (cycle is new game-> novice ->start skirmish -> exit to city -> first memory snapshot -> start skirmish -> exit to city -> second memory snapshot) it seems that there's nothing wrong. Only new objects are new agents (12 of them) and their equipment (~110 total items). That is fine, as the agents are not to be removed after skirmish (they are on the base). It doesn't show any battle units or other battle objects persisting.

I have fixed a leak with battleunits that made battle units stay alive after being erased - there was a lot of circular refs. I have also (could you confirm it was proper way to do it) fixed agents staying alive - now destroy is called for agents in gamestate's destructor. Maybe I was wrong to do that, but I was seeing alien agents staying alive after battle before I started calling destroy on them before erasing them from agent staterefmap, so I decided to call it manually and now these agents properly go away. And I don't see destroy called anywhere else so I added it to gamestate destructor.

Could you please run that with the sanitizer and see if something leaks?

 

Still leaking for me: attached warnining log (including your extra ~Battle() debug messages and the ASAN exit messages)

EDIT: To be fair, this weird cyclic tree of sp<> objects appears to be rather bug-prone. I might try to tweak the ownership so there's a "weak" StateRef<> or something, that should hopefully solve this sort of stuff... maybe?

 

Isn't "weak" more expensive on every call? like, it has to confirm the object still there, and only then can it access it?

If it isn't more expensive to use "weak" pointers, we should actually do just that, and only store unique pointer in the staterefmap

Unless, of course, we're talking about city projectiles. City projectile has a "Firer" and that "firer" is a vehicle and if it is destroyed before projectile hits then there is no firer anymore.

Maybe we just clean up dead objects in our staterefmaps on some regular basis, not immediately (vehicle dies -> erase it)

Another idea:
- Use shared pointer when we need to keep object alive and we know for sure it's going to go our of scope soon
- Use simple pointer in StateRef

No StateRef should be valid if object was erased from StateRefMap. So why check? We could just use simple pointer in StateRef.

However, now that I've checked, we have a firerVehicle (previously just firer) which StateRefs to Vehicle, which if you save the game after firer dies but before projectile expires and then re-load should crash because there's no such ID in the staterefmap because vehicle is erased when it's dead. But if you don't save-load it works fine because StateRef stores sp to the vehicle.

Come to think of it, how do you solve this?
I think we store all the stuff we must know about the shot regardless of the firer change. Like, organization that fired. Because otherwise, what about this:

BattleUnit firer fires a projectile, gets MCed, projectile hits, now loss of relationship should apply to new owner? That would allow, for example, to do the following: MC alien, Fire at organisation unit, cease MC before projectile hits, organisation now hates aliens more! Well that is if aliens and org units spawn on same map, but when org is hostile, they do (if you alien exterminate a building and org is hostile guards spawn as well).

That should not happen. Original firer organisation should be referenced here as the attacker. Hence, StateRef for firer in projectiles is invalid. As it is in everything else related to attacks (explosives? something else?). We should store organization for all relationship purposes and store firer only for purpose of accessing the firer - that is, experience, or to tell the unit which unit attacked it (so it can turn or return fire at the unit). This link, we can afford to die. If vehicle was destroyed before its projectile hit, it doesn't matter which vehicle fired the projectile, there's nowhere to return fire anymore, and if unit firer died, there's noone to give XP to.

My conslusion:

I think best idea is, if weak pointer is not more expensive than shared pointer, to use that. If not use simple pointer.

StateRef should work consistently over save-load cycle. That means, if we save-load every tick, everything should be the same as if we don't. That means, we should expect StateRef to NOT work if refered object was erased from StateRefMap. That means, StateRef is by definition a weak pointer - it does not prevent object from being erased from memory.

In places where we want object to persist, we should instead refer to specific properties of the object (like, Org that fired the shot). This will be consistent over save-load cycle.

If weak pointer is more expensive than shard, we should just use simple pointers. Then we only check if object was erased manually in cases where it could happen. Like, there NEVER should be a situation when battleunit's agent is erased, or item's agent/unit owner is erased, or vehicle's organisation is erased. So we don't check. But we do check in projectile if firerVehicle is erased (not unit, since units are not erased until battle is over).

 

Okay, so the leak definitely was agent's destroy not called.

Skirmish erases agents from base and replaces then with what you chose. This didn't call destroy() and thus it was those agents that leaked actually. Well that and leaking battle units that were fixed in the previous fix.

I added a logwarning to agent's destructor and sure enough, it's called for those agents that participated in battle when you exit to main menu. It isn't called for those agents that were erased by skirmish though.

So question remains, what was originally supposed to call destroy() on StateRefs? Or are we supposed to call destroy() on StateRefs manually?

 

I guess I should compile the list of questions to make it easier since I wrote so much:

1) Agent Inventory, is there an ETA, or should I take over and finish it? Want to release something playable within a month.
2) Are any of the tools you use to check leaks cross-platform?
3) What was originally supposed to call destroy() on StateRefs? Or are we supposed to call destroy() on StateRefs manually? Why isn't it called anywhere then? I am manually calling agent.destroy() before erasing - is this correct?
4) Please confirm no more leaks (I pushed to my github)

 

Okay I'm doing inventory. Nearly done actually, all I have to do is figure out how to neatly implement five different sources of items on "ground" (battle, vehicle, base, building or just "near agent temporarily while you're messing with it"). And we have a working inventory system. I hope to do it in a couple of days.

 

Okay, done. Unless it breaks under testing tomorrow, we have a working inventory (working in all cases, just need to implement detection after we implement agents moving in vehicle or on their own, so for now only battle and base mode works).

So, should we release a playable alpha now?

 

if its can be realeased and after this anyone
can download it copy in OpenApoc folder orig Apoc game
and just launch and start skirmish and play

then YES)