From 3607ffaf663570e579f66a51740a9dfa5e7451bb Mon Sep 17 00:00:00 2001 From: Alexander Kromm Date: Thu, 30 Jan 2020 00:10:07 +0700 Subject: [PATCH] fix Dictionary and DictionaryIterator memory leaks --- src/scripting/thingdef_data.cpp | 15 --- src/serializer.cpp | 2 +- src/utility/dictionary.cpp | 175 ++++++++++++++++++++-------- src/utility/dictionary.h | 60 +++++++++- src/version.h | 4 +- wadsrc/static/zscript/dictionary.zs | 19 ++- 6 files changed, 200 insertions(+), 75 deletions(-) diff --git a/src/scripting/thingdef_data.cpp b/src/scripting/thingdef_data.cpp index c308b00e38..5f61bbfabc 100644 --- a/src/scripting/thingdef_data.cpp +++ b/src/scripting/thingdef_data.cpp @@ -843,21 +843,6 @@ void InitThingdef() wbplayerstruct->Size = sizeof(wbplayerstruct_t); wbplayerstruct->Align = alignof(wbplayerstruct_t); - auto dictionarystruct = NewStruct("Dictionary", nullptr, true); - dictionarystruct->Size = sizeof(Dictionary); - dictionarystruct->Align = alignof(Dictionary); - NewPointer(dictionarystruct, false)->InstallHandlers( - [](FSerializer &ar, const char *key, const void *addr) - { - ar(key, *(Dictionary **)addr); - }, - [](FSerializer &ar, const char *key, void *addr) - { - Serialize(ar, key, *(Dictionary **)addr, nullptr); - return true; - } - ); - FAutoSegIterator probe(CRegHead, CRegTail); while (*++probe != NULL) diff --git a/src/serializer.cpp b/src/serializer.cpp index 6e58973e20..860dcad2a9 100644 --- a/src/serializer.cpp +++ b/src/serializer.cpp @@ -2190,7 +2190,7 @@ Dictionary *DictionaryFromString(const FString &string) return nullptr; } - Dictionary *const dict = new Dictionary; + Dictionary *const dict = Create(); if (string.IsEmpty()) { diff --git a/src/utility/dictionary.cpp b/src/utility/dictionary.cpp index 6da43dca18..7e9e14a1f6 100644 --- a/src/utility/dictionary.cpp +++ b/src/utility/dictionary.cpp @@ -3,20 +3,87 @@ #include "scripting/vm/vm.h" #include "serializer.h" +#include + +//===================================================================================== +// +// DObject implementations for Dictionary and DictionaryIterator +// +//===================================================================================== + +IMPLEMENT_CLASS(Dictionary, false, false); + +IMPLEMENT_CLASS(DictionaryIterator, false, true); + +IMPLEMENT_POINTERS_START(DictionaryIterator) +IMPLEMENT_POINTER(Dict) +IMPLEMENT_POINTERS_END + +//===================================================================================== +// +// Dictionary functions +// +//===================================================================================== + +void Dictionary::Serialize(FSerializer &arc) +{ + Super::Serialize(arc); + + constexpr char key[] { "dictionary" }; + + if (arc.isWriting()) + { + // Pass this instance to serializer. + Dictionary *pointerToThis = this; + arc(key, pointerToThis); + } + else + { + // Receive new Dictionary, copy contents, clean up. + Dictionary *pointerToDeserializedDictionary; + arc(key, pointerToDeserializedDictionary); + TransferFrom(*pointerToDeserializedDictionary); + delete pointerToDeserializedDictionary; + } +} + +static Dictionary *DictCreate() +{ + Dictionary *dict { Create() }; + + return dict; +} + +static void DictInsert(Dictionary *dict, const FString &key, const FString &value) +{ + dict->Insert(key, value); +} + +static void DictAt(const Dictionary *dict, const FString &key, FString *result) +{ + const FString *value = dict->CheckKey(key); + *result = value ? *value : ""; +} + +static void DictToString(const Dictionary *dict, FString *result) +{ + *result = DictionaryToString(*dict); +} + +static void DictRemove(Dictionary *dict, const FString &key) +{ + dict->Remove(key); +} + //===================================================================================== // // Dictionary exports // //===================================================================================== -DEFINE_ACTION_FUNCTION(_Dictionary, Create) +DEFINE_ACTION_FUNCTION_NATIVE(_Dictionary, Create, DictCreate) { - ACTION_RETURN_POINTER(new Dictionary); -} - -static void DictInsert(Dictionary *dict, const FString &key, const FString &value) -{ - dict->Insert(key, value); + ACTION_RETURN_POINTER(DictCreate()); } DEFINE_ACTION_FUNCTION_NATIVE(_Dictionary, Insert, DictInsert) @@ -28,12 +95,6 @@ DEFINE_ACTION_FUNCTION_NATIVE(_Dictionary, Insert, DictInsert) return 0; } -static void DictAt(const Dictionary *dict, const FString &key, FString *result) -{ - const FString *value = dict->CheckKey(key); - *result = value ? *value : ""; -} - DEFINE_ACTION_FUNCTION_NATIVE(_Dictionary, At, DictAt) { PARAM_SELF_STRUCT_PROLOGUE(Dictionary); @@ -43,11 +104,6 @@ DEFINE_ACTION_FUNCTION_NATIVE(_Dictionary, At, DictAt) ACTION_RETURN_STRING(result); } -static void DictToString(const Dictionary *dict, FString *result) -{ - *result = DictionaryToString(*dict); -} - DEFINE_ACTION_FUNCTION_NATIVE(_Dictionary, ToString, DictToString) { PARAM_SELF_STRUCT_PROLOGUE(Dictionary); @@ -56,21 +112,11 @@ DEFINE_ACTION_FUNCTION_NATIVE(_Dictionary, ToString, DictToString) ACTION_RETURN_STRING(result); } -static Dictionary *DictFromString(const FString& string) -{ - return DictionaryFromString(string); -} - -DEFINE_ACTION_FUNCTION_NATIVE(_Dictionary, FromString, DictFromString) +DEFINE_ACTION_FUNCTION_NATIVE(_Dictionary, FromString, DictionaryFromString) { PARAM_PROLOGUE; PARAM_STRING(string); - ACTION_RETURN_POINTER(DictFromString(string)); -} - -static void DictRemove(Dictionary *dict, const FString &key) -{ - dict->Remove(key); + ACTION_RETURN_POINTER(DictionaryFromString(string)); } DEFINE_ACTION_FUNCTION_NATIVE(_Dictionary, Remove, DictRemove) @@ -83,21 +129,65 @@ DEFINE_ACTION_FUNCTION_NATIVE(_Dictionary, Remove, DictRemove) //===================================================================================== // -// DictionaryIterator exports +// DictionaryIterator functions // //===================================================================================== -DictionaryIterator::DictionaryIterator(const Dictionary &dict) - : Iterator(dict) +DictionaryIterator::DictionaryIterator() + : Iterator(nullptr) , Pair(nullptr) + , Dict(nullptr) { } -static DictionaryIterator *DictIteratorCreate(const Dictionary *dict) +void DictionaryIterator::Serialize(FSerializer &arc) { - return new DictionaryIterator(*dict); + if (arc.isWriting()) + { + I_Error("Attempt to save pointer to unhandled type DictionaryIterator"); + } } +void DictionaryIterator::init(Dictionary *dict) +{ + Iterator = std::make_unique(*dict); + Dict = dict; + + GC::WriteBarrier(this, Dict); +} + +static DictionaryIterator *DictIteratorCreate(Dictionary *dict) +{ + DictionaryIterator *iterator = Create(); + iterator->init(dict); + + return iterator; +} + +static int DictIteratorNext(DictionaryIterator *self) +{ + assert(self->Iterator != nullptr); + + const bool hasNext { self->Iterator->NextPair(self->Pair) }; + return hasNext; +} + +static void DictIteratorKey(const DictionaryIterator *self, FString *result) +{ + *result = self->Pair ? self->Pair->Key : FString {}; +} + +static void DictIteratorValue(const DictionaryIterator *self, FString *result) +{ + *result = self->Pair ? self->Pair->Value : FString {}; +} + +//===================================================================================== +// +// DictionaryIterator exports +// +//===================================================================================== + DEFINE_ACTION_FUNCTION_NATIVE(_DictionaryIterator, Create, DictIteratorCreate) { PARAM_PROLOGUE; @@ -105,22 +195,12 @@ DEFINE_ACTION_FUNCTION_NATIVE(_DictionaryIterator, Create, DictIteratorCreate) ACTION_RETURN_POINTER(DictIteratorCreate(dict)); } -static int DictIteratorNext(DictionaryIterator *self) -{ - return self->Iterator.NextPair(self->Pair); -} - DEFINE_ACTION_FUNCTION_NATIVE(_DictionaryIterator, Next, DictIteratorNext) { PARAM_SELF_STRUCT_PROLOGUE(DictionaryIterator); ACTION_RETURN_BOOL(DictIteratorNext(self)); } -static void DictIteratorKey(const DictionaryIterator *self, FString *result) -{ - *result = self->Pair ? self->Pair->Key : FString {}; -} - DEFINE_ACTION_FUNCTION_NATIVE(_DictionaryIterator, Key, DictIteratorKey) { PARAM_SELF_STRUCT_PROLOGUE(DictionaryIterator); @@ -129,11 +209,6 @@ DEFINE_ACTION_FUNCTION_NATIVE(_DictionaryIterator, Key, DictIteratorKey) ACTION_RETURN_STRING(result); } -static void DictIteratorValue(const DictionaryIterator *self, FString *result) -{ - *result = self->Pair ? self->Pair->Value : FString {}; -} - DEFINE_ACTION_FUNCTION_NATIVE(_DictionaryIterator, Value, DictIteratorValue) { PARAM_SELF_STRUCT_PROLOGUE(DictionaryIterator); diff --git a/src/utility/dictionary.h b/src/utility/dictionary.h index 4ccd11370f..d97099482a 100644 --- a/src/utility/dictionary.h +++ b/src/utility/dictionary.h @@ -2,13 +2,63 @@ #include "tarray.h" #include "zstring.h" +#include "dobject.h" -using Dictionary = TMap; +#include -struct DictionaryIterator +/** + * @brief The Dictionary class exists to be exported to ZScript. + * + * It is a string-to-string map. + * + * It is derived from DObject to be a part of normal GC process. + */ +class Dictionary final : public DObject, public TMap { - explicit DictionaryIterator(const Dictionary &dict); + DECLARE_CLASS(Dictionary, DObject) - Dictionary::ConstIterator Iterator; - Dictionary::ConstPair *Pair; +public: + + void Serialize(FSerializer &arc) override; +}; + +/** + * @brief The DictionaryIterator class exists to be exported to ZScript. + * + * It provides iterating over a Dictionary. The order is not specified. + * + * It is derived from DObject to be a part of normal GC process. + */ +class DictionaryIterator final : public DObject +{ + DECLARE_CLASS(DictionaryIterator, DObject) + HAS_OBJECT_POINTERS + +public: + + ~DictionaryIterator() override = default; + + /** + * IMPLEMENT_CLASS macro needs a constructor without parameters. + * + * @see init(). + */ + DictionaryIterator(); + + void Serialize(FSerializer &arc) override; + + /** + * @brief init function complements constructor. + * @attention always call init after constructing DictionaryIterator. + */ + void init(Dictionary *dict); + + std::unique_ptr Iterator; + Dictionary::ConstPair *Pair; + + /** + * @brief Dictionary attribute exists for holding a pointer to iterated + * dictionary, so it is known by GC. + */ + Dictionary *Dict; }; diff --git a/src/version.h b/src/version.h index d46902acb4..6936de47df 100644 --- a/src/version.h +++ b/src/version.h @@ -84,11 +84,11 @@ const char *GetVersionString(); #define SAVEGAME_EXT "zds" // MINSAVEVER is the minimum level snapshot version that can be loaded. -#define MINSAVEVER 4556 +#define MINSAVEVER 4558 // Use 4500 as the base git save version, since it's higher than the // SVN revision ever got. -#define SAVEVER 4557 +#define SAVEVER 4558 // This is so that derivates can use the same savegame versions without worrying about engine compatibility #define GAMESIG "GZDOOM" diff --git a/wadsrc/static/zscript/dictionary.zs b/wadsrc/static/zscript/dictionary.zs index 432cec891f..0bd48cd692 100644 --- a/wadsrc/static/zscript/dictionary.zs +++ b/wadsrc/static/zscript/dictionary.zs @@ -1,4 +1,11 @@ -struct Dictionary native +/** + * Dictionary provides key-value storage. + * + * Both keys and values are strings. + * + * @note keys are case-sensitive. + */ +class Dictionary { native static Dictionary Create(); @@ -23,7 +30,15 @@ struct Dictionary native native String ToString() const; } -struct DictionaryIterator native +/** + * Provides iterating over a Dictionary. + * + * Order is not specified. + * + * DictionaryIterator is not serializable. To make DictionaryIterator a class + * member, use `transient` keyword. + */ +class DictionaryIterator { native static DictionaryIterator Create(Dictionary dict);