Protect Proces::GetMemoryRegionInfo and ::GetFileLoadAddress with a lock

[scott.smith]

Summary

Both routines (on Linux, at least) utilize a cache; protect the cache with a mutex to allow concurrent callers.

[labath]

This is not necessary. NativeProcess classes are only used in lldb-server, which is completely single threaded. If you want to change that, then we should start with a discussion of what you intend to achieve.

[scott.smith]

In D32568#739190, @labath wrote:

This is not necessary. NativeProcess classes are only used in lldb-server, which is completely single threaded. If you want to change that, then we should start with a discussion of what you intend to achieve.

Let me post the other two changes to start a broader discussion. We can center the conversation around whether/how to prime the caches; the other two changes naturally follow from that.

[labath]

In D32568#739607, @scott.smith wrote:

In D32568#739190, @labath wrote:

This is not necessary. NativeProcess classes are only used in lldb-server, which is completely single threaded. If you want to change that, then we should start with a discussion of what you intend to achieve.

Let me post the other two changes to start a broader discussion. We can center the conversation around whether/how to prime the caches; the other two changes naturally follow from that.

All your other changes are client-side, so still think this will not matter, but I'll take a look. :)

I meant to respond to the discussion today, but I got sidetracked by the ipv6 thingy. I'll try to look at that tomorrow.

[scott.smith]

In D32568#739723, @labath wrote:

All your other changes are client-side, so still think this will not matter, but I'll take a look. :)

Interesting. Maybe this only matters in the context of unit tests? I was still getting crashes without this change, but now I'm not (I've since git pull'd and switched to another machine). I'll keep investigating.

[scott.smith]

Further testing shows this is not necessary. I'll abandon it.

[labath]

Cool, glad that's sorted. I've had some ideas about introducing limited amount of paralellism to the server side, but that's probably is not interesting to you if you're not doing remote debugging.

source/Plugins/Process/Linux/NativeProcessLinux.h

protected:
   GetSoftwareBreakpointTrapOpcode(size_t trap_opcode_size_hint,
                                   size_t &actual_opcode_size,
                                   const uint8_t *&trap_opcode_bytes) override;
 
 private:
   MainLoop::SignalHandleUP m_sigchld_handle;
   ArchSpec m_arch;
 
+  // Mutex to protect the memory region cache.
+  std::recursive_mutex m_memory_region_mutex;
   LazyBool m_supports_mem_region;
   std::vector<std::pair<MemoryRegionInfo, FileSpec>> m_mem_region_cache;
 
   lldb::tid_t m_pending_notification_tid;
 
   // List of thread ids stepping with a breakpoint with the address of
   // the relevan breakpoint
   std::map<lldb::tid_t, lldb::addr_t> m_threads_stepping_with_breakpoint;

source/Plugins/Process/Linux/NativeProcessLinux.cpp

   if (name)
     memory_region_info.SetName(name);
 
   return Error();
 }
 
 Error NativeProcessLinux::GetMemoryRegionInfo(lldb::addr_t load_addr,
                                               MemoryRegionInfo &range_info) {
+  std::lock_guard<std::recursive_mutex> guard(m_memory_region_mutex);
   // FIXME review that the final memory region returned extends to the end of
   // the virtual address space,
   // with no perms if it is not mapped.
 
   // Use an approach that reads memory regions from /proc/{pid}/maps.
   // Assume proc maps entries are in ascending order.
   // FIXME assert if we find differently.
 

     return error;
   }
 
   return error;
 }
 
 Error NativeProcessLinux::GetLoadedModuleFileSpec(const char *module_path,
                                                   FileSpec &file_spec) {
+  std::lock_guard<std::recursive_mutex> guard(m_memory_region_mutex);
   Error error = PopulateMemoryRegionCache();
   if (error.Fail())
     return error;
 
   FileSpec module_file_spec(module_path, true);
 
   file_spec.Clear();
   for (const auto &it : m_mem_region_cache) {
     if (it.second.GetFilename() == module_file_spec.GetFilename()) {
       file_spec = it.second;
       return Error();
     }
   }
   return Error("Module file (%s) not found in /proc/%" PRIu64 "/maps file!",
                module_file_spec.GetFilename().AsCString(), GetID());
 }
 
 Error NativeProcessLinux::GetFileLoadAddress(const llvm::StringRef &file_name,
                                              lldb::addr_t &load_addr) {
+  std::lock_guard<std::recursive_mutex> guard(m_memory_region_mutex);
   load_addr = LLDB_INVALID_ADDRESS;
   Error error = PopulateMemoryRegionCache();
   if (error.Fail())
     return error;
 
   FileSpec file(file_name, false);
   for (const auto &it : m_mem_region_cache) {
     if (it.second == file) {

source/Plugins/Process/NetBSD/NativeProcessNetBSD.h

protected:
   Error
   GetSoftwareBreakpointTrapOpcode(size_t trap_opcode_size_hint,
                                   size_t &actual_opcode_size,
                                   const uint8_t *&trap_opcode_bytes) override;
 
 private:
   MainLoop::SignalHandleUP m_sigchld_handle;
   ArchSpec m_arch;
+
+  // Mutex to protect the memory region cache.
+  std::recursive_mutex m_memory_region_mutex;
   LazyBool m_supports_mem_region;
   std::vector<std::pair<MemoryRegionInfo, FileSpec>> m_mem_region_cache;
 
   // ---------------------------------------------------------------------
   // Private Instance Methods
   // ---------------------------------------------------------------------
   NativeProcessNetBSD();
 

source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp

Error NativeProcessNetBSD::Kill() {
   }
 
   return error;
 }
 
 Error NativeProcessNetBSD::GetMemoryRegionInfo(lldb::addr_t load_addr,
                                                MemoryRegionInfo &range_info) {
 
+  std::lock_guard<std::recursive_mutex> guard(m_memory_region_mutex);
   if (m_supports_mem_region == LazyBool::eLazyBoolNo) {
     // We're done.
     return Error("unsupported");
   }
 
   Error error = PopulateMemoryRegionCache();
   if (error.Fail()) {
     return error;

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp

 }
 
 Error GDBRemoteCommunicationClient::GetMemoryRegionInfo(
     lldb::addr_t addr, lldb_private::MemoryRegionInfo &region_info) {
   Error error;
   region_info.Clear();
 
   if (m_supports_memory_region_info != eLazyBoolNo) {
-    m_supports_memory_region_info = eLazyBoolYes;
     char packet[64];
     const int packet_len = ::snprintf(
         packet, sizeof(packet), "qMemoryRegionInfo:%" PRIx64, (uint64_t)addr);
     assert(packet_len < (int)sizeof(packet));
     UNUSED_IF_ASSERT_DISABLED(packet_len);
     StringExtractorGDBRemote response;
     if (SendPacketAndWaitForResponse(packet, response, false) ==
         PacketResult::Success) {

           region_info.SetWritable(MemoryRegionInfo::eNo);
           region_info.SetExecutable(MemoryRegionInfo::eNo);
           region_info.SetMapped(MemoryRegionInfo::eNo);
         }
       } else {
         // We got an invalid address range back
         error.SetErrorString("Server returned invalid range");
       }
+      m_supports_memory_region_info = eLazyBoolYes;
     } else {
       m_supports_memory_region_info = eLazyBoolNo;
     }
   }
 
   if (m_supports_memory_region_info == eLazyBoolNo) {
     error.SetErrorString("qMemoryRegionInfo is not supported");
   }