Hi,
Not 100% sure about initialization (last hunk GetSharedModuleList()), but
as I see it, it was racy, unless it is always protected by something in
Target.
Other two are minor things, with ModuleList::operator= priority inversion
probably never occurring in current code as is. However it might fight back
in future, and since I've noticed it...
Please commit if OK for trunk.
Cheers,
/Piotr
diff --git a/source/Core/ModuleList.cpp b/source/Core/ModuleList.cpp
index 215611e..d3c803f 100644
--- a/source/Core/ModuleList.cpp
+++ b/source/Core/ModuleList.cpp
@@ -11,6 +11,7 @@
// C Includes
// C++ Includes
+#include <cstdint>
// Other libraries and framework includes
// Project includes
#include "lldb/Core/Log.h"
@@ -40,7 +41,8 @@ ModuleList::ModuleList() :
//----------------------------------------------------------------------
ModuleList::ModuleList(const ModuleList& rhs) :
m_modules(),
- m_modules_mutex (Mutex::eMutexTypeRecursive)
+ m_modules_mutex (Mutex::eMutexTypeRecursive),
+ m_notifier(NULL)
{
Mutex::Locker lhs_locker(m_modules_mutex);
Mutex::Locker rhs_locker(rhs.m_modules_mutex);
@@ -62,9 +64,28 @@ ModuleList::operator= (const ModuleList& rhs)
{
if (this != &rhs)
{
- Mutex::Locker lhs_locker(m_modules_mutex);
- Mutex::Locker rhs_locker(rhs.m_modules_mutex);
- m_modules = rhs.m_modules;
+ // That's probably me nit-picking, but in theoretical situation:
+ //
+ // * that two threads A B and
+ // * two ModuleList's x y do opposite assignemnts ie.:
+ //
+ // in thread A: | in thread B:
+ // x = y; | y = x;
+ //
+ // This establishes correct(same) lock taking order and thus
+ // avoids priority inversion.
+ if (uintptr_t(this) > uintptr_t(&rhs))
+ {
+ Mutex::Locker lhs_locker(m_modules_mutex);
+ Mutex::Locker rhs_locker(rhs.m_modules_mutex);
+ m_modules = rhs.m_modules;
+ }
+ else
+ {
+ Mutex::Locker rhs_locker(rhs.m_modules_mutex);
+ Mutex::Locker lhs_locker(m_modules_mutex);
+ m_modules = rhs.m_modules;
+ }
}
return *this;
}
@@ -832,6 +853,10 @@ ModuleList::GetIndexForModule (const Module *module) const
static ModuleList &
GetSharedModuleList ()
{
+ // TODO: Replace with pthread_once alike or spinlock...
+ static Mutex g_init_mutex (Mutex::eMutexTypeRecursive);
+
+ Mutex::Locker locker (g_init_mutex);
// NOTE: Intentionally leak the module list so a program doesn't have to
// cleanup all modules and object files as it exits. This just wastes time
// doing a bunch of cleanup that isn't required.
From 8ab8dc63bd6d46d1a0d7616b8683b88ffc7d7c2e Mon Sep 17 00:00:00 2001
From: Piotr Rak <[email protected]>
Date: Sun, 23 Mar 2014 21:57:28 +0100
Subject: [PATCH] Minor lldb_private::ModuleList fixes.
o uninitialized m_notifier in copy ctor.
o potential priority inversion in operator=
o race during initialization in GetSharedModuleList
---
source/Core/ModuleList.cpp | 33 +++++++++++++++++++++++++++++----
1 file changed, 29 insertions(+), 4 deletions(-)
diff --git a/source/Core/ModuleList.cpp b/source/Core/ModuleList.cpp
index 215611e..d3c803f 100644
--- a/source/Core/ModuleList.cpp
+++ b/source/Core/ModuleList.cpp
@@ -11,6 +11,7 @@
// C Includes
// C++ Includes
+#include <cstdint>
// Other libraries and framework includes
// Project includes
#include "lldb/Core/Log.h"
@@ -40,7 +41,8 @@ ModuleList::ModuleList() :
//----------------------------------------------------------------------
ModuleList::ModuleList(const ModuleList& rhs) :
m_modules(),
- m_modules_mutex (Mutex::eMutexTypeRecursive)
+ m_modules_mutex (Mutex::eMutexTypeRecursive),
+ m_notifier(NULL)
{
Mutex::Locker lhs_locker(m_modules_mutex);
Mutex::Locker rhs_locker(rhs.m_modules_mutex);
@@ -62,9 +64,28 @@ ModuleList::operator= (const ModuleList& rhs)
{
if (this != &rhs)
{
- Mutex::Locker lhs_locker(m_modules_mutex);
- Mutex::Locker rhs_locker(rhs.m_modules_mutex);
- m_modules = rhs.m_modules;
+ // That's probably me nit-picking, but in theoretical situation:
+ //
+ // * that two threads A B and
+ // * two ModuleList's x y do opposite assignemnts ie.:
+ //
+ // in thread A: | in thread B:
+ // x = y; | y = x;
+ //
+ // This establishes correct(same) lock taking order and thus
+ // avoids priority inversion.
+ if (uintptr_t(this) > uintptr_t(&rhs))
+ {
+ Mutex::Locker lhs_locker(m_modules_mutex);
+ Mutex::Locker rhs_locker(rhs.m_modules_mutex);
+ m_modules = rhs.m_modules;
+ }
+ else
+ {
+ Mutex::Locker rhs_locker(rhs.m_modules_mutex);
+ Mutex::Locker lhs_locker(m_modules_mutex);
+ m_modules = rhs.m_modules;
+ }
}
return *this;
}
@@ -832,6 +853,10 @@ ModuleList::GetIndexForModule (const Module *module) const
static ModuleList &
GetSharedModuleList ()
{
+ // TODO: Replace with pthread_once alike or spinlock...
+ static Mutex g_init_mutex (Mutex::eMutexTypeRecursive);
+
+ Mutex::Locker locker (g_init_mutex);
// NOTE: Intentionally leak the module list so a program doesn't have to
// cleanup all modules and object files as it exits. This just wastes time
// doing a bunch of cleanup that isn't required.
--
1.9.0
_______________________________________________
lldb-dev mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
