Merge branch 'paranoia-mobj-reference-count' into 'master'

Tune up PARANOIA warnings for mobj reference count

See merge request KartKrew/Kart!1070

src/d_think.h

@@ -17,6 +17,8 @@
 #ifndef __D_THINK__
 #define __D_THINK__
 
+#include "doomdef.h"
+
 #ifdef __cplusplus
 extern "C" {
 #endif
@@ -53,6 +55,11 @@ struct thinker_t
 	// killough 11/98: count of how many other objects reference
 	// this one using pointers. Used for garbage collection.
 	INT32 references;
+
+#ifdef PARANOIA
+	INT32 debug_mobjtype;
+	tic_t debug_time;
+#endif
 };
 
 #ifdef __cplusplus

src/p_local.h

@@ -66,10 +66,6 @@ extern "C" {
 
 #define P_GetPlayerViewHeight(player) (41*player->mo->height/48)
 
-#ifdef PARANOIA
-#define SCRAMBLE_REMOVED // Force debug build to crash when Removed mobj is accessed
-#endif
-
 typedef enum
 {
 	THINK_POLYOBJ,
@@ -288,9 +284,6 @@ extern mapthing_t *itemrespawnque[ITEMQUESIZE];
 extern tic_t itemrespawntime[ITEMQUESIZE];
 extern size_t iquehead, iquetail;
 extern consvar_t cv_gravity, cv_movebob;
-#ifdef SCRAMBLE_REMOVED
-extern consvar_t cv_scrambleremoved;
-#endif
 
 void P_RespawnBattleBoxes(void);
 mobjtype_t P_GetMobjtype(UINT16 mthingtype);

src/p_mobj.c

@@ -53,8 +53,6 @@
 static CV_PossibleValue_t CV_BobSpeed[] = {{0, "MIN"}, {4*FRACUNIT, "MAX"}, {0, NULL}};
 consvar_t cv_movebob = CVAR_INIT ("movebob", "1.0", CV_FLOAT|CV_SAVE, CV_BobSpeed, NULL);
 
-consvar_t cv_scrambleremoved = CVAR_INIT ("scrambleremoved", "On", CV_NETVAR, CV_OnOff, NULL);
-
 actioncache_t actioncachehead;
 
 static mobj_t *overlaycap = NULL;
@@ -11077,6 +11075,9 @@ mapthing_t *itemrespawnque[ITEMQUESIZE];
 tic_t itemrespawntime[ITEMQUESIZE];
 size_t iquehead, iquetail;
 
+#ifdef PARANOIA
+#define SCRAMBLE_REMOVED // Force debug build to crash when Removed mobj is accessed
+#endif
 void P_RemoveMobj(mobj_t *mobj)
 {
 	I_Assert(mobj != NULL);
@@ -11209,13 +11210,15 @@ void P_RemoveMobj(mobj_t *mobj)
 
 	P_RemoveThinker((thinker_t *)mobj);
 
+#ifdef PARANOIA
+	// Saved to avoid being scrambled like below...
+	mobj->thinker.debug_mobjtype = mobj->type;
+#endif
+
 	// DBG: set everything in mobj_t to 0xFF instead of leaving it. debug memory error.
 #ifdef SCRAMBLE_REMOVED
 	// Invalidate mobj_t data to cause crashes if accessed!
-	if (cv_scrambleremoved.value)
-	{
-		memset((UINT8 *)mobj + sizeof(thinker_t), 0xff, sizeof(mobj_t) - sizeof(thinker_t));
-	}
+	memset((UINT8 *)mobj + sizeof(thinker_t), 0xff, sizeof(mobj_t) - sizeof(thinker_t));
 #endif
 }
 

src/p_tick.c

@@ -42,6 +42,10 @@
 #include "k_specialstage.h"
 #include "acs/interface.h"
 
+#ifdef PARANOIA
+#include "deh_tables.h" // MOBJTYPE_LIST
+#endif
+
 tic_t leveltime;
 
 INT32 P_AltFlip(INT32 n, tic_t tics)
@@ -228,8 +232,49 @@ void P_AddThinker(const thinklistnum_t n, thinker_t *thinker)
 	thlist[n].prev = thinker;
 
 	thinker->references = 0;    // killough 11/98: init reference counter to 0
+
+#ifdef PARANOIA
+	thinker->debug_mobjtype = MT_NULL;
+#endif
 }
 
+#ifdef PARANOIA
+static const char *MobjTypeName(const mobj_t *mobj)
+{
+	actionf_p1 p1 = mobj->thinker.function.acp1;
+
+	if (p1 == (actionf_p1)P_MobjThinker)
+	{
+		return MOBJTYPE_LIST[mobj->type];
+	}
+	else if (p1 == (actionf_p1)P_RemoveThinkerDelayed)
+	{
+		if (mobj->thinker.debug_mobjtype != MT_NULL)
+		{
+			return MOBJTYPE_LIST[mobj->thinker.debug_mobjtype];
+		}
+	}
+
+	return "<Not a mobj>";
+}
+
+static const char *MobjThinkerName(const mobj_t *mobj)
+{
+	actionf_p1 p1 = mobj->thinker.function.acp1;
+
+	if (p1 == (actionf_p1)P_MobjThinker)
+	{
+		return "P_MobjThinker";
+	}
+	else if (p1 == (actionf_p1)P_RemoveThinkerDelayed)
+	{
+		return "P_RemoveThinkerDelayed";
+	}
+
+	return "<Unknown Thinker>";
+}
+#endif
+
 //
 // killough 11/98:
 //
@@ -251,20 +296,34 @@ static thinker_t *currentthinker;
 void P_RemoveThinkerDelayed(thinker_t *thinker)
 {
 	thinker_t *next;
-#ifdef PARANOIA
-#define BEENAROUNDBIT (0x40000000) // has to be sufficiently high that it's unlikely to happen in regular gameplay. If you change this, pay attention to the bit pattern of INT32_MIN.
-	if (thinker->references & ~BEENAROUNDBIT)
+
+	if (thinker->references != 0)
 	{
-		if (thinker->references & BEENAROUNDBIT) // Usually gets cleared up in one frame; what's going on here, then?
-			CONS_Printf("Number of potentially faulty references: %d\n", (thinker->references & ~BEENAROUNDBIT));
-		thinker->references |= BEENAROUNDBIT;
+#ifdef PARANOIA
+		if (thinker->debug_time > leveltime)
+		{
+			thinker->debug_time = leveltime + 2; // do not print errors again
+		}
+		// Removed mobjs can be the target of another mobj. In
+		// that case, the other mobj will manage its reference
+		// to the removed mobj in P_MobjThinker. However, if
+		// the removed mobj is removed after the other object
+		// thinks, the reference management is delayed by one
+		// tic.
+		else if (thinker->debug_time < leveltime)
+		{
+			CONS_Printf(
+					"PARANOIA/P_RemoveThinkerDelayed: %p %s references=%d\n",
+					(void*)thinker,
+					MobjTypeName((mobj_t*)thinker),
+					thinker->references
+			);
+
+			thinker->debug_time = leveltime + 2; // do not print this error again
+		}
+#endif
 		return;
 	}
-#undef BEENAROUNDBIT
-#else
-	if (thinker->references)
-		return;
-#endif
 
 	/* Remove from main thinker list */
 	next = thinker->next;
@@ -308,12 +367,45 @@ void P_RemoveThinker(thinker_t *thinker)
  * references, and delay removal until the count is 0.
  */
 
-mobj_t *P_SetTarget(mobj_t **mop, mobj_t *targ)
+mobj_t *P_SetTarget2(mobj_t **mop, mobj_t *targ
+#ifdef PARANOIA
+		, const char *source_file, int source_line
+#endif
+)
 {
-	if (*mop)              // If there was a target already, decrease its refcount
+	if (*mop) // If there was a target already, decrease its refcount
+	{
 		(*mop)->thinker.references--;
-if ((*mop = targ) != NULL) // Set new target and if non-NULL, increase its counter
+
+#ifdef PARANOIA
+		if ((*mop)->thinker.references < 0)
+		{
+			CONS_Printf(
+					"PARANOIA/P_SetTarget: %p %s %s references=%d, references go negative! (%s:%d)\n",
+					(void*)*mop,
+					MobjTypeName(*mop),
+					MobjThinkerName(*mop),
+					(*mop)->thinker.references,
+					source_file,
+					source_line
+			);
+		}
+
+		(*mop)->thinker.debug_time = leveltime;
+#endif
+	}
+
+	if (targ != NULL) // Set new target and if non-NULL, increase its counter
+	{
 		targ->thinker.references++;
+
+#ifdef PARANOIA
+		targ->thinker.debug_time = leveltime;
+#endif
+	}
+
+	*mop = targ;
+
 	return targ;
 }
 

src/p_tick.h

@@ -14,6 +14,8 @@
 #ifndef __P_TICK__
 #define __P_TICK__
 
+#include "doomdef.h"
+
 #ifdef __cplusplus
 extern "C" {
 #endif
@@ -33,7 +35,18 @@ void P_Ticker(boolean run);
 void P_PreTicker(INT32 frames);
 void P_DoTeamscrambling(void);
 void P_RemoveThinkerDelayed(thinker_t *thinker); //killed
-mobj_t *P_SetTarget(mobj_t **mo, mobj_t *target);   // killough 11/98
+
+mobj_t *P_SetTarget2(mobj_t **mo, mobj_t *target
+#ifdef PARANOIA
+		, const char *source_file, int source_line
+#endif
+);
+
+#ifdef PARANOIA
+#define P_SetTarget(...) P_SetTarget2(__VA_ARGS__, __FILE__, __LINE__)
+#else
+#define P_SetTarget P_SetTarget2
+#endif
 
 // Negate the value for tics
 INT32 P_AltFlip(INT32 value, tic_t tics);