asgardius/virtualx-engine
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Merge pull request #85248 from RandomShaper/fix_gdlamb_doublefree_2

Browse source 
Fix lambda cross-thread dynamics (take 2)
This commit is contained in:
Rémi Verschelde 2023-11-23 22:57:25 +01:00
commit 070ac8dfcd
No known key found for this signature in database
GPG key ID: C3336907360768E1
5 changed files with 118 additions and 97 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

39
core/templates/list.h
View file

@ -132,6 +132,8 @@ public:
data->erase(this);
}
void transfer_to_back(List<T, A> *p_dst_list);
_FORCE_INLINE_ Element() {}
};
@ -762,4 +764,41 @@ public:
}
};
template <class T, class A>
void List<T, A>::Element::transfer_to_back(List<T, A> *p_dst_list) {
// Detach from current.
if (data->first == this) {
data->first = data->first->next_ptr;
}
if (data->last == this) {
data->last = data->last->prev_ptr;
}
if (prev_ptr) {
prev_ptr->next_ptr = next_ptr;
}
if (next_ptr) {
next_ptr->prev_ptr = prev_ptr;
}
data->size_cache--;
// Attach to the back of the new one.
if (!p_dst_list->_data) {
p_dst_list->_data = memnew_allocator(_Data, A);
p_dst_list->_data->first = this;
p_dst_list->_data->last = nullptr;
p_dst_list->_data->size_cache = 0;
prev_ptr = nullptr;
} else {
p_dst_list->_data->last->next_ptr = this;
prev_ptr = p_dst_list->_data->last;
}
p_dst_list->_data->last = this;
next_ptr = nullptr;
data = p_dst_list->_data;
p_dst_list->_data->size_cache++;
}
#endif // LIST_H

149
modules/gdscript/gdscript.cpp
View file

@ -1391,108 +1391,51 @@ String GDScript::debug_get_script_name(const Ref<Script> &p_script) {
}
#endif
thread_local GDScript::UpdatableFuncPtr GDScript::func_ptrs_to_update_thread_local;
GDScript::UpdatableFuncPtr *GDScript::func_ptrs_to_update_main_thread = &func_ptrs_to_update_thread_local;
GDScript::UpdatableFuncPtr GDScript::func_ptrs_to_update_main_thread;
thread_local GDScript::UpdatableFuncPtr *GDScript::func_ptrs_to_update_thread_local = nullptr;
List<GDScript::UpdatableFuncPtrElement>::Element *GDScript::_add_func_ptr_to_update(GDScriptFunction **p_func_ptr_ptr) {
MutexLock lock(func_ptrs_to_update_mutex);
List<UpdatableFuncPtrElement>::Element *result = func_ptrs_to_update_elems.push_back(UpdatableFuncPtrElement());
GDScript::UpdatableFuncPtrElement GDScript::_add_func_ptr_to_update(GDScriptFunction **p_func_ptr_ptr) {
UpdatableFuncPtrElement result = {};
{
MutexLock lock2(func_ptrs_to_update_thread_local.mutex);
result->get().element = func_ptrs_to_update_thread_local.ptrs.push_back(p_func_ptr_ptr);
result->get().mutex = &func_ptrs_to_update_thread_local.mutex;
MutexLock lock(func_ptrs_to_update_thread_local->mutex);
result.element = func_ptrs_to_update_thread_local->ptrs.push_back(p_func_ptr_ptr);
result.func_ptr = func_ptrs_to_update_thread_local;
if (likely(func_ptrs_to_update_thread_local.initialized)) {
if (likely(func_ptrs_to_update_thread_local->initialized)) {
return result;
}
func_ptrs_to_update_thread_local.initialized = true;
func_ptrs_to_update_thread_local->initialized = true;
}
func_ptrs_to_update.push_back(&func_ptrs_to_update_thread_local);
MutexLock lock(func_ptrs_to_update_mutex);
func_ptrs_to_update.push_back(func_ptrs_to_update_thread_local);
func_ptrs_to_update_thread_local->rc++;
return result;
}
void GDScript::_remove_func_ptr_to_update(List<UpdatableFuncPtrElement>::Element *p_func_ptr_element) {
// None of these checks should ever fail, unless there's a bug.
// They can be removed once we are sure they never catch anything.
// Left here now due to extra safety needs late in the release cycle.
ERR_FAIL_NULL(p_func_ptr_element);
MutexLock lock(func_ptrs_to_update_thread_local.mutex);
ERR_FAIL_NULL(p_func_ptr_element->get().element);
ERR_FAIL_NULL(p_func_ptr_element->get().mutex);
MutexLock lock2(*p_func_ptr_element->get().mutex);
p_func_ptr_element->get().element->erase();
p_func_ptr_element->erase();
void GDScript::_remove_func_ptr_to_update(const UpdatableFuncPtrElement &p_func_ptr_element) {
ERR_FAIL_NULL(p_func_ptr_element.element);
ERR_FAIL_NULL(p_func_ptr_element.func_ptr);
MutexLock lock(p_func_ptr_element.func_ptr->mutex);
p_func_ptr_element.element->erase();
}
void GDScript::_fixup_thread_function_bookkeeping() {
// Transfer the ownership of these update items to the main thread,
// because the current one is dying, leaving theirs orphan, dangling.
HashSet<GDScript *> scripts;
DEV_ASSERT(!Thread::is_main_thread());
MutexLock lock(func_ptrs_to_update_main_thread->mutex);
{
MutexLock lock2(func_ptrs_to_update_thread_local.mutex);
MutexLock lock(func_ptrs_to_update_main_thread.mutex);
MutexLock lock2(func_ptrs_to_update_thread_local->mutex);
while (!func_ptrs_to_update_thread_local.ptrs.is_empty()) {
// Transfer the thread-to-script records from the dying thread to the main one.
List<GDScriptFunction **>::Element *E = func_ptrs_to_update_thread_local.ptrs.front();
List<GDScriptFunction **>::Element *new_E = func_ptrs_to_update_main_thread->ptrs.push_front(E->get());
GDScript *script = (*E->get())->get_script();
if (!scripts.has(script)) {
scripts.insert(script);
// Replace dying thread by the main thread in the script-to-thread records.
MutexLock lock3(script->func_ptrs_to_update_mutex);
DEV_ASSERT(script->func_ptrs_to_update.find(&func_ptrs_to_update_thread_local));
{
for (List<UpdatableFuncPtrElement>::Element *F = script->func_ptrs_to_update_elems.front(); F; F = F->next()) {
bool is_dying_thread_entry = F->get().mutex == &func_ptrs_to_update_thread_local.mutex;
if (is_dying_thread_entry) {
// This may lead to multiple main-thread entries, but that's not a problem
// and allows to reuse the element, which is needed, since it's tracked by pointer.
F->get().element = new_E;
F->get().mutex = &func_ptrs_to_update_main_thread->mutex;
}
}
}
}
E->erase();
}
}
func_ptrs_to_update_main_thread->initialized = true;
{
// Remove orphan thread-to-script entries from every script.
// FIXME: This involves iterating through every script whenever a thread dies.
// While it's OK that thread creation/destruction are heavy operations,
// additional bookkeeping can be used to outperform this brute-force approach.
GDScriptLanguage *gd_lang = GDScriptLanguage::get_singleton();
MutexLock lock2(gd_lang->mutex);
for (SelfList<GDScript> *s = gd_lang->script_list.first(); s; s = s->next()) {
GDScript *script = s->self();
for (List<UpdatableFuncPtr *>::Element *E = script->func_ptrs_to_update.front(); E; E = E->next()) {
bool is_dying_thread_entry = &E->get()->mutex == &func_ptrs_to_update_thread_local.mutex;
if (is_dying_thread_entry) {
E->erase();
break;
}
}
}
while (!func_ptrs_to_update_thread_local->ptrs.is_empty()) {
List<GDScriptFunction **>::Element *E = func_ptrs_to_update_thread_local->ptrs.front();
E->transfer_to_back(&func_ptrs_to_update_main_thread.ptrs);
func_ptrs_to_update_thread_local->transferred = true;
}
}
@ -1516,12 +1459,29 @@ void GDScript::clear(ClearData *p_clear_data) {
{
MutexLock outer_lock(func_ptrs_to_update_mutex);
for (UpdatableFuncPtr *updatable : func_ptrs_to_update) {
MutexLock inner_lock(updatable->mutex);
for (GDScriptFunction **func_ptr_ptr : updatable->ptrs) {
*func_ptr_ptr = nullptr;
bool destroy = false;
{
MutexLock inner_lock(updatable->mutex);
if (updatable->transferred) {
func_ptrs_to_update_main_thread.mutex.lock();
}
for (GDScriptFunction **func_ptr_ptr : updatable->ptrs) {
*func_ptr_ptr = nullptr;
}
DEV_ASSERT(updatable->rc != 0);
updatable->rc--;
if (updatable->rc == 0) {
destroy = true;
}
if (updatable->transferred) {
func_ptrs_to_update_main_thread.mutex.unlock();
}
}
if (destroy) {
DEV_ASSERT(updatable != &func_ptrs_to_update_main_thread);
memdelete(updatable);
}
}
func_ptrs_to_update_elems.clear();
}
RBSet<GDScript *> must_clear_dependencies = get_must_clear_dependencies();
@ -2141,8 +2101,25 @@ void GDScriptLanguage::remove_named_global_constant(const StringName &p_name) {
named_globals.erase(p_name);
}
void GDScriptLanguage::thread_enter() {
GDScript::func_ptrs_to_update_thread_local = memnew(GDScript::UpdatableFuncPtr);
}
void GDScriptLanguage::thread_exit() {
GDScript::_fixup_thread_function_bookkeeping();
bool destroy = false;
{
MutexLock lock(GDScript::func_ptrs_to_update_thread_local->mutex);
DEV_ASSERT(GDScript::func_ptrs_to_update_thread_local->rc != 0);
GDScript::func_ptrs_to_update_thread_local->rc--;
if (GDScript::func_ptrs_to_update_thread_local->rc == 0) {
destroy = true;
}
}
if (destroy) {
memdelete(GDScript::func_ptrs_to_update_thread_local);
}
}
void GDScriptLanguage::init() {
@ -2177,6 +2154,8 @@ void GDScriptLanguage::init() {
_add_global(E.name, E.ptr);
}
GDScript::func_ptrs_to_update_thread_local = &GDScript::func_ptrs_to_update_main_thread;
#ifdef TESTS_ENABLED
GDScriptTests::GDScriptTestRunner::handle_cmdline();
#endif
@ -2226,6 +2205,8 @@ void GDScriptLanguage::finish() {
}
script_list.clear();
function_list.clear();
DEV_ASSERT(GDScript::func_ptrs_to_update_main_thread.rc == 1);
}
void GDScriptLanguage::profiling_start() {

19
modules/gdscript/gdscript.h
View file

@ -121,21 +121,23 @@ class GDScript : public Script {
struct UpdatableFuncPtr {
List<GDScriptFunction **> ptrs;
Mutex mutex;
bool initialized = false;
bool initialized : 1;
bool transferred : 1;
uint32_t rc = 1;
UpdatableFuncPtr() :
initialized(false), transferred(false) {}
};
struct UpdatableFuncPtrElement {
List<GDScriptFunction **>::Element *element = nullptr;
Mutex *mutex = nullptr;
UpdatableFuncPtr *func_ptr = nullptr;
};
static thread_local UpdatableFuncPtr func_ptrs_to_update_thread_local;
static thread_local LocalVector<List<UpdatableFuncPtr *>::Element> func_ptrs_to_update_entries_thread_local;
static UpdatableFuncPtr *func_ptrs_to_update_main_thread;
static UpdatableFuncPtr func_ptrs_to_update_main_thread;
static thread_local UpdatableFuncPtr *func_ptrs_to_update_thread_local;
List<UpdatableFuncPtr *> func_ptrs_to_update;
List<UpdatableFuncPtrElement> func_ptrs_to_update_elems;
Mutex func_ptrs_to_update_mutex;
List<UpdatableFuncPtrElement>::Element *_add_func_ptr_to_update(GDScriptFunction **p_func_ptr_ptr);
static void _remove_func_ptr_to_update(List<UpdatableFuncPtrElement>::Element *p_func_ptr_element);
UpdatableFuncPtrElement _add_func_ptr_to_update(GDScriptFunction **p_func_ptr_ptr);
static void _remove_func_ptr_to_update(const UpdatableFuncPtrElement &p_func_ptr_element);
static void _fixup_thread_function_bookkeeping();
@ -561,6 +563,7 @@ public:
/* MULTITHREAD FUNCTIONS */
virtual void thread_enter() override;
virtual void thread_exit() override;
/* DEBUGGER FUNCTIONS */

4
modules/gdscript/gdscript_lambda_callable.cpp
View file

@ -296,7 +296,5 @@ GDScriptLambdaSelfCallable::GDScriptLambdaSelfCallable(Object *p_self, GDScriptF
}
GDScriptLambdaSelfCallable::~GDScriptLambdaSelfCallable() {
if (updatable_func_ptr_element) {
GDScript::_remove_func_ptr_to_update(updatable_func_ptr_element);
}
GDScript::_remove_func_ptr_to_update(updatable_func_ptr_element);
}

4
modules/gdscript/gdscript_lambda_callable.h
View file

@ -45,7 +45,7 @@ class GDScriptLambdaCallable : public CallableCustom {
GDScriptFunction *function = nullptr;
Ref<GDScript> script;
uint32_t h;
List<GDScript::UpdatableFuncPtrElement>::Element *updatable_func_ptr_element = nullptr;
GDScript::UpdatableFuncPtrElement updatable_func_ptr_element;
Vector<Variant> captures;
@ -72,7 +72,7 @@ class GDScriptLambdaSelfCallable : public CallableCustom {
Ref<RefCounted> reference; // For objects that are RefCounted, keep a reference.
Object *object = nullptr; // For non RefCounted objects, use a direct pointer.
uint32_t h;
List<GDScript::UpdatableFuncPtrElement>::Element *updatable_func_ptr_element = nullptr;
GDScript::UpdatableFuncPtrElement updatable_func_ptr_element;
Vector<Variant> captures;