Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[lldb] Improve locking in PathMappingLists (NFC) #114576

Merged
merged 2 commits into from
Nov 1, 2024
Merged

[lldb] Improve locking in PathMappingLists (NFC) #114576

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9ebfdc8
[lldb] Improve locking in PathMappingLists (NFC)
JDevlieghere Nov 1, 2024
081c9c1
Invert order in which we acquire mutexes in operator=
JDevlieghere Nov 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 13 additions & 6 deletions lldb/include/lldb/Target/PathMappingList.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ class PathMappingList {
typedef void (*ChangedCallback)(const PathMappingList &path_list,
void *baton);

// Constructors and Destructors
PathMappingList();

PathMappingList(ChangedCallback callback, void *callback_baton);
Expand Down Expand Up @@ -53,12 +52,12 @@ class PathMappingList {
llvm::json::Value ToJSON();

bool IsEmpty() const {
std::lock_guard<std::recursive_mutex> lock(m_mutex);
std::lock_guard<std::mutex> lock(m_pairs_mutex);
return m_pairs.empty();
}

size_t GetSize() const {
std::lock_guard<std::recursive_mutex> lock(m_mutex);
std::lock_guard<std::mutex> lock(m_pairs_mutex);
return m_pairs.size();
}

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

uint32_t GetModificationID() const {
std::lock_guard<std::recursive_mutex> lock(m_mutex);
std::lock_guard<std::mutex> lock(m_pairs_mutex);
return m_mod_id;
}

protected:
mutable std::recursive_mutex m_mutex;
typedef std::pair<ConstString, ConstString> pair;
typedef std::vector<pair> collection;
typedef collection::iterator iterator;
typedef collection::const_iterator const_iterator;

void AppendImpl(llvm::StringRef path, llvm::StringRef replacement);
void Notify(bool notify) const;

iterator FindIteratorForPath(ConstString path);

const_iterator FindIteratorForPath(ConstString path) const;

collection m_pairs;
mutable std::mutex m_pairs_mutex;

ChangedCallback m_callback = nullptr;
void *m_callback_baton = nullptr;
uint32_t m_mod_id = 0; // Incremented anytime anything is added or removed.
mutable std::mutex m_callback_mutex;

/// Incremented anytime anything is added to or removed from m_pairs. Also
/// protected by m_pairs_mutex.
uint32_t m_mod_id = 0;
};

} // namespace lldb_private
Expand Down
163 changes: 94 additions & 69 deletions lldb/source/Target/PathMappingList.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,10 @@ PathMappingList::PathMappingList(const PathMappingList &rhs)

const PathMappingList &PathMappingList::operator=(const PathMappingList &rhs) {
if (this != &rhs) {
std::scoped_lock<std::recursive_mutex, std::recursive_mutex> locks(m_mutex, rhs.m_mutex);
std::scoped_lock<std::mutex, std::mutex> callback_locks(
m_callback_mutex, rhs.m_callback_mutex);
JDevlieghere marked this conversation as resolved.
Show resolved Hide resolved
std::scoped_lock<std::mutex, std::mutex> pairs_locks(m_pairs_mutex,
rhs.m_pairs_mutex);
m_pairs = rhs.m_pairs;
m_callback = nullptr;
m_callback_baton = nullptr;
Expand All @@ -59,85 +62,105 @@ const PathMappingList &PathMappingList::operator=(const PathMappingList &rhs) {

PathMappingList::~PathMappingList() = default;

void PathMappingList::Append(llvm::StringRef path, llvm::StringRef replacement,
bool notify) {
std::lock_guard<std::recursive_mutex> lock(m_mutex);
void PathMappingList::AppendImpl(llvm::StringRef path,
llvm::StringRef replacement) {
++m_mod_id;
m_pairs.emplace_back(pair(NormalizePath(path), NormalizePath(replacement)));
}

void PathMappingList::Notify(bool notify) const {
std::lock_guard<std::mutex> lock(m_callback_mutex);
if (notify && m_callback)
m_callback(*this, m_callback_baton);
}

void PathMappingList::Append(llvm::StringRef path, llvm::StringRef replacement,
bool notify) {
{
std::lock_guard<std::mutex> lock(m_pairs_mutex);
AppendImpl(path, replacement);
}
Notify(notify);
}

void PathMappingList::Append(const PathMappingList &rhs, bool notify) {
std::scoped_lock<std::recursive_mutex, std::recursive_mutex> locks(m_mutex, rhs.m_mutex);
++m_mod_id;
if (!rhs.m_pairs.empty()) {
{
std::scoped_lock<std::mutex, std::mutex> locks(m_pairs_mutex,
rhs.m_pairs_mutex);
++m_mod_id;
if (rhs.m_pairs.empty())
return;
const_iterator pos, end = rhs.m_pairs.end();
for (pos = rhs.m_pairs.begin(); pos != end; ++pos)
m_pairs.push_back(*pos);
if (notify && m_callback)
m_callback(*this, m_callback_baton);
}
Notify(notify);
}

bool PathMappingList::AppendUnique(llvm::StringRef path,
llvm::StringRef replacement, bool notify) {
auto normalized_path = NormalizePath(path);
auto normalized_replacement = NormalizePath(replacement);
std::lock_guard<std::recursive_mutex> lock(m_mutex);
for (const auto &pair : m_pairs) {
if (pair.first.GetStringRef() == normalized_path &&
pair.second.GetStringRef() == normalized_replacement)
return false;
{
std::lock_guard<std::mutex> lock(m_pairs_mutex);
for (const auto &pair : m_pairs) {
if (pair.first.GetStringRef() == normalized_path &&
pair.second.GetStringRef() == normalized_replacement)
return false;
}
AppendImpl(path, replacement);
}
Append(path, replacement, notify);
Notify(notify);
return true;
}

void PathMappingList::Insert(llvm::StringRef path, llvm::StringRef replacement,
uint32_t index, bool notify) {
std::lock_guard<std::recursive_mutex> lock(m_mutex);
++m_mod_id;
iterator insert_iter;
if (index >= m_pairs.size())
insert_iter = m_pairs.end();
else
insert_iter = m_pairs.begin() + index;
m_pairs.emplace(insert_iter, pair(NormalizePath(path),
NormalizePath(replacement)));
if (notify && m_callback)
m_callback(*this, m_callback_baton);
{
std::lock_guard<std::mutex> lock(m_pairs_mutex);
++m_mod_id;
iterator insert_iter;
if (index >= m_pairs.size())
insert_iter = m_pairs.end();
else
insert_iter = m_pairs.begin() + index;
m_pairs.emplace(insert_iter,
pair(NormalizePath(path), NormalizePath(replacement)));
}
Notify(notify);
}

bool PathMappingList::Replace(llvm::StringRef path, llvm::StringRef replacement,
uint32_t index, bool notify) {
std::lock_guard<std::recursive_mutex> lock(m_mutex);
if (index >= m_pairs.size())
return false;
++m_mod_id;
m_pairs[index] = pair(NormalizePath(path), NormalizePath(replacement));
if (notify && m_callback)
m_callback(*this, m_callback_baton);
{
std::lock_guard<std::mutex> lock(m_pairs_mutex);
if (index >= m_pairs.size())
return false;
++m_mod_id;
m_pairs[index] = pair(NormalizePath(path), NormalizePath(replacement));
}
Notify(notify);
return true;
}

bool PathMappingList::Remove(size_t index, bool notify) {
std::lock_guard<std::recursive_mutex> lock(m_mutex);
if (index >= m_pairs.size())
return false;
{
std::lock_guard<std::mutex> lock(m_pairs_mutex);
if (index >= m_pairs.size())
return false;

++m_mod_id;
iterator iter = m_pairs.begin() + index;
m_pairs.erase(iter);
if (notify && m_callback)
m_callback(*this, m_callback_baton);
++m_mod_id;
iterator iter = m_pairs.begin() + index;
m_pairs.erase(iter);
}
Notify(notify);
return true;
}

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

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

llvm::json::Value PathMappingList::ToJSON() {
llvm::json::Array entries;
std::lock_guard<std::recursive_mutex> lock(m_mutex);
std::lock_guard<std::mutex> lock(m_pairs_mutex);
for (const auto &pair : m_pairs) {
llvm::json::Array entry{pair.first.GetStringRef().str(),
pair.second.GetStringRef().str()};
Expand All @@ -165,12 +188,13 @@ llvm::json::Value PathMappingList::ToJSON() {
}

void PathMappingList::Clear(bool notify) {
std::lock_guard<std::recursive_mutex> lock(m_mutex);
if (!m_pairs.empty())
++m_mod_id;
m_pairs.clear();
if (notify && m_callback)
m_callback(*this, m_callback_baton);
{
std::lock_guard<std::mutex> lock(m_pairs_mutex);
if (!m_pairs.empty())
++m_mod_id;
m_pairs.clear();
}
Notify(notify);
}

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

std::optional<FileSpec> PathMappingList::RemapPath(llvm::StringRef mapping_path,
bool only_if_exists) const {
std::lock_guard<std::recursive_mutex> lock(m_mutex);
std::lock_guard<std::mutex> lock(m_pairs_mutex);
if (m_pairs.empty() || mapping_path.empty())
return {};
LazyBool path_is_relative = eLazyBoolCalculate;
Expand Down Expand Up @@ -235,7 +259,7 @@ std::optional<llvm::StringRef>
PathMappingList::ReverseRemapPath(const FileSpec &file, FileSpec &fixed) const {
std::string path = file.GetPath();
llvm::StringRef path_ref(path);
std::lock_guard<std::recursive_mutex> lock(m_mutex);
std::lock_guard<std::mutex> lock(m_pairs_mutex);
for (const auto &it : m_pairs) {
llvm::StringRef removed_prefix = it.second.GetStringRef();
if (!path_ref.consume_front(it.second.GetStringRef()))
Expand Down Expand Up @@ -264,34 +288,35 @@ PathMappingList::FindFile(const FileSpec &orig_spec) const {

bool PathMappingList::Replace(llvm::StringRef path, llvm::StringRef new_path,
bool notify) {
std::lock_guard<std::recursive_mutex> lock(m_mutex);
uint32_t idx = FindIndexForPath(path);
if (idx < m_pairs.size()) {
{
std::lock_guard<std::mutex> lock(m_pairs_mutex);
uint32_t idx = FindIndexForPath(path);
Comment on lines +292 to +293
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JDevlieghere It seems these two lines lead to deadlock. FindIndexForPath attempts to lock m_pairs_mutex too, but can't.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, I fixed the issue in the re-land. @kastiglione confirmed offline that that fixed the deadlock he observed.

if (idx >= m_pairs.size())
return false;
++m_mod_id;
m_pairs[idx].second = ConstString(new_path);
if (notify && m_callback)
m_callback(*this, m_callback_baton);
return true;
}
return false;
Notify(notify);
return true;
}

bool PathMappingList::Remove(ConstString path, bool notify) {
std::lock_guard<std::recursive_mutex> lock(m_mutex);
iterator pos = FindIteratorForPath(path);
if (pos != m_pairs.end()) {
{
std::lock_guard<std::mutex> lock(m_pairs_mutex);
iterator pos = FindIteratorForPath(path);
if (pos == m_pairs.end())
return false;

++m_mod_id;
m_pairs.erase(pos);
if (notify && m_callback)
m_callback(*this, m_callback_baton);
return true;
}
return false;
Notify(notify);
return true;
}

PathMappingList::const_iterator
PathMappingList::FindIteratorForPath(ConstString path) const {
std::lock_guard<std::recursive_mutex> lock(m_mutex);
std::lock_guard<std::mutex> lock(m_pairs_mutex);
const_iterator pos;
const_iterator begin = m_pairs.begin();
const_iterator end = m_pairs.end();
Expand All @@ -305,7 +330,7 @@ PathMappingList::FindIteratorForPath(ConstString path) const {

PathMappingList::iterator
PathMappingList::FindIteratorForPath(ConstString path) {
std::lock_guard<std::recursive_mutex> lock(m_mutex);
std::lock_guard<std::mutex> lock(m_pairs_mutex);
iterator pos;
iterator begin = m_pairs.begin();
iterator end = m_pairs.end();
Expand All @@ -319,7 +344,7 @@ PathMappingList::FindIteratorForPath(ConstString path) {

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

uint32_t PathMappingList::FindIndexForPath(llvm::StringRef orig_path) const {
const ConstString path = ConstString(NormalizePath(orig_path));
std::lock_guard<std::recursive_mutex> lock(m_mutex);
std::lock_guard<std::mutex> lock(m_pairs_mutex);
const_iterator pos;
const_iterator begin = m_pairs.begin();
const_iterator end = m_pairs.end();
Expand Down
Loading