Skip to content

Commit 74b56c7

Browse files
JDevlieghereJDevlieghere
committed
Revert "[lldb] Improve locking in PathMappingLists (NFC) (#114576)"
This reverts commit c820994.
1 parent 4c8cc5a commit 74b56c7

File tree

2 files changed

+75
-107
lines changed

2 files changed

+75
-107
lines changed

‎lldb/include/lldb/Target/PathMappingList.h‎

Lines changed: 6 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ class PathMappingList {
2525
typedef void (*ChangedCallback)(const PathMappingList &path_list,
2626
void *baton);
2727

28+
// Constructors and Destructors
2829
PathMappingList();
2930

3031
PathMappingList(ChangedCallback callback, void *callback_baton);
@@ -52,12 +53,12 @@ class PathMappingList {
5253
llvm::json::Value ToJSON();
5354

5455
bool IsEmpty() const {
55-
std::lock_guard<std::mutex> lock(m_pairs_mutex);
56+
std::lock_guard<std::recursive_mutex> lock(m_mutex);
5657
return m_pairs.empty();
5758
}
5859

5960
size_t GetSize() const {
60-
std::lock_guard<std::mutex> lock(m_pairs_mutex);
61+
std::lock_guard<std::recursive_mutex> lock(m_mutex);
6162
return m_pairs.size();
6263
}
6364

@@ -136,33 +137,25 @@ class PathMappingList {
136137
uint32_t FindIndexForPath(llvm::StringRef path) const;
137138

138139
uint32_t GetModificationID() const {
139-
std::lock_guard<std::mutex> lock(m_pairs_mutex);
140+
std::lock_guard<std::recursive_mutex> lock(m_mutex);
140141
return m_mod_id;
141142
}
142143

143144
protected:
145+
mutable std::recursive_mutex m_mutex;
144146
typedef std::pair<ConstString, ConstString> pair;
145147
typedef std::vector<pair> collection;
146148
typedef collection::iterator iterator;
147149
typedef collection::const_iterator const_iterator;
148150

149-
void AppendImpl(llvm::StringRef path, llvm::StringRef replacement);
150-
void Notify(bool notify) const;
151-
152151
iterator FindIteratorForPath(ConstString path);
153152

154153
const_iterator FindIteratorForPath(ConstString path) const;
155154

156155
collection m_pairs;
157-
mutable std::mutex m_pairs_mutex;
158-
159156
ChangedCallback m_callback = nullptr;
160157
void *m_callback_baton = nullptr;
161-
mutable std::mutex m_callback_mutex;
162-
163-
/// Incremented anytime anything is added to or removed from m_pairs. Also
164-
/// protected by m_pairs_mutex.
165-
uint32_t m_mod_id = 0;
158+
uint32_t m_mod_id = 0; // Incremented anytime anything is added or removed.
166159
};
167160

168161
} // namespace lldb_private

‎lldb/source/Target/PathMappingList.cpp‎

Lines changed: 69 additions & 94 deletions
Original file line numberDiff line numberDiff line change
@@ -48,10 +48,7 @@ PathMappingList::PathMappingList(const PathMappingList &rhs)
4848

4949
const PathMappingList &PathMappingList::operator=(const PathMappingList &rhs) {
5050
if (this != &rhs) {
51-
std::scoped_lock<std::mutex, std::mutex> callback_locks(
52-
m_callback_mutex, rhs.m_callback_mutex);
53-
std::scoped_lock<std::mutex, std::mutex> pairs_locks(m_pairs_mutex,
54-
rhs.m_pairs_mutex);
51+
std::scoped_lock<std::recursive_mutex, std::recursive_mutex> locks(m_mutex, rhs.m_mutex);
5552
m_pairs = rhs.m_pairs;
5653
m_callback = nullptr;
5754
m_callback_baton = nullptr;
@@ -62,105 +59,85 @@ const PathMappingList &PathMappingList::operator=(const PathMappingList &rhs) {
6259

6360
PathMappingList::~PathMappingList() = default;
6461

65-
void PathMappingList::AppendImpl(llvm::StringRef path,
66-
llvm::StringRef replacement) {
62+
void PathMappingList::Append(llvm::StringRef path, llvm::StringRef replacement,
63+
bool notify) {
64+
std::lock_guard<std::recursive_mutex> lock(m_mutex);
6765
++m_mod_id;
6866
m_pairs.emplace_back(pair(NormalizePath(path), NormalizePath(replacement)));
69-
}
70-
71-
void PathMappingList::Notify(bool notify) const {
72-
std::lock_guard<std::mutex> lock(m_callback_mutex);
7367
if (notify && m_callback)
7468
m_callback(*this, m_callback_baton);
7569
}
7670

77-
void PathMappingList::Append(llvm::StringRef path, llvm::StringRef replacement,
78-
bool notify) {
79-
{
80-
std::lock_guard<std::mutex> lock(m_pairs_mutex);
81-
AppendImpl(path, replacement);
82-
}
83-
Notify(notify);
84-
}
85-
8671
void PathMappingList::Append(const PathMappingList &rhs, bool notify) {
87-
{
88-
std::scoped_lock<std::mutex, std::mutex> locks(m_pairs_mutex,
89-
rhs.m_pairs_mutex);
90-
++m_mod_id;
91-
if (rhs.m_pairs.empty())
92-
return;
72+
std::scoped_lock<std::recursive_mutex, std::recursive_mutex> locks(m_mutex, rhs.m_mutex);
73+
++m_mod_id;
74+
if (!rhs.m_pairs.empty()) {
9375
const_iterator pos, end = rhs.m_pairs.end();
9476
for (pos = rhs.m_pairs.begin(); pos != end; ++pos)
9577
m_pairs.push_back(*pos);
78+
if (notify && m_callback)
79+
m_callback(*this, m_callback_baton);
9680
}
97-
Notify(notify);
9881
}
9982

10083
bool PathMappingList::AppendUnique(llvm::StringRef path,
10184
llvm::StringRef replacement, bool notify) {
10285
auto normalized_path = NormalizePath(path);
10386
auto normalized_replacement = NormalizePath(replacement);
104-
{
105-
std::lock_guard<std::mutex> lock(m_pairs_mutex);
106-
for (const auto &pair : m_pairs) {
107-
if (pair.first.GetStringRef() == normalized_path &&
108-
pair.second.GetStringRef() == normalized_replacement)
109-
return false;
110-
}
111-
AppendImpl(path, replacement);
87+
std::lock_guard<std::recursive_mutex> lock(m_mutex);
88+
for (const auto &pair : m_pairs) {
89+
if (pair.first.GetStringRef() == normalized_path &&
90+
pair.second.GetStringRef() == normalized_replacement)
91+
return false;
11292
}
113-
Notify(notify);
93+
Append(path, replacement, notify);
11494
return true;
11595
}
11696

11797
void PathMappingList::Insert(llvm::StringRef path, llvm::StringRef replacement,
11898
uint32_t index, bool notify) {
119-
{
120-
std::lock_guard<std::mutex> lock(m_pairs_mutex);
121-
++m_mod_id;
122-
iterator insert_iter;
123-
if (index >= m_pairs.size())
124-
insert_iter = m_pairs.end();
125-
else
126-
insert_iter = m_pairs.begin() + index;
127-
m_pairs.emplace(insert_iter,
128-
pair(NormalizePath(path), NormalizePath(replacement)));
129-
}
130-
Notify(notify);
99+
std::lock_guard<std::recursive_mutex> lock(m_mutex);
100+
++m_mod_id;
101+
iterator insert_iter;
102+
if (index >= m_pairs.size())
103+
insert_iter = m_pairs.end();
104+
else
105+
insert_iter = m_pairs.begin() + index;
106+
m_pairs.emplace(insert_iter, pair(NormalizePath(path),
107+
NormalizePath(replacement)));
108+
if (notify && m_callback)
109+
m_callback(*this, m_callback_baton);
131110
}
132111

133112
bool PathMappingList::Replace(llvm::StringRef path, llvm::StringRef replacement,
134113
uint32_t index, bool notify) {
135-
{
136-
std::lock_guard<std::mutex> lock(m_pairs_mutex);
137-
if (index >= m_pairs.size())
138-
return false;
139-
++m_mod_id;
140-
m_pairs[index] = pair(NormalizePath(path), NormalizePath(replacement));
141-
}
142-
Notify(notify);
114+
std::lock_guard<std::recursive_mutex> lock(m_mutex);
115+
if (index >= m_pairs.size())
116+
return false;
117+
++m_mod_id;
118+
m_pairs[index] = pair(NormalizePath(path), NormalizePath(replacement));
119+
if (notify && m_callback)
120+
m_callback(*this, m_callback_baton);
143121
return true;
144122
}
145123

146124
bool PathMappingList::Remove(size_t index, bool notify) {
147-
{
148-
std::lock_guard<std::mutex> lock(m_pairs_mutex);
149-
if (index >= m_pairs.size())
150-
return false;
125+
std::lock_guard<std::recursive_mutex> lock(m_mutex);
126+
if (index >= m_pairs.size())
127+
return false;
151128

152-
++m_mod_id;
153-
iterator iter = m_pairs.begin() + index;
154-
m_pairs.erase(iter);
155-
}
156-
Notify(notify);
129+
++m_mod_id;
130+
iterator iter = m_pairs.begin() + index;
131+
m_pairs.erase(iter);
132+
if (notify && m_callback)
133+
m_callback(*this, m_callback_baton);
157134
return true;
158135
}
159136

160137
// For clients which do not need the pair index dumped, pass a pair_index >= 0
161138
// to only dump the indicated pair.
162139
void PathMappingList::Dump(Stream *s, int pair_index) {
163-
std::lock_guard<std::mutex> lock(m_pairs_mutex);
140+
std::lock_guard<std::recursive_mutex> lock(m_mutex);
164141
unsigned int numPairs = m_pairs.size();
165142

166143
if (pair_index < 0) {
@@ -178,7 +155,7 @@ void PathMappingList::Dump(Stream *s, int pair_index) {
178155

179156
llvm::json::Value PathMappingList::ToJSON() {
180157
llvm::json::Array entries;
181-
std::lock_guard<std::mutex> lock(m_pairs_mutex);
158+
std::lock_guard<std::recursive_mutex> lock(m_mutex);
182159
for (const auto &pair : m_pairs) {
183160
llvm::json::Array entry{pair.first.GetStringRef().str(),
184161
pair.second.GetStringRef().str()};
@@ -188,13 +165,12 @@ llvm::json::Value PathMappingList::ToJSON() {
188165
}
189166

190167
void PathMappingList::Clear(bool notify) {
191-
{
192-
std::lock_guard<std::mutex> lock(m_pairs_mutex);
193-
if (!m_pairs.empty())
194-
++m_mod_id;
195-
m_pairs.clear();
196-
}
197-
Notify(notify);
168+
std::lock_guard<std::recursive_mutex> lock(m_mutex);
169+
if (!m_pairs.empty())
170+
++m_mod_id;
171+
m_pairs.clear();
172+
if (notify && m_callback)
173+
m_callback(*this, m_callback_baton);
198174
}
199175

200176
bool PathMappingList::RemapPath(ConstString path,
@@ -220,7 +196,7 @@ static void AppendPathComponents(FileSpec &path, llvm::StringRef components,
220196

221197
std::optional<FileSpec> PathMappingList::RemapPath(llvm::StringRef mapping_path,
222198
bool only_if_exists) const {
223-
std::lock_guard<std::mutex> lock(m_pairs_mutex);
199+
std::lock_guard<std::recursive_mutex> lock(m_mutex);
224200
if (m_pairs.empty() || mapping_path.empty())
225201
return {};
226202
LazyBool path_is_relative = eLazyBoolCalculate;
@@ -259,7 +235,7 @@ std::optional<llvm::StringRef>
259235
PathMappingList::ReverseRemapPath(const FileSpec &file, FileSpec &fixed) const {
260236
std::string path = file.GetPath();
261237
llvm::StringRef path_ref(path);
262-
std::lock_guard<std::mutex> lock(m_pairs_mutex);
238+
std::lock_guard<std::recursive_mutex> lock(m_mutex);
263239
for (const auto &it : m_pairs) {
264240
llvm::StringRef removed_prefix = it.second.GetStringRef();
265241
if (!path_ref.consume_front(it.second.GetStringRef()))
@@ -288,35 +264,34 @@ PathMappingList::FindFile(const FileSpec &orig_spec) const {
288264

289265
bool PathMappingList::Replace(llvm::StringRef path, llvm::StringRef new_path,
290266
bool notify) {
291-
{
292-
std::lock_guard<std::mutex> lock(m_pairs_mutex);
293-
uint32_t idx = FindIndexForPath(path);
294-
if (idx >= m_pairs.size())
295-
return false;
267+
std::lock_guard<std::recursive_mutex> lock(m_mutex);
268+
uint32_t idx = FindIndexForPath(path);
269+
if (idx < m_pairs.size()) {
296270
++m_mod_id;
297271
m_pairs[idx].second = ConstString(new_path);
272+
if (notify && m_callback)
273+
m_callback(*this, m_callback_baton);
274+
return true;
298275
}
299-
Notify(notify);
300-
return true;
276+
return false;
301277
}
302278

303279
bool PathMappingList::Remove(ConstString path, bool notify) {
304-
{
305-
std::lock_guard<std::mutex> lock(m_pairs_mutex);
306-
iterator pos = FindIteratorForPath(path);
307-
if (pos == m_pairs.end())
308-
return false;
309-
280+
std::lock_guard<std::recursive_mutex> lock(m_mutex);
281+
iterator pos = FindIteratorForPath(path);
282+
if (pos != m_pairs.end()) {
310283
++m_mod_id;
311284
m_pairs.erase(pos);
285+
if (notify && m_callback)
286+
m_callback(*this, m_callback_baton);
287+
return true;
312288
}
313-
Notify(notify);
314-
return true;
289+
return false;
315290
}
316291

317292
PathMappingList::const_iterator
318293
PathMappingList::FindIteratorForPath(ConstString path) const {
319-
std::lock_guard<std::mutex> lock(m_pairs_mutex);
294+
std::lock_guard<std::recursive_mutex> lock(m_mutex);
320295
const_iterator pos;
321296
const_iterator begin = m_pairs.begin();
322297
const_iterator end = m_pairs.end();
@@ -330,7 +305,7 @@ PathMappingList::FindIteratorForPath(ConstString path) const {
330305

331306
PathMappingList::iterator
332307
PathMappingList::FindIteratorForPath(ConstString path) {
333-
std::lock_guard<std::mutex> lock(m_pairs_mutex);
308+
std::lock_guard<std::recursive_mutex> lock(m_mutex);
334309
iterator pos;
335310
iterator begin = m_pairs.begin();
336311
iterator end = m_pairs.end();
@@ -344,7 +319,7 @@ PathMappingList::FindIteratorForPath(ConstString path) {
344319

345320
bool PathMappingList::GetPathsAtIndex(uint32_t idx, ConstString &path,
346321
ConstString &new_path) const {
347-
std::lock_guard<std::mutex> lock(m_pairs_mutex);
322+
std::lock_guard<std::recursive_mutex> lock(m_mutex);
348323
if (idx < m_pairs.size()) {
349324
path = m_pairs[idx].first;
350325
new_path = m_pairs[idx].second;
@@ -355,7 +330,7 @@ bool PathMappingList::GetPathsAtIndex(uint32_t idx, ConstString &path,
355330

356331
uint32_t PathMappingList::FindIndexForPath(llvm::StringRef orig_path) const {
357332
const ConstString path = ConstString(NormalizePath(orig_path));
358-
std::lock_guard<std::mutex> lock(m_pairs_mutex);
333+
std::lock_guard<std::recursive_mutex> lock(m_mutex);
359334
const_iterator pos;
360335
const_iterator begin = m_pairs.begin();
361336
const_iterator end = m_pairs.end();

0 commit comments

Comments
 (0)