From 63be16b2ac3166e049da5e510a641c7a659ab6d2 Mon Sep 17 00:00:00 2001 From: howsohazard <143410553+howsohazard@users.noreply.github.com> Date: Fri, 26 Jan 2024 07:56:17 -0500 Subject: [PATCH] 19108: Fixes resource leak with strings that could also lead to erroneous mixing of strings when handling strings with few references (#64) --- src/Amalgam/AmalgamMain.cpp | 6 +- src/Amalgam/interpreter/Interpreter.cpp | 4 +- src/Amalgam/interpreter/Interpreter.h | 2 +- .../interpreter/InterpreterOpcodesBase.cpp | 16 ++-- .../InterpreterOpcodesEntityAccess.cpp | 3 +- src/Amalgam/out.txt | 82 +++++++++---------- src/Amalgam/string/StringInternPool.h | 10 +-- 7 files changed, 62 insertions(+), 61 deletions(-) diff --git a/src/Amalgam/AmalgamMain.cpp b/src/Amalgam/AmalgamMain.cpp index e98c7b14..154b9d8a 100644 --- a/src/Amalgam/AmalgamMain.cpp +++ b/src/Amalgam/AmalgamMain.cpp @@ -318,9 +318,9 @@ PLATFORM_MAIN_CONSOLE if(num_strings_used > 0) { std::cerr << "ERROR: Num strings still in use: " << num_strings_used << std::endl; - std::vector in_use = string_intern_pool.GetNonStaticStringsInUse(); - for(auto &s : in_use) - std::cerr << '"' << s << '"' << std::endl; + std::vector> in_use = string_intern_pool.GetNonStaticStringsInUse(); + for(auto &[s, count] : in_use) + std::cerr << '"' << s << "\":" << count << std::endl; } std::cout << "Memory reclaimation complete." << std::endl; diff --git a/src/Amalgam/interpreter/Interpreter.cpp b/src/Amalgam/interpreter/Interpreter.cpp index 1cd0f590..a30a5edd 100644 --- a/src/Amalgam/interpreter/Interpreter.cpp +++ b/src/Amalgam/interpreter/Interpreter.cpp @@ -435,7 +435,7 @@ EvaluableNodeReference Interpreter::ConvertArgsToCallStack(EvaluableNodeReferenc } else if(!args.unique) { - args.SetReference(enm.AllocNode(args)); + args.SetReference(enm.AllocNode(args, EvaluableNodeManager::ENMM_REMOVE_ALL)); } EvaluableNode *call_stack = enm.AllocNode(ENT_LIST); @@ -675,7 +675,6 @@ double Interpreter::InterpretNodeIntoNumberValue(EvaluableNode *n) double value = result_value.GetValueAsNumber(); evaluableNodeManager->FreeNodeTreeIfPossible(result); - result.FreeImmediateResources(); return value; } @@ -709,7 +708,6 @@ bool Interpreter::InterpretNodeIntoBoolValue(EvaluableNode *n, bool value_if_nul bool value = result_value.GetValueAsBoolean(); evaluableNodeManager->FreeNodeTreeIfPossible(result); - result.FreeImmediateResources(); return value; } diff --git a/src/Amalgam/interpreter/Interpreter.h b/src/Amalgam/interpreter/Interpreter.h index 0f238fda..b6a968c5 100644 --- a/src/Amalgam/interpreter/Interpreter.h +++ b/src/Amalgam/interpreter/Interpreter.h @@ -110,7 +110,7 @@ class Interpreter if(EvaluableNode::IsAssociativeArray(new_context)) { if(!new_context.unique) - new_context.SetReference(evaluableNodeManager->AllocNode(new_context)); + new_context.SetReference(evaluableNodeManager->AllocNode(new_context, EvaluableNodeManager::ENMM_REMOVE_ALL)); } else //not assoc, make a new one { diff --git a/src/Amalgam/interpreter/InterpreterOpcodesBase.cpp b/src/Amalgam/interpreter/InterpreterOpcodesBase.cpp index aa71e4c7..6f02e36c 100644 --- a/src/Amalgam/interpreter/InterpreterOpcodesBase.cpp +++ b/src/Amalgam/interpreter/InterpreterOpcodesBase.cpp @@ -333,7 +333,7 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_SEQUENCE(EvaluableNode *en evaluableNodeManager->FreeNodeTreeIfPossible(result); //request immediate values when not last, since any allocs for returns would be wasted //concludes won't be immediate - result = InterpretNode(ocn[i], immediate_result || i < ocn_size - 1); + result = InterpretNode(ocn[i], immediate_result || i + 1 < ocn_size); } return result; } @@ -613,7 +613,7 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_WHILE(EvaluableNode *en, b //concludes won't be immediate //but because previous_result may be used, that can't be immediate, so the last param //cannot be evaulated as immediate - new_result = InterpretNode(ocn[i], i < ocn_size - 1); + new_result = InterpretNode(ocn[i], i + 1 < ocn_size); if(!new_result.IsImmediateValue() && new_result != nullptr && new_result->GetType() == ENT_CONCLUDE) { @@ -626,7 +626,7 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_WHILE(EvaluableNode *en, b } //don't free the last new_result - if(i < ocn_size - 1) + if(i + 1 < ocn_size) evaluableNodeManager->FreeNodeTreeIfPossible(new_result); } @@ -666,7 +666,7 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_LET(EvaluableNode *en, boo evaluableNodeManager->FreeNodeTreeIfPossible(result); //request immediate values when not last, since any allocs for returns would be wasted //concludes won't be immediate - result = InterpretNode(ocn[i], immediate_result || i < ocn_size - 1); + result = InterpretNode(ocn[i], immediate_result || i + 1 < ocn_size); } //all finished with new context, but can't free it in case returning something @@ -770,7 +770,7 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_DECLARE(EvaluableNode *en, evaluableNodeManager->FreeNodeTreeIfPossible(result); //request immediate values when not last, since any allocs for returns would be wasted //concludes won't be immediate - result = InterpretNode(ocn[i], immediate_result || i < ocn_size - 1); + result = InterpretNode(ocn[i], immediate_result || i + 1 < ocn_size); } return result; @@ -1036,8 +1036,9 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_RETRIEVE(EvaluableNode *en if(to_lookup == nullptr || IsEvaluableNodeTypeImmediate(to_lookup->GetType())) { StringInternPool::StringID symbol_name_sid = EvaluableNode::ToStringIDIfExists(to_lookup); + EvaluableNode* symbol_value = GetCallStackSymbol(symbol_name_sid); evaluableNodeManager->FreeNodeTreeIfPossible(to_lookup); - return EvaluableNodeReference(GetCallStackSymbol(symbol_name_sid), false); + return EvaluableNodeReference(symbol_value, false); } else if(to_lookup->IsAssociativeArray()) { @@ -1070,11 +1071,12 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_RETRIEVE(EvaluableNode *en continue; } + EvaluableNode *symbol_value = GetCallStackSymbol(symbol_name_sid); //if there are values passed in, free them to be clobbered EvaluableNodeReference cnr(cn, to_lookup.unique); evaluableNodeManager->FreeNodeTreeIfPossible(cnr); - cn = GetCallStackSymbol(symbol_name_sid); + cn = symbol_value; } return EvaluableNodeReference(to_lookup, false); diff --git a/src/Amalgam/interpreter/InterpreterOpcodesEntityAccess.cpp b/src/Amalgam/interpreter/InterpreterOpcodesEntityAccess.cpp index a1992a66..cfbbf933 100644 --- a/src/Amalgam/interpreter/InterpreterOpcodesEntityAccess.cpp +++ b/src/Amalgam/interpreter/InterpreterOpcodesEntityAccess.cpp @@ -335,12 +335,13 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_RETRIEVE_FROM_ENTITY_and_D if(to_lookup == nullptr || IsEvaluableNodeTypeImmediate(to_lookup->GetType())) { StringInternPool::StringID label_sid = EvaluableNode::ToStringIDIfExists(to_lookup); - evaluableNodeManager->FreeNodeTreeIfPossible(to_lookup); ExecutionCycleCount num_steps_executed = 0; EvaluableNodeReference value = target_entity->GetValueAtLabel(label_sid, evaluableNodeManager, direct, target_entity == curEntity); curExecutionStep += num_steps_executed; + evaluableNodeManager->FreeNodeTreeIfPossible(to_lookup); + return value; } else if(to_lookup->IsAssociativeArray()) diff --git a/src/Amalgam/out.txt b/src/Amalgam/out.txt index 88db4fcd..18237637 100644 --- a/src/Amalgam/out.txt +++ b/src/Amalgam/out.txt @@ -1045,7 +1045,7 @@ abcdef 4 "d" ) -(list 2 "d" 1 0 3) +(list 2 1 "d" 0 3) (list 1 2 @@ -1240,7 +1240,7 @@ current_index: 2 8 ) accum_string "abcdef" - argv (list "C:\\Users\\Chris Hazard\\Desktop\\Howso_repos\\amalgam\\src\\Amalgam\\./amlg_code/full_test.amlg") + argv (list "C:\\Users\\ChristopherHazard\\Desktop\\Howso_repos\\amalgam\\src\\Amalgam\\./amlg_code/full_test.amlg") bar (declare (assoc x 6) (+ x 2) @@ -1253,10 +1253,10 @@ current_index: 2 A (assoc B 2) B 2 ) - interpreter "C:\\Users\\Chris Hazard\\Desktop\\Howso_repos\\amalgam\\x64\\MT_Release_EXE\\Amalgam.exe" + interpreter "C:\\Users\\ChristopherHazard\\Desktop\\Howso_repos\\amalgam\\x64\\MT_Release_EXE\\Amalgam.exe" raaa 2 rwww 1 - start_time 1706155111.185343 + start_time 1706237472.540396 www 1 x 12 zz 10 @@ -1283,7 +1283,7 @@ current_index: 2 8 ) accum_string "abcdef" - argv (list "C:\\Users\\Chris Hazard\\Desktop\\Howso_repos\\amalgam\\src\\Amalgam\\./amlg_code/full_test.amlg") + argv (list "C:\\Users\\ChristopherHazard\\Desktop\\Howso_repos\\amalgam\\src\\Amalgam\\./amlg_code/full_test.amlg") bar (declare (assoc x 6) (+ x 2) @@ -1296,10 +1296,10 @@ current_index: 2 A (assoc B 2) B 2 ) - interpreter "C:\\Users\\Chris Hazard\\Desktop\\Howso_repos\\amalgam\\x64\\MT_Release_EXE\\Amalgam.exe" + interpreter "C:\\Users\\ChristopherHazard\\Desktop\\Howso_repos\\amalgam\\x64\\MT_Release_EXE\\Amalgam.exe" raaa 2 rwww 1 - start_time 1706155111.185343 + start_time 1706237472.540396 www 1 x 12 zz 10 @@ -1325,7 +1325,7 @@ current_index: 2 8 ) accum_string "abcdef" - argv (list "C:\\Users\\Chris Hazard\\Desktop\\Howso_repos\\amalgam\\src\\Amalgam\\./amlg_code/full_test.amlg") + argv (list "C:\\Users\\ChristopherHazard\\Desktop\\Howso_repos\\amalgam\\src\\Amalgam\\./amlg_code/full_test.amlg") bar (declare (assoc x 6) (+ x 2) @@ -1338,10 +1338,10 @@ current_index: 2 A (assoc B 2) B 2 ) - interpreter "C:\\Users\\Chris Hazard\\Desktop\\Howso_repos\\amalgam\\x64\\MT_Release_EXE\\Amalgam.exe" + interpreter "C:\\Users\\ChristopherHazard\\Desktop\\Howso_repos\\amalgam\\x64\\MT_Release_EXE\\Amalgam.exe" raaa 2 rwww 1 - start_time 1706155111.185343 + start_time 1706237472.540396 www 1 x 12 zz 10 @@ -1489,7 +1489,7 @@ infinity test c or d: (list "c" @(get (target 0) 0) "d" @(get (target 0) 0)) (assoc a 29 b 44 c 27) -(list "5" "2" "1") +(list "7" "2" "1") --get_rand_seed-- °³È¼¿\¨KOaVÆT zÿ @@ -1609,7 +1609,7 @@ e: - .inf 25: (assoc a 1) -current date-time in epoch: 2024-01-24-22.58.31.5508810 +current date-time in epoch: 2024-01-25-21.51.12.5883020 2020-06-07 00:22:59 1391230800 1391230800 @@ -2685,10 +2685,10 @@ MergeEntityChild1 (associate "x" 3 "y" 4) MergeEntityChild2 (associate "p" 3 "q" 4) -_2169689611 -(associate "e" 3 "f" 4) _2280722175 (associate "E" 3 "F" 4) +_2169689611 +(associate "e" 3 "f" 4) --union_entities-- (associate "b" 4 "a" 3 "c" 3) MergeEntityChild1 @@ -2706,17 +2706,6 @@ MergeEntityChild2 "w" 7 ) -_2169689611 -(associate - "e" - 3 - "f" - 4 - "g" - 5 - "h" - 6 -) _2280722175 (associate "E" @@ -2728,11 +2717,7 @@ _2280722175 "H" 6 ) -(parallel - ##p - (list "_3990396532" "_3330773578" "_3990396532" "_3330773578") -) -_3330773578 +_2169689611 (associate "e" 3 @@ -2743,6 +2728,10 @@ _3330773578 "h" 6 ) +(parallel + ##p + (list "_3990396532" "_3330773578" "_3990396532" "_3330773578") +) _3990396532 (associate "E" @@ -2754,6 +2743,17 @@ _3990396532 "H" 6 ) +_3330773578 +(associate + "e" + 3 + "f" + 4 + "g" + 5 + "h" + 6 +) --difference_entities-- (declare (assoc _ (null)) @@ -3375,17 +3375,17 @@ MergeEntityChild1 (associate "x" 3 "y" 4 "z" 5) MergeEntityChild2 (associate "p" 3 "q" 4 "u" 5) -_2169689611 -(associate "e" 3 "f" 4) _2280722175 +(associate "E" 3 "F" 4) +_2169689611 (associate - "E" + "e" 3 - "F" + "f" 4 - "G" + "g" 5 - "H" + "h" 6 ) --get_entity_comments-- @@ -3442,7 +3442,7 @@ deep sets --set_entity_root_permission-- RootTest -1706155111.773997 +1706237472.781054 (true) RootTest @@ -3666,7 +3666,7 @@ hello ) ) ) - (set_entity_rand_seed new_entity "U—EO¢Ž0Ñ”-I´»ÿ") + (set_entity_rand_seed new_entity "ù^\n·brjÎ0Ñ”-I´»ÿ") (set_entity_rand_seed (first (create_entities @@ -3676,7 +3676,7 @@ hello ) ) ) - "C(Å°vµ³ˆ6¯RbHƒÿ" + "5ȳ\0ôNbýhû­eÕÿ" ) (set_entity_rand_seed (first @@ -3709,7 +3709,7 @@ hello ) ) ) - (set_entity_rand_seed new_entity "U—EO¢Ž0Ñ”-I´»ÿ") + (set_entity_rand_seed new_entity "ù^\n·brjÎ0Ñ”-I´»ÿ") (set_entity_rand_seed (first (create_entities @@ -4658,4 +4658,4 @@ Expecting 1000: 1000 concurrent entity writes successful: (true) --total execution time-- -1.1163010597229004 +1.728956937789917 diff --git a/src/Amalgam/string/StringInternPool.h b/src/Amalgam/string/StringInternPool.h index c7d99bdf..e09c3693 100644 --- a/src/Amalgam/string/StringInternPool.h +++ b/src/Amalgam/string/StringInternPool.h @@ -161,7 +161,7 @@ class StringInternPool int64_t refcount = DecrementRefCount(id); //if extra references, just return, but if it is 1, then it will try to clear - if(refcount == 1) + if(refcount <= 1) ids_need_removal = true; } @@ -188,7 +188,7 @@ class StringInternPool //remove any that are the last reference int64_t refcount = DecrementRefCount(id); - if(refcount == 1) + if(refcount <= 1) RemoveId(id); } @@ -244,17 +244,17 @@ class StringInternPool } //returns a vector of all the strings still in use. Intended for debugging. - std::vector GetNonStaticStringsInUse() + std::vector> GetNonStaticStringsInUse() { #if defined(MULTITHREAD_SUPPORT) || defined(MULTITHREAD_INTERFACE) Concurrency::ReadLock lock(sharedMutex); #endif - std::vector in_use; + std::vector> in_use; for(size_t id = 0; id < idToStringAndRefCount.size(); id++) { if(!IsStringIDStatic(id) && idToStringAndRefCount[id].second > 0) - in_use.push_back(idToStringAndRefCount[id].first); + in_use.emplace_back(idToStringAndRefCount[id].first, idToStringAndRefCount[id].second); } return in_use; }