From a3f6a41ee9256440d2add78e23a115df35de70ab Mon Sep 17 00:00:00 2001 From: SkyratBot <59378654+SkyratBot@users.noreply.github.com> Date: Fri, 26 Mar 2021 11:30:59 +0100 Subject: [PATCH] [MIRROR] Ref Tracking: Revengance (#4413) * Ref Tracking: Revengance (#57728) * Ref Tracking: Revengance Fixes reference tracking ignoring self references due to a poorly thought out tick checking system. Fixes reference tracking ignoring the contents of assoc lists Makes the reference tracking printouts actually describe what list the ref is in, rather then just saying "list" Adds REFERENCE_TRACKING_DEBUG, a define which toggles tracking info for the ref tracking procs, which allows for oversight on how the proc is working Allows for direct calls of qdel_and_find_ref_if_fail(), makes it use ref rather then REF(), fixing it breaking for mobs. (Ditto for the qdel hint which does the same thing) Moves REAGENTS_TESTING out of the reftracking define block Makes unit tests define REFERENCE_TRACKING, REFERENCE_TRACKING_DEBUG, and FIND_REF_NO_CHECK_TICK Adds a unit test that sanity checks the reference finder proc * Ref Tracking: Revengance Co-authored-by: LemonInTheDark <58055496+LemonInTheDark@users.noreply.github.com> --- code/_compile_options.dm | 22 ++++-- code/controllers/subsystem/garbage.dm | 4 +- code/datums/datum.dm | 6 +- .../view_variables/reference_tracking.dm | 54 +++++++++++---- code/modules/unit_tests/_unit_tests.dm | 4 ++ .../unit_tests/find_reference_sanity.dm | 69 +++++++++++++++++++ 6 files changed, 136 insertions(+), 23 deletions(-) create mode 100644 code/modules/unit_tests/find_reference_sanity.dm diff --git a/code/_compile_options.dm b/code/_compile_options.dm index 4671f145c1a..4dd68a5d02b 100644 --- a/code/_compile_options.dm +++ b/code/_compile_options.dm @@ -15,12 +15,8 @@ //#define REFERENCE_TRACKING #ifdef REFERENCE_TRACKING -/* -* Enables debug messages for every single reaction step. This is 1 message per 0.5s for a SINGLE reaction. Useful for tracking down bugs/asking me for help in the main reaction handiler (equilibrium.dm). -* -* * Requires TESTING to be defined to work. -*/ -//#define REAGENTS_TESTING +///Used for doing dry runs of the reference finder, to test for feature completeness +//#define REFERENCE_TRACKING_DEBUG ///Run a lookup on things hard deleting by default. //#define GC_FAILURE_HARD_LOOKUP @@ -30,6 +26,13 @@ #endif //ifdef REFERENCE_TRACKING +/* +* Enables debug messages for every single reaction step. This is 1 message per 0.5s for a SINGLE reaction. Useful for tracking down bugs/asking me for help in the main reaction handiler (equilibrium.dm). +* +* * Requires TESTING to be defined to work. +*/ +//#define REAGENTS_TESTING + #define VISUALIZE_ACTIVE_TURFS //Highlights atmos active turfs in green #define TRACK_MAX_SHARE //Allows max share tracking, for use in the atmos debugging ui #endif //ifdef TESTING @@ -72,6 +75,13 @@ #define TESTING #endif +#if defined(UNIT_TESTS) +//Hard del testing defines +#define REFERENCE_TRACKING +#define REFERENCE_TRACKING_DEBUG +#define FIND_REF_NO_CHECK_TICK +#endif + #ifdef TGS // TGS performs its own build of dm.exe, but includes a prepended TGS define. #define CBT diff --git a/code/controllers/subsystem/garbage.dm b/code/controllers/subsystem/garbage.dm index f59433cfc8a..1469197a521 100644 --- a/code/controllers/subsystem/garbage.dm +++ b/code/controllers/subsystem/garbage.dm @@ -332,9 +332,9 @@ SUBSYSTEM_DEF(garbage) if (QDEL_HINT_FINDREFERENCE) //qdel will, if REFERENCE_TRACKING is enabled, display all references to this object, then queue the object for deletion. SSgarbage.Queue(D) D.find_references() - if (QDEL_HINT_IFFAIL_FINDREFERENCE) + if (QDEL_HINT_IFFAIL_FINDREFERENCE) //qdel will, if REFERENCE_TRACKING is enabled and the object fails to collect, display all references to this object. SSgarbage.Queue(D) - SSgarbage.reference_find_on_fail[REF(D)] = TRUE + SSgarbage.reference_find_on_fail["\ref[D]"] = TRUE #endif else #ifdef TESTING diff --git a/code/datums/datum.dm b/code/datums/datum.dm index 699bfb80322..1211999ff43 100644 --- a/code/datums/datum.dm +++ b/code/datums/datum.dm @@ -51,9 +51,13 @@ */ var/list/cooldowns -#ifdef TESTING +#ifdef REFERENCE_TRACKING var/running_find_references var/last_find_references = 0 + #ifdef REFERENCE_TRACKING_DEBUG + ///Stores info about where refs are found, used for sanity checks and testing + var/list/found_refs + #endif #endif #ifdef DATUMVAR_DEBUGGING_MODE diff --git a/code/modules/admin/view_variables/reference_tracking.dm b/code/modules/admin/view_variables/reference_tracking.dm index 699b25f2d43..93cdb4991ab 100644 --- a/code/modules/admin/view_variables/reference_tracking.dm +++ b/code/modules/admin/view_variables/reference_tracking.dm @@ -31,17 +31,18 @@ usr.client.running_find_references = type testing("Beginning search for references to a [type].") - last_find_references = world.time - DoSearchVar(GLOB) //globals + var/starting_time = world.time + + DoSearchVar(GLOB, "GLOB") //globals for(var/datum/thing in world) //atoms (don't beleive its lies) - DoSearchVar(thing, "World -> [thing]") + DoSearchVar(thing, "World -> [thing.type]", search_time = starting_time) for(var/datum/thing) //datums - DoSearchVar(thing, "World -> [thing]") + DoSearchVar(thing, "Datums -> [thing.type]", search_time = starting_time) for(var/client/thing) //clients - DoSearchVar(thing, "World -> [thing]") + DoSearchVar(thing, "Clients -> [thing.type]", search_time = starting_time) testing("Completed search for references to a [type].") if(usr?.client) @@ -71,43 +72,65 @@ qdel_and_find_ref_if_fail(src, TRUE) -/datum/proc/DoSearchVar(potential_container, container_name, recursive_limit = 64) +/datum/proc/DoSearchVar(potential_container, container_name, recursive_limit = 64, search_time = world.time) + #ifdef REFERENCE_TRACKING_DEBUG + if(!found_refs) + found_refs = list() + #endif + if(usr?.client && !usr.client.running_find_references) return if(!recursive_limit) + testing("Recursion limit reached. [container_name]") return if(istype(potential_container, /datum)) var/datum/datum_container = potential_container - if(datum_container.last_find_references == last_find_references) + if(datum_container.last_find_references == search_time) return - datum_container.last_find_references = last_find_references + datum_container.last_find_references = search_time var/list/vars_list = datum_container.vars for(var/varname in vars_list) - if (varname == "vars") + if (varname == "vars" || varname == "vis_locs") //Fun fact, vis_locs don't count for references continue var/variable = vars_list[varname] if(variable == src) - testing("Found [type] \ref[src] in [datum_container.type]'s [varname] var. [container_name]") + #ifdef REFERENCE_TRACKING_DEBUG + found_refs[varname] = TRUE + #endif + testing("Found [type] \ref[src] in [datum_container.type]'s \ref[datum_container] [varname] var. [container_name]") else if(islist(variable)) - DoSearchVar(variable, "[container_name] -> list", recursive_limit - 1) + DoSearchVar(variable, "[container_name] \ref[datum_container] -> [varname] (list)", recursive_limit - 1, search_time) else if(islist(potential_container)) var/normal = IS_NORMAL_LIST(potential_container) for(var/element_in_list in potential_container) + //Check normal entrys if(element_in_list == src) + #ifdef REFERENCE_TRACKING_DEBUG + found_refs[potential_container] = TRUE + #endif testing("Found [type] \ref[src] in list [container_name].") + //Check assoc entrys else if(element_in_list && !isnum(element_in_list) && normal && potential_container[element_in_list] == src) + #ifdef REFERENCE_TRACKING_DEBUG + found_refs[potential_container] = TRUE + #endif testing("Found [type] \ref[src] in list [container_name]\[[element_in_list]\]") + //Check normal sublists else if(islist(element_in_list)) - DoSearchVar(element_in_list, "[container_name] -> list", recursive_limit - 1) + DoSearchVar(element_in_list, "[container_name] -> [element_in_list] (list)", recursive_limit - 1, search_time) + + //Check assoc sublists + else if(element_in_list && !isnum(element_in_list) && normal && islist(potential_container[element_in_list])) + DoSearchVar(potential_container[element_in_list], "[container_name]\[[element_in_list]\] -> [potential_container[element_in_list]] (list)", recursive_limit - 1, search_time) #ifndef FIND_REF_NO_CHECK_TICK CHECK_TICK @@ -115,7 +138,10 @@ /proc/qdel_and_find_ref_if_fail(datum/thing_to_del, force = FALSE) - SSgarbage.reference_find_on_fail[REF(thing_to_del)] = TRUE - qdel(thing_to_del, force) + thing_to_del.qdel_and_find_ref_if_fail(force) + +/datum/proc/qdel_and_find_ref_if_fail(force = FALSE) + SSgarbage.reference_find_on_fail["\ref[src]"] = TRUE + qdel(src, force) #endif diff --git a/code/modules/unit_tests/_unit_tests.dm b/code/modules/unit_tests/_unit_tests.dm index d0618a6d9f0..a8198785f44 100644 --- a/code/modules/unit_tests/_unit_tests.dm +++ b/code/modules/unit_tests/_unit_tests.dm @@ -85,6 +85,10 @@ #include "timer_sanity.dm" #include "unit_test.dm" +#ifdef REFERENCE_TRACKING //Don't try and parse this file if ref tracking isn't turned on. IE: don't parse ref tracking please mr linter +#include "find_reference_sanity.dm" +#endif + #undef TEST_ASSERT #undef TEST_ASSERT_EQUAL #undef TEST_ASSERT_NOTEQUAL diff --git a/code/modules/unit_tests/find_reference_sanity.dm b/code/modules/unit_tests/find_reference_sanity.dm new file mode 100644 index 00000000000..e958c1e2ddf --- /dev/null +++ b/code/modules/unit_tests/find_reference_sanity.dm @@ -0,0 +1,69 @@ +///Used to test the completeness of the reference finder proc. +/datum/unit_test/find_reference_sanity + +/atom/movable/ref_holder + var/atom/movable/ref_test/test + var/list/test_list = list() + var/list/test_assoc_list = list() + +/atom/movable/ref_test + var/atom/movable/ref_test/self_ref + +/datum/unit_test/find_reference_sanity/Run() + var/atom/movable/ref_test/victim = allocate(/atom/movable/ref_test) + var/atom/movable/ref_holder/testbed = allocate(/atom/movable/ref_holder) + + //Sanity check + victim.DoSearchVar(testbed, "Sanity Check", search_time = 1) //We increment search time to get around an optimization + TEST_ASSERT(!victim.found_refs.len, "The ref-tracking tool found a ref where none existed") + victim.found_refs.Cut() + + //Set up for the first round of tests + testbed.test = victim + testbed.test_list += victim + testbed.test_assoc_list["baseline"] = victim + + victim.DoSearchVar(testbed, "First Run", search_time = 2) + + TEST_ASSERT(victim.found_refs["test"], "The ref-tracking tool failed to find a regular value") + TEST_ASSERT(victim.found_refs[testbed.test_list], "The ref-tracking tool failed to find a list entry") + TEST_ASSERT(victim.found_refs[testbed.test_assoc_list], "The ref-tracking tool failed to find an assoc list value") + victim.found_refs.Cut() + testbed.test = null + testbed.test_list.Cut() + testbed.test_assoc_list.Cut() + + //Second round, bit harder this time + testbed.overlays += victim + testbed.vis_contents += victim + testbed.test_assoc_list[victim] = TRUE + + victim.DoSearchVar(testbed, "Second Run", search_time = 3) + + //This is another sanity check + TEST_ASSERT(!victim.found_refs[testbed.overlays], "The ref-tracking tool found an overlays entry? That shouldn't be possible") + TEST_ASSERT(victim.found_refs[testbed.vis_contents], "The ref-tracking tool failed to find a vis_contents entry") + TEST_ASSERT(victim.found_refs[testbed.test_assoc_list], "The ref-tracking tool failed to find an assoc list key") + victim.found_refs.Cut() + testbed.overlays.Cut() + testbed.vis_contents.Cut() + testbed.test_assoc_list.Cut() + + //Let's get a bit esoteric + victim.self_ref = victim + var/list/to_find = list(victim) + testbed.test_list += list(to_find) + var/list/to_find_assoc = list(victim) + testbed.test_assoc_list["Nesting"] = to_find_assoc + + victim.DoSearchVar(victim, "Third Run Self", search_time = 4) + victim.DoSearchVar(testbed, "Third Run Testbed", search_time = 4) + TEST_ASSERT(victim.found_refs["self_ref"], "The ref-tracking tool failed to find a self reference") + TEST_ASSERT(victim.found_refs[to_find], "The ref-tracking tool failed to find a nested list entry") + TEST_ASSERT(victim.found_refs[to_find_assoc], "The ref-tracking tool failed to find a nested assoc list entry") + victim.found_refs.Cut() + victim.self_ref = null + to_find.Cut() + testbed.test_list.Cut() + to_find_assoc.Cut() + testbed.test_assoc_list.Cut()