Data races

It seems there’s a lot of data races, particularly in the GUI system, for example see gist.githubusercontent.com/xyzz … tfile1.txt

Could somebody go through it and fix these? I would do it but I’m very unfamiliar with VCMI code base so this could take lots of time. Btw, some of these races do cause reproducible crashes on Android, not sure about PC.

To get a similar output just compile VCMI with -fsanitize=thread -fPIC -pie, recent versions of gcc and clang both implement this option.

Hmm… I don’t see “lot” of data races. Largely they came from 2 sources:

  1. Console handler - likely 2 threads writing to log/console in the same time. Probably log writer should be protected by mutex.

  2. Sound finished callbacks - likely due to SDL player calling that callback from another thread. Should be looked into - for some reason stack for another thread is missing.

There is also access to LOCPLINT - now that one may be dangerous if reader thread does not checks for validness.

Did you check it yourself? Because I didn’t post the whole log due to its size. There was an annoying one where current tile position was getting destroyed before it got passed to CAdvMapInt::tileHovered which actually caused crash. For now I “solved” it in my VCMI clone just by telling it to use a copy of mapPos instead of the reference which most likely indicates the presence of a race condition somewhere.

Not yet. I just looked through your log to check for something interesting.

Can you point exact location of this bug? I can’t find any commits from you on github.

Something like this may happen if we’re passing reference to value allocated on stack. I remember finding something like that during and after switch to std::bind from boost.

The method I changed is CAdvMapInt::tileHovered. It gets there from CTerrainRect::mouseMoved. I attached a gdb to my Android device and checked stack frames, in CTerrainRect::mouseMoved “pom” is correct; in CAdvMapInt::tileHovered mapPos (which is a ref to member curHoveredTile) is (-1, -1, -1). Seems to indicate a race on that variable, unless I missed something.