From 55467d190a854a1903ec42967a24976f95fc4153 Mon Sep 17 00:00:00 2001 From: James R Date: Mon, 20 Mar 2023 14:42:44 -0700 Subject: [PATCH 1/4] Fix code formatting in P_SetTarget --- src/p_tick.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/p_tick.c b/src/p_tick.c index 4ed8fd83a..ac6413d73 100644 --- a/src/p_tick.c +++ b/src/p_tick.c @@ -310,10 +310,18 @@ void P_RemoveThinker(thinker_t *thinker) mobj_t *P_SetTarget(mobj_t **mop, mobj_t *targ) { - 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 + } + + if (targ != NULL) // Set new target and if non-NULL, increase its counter + { targ->thinker.references++; + } + + *mop = targ; + return targ; } From 509a023329ea1cd583c500a3287c488e5ee3e3a6 Mon Sep 17 00:00:00 2001 From: James R Date: Mon, 20 Mar 2023 19:32:11 -0700 Subject: [PATCH 2/4] PARANOIA: add debug to P_SetTarget if references go negative Prints mobj address, mobj type, thinker function, reference count and source code line number. --- src/d_think.h | 6 +++++ src/p_mobj.c | 5 ++++ src/p_tick.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++- src/p_tick.h | 15 +++++++++++- 4 files changed, 90 insertions(+), 2 deletions(-) diff --git a/src/d_think.h b/src/d_think.h index 75b7137f1..186e151a0 100644 --- a/src/d_think.h +++ b/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,10 @@ 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; +#endif }; #ifdef __cplusplus diff --git a/src/p_mobj.c b/src/p_mobj.c index b910f4a3d..8e64ae584 100644 --- a/src/p_mobj.c +++ b/src/p_mobj.c @@ -11196,6 +11196,11 @@ 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! diff --git a/src/p_tick.c b/src/p_tick.c index ac6413d73..27b52c2b7 100644 --- a/src/p_tick.c +++ b/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 ""; +} + +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 ""; +} +#endif + // // killough 11/98: // @@ -308,11 +353,30 @@ 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 { (*mop)->thinker.references--; + +#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 + ); + } +#endif } if (targ != NULL) // Set new target and if non-NULL, increase its counter diff --git a/src/p_tick.h b/src/p_tick.h index a1018d721..a17a5358c 100644 --- a/src/p_tick.h +++ b/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); From 17aaf178d587970a3a845aa6c2f3807077996a77 Mon Sep 17 00:00:00 2001 From: James R Date: Mon, 20 Mar 2023 19:37:36 -0700 Subject: [PATCH 3/4] PARANOIA: fix faulty references warning - Print mobj address and mobj type in addition to reference count - Print negative reference counts correctly - Don't print warning twice for the same mobj (don't spam the console) --- src/d_think.h | 1 + src/p_tick.c | 42 +++++++++++++++++++++++++++++++----------- 2 files changed, 32 insertions(+), 11 deletions(-) diff --git a/src/d_think.h b/src/d_think.h index 186e151a0..d833f4ca0 100644 --- a/src/d_think.h +++ b/src/d_think.h @@ -58,6 +58,7 @@ struct thinker_t #ifdef PARANOIA INT32 debug_mobjtype; + tic_t debug_time; #endif }; diff --git a/src/p_tick.c b/src/p_tick.c index 27b52c2b7..acd7d2f95 100644 --- a/src/p_tick.c +++ b/src/p_tick.c @@ -296,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; @@ -376,12 +390,18 @@ mobj_t *P_SetTarget2(mobj_t **mop, mobj_t *targ 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; From 67f4505a060292fe9e7e4cfdd0b2a3e10ec4c7bb Mon Sep 17 00:00:00 2001 From: James R Date: Mon, 20 Mar 2023 19:48:12 -0700 Subject: [PATCH 4/4] Revert "Add cv_scrambleremoved, disable scramble on P_RemoveMobj" This reverts commit a55ddef52813dc459eb9afa7370deab0273340c2. --- src/p_local.h | 7 ------- src/p_mobj.c | 10 ++++------ 2 files changed, 4 insertions(+), 13 deletions(-) diff --git a/src/p_local.h b/src/p_local.h index 973af8621..eb4633ff4 100644 --- a/src/p_local.h +++ b/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); diff --git a/src/p_mobj.c b/src/p_mobj.c index 8e64ae584..384fd6ebb 100644 --- a/src/p_mobj.c +++ b/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; @@ -11064,6 +11062,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); @@ -11204,10 +11205,7 @@ void P_RemoveMobj(mobj_t *mobj) // 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 }