[lldb] Make access to ReflectionContext thread safe #7113
Conversation
|
@swift-ci test |
lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntimeDynamicTypeResolution.cpp
Show resolved
Hide resolved
lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntime.cpp
Outdated
Show resolved
Hide resolved
lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntime.cpp
Outdated
Show resolved
Hide resolved
|
@kastiglione @JDevlieghere I would appreciate a second and third pair of eyes on this. The underlying assumption here is that calls into the Swift Reflection API are not thread safe since they build up in-memory reflection data structures and various caches. This is addressed by the ThreadSafeReflectionContext RAII construct. The second problem is that the ForEach loop in SetupReflection will request a ReflectionContext which recurses into SetupReflection. This is addressed by making the mutex recursive and setting the initialized flag before entering the ForEach loop. |
lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntime.cpp
Outdated
Show resolved
Hide resolved
lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntime.cpp
Outdated
Show resolved
Hide resolved
augusto2112
force-pushed
the
thread-safe-refl-context
branch
from
81c6304 to
c79cc76
Compare
|
@swift-ci test |
|
@swift-ci test windows |
lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntime.cpp
Outdated
Show resolved
Hide resolved
lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntime.cpp
Outdated
Show resolved
Hide resolved
augusto2112
force-pushed
the
thread-safe-refl-context
branch
from
c79cc76 to
aa2fc0f
Compare
lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntime.cpp
Outdated
Show resolved
Hide resolved
lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntime.cpp
Outdated
Show resolved
Hide resolved
|
@swift-ci test |
augusto2112
force-pushed
the
thread-safe-refl-context
branch
from
aa2fc0f to
35d2e12
Compare
|
@swift-ci test |
|
@swift-ci test windows |
lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntime.cpp
Outdated
Show resolved
Hide resolved
lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntimeImpl.h
Outdated
Show resolved
Hide resolved
ReflectionContext can potentially be accessed by multiple threads at the same time, make accessing ReflectionContext thread safe by hiding it behind a lock. rdar://112207497
augusto2112
force-pushed
the
thread-safe-refl-context
branch
from
35d2e12 to
c8aa68b
Compare
|
@swift-ci test |
|
@swift-ci test windows |
lldb/source/Core/ModuleList.cpp
Outdated
| @@ -310,6 +310,14 @@ ModuleList::ModuleList(const ModuleList &rhs) : m_modules(), m_modules_mutex() { | |||
| m_modules = rhs.m_modules; | |||
| } | |||
|
|
|||
| ModuleList::ModuleList(ModuleList &&rhs) : m_modules(), m_modules_mutex() { | |||
| std::lock_guard<std::recursive_mutex> lhs_guard(m_modules_mutex); | |||
There was a problem hiding this comment.
This should not be needed, since nobody can have access to this object while we're still in the constructor.
|
@swift-ci test windows |
augusto2112
force-pushed
the
thread-safe-refl-context
branch
from
c8aa68b to
ead9772
Compare
|
@swift-ci test |
|
@swift-ci test windows |
|
|
||
|
|
||
| std::lock_guard<std::recursive_mutex> lock(m_reflection_ctx_mutex); |
There was a problem hiding this comment.
Yes, I left it to maintain the invariant that this function has to lock that mutex, in case we ever call it from somewhere else.
|
|
||
|
|
||
| std::lock_guard<std::recursive_mutex> lock(m_reflection_ctx_mutex); | ||
| if (m_initialized_reflection_ctx) |
There was a problem hiding this comment.
Should we hoist this into GetReflectionContext(), since we also check the variable there for ProcessAllModules?
|
|
||
| void ModuleList::Swap(ModuleList &other) { | ||
| // scoped_lock locks both mutexes at once. | ||
| std::scoped_lock lock(m_modules_mutex, other.m_modules_mutex); |
ReflectionContext can potentially be accessed by multiple threads at the same time, make accessing ReflectionContext thread safe by hiding it behind a lock.
rdar://112207497