DLL development discussions

[JosEPh_II]

We have two different CTD's reported. Today i had 2 AIAutoPlay's running and played one game myself in sum about 4000 turns but i never saw one of them. The only thing i found was that sometimes NO_BUILDING is built i found three places in the code there this seems possible.

The one CTD reported by Joe is in CvUnit::killUnconditional but it never happens to me not even with the saves he posted. This makes it very difficult to say if possible changes fix it or not or....

The other one reported by Strategyonly is very i never had something like this happening.

		// if no longer automated, then we want to bail
		return (!isDelayedDeath() && !getGroup()->isAutomated());
05732400  mov         ecx,esi  
[COLOR="Red"]05732402  call        CvUnitInfo::isNoRevealMap (05733BE0h) [/COLOR] 
05732407  test        al,al  
05732409  jne         $L228152+245h (0573241Dh)  
0573240B  mov         ecx,esi  
0573240D  call        CvUnit::getGroup (05697670h)  
05732412  mov         ecx,eax  
05732414  call        CvSelectionGroup::isAutomated (056613E0h)  
05732419  test        al,al  
0573241B  je          $L228152+29Ah (05732472h)  
0573241D  pop         edi  
0573241E  pop         ebp  
0573241F  pop         esi

Very strange i would like to help more but i'am very very burned out this week sorry.

So far All of my CTDs have been during the End of Turn wait. And so far even with the savegae and Mini I posted I have been able to eventually continue the game. (I've posted 2 more minidumps since that particular save.)

I know that these kind of CTDs are real pain in the *** types to find and correct.

It was were I had to save every turn or I would CTD. Now I can go up to 4 turns (but so far not past) without saving and not CTD. But if I forget and and don't save every 4th turn I will CTD.

I have also noticed that graphics paging is taking a bit longer to fill in the new location after a map jump. Related? Who knows?

Current computer specs are: win XP Pro x64 on a Dual core E7400 Wolfdale IBM CPU on a Gigabyte Sata Mobo with 8GB of DDR2 1066Mhz ram with an ATI Radeon HD6450 with 1GB of DDR3 ram.

My i7 2600K Win 7 Pro 64 with 8GB 1333Mhz DDR3 is down with a bad mobo. Or I would play this game on it too for comparison.

Not sure if any of this info helps at all, but here it is for reference.

JosEPh

 

[Thunderbrd]

We have two different CTD's reported. Today i had 2 AIAutoPlay's running and played one game myself in sum about 4000 turns but i never saw one of them. The only thing i found was that sometimes NO_BUILDING is built i found three places in the code there this seems possible.

The one CTD reported by Joe is in CvUnit::killUnconditional but it never happens to me not even with the saves he posted. This makes it very difficult to say if possible changes fix it or not or....

The other one reported by Strategyonly is very i never had something like this happening.

		// if no longer automated, then we want to bail
		return (!isDelayedDeath() && !getGroup()->isAutomated());
05732400  mov         ecx,esi  
[COLOR="Red"]05732402  call        CvUnitInfo::isNoRevealMap (05733BE0h) [/COLOR] 
05732407  test        al,al  
05732409  jne         $L228152+245h (0573241Dh)  
0573240B  mov         ecx,esi  
0573240D  call        CvUnit::getGroup (05697670h)  
05732412  mov         ecx,eax  
05732414  call        CvSelectionGroup::isAutomated (056613E0h)  
05732419  test        al,al  
0573241B  je          $L228152+29Ah (05732472h)  
0573241D  pop         edi  
0573241E  pop         ebp  
0573241F  pop         esi

Very strange i would like to help more but i'am very very burned out this week sorry.

Quite understandable. I MAY be making some interesting headway with SO's crash but I can't be sure yet. I have some new theories (and methods to dig into things further) I'm investigating at the moment.

 

[olduser]

Quite understandable. I MAY be making some interesting headway with SO's crash but I can't be sure yet. I have some new theories (and methods to dig into things further) I'm investigating at the moment.

I set a Breakpoint in that line. With the DLL i commited today i don't see that error in the diassembly but when i use your last DLL it has the error shown in the diassembly. Then i use one of the saves with this error the game still crashes even without the wrong call in the diassembly, it is possible that a save created with a corrupt DLL is also corrupt.

 

[strategyonly]

it is possible that a save created with a corrupt DLL is also corrupt.

So essentially the game is no good any longer?

 

[Thunderbrd]

I set a Breakpoint in that line. With the DLL i commited today i don'd see that error in the diassembly but when i use your last DLL it has the error shown in the diassembly. Then i use one of the saves with this error the game still crashes even without the wrong call in the diassembly, it is possible that a save created with a corrupt DLL is also corrupt.

Could also mean there's another problem somewhere and that the error we're seeing is a red herring.

When I 'backed out' of that breakpoint I saw some interesting stack reports but I'm not familiar enough with the backout feature to know what I'm seeing.

Oddly too, when I had stopped the game at that breakpoint, then went to the isDelayedDeath function and attempted to put a breakpoint there, it automatically sent me to the isNoRevealMap function (which all the more odd is actually unitINFOS not unit.cpp!) where it then placed the attempted breakpoint instead!!!

One thing that struck me too when I looked around after stopping at the isDelayedDeath call breakpoint was that other function calls wouldn't give any hover information. When I stopped with a headache last night I was wondering if the underlying unit being evaluated during this ai update was actually already killed or was somehow designated as a NULL unit thus wasn't able to be referenced and attempts to do so might've been passing all the way to core unit data since there was no memory space allocated for the isDelayedDeath call. Meh... probly don't have a clue but it was my going theory.

BTW, how did you compile a dll that doesn't show the error? And could this be a signal that something is wrong with my compiler? What would you want to be that I could take the same code files you have, compile and then get the same error showing up again?

 

[olduser]

Could also mean there's another problem somewhere and that the error we're seeing is a red herring.

When I 'backed out' of that breakpoint I saw some interesting stack reports but I'm not familiar enough with the backout feature to know what I'm seeing.

Oddly too, when I had stopped the game at that breakpoint, then went to the isDelayedDeath function and attempted to put a breakpoint there, it automatically sent me to the isNoRevealMap function (which all the more odd is actually unitINFOS not unit.cpp!) where it then placed the attempted breakpoint instead!!!

One thing that struck me too when I looked around after stopping at the isDelayedDeath call breakpoint was that other function calls wouldn't give any hover information. When I stopped with a headache last night I was wondering if the underlying unit being evaluated during this ai update was actually already killed or was somehow designated as a NULL unit thus wasn't able to be referenced and attempts to do so might've been passing all the way to core unit data since there was no memory space allocated for the isDelayedDeath call. Meh... probly don't have a clue but it was my going theory.

BTW, how did you compile a dll that doesn't show the error? And could this be a signal that something is wrong with my compiler? What would you want to be that I could take the same code files you have, compile and then get the same error showing up again?

One possibe reason could be not doing this.

I strongly suggest that from now on we always perform a Full Rebuild when we build the Dll in the Release or Final_Release configurations. Just using Build can lead to such problems because of the Compiler Optimizations.

Because i think that i had the same error in one of my commits.

 

[AIAndy]

Sorry for the late answer.
I'am interested in this but first i want to finish the things i work on right now.

I have checked in a branch with the source changes I made now. You can find it under Branches/NewerBoostSources.

It requires that you build a version of Boost (I used 1.44.0) with MSVC 7.1 and reference the right directories in MakefilePaths.
A helper DLL which is built with the 1.32.0 Boost from Civ4 has its files in the Helper subdirectory. It needs to be built and then placed in the Asset directory same as the normal C2C DLL.
I used VS2013 so only that project file has changes but that should only matter for the helper DLL as there are no important changes to the project file of the C2C DLL.

The current problem is that some calls from Python into the DLL cause exceptions and I can't properly debug it as my debugger gets caught in some false breakpoints and does not continue which are probably caused by some heap corruption issue.

 

[Thunderbrd]

Alright so I'm a bit embarrassed that I STILL don't understand this caching mechanism in use here.

But I WAS able to determine that it's returning a 0 in this function every time it's called after the first time in a given round.

My question is WHY?!? This is really quite an important function and with the caching returning a result of 0 no matter what it's causing serious problems (problems I wonder if are present in many OTHER areas of code where this method is employed!)

Here's the function:

int CvArea::getNumRevealedTerrainTiles(TeamTypes eTeam, TerrainTypes eTerrain) const
{
	int iResult;

	EnterCriticalSection(&m_cPlotTypeCacheSection);

	if ( m_iCachedTurnPlotTypeCounts != GC.getGameINLINE().getGameTurn() ||
		 m_eCachedTeamPlotTypeCounts != eTeam )
	{
		m_plotFeatureCountCache.clear();
		m_plotTerrainCountCache.clear();
	}

	m_iCachedTurnPlotTypeCounts = GC.getGameINLINE().getGameTurn();
	m_eCachedTeamPlotTypeCounts = eTeam;

	std::map<TerrainTypes,int>::const_iterator	itr = m_plotTerrainCountCache.find(eTerrain);

	bool bHaveCachedResult = (itr == m_plotTerrainCountCache.end());

	LeaveCriticalSection(&m_cPlotTypeCacheSection);

	if ( bHaveCachedResult)
	{
		iResult = itr->second;
	}
	else
	{
		iResult = 0;

		for(int iI = 0; iI < GC.getMapINLINE().numPlotsINLINE(); iI++)
		{
			CvPlot*	pPlot = GC.getMapINLINE().plotByIndexINLINE(iI);
			if ( pPlot != NULL &&
				 pPlot->area() == this &&
				 pPlot->isRevealed(eTeam, false) &&
				 pPlot->getTerrainType() == eTerrain )
			{
				iResult++;
			}
		}

		EnterCriticalSection(&m_cPlotTypeCacheSection);
		m_plotTerrainCountCache.insert(std::make_pair(eTerrain, iResult));
		LeaveCriticalSection(&m_cPlotTypeCacheSection);
	}

	return iResult;
}

So... can anyone see where the error might be? I REALLY need to read up on #maps again! I'd probably understand it this time.

 

[AIAndy]

The logic in this line looks inverted:

bool bHaveCachedResult = (itr == m_plotTerrainCountCache.end());

When the entry is not found in the map, the iterator points at the end, but if it is found, it will point at the actual entry, not at the end. So this should be:

bool bHaveCachedResult = (itr != m_plotTerrainCountCache.end());

 

[Thunderbrd]

That makes sense even with my limited understanding. THANKS!

 

[Thunderbrd]

Ok, here's another mystery to me...

Why on Earth, whenever a building adds a resource, must it go through the whole connected network and remove one of all resources in the network, then add the given resource to the network, only to then return one of all resources in the network??? Why aren't we simply adding this one to the network?

This seems to be creating some compilation errors in how many resources may exist in the network after its all said and done. This assertion I'm still working on proving and its possible it might not be causing the trouble I think it might. But it does seem like a LOT of processing that's totally unnecessary!

void CvCity::changeFreeBonus(BonusTypes eIndex, int iChange)
{
	FAssertMsg(eIndex >= 0, "eIndex expected to be >= 0");
	FAssertMsg(eIndex < GC.getNumBonusInfos(), "eIndex expected to be < GC.getNumBonusInfos()");

	if (iChange != 0)
	{
		GET_PLAYER(getOwnerINLINE()).startDeferredPlotGroupBonusCalculation();

		plot()->updatePlotGroupBonus(false);
		m_paiFreeBonus[eIndex] += iChange;
		FAssert(getFreeBonus(eIndex) >= 0);
		plot()->updatePlotGroupBonus(true);

		GET_PLAYER(getOwnerINLINE()).endDeferredPlotGroupBonusCalculation();
	}
}

void CvPlot::updatePlotGroupBonus(bool bAdd)
{
	PROFILE_FUNC();

	CvCity* pPlotCity;
	CvPlotGroup* pPlotGroup;
	BonusTypes eNonObsoleteBonus;
	int iI;

	if (!isOwned())
	{
		return;
	}

	pPlotGroup = getPlotGroup(getOwnerINLINE());

	if (pPlotGroup != NULL)
	{
		pPlotCity = getPlotCity();

		if (pPlotCity != NULL)
		{
			PROFILE("CvPlot::updatePlotGroupBonus.PlotCity");

			for (iI = 0; iI < GC.getNumBonusInfos(); ++iI)
			{
				if (!GET_TEAM(getTeam()).isBonusObsolete((BonusTypes)iI))
				{
					pPlotGroup->changeNumBonuses(((BonusTypes)iI), (pPlotCity->getFreeBonus((BonusTypes)iI) * ((bAdd) ? 1 : -1)));
				}
			}

			if (pPlotCity->isCapital())
			{
				PROFILE("CvPlot::updatePlotGroupBonus.Capital");

				for (iI = 0; iI < GC.getNumBonusInfos(); ++iI)
				{
					pPlotGroup->changeNumBonuses(((BonusTypes)iI), (GET_PLAYER(getOwnerINLINE()).getBonusExport((BonusTypes)iI) * ((bAdd) ? -1 : 1)));
					pPlotGroup->changeNumBonuses(((BonusTypes)iI), (GET_PLAYER(getOwnerINLINE()).getBonusImport((BonusTypes)iI) * ((bAdd) ? 1 : -1)));
				}
			}

			if ( pPlotCity->getOwnerINLINE() == getOwnerINLINE() )
			{
				PROFILE("CvPlot::updatePlotGroupBonus.Buildings");

				for (iI = 0; iI < GC.getNumBuildingInfos(); ++iI)
				{
					if (GC.getBuildingInfo((BuildingTypes)iI).getFreeTradeRegionBuildingClass() != NO_BUILDINGCLASS)
					{
						if ( pPlotCity->getNumBuilding((BuildingTypes)iI) > 0 )
						{
							BuildingTypes eFreeBuilding = (BuildingTypes)GC.getCivilizationInfo(GET_PLAYER(pPlotCity->getOwnerINLINE()).getCivilizationType()).getCivilizationBuildings(GC.getBuildingInfo((BuildingTypes)iI).getFreeTradeRegionBuildingClass());

							pPlotGroup->changeNumFreeTradeRegionBuildings(eFreeBuilding, ((bAdd) ? 1 : -1));
						}
					}
				}
			}
		}

		eNonObsoleteBonus = getNonObsoleteBonusType(getTeam());

		if (eNonObsoleteBonus != NO_BONUS)
		{
			PROFILE("CvPlot::updatePlotGroupBonus.NonObsoletePlotBonus");

			if (GET_TEAM(getTeam()).isHasTech((TechTypes)(GC.getBonusInfo(eNonObsoleteBonus).getTechCityTrade())))
			{
// Super forts C2C adaptation
				//	Now that improvements are separately tagged as to whether they are universal resource providers
				//	the 'fort' check here is not needed
				if (isCity(false, getTeam()) ||
// Super forts C2C adaptation end
					((getImprovementType() != NO_IMPROVEMENT) && GC.getImprovementInfo(getImprovementType()).isImprovementBonusTrade(eNonObsoleteBonus)))
				{
					if ((pPlotGroup != NULL) && isBonusNetwork(getTeam()))
					{
						pPlotGroup->changeNumBonuses(eNonObsoleteBonus, ((bAdd) ? 1 : -1));
					}
				}
			}
		}
	}
}

I just cannot make heads or tails of the logic here... I can understand if it wants to blank it out and rebuild the count on the network from scratch each time (though even that seems excessive!) But why it wants to just subtract one of everything and then add one of everything after it's added the new bonus into the count is just... huh?

 

[AIAndy]

The idea here seems to be to remove all the current effect of the plot on the plot group, then make the change locally and finally readd the effect of the plot to the plot group.
You could probably change that to work incremental but that needs good understanding of the mechanisms involved.

 

[Thunderbrd]

Ok, further reading it over again and I get it.

hmm... so we're having an issue with sulfur disappearing on some games - reducing gradually and going into the negative even.

So I'm thinking... it looks like if one were to change the value of m_paiFreeBonus[eIndex] directly without doing these update calls, you could end up causing something like this.

Now, I can't see anywhere in the dll where this would be taking place. And I don't think python can directly manipulate that variable without a call to changeFreeBonus right? so... hmm...

 

[Thunderbrd]

Ok, another odd issue I'm at a loss for here:

		// Sanguo Mod Performance start, added by poyuzhe 07.27.09
	UnitTypes eUnit;
	std::vector<UnitTypes> aUpgradeUnits;

	for (iI = 0; iI < GC.getNumUnitInfos(); iI++)
	{
		eUnit = (UnitTypes)iI;
		aUpgradeUnits.clear();
		do
		{
			for (int iJ = 0; iJ < GC.getNumUnitClassInfos(); iJ++)
			{
				if (GC.getUnitInfo(eUnit).getUpgradeUnitClass(iJ))
				{
					GC.getUnitInfo((UnitTypes)iI).addUpgradeUnitClassTypes(iJ);
					aUpgradeUnits.push_back((UnitTypes)GC.getUnitClassInfo((UnitClassTypes)iJ).getDefaultUnitIndex());
				}
			}
			if (!aUpgradeUnits.empty())
			{
				eUnit = aUpgradeUnits.front();
				aUpgradeUnits.erase(aUpgradeUnits.begin());
			}
			else
			{
				break;
			}
		}while(true);
	}

if (GC.getUnitInfo(eUnit).getUpgradeUnitClass(iJ))

is sending a -1 value for eUnit which can certainly be problematic of course. The really wierd thing about this though is that

eUnit = (UnitTypes)iI;

iI shows a value of 151. So how is it that we can be coming up with a NO_UNIT result when there's a valid index here?

I suppose the easy fix here is to create eUnit != NO_UNIT qualifiers to avoid the problem entirely, ala:

	for (iI = 0; iI < GC.getNumUnitInfos(); iI++)
	{
		eUnit = (UnitTypes)iI;
		aUpgradeUnits.clear();
		do
		{
			for (int iJ = 0; iJ < GC.getNumUnitClassInfos(); iJ++)
			{
				if (eUnit != NO_UNIT)
				{
					if (GC.getUnitInfo(eUnit).getUpgradeUnitClass(iJ))
					{
						GC.getUnitInfo((UnitTypes)iI).addUpgradeUnitClassTypes(iJ);
						aUpgradeUnits.push_back((UnitTypes)GC.getUnitClassInfo((UnitClassTypes)iJ).getDefaultUnitIndex());
					}
				}
			}
			if (!aUpgradeUnits.empty())
			{
				if (eUnit != NO_UNIT)
				{
					eUnit = aUpgradeUnits.front();
					aUpgradeUnits.erase(aUpgradeUnits.begin());
				}
			}
			else
			{
				break;
			}
		}while(true);
	}

That said, I can't be sure this is quite right either since it does appear that perhaps the only way that eUnit would've taken on a -1 value is here:
eUnit = aUpgradeUnits.front();

Not being QUITE sure how .front() works I'm a little wary of that bit.

Anyhow, can anyone else see the deeper issue here I might be missing? Or is it possible that a valid unit index can actually have a null unit value? (And if so... what could explain that? I would think it could be something modular like maybe I don't have an active module the player that generated this save has or a replacement (though I don't THINK we have any units using replacements yet...))

 

[AIAndy]

front() just returns the first element of the vector. It looks like the vector is used as a FIFO here (which btw is not very efficient in this way).
So the -1 was added in this line and then later retrieved and put into eUnit:

aUpgradeUnits.push_back((UnitTypes)GC.getUnitClassInfo((UnitClassTypes)iJ).getDefaultUnitIndex());

So the source of the problem is that not all unit classes have a default unit index set and the default value is NO_UNIT. So best check for NO_UNIT here before pushing it into the vector.

 

[Thunderbrd]

Ahh... yes. I often don't even spot it when we swap from discussing unit to unitclass.

So like:

					if (GC.getUnitInfo(eUnit).getUpgradeUnitClass(iJ))
					{
						GC.getUnitInfo((UnitTypes)iI).addUpgradeUnitClassTypes(iJ);
						if (GC.getUnitClassInfo((UnitClassTypes)iJ).getDefaultUnitIndex() != NO_UNIT)
						{
							aUpgradeUnits.push_back((UnitTypes)GC.getUnitClassInfo((UnitClassTypes)iJ).getDefaultUnitIndex());
						}
					}

should sort this out sufficiently then?

 

[Thunderbrd]

Ok, so my first attempt to address this works after a cache delete (not doing so will result in a hang). The second attempt there that you guided me to actually makes all units a civ specific unit of all defined civs (a horrific effect in the hover infos on units!)

So I'm reverting to the previous solution I gave.

 

[Thunderbrd]

ACK! Stumped again! (But I have a theory...)

So... a barbarian city defender is upgrading and it hits this line:

	if ( !bGroup && (getGroup() == NULL || getGroup()->getNumUnits() > 1) )
	{
		joinGroup(NULL, true);
	}

A third parameter in joinGroup is bRejoin and it's set by default to true.

So it follows joinGroup(NULL, true); to:

void CvUnit::joinGroup(CvSelectionGroup* pSelectionGroup, bool bRemoveSelected, bool bRejoin)
{
	PROFILE_FUNC();

	CvSelectionGroup* pOldSelectionGroup;
	CvSelectionGroup* pNewSelectionGroup;
	CvPlot* pPlot;

	pOldSelectionGroup = GET_PLAYER(getOwnerINLINE()).getSelectionGroup(getGroupID());

	if ((pSelectionGroup != pOldSelectionGroup) || (pOldSelectionGroup == NULL))
	{
		pPlot = plot();

		if (pSelectionGroup != NULL)
		{
			pNewSelectionGroup = pSelectionGroup;
		}
		else
		{
			if (bRejoin)
			{
				pNewSelectionGroup = GET_PLAYER(getOwnerINLINE()).addSelectionGroup();
				pNewSelectionGroup->init(pNewSelectionGroup->getID(), getOwnerINLINE());
			}
			else
			{
				pNewSelectionGroup = NULL;
			}
		}

And as soon as it hits the section within the bRejoin boolean switch it goes after the group id and crashes. Interestingly enough it looks like the group id may well be valid and yet it still crashes. There's admittedly over 1 million in the ID value though so we may be hitting an overload in the number of barbarians I think. There does seem to be a LOT of barbs in this game.

pNewSelectionGroup = GET_PLAYER(getOwnerINLINE()).addSelectionGroup(); only has one appearance aside from it's function itself and this is it. It reads:

CvSelectionGroup* CvPlayer::addSelectionGroup()
{
	return ((CvSelectionGroup *)(m_selectionGroups.add()));
}

Seems to be straight to me... I dunno. What's happening here I wonder... is this just an overload? Are there simply too many barbs in this game as Koshling has seen in others?

Another interesting note that immediately precedes the first quoted code portion here up above:

	//	Koshling - Forcing the unit into a new group causes rapid cycling through the group id
	//	space, which is a scaling issue, so only do it when necessary
	//	Note - it used o do this unconditionally for cargo and changing that behaviour
	//	might be dangerous, but it solves some scaling problems and I cannot think of a reason why
	//	it should be problematics, nor is it causing any isues in test cases I have tried

hmph. I did allow HUMAN player cargo to group and the crash is taking place on an AI unit (unless he has automated upgrades on... I just thought about that possibility - ewww) but even if that were the case it would still hit portions above and rejoin the group it was already a part of before the upgrade! So I don't THINK that has anything to do with it (plus this save was last played in March and it's been since then that I made my change that would allow grouping of units on a transport for the human player only.)


Anyone have any insights or clues as to what could be wrong here?