[lldb] Fix unsafe map mutation in ProcessElfCore::FindModuleUUID

[dmpots]

The ProcessElfCore::FindModuleUUID function can be called by multiple threads at the same time when target.parallel-module-load is true. We were using the operator[] to lookup the UUID which will mutate the map when the key is not present. This is unsafe in a multi-threaded contex so we now use a read-only find operation and explicitly return an invalid UUID when the key is not present.

The m_uuids map can follow a create-then-query pattern. It is populated in the DoLoadCore function which looks like it is only called in a single-threaded context so we do not need extra locking as long as we keep the other accesses read-only.

Other fixes I considered

• Use a lock to protect access - We don't need to modify the map after creation so we can allow concurrent read-only access.
• Store the map in a const pointer container to prevent accidental mutation in other places.
  • Only accessed in one place currently so just added a comment.
• Store the UUID in the NT_FILE_Entry after building the mapping correctly in UpdateBuildIdForNTFileEntries. - The map lets us avoid a linear search in FindModuleUUID.

This commit also reverts the temporary workaround from #159395 which disabled parallel module loading to avoid the test failure.

Fixes #159377

[llvmbot]

@llvm/pr-subscribers-lldb

Author: David Peixotto (dmpots)

Changes

The ProcessElfCore::FindModuleUUID function can be called by multiple threads at the same time when target.parallel-module-load is true. We were using the operator[] to lookup the UUID which will mutate the map when the key is not present. This is unsafe in a multi-threaded contex so we now use a read-only find operation and explicitly return an invalid UUID when the key is not present.

The m_uuids map can follow a create-then-query pattern. It is populated in the DoLoadCore function which looks like it is only called in a single-threaded context so we do not need extra locking as long as we keep the other accesses read-only.

Other fixes I considered

• Use a lock to protect access - We don't need to modify the map after creation so we can allow concurrent read-only access.
• Store the map in a const pointer container to prevent accidental mutation in other places.
  • Only accessed in one place currently so just added a comment.
• Store the UUID in the NT_FILE_Entry after building the mapping correctly in UpdateBuildIdForNTFileEntries. - The map lets us avoid a linear search in FindModuleUUID.

This commit also reverts the temporary workaround from #159395 which disabled parallel module loading to avoid the test failure.

Fixes #159377

---

Full diff: https://github.com/llvm/llvm-project/pull/159444.diff

2 Files Affected:

• (modified) lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp (+7-1)
• (modified) lldb/test/API/functionalities/postmortem/netbsd-core/TestNetBSDCore.py (-2)

diff --git a/lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp b/lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
index 8f5f1242116f5..084f8012bcc8d 100644
--- a/lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
+++ b/lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
@@ -308,7 +308,13 @@ llvm::StringRef ProcessElfCore::GetMainExecutablePath() {
 }
 
 UUID ProcessElfCore::FindModuleUUID(const llvm::StringRef path) {
-  return m_uuids[std::string(path)];
+  // Lookup the UUID for the given path in the map.
+  // Note that this could be called by multiple threads so make sure
+  // we access the map in a thread safe way (i.e. don't use operator[]).
+  auto it = m_uuids.find(std::string(path));
+  if (it != m_uuids.end())
+    return it->second;
+  return UUID();
 }
 
 lldb_private::DynamicLoader *ProcessElfCore::GetDynamicLoader() {
diff --git a/lldb/test/API/functionalities/postmortem/netbsd-core/TestNetBSDCore.py b/lldb/test/API/functionalities/postmortem/netbsd-core/TestNetBSDCore.py
index 179dbdf88fa8a..ff1ef21e02e31 100644
--- a/lldb/test/API/functionalities/postmortem/netbsd-core/TestNetBSDCore.py
+++ b/lldb/test/API/functionalities/postmortem/netbsd-core/TestNetBSDCore.py
@@ -117,8 +117,6 @@ def check_backtrace(self, thread, filename, backtrace):
                 )
 
     def do_test(self, filename, pid, region_count):
-        # Temporary workaround for https://github.com/llvm/llvm-project/issues/159377
-        self.runCmd("settings set target.parallel-module-load false")
         target = self.dbg.CreateTarget(filename)
         process = target.LoadCore(filename + ".core")
 

[DavidSpickett]

The tests are stable on 32-bit Arm with this applied.