lldb/minidump: Refactor memory region computation code

The goal of this refactor is to enable ProcessMinidump to take into
account the loaded modules and their sections when computing the
permissions of various ranges of memory, as discussed in D66638.

This patch moves some of the responsibility for computing the ranges
from MinidumpParser into ProcessMinidump. MinidumpParser still does the
parsing, but ProcessMinidump becomes responsible for answering the
actual queries about memory ranges. This will enable it (in a follow-up
patch) to augment the information obtained from the parser with data
obtained from actual object files.

The changes in the actual code are fairly straight-forward and just
involve moving code around. MinidumpParser::GetMemoryRegions is renamed
to BuildMemoryRegions to emphasize that it does no caching. The only new
thing is the additional bool flag returned from this function. This
indicates whether the returned regions describe all memory mapped into
the target process. Data obtained from /proc/maps and the MemoryInfoList
stream is considered to be exhaustive. Data obtained from Memory(64)List
is not. This will be used to determine whether we need to augment the
data or not.

This reshuffle means that it is no longer possible/easy to test some of
this code via unit tests, as constructing a ProcessMinidump instance is
hard. Instead, I update the unit tests to only test the parsing of the
actual data, and test the answering of queries through a lit test using
the "memory region" command. The patch also includes some tweaks to the
MemoryRegion class to make the unit tests easier to write.

Reviewers: amccarth, clayborg

Subscribers: lldb-commits

Differential Revision: https://reviews.llvm.org/D69035

4 files changed, 62 insertions, 64 deletions

diff --git a/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp b/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
index 70933f91fe5..47cfd5bd273 100644
--- a/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
+++ b/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
@@ -518,58 +518,26 @@ CreateRegionsCacheFromMemory64List(MinidumpParser &parser,
   return !regions.empty();
 }
 
-MemoryRegionInfo
-MinidumpParser::FindMemoryRegion(lldb::addr_t load_addr) const {
-  auto begin = m_regions.begin();
-  auto end = m_regions.end();
-  auto pos = std::lower_bound(begin, end, load_addr);
-  if (pos != end && pos->GetRange().Contains(load_addr))
-    return *pos;
-  
-  MemoryRegionInfo region;
-  if (pos == begin)
-    region.GetRange().SetRangeBase(0);
-  else {
-    auto prev = pos - 1;
-    if (prev->GetRange().Contains(load_addr))
-      return *prev;
-    region.GetRange().SetRangeBase(prev->GetRange().GetRangeEnd());
-  }
-  if (pos == end)
-    region.GetRange().SetRangeEnd(UINT64_MAX);
-  else
-    region.GetRange().SetRangeEnd(pos->GetRange().GetRangeBase());
-  region.SetReadable(MemoryRegionInfo::eNo);
-  region.SetWritable(MemoryRegionInfo::eNo);
-  region.SetExecutable(MemoryRegionInfo::eNo);
-  region.SetMapped(MemoryRegionInfo::eNo);
-  return region;
-}
-
-MemoryRegionInfo
-MinidumpParser::GetMemoryRegionInfo(lldb::addr_t load_addr) {
-  if (!m_parsed_regions)
-    GetMemoryRegions();
-  return FindMemoryRegion(load_addr);
-}
-
-const MemoryRegionInfos &MinidumpParser::GetMemoryRegions() {
-  if (!m_parsed_regions) {
-    m_parsed_regions = true;
-    // We haven't cached our memory regions yet we will create the region cache
-    // once. We create the region cache using the best source. We start with
-    // the linux maps since they are the most complete and have names for the
-    // regions. Next we try the MemoryInfoList since it has
-    // read/write/execute/map data, and then fall back to the MemoryList and
-    // Memory64List to just get a list of the memory that is mapped in this
-    // core file
-    if (!CreateRegionsCacheFromLinuxMaps(*this, m_regions))
-      if (!CreateRegionsCacheFromMemoryInfoList(*this, m_regions))
-        if (!CreateRegionsCacheFromMemoryList(*this, m_regions))
-          CreateRegionsCacheFromMemory64List(*this, m_regions);
-    llvm::sort(m_regions.begin(), m_regions.end());
-  }
-  return m_regions;
+std::pair<MemoryRegionInfos, bool> MinidumpParser::BuildMemoryRegions() {
+  // We create the region cache using the best source. We start with
+  // the linux maps since they are the most complete and have names for the
+  // regions. Next we try the MemoryInfoList since it has
+  // read/write/execute/map data, and then fall back to the MemoryList and
+  // Memory64List to just get a list of the memory that is mapped in this
+  // core file
+  MemoryRegionInfos result;
+  const auto &return_sorted = [&](bool is_complete) {
+    llvm::sort(result);
+    return std::make_pair(std::move(result), is_complete);
+  };
+  if (CreateRegionsCacheFromLinuxMaps(*this, result))
+    return return_sorted(true);
+  if (CreateRegionsCacheFromMemoryInfoList(*this, result))
+    return return_sorted(true);
+  if (CreateRegionsCacheFromMemoryList(*this, result))
+    return return_sorted(false);
+  CreateRegionsCacheFromMemory64List(*this, result);
+  return return_sorted(false);
 }
 
 #define ENUM_TO_CSTR(ST)                                                       \
diff --git a/lldb/source/Plugins/Process/minidump/MinidumpParser.h b/lldb/source/Plugins/Process/minidump/MinidumpParser.h
index d206fe6c9a0..4bcb2b47d45 100644
--- a/lldb/source/Plugins/Process/minidump/MinidumpParser.h
+++ b/lldb/source/Plugins/Process/minidump/MinidumpParser.h
@@ -88,9 +88,9 @@ public:
 
   llvm::ArrayRef<uint8_t> GetMemory(lldb::addr_t addr, size_t size);
 
-  MemoryRegionInfo GetMemoryRegionInfo(lldb::addr_t load_addr);
-
-  const MemoryRegionInfos &GetMemoryRegions();
+  /// Returns a list of memory regions and a flag indicating whether the list is
+  /// complete (includes all regions mapped into the process memory).
+  std::pair<MemoryRegionInfos, bool> BuildMemoryRegions();
 
   static llvm::StringRef GetStreamTypeAsString(StreamType stream_type);
 
@@ -100,14 +100,10 @@ private:
   MinidumpParser(lldb::DataBufferSP data_sp,
                  std::unique_ptr<llvm::object::MinidumpFile> file);
 
-  MemoryRegionInfo FindMemoryRegion(lldb::addr_t load_addr) const;
-
 private:
   lldb::DataBufferSP m_data_sp;
   std::unique_ptr<llvm::object::MinidumpFile> m_file;
   ArchSpec m_arch;
-  MemoryRegionInfos m_regions;
-  bool m_parsed_regions = false;
 };
 
 } // end namespace minidump
diff --git a/lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp b/lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
index e30a3c82a88..5dcaaaa34d9 100644
--- a/lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ b/lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -334,15 +334,46 @@ ArchSpec ProcessMinidump::GetArchitecture() {
   return ArchSpec(triple);
 }
 
+void ProcessMinidump::BuildMemoryRegions() {
+  if (m_memory_regions)
+    return;
+  m_memory_regions.emplace();
+  bool is_complete;
+  std::tie(*m_memory_regions, is_complete) =
+      m_minidump_parser->BuildMemoryRegions();
+  // TODO: Use loaded modules to complete the region list.
+}
+
 Status ProcessMinidump::GetMemoryRegionInfo(lldb::addr_t load_addr,
-                                            MemoryRegionInfo &range_info) {
-  range_info = m_minidump_parser->GetMemoryRegionInfo(load_addr);
+                                            MemoryRegionInfo &region) {
+  BuildMemoryRegions();
+  auto pos = llvm::upper_bound(*m_memory_regions, load_addr);
+  if (pos != m_memory_regions->begin() &&
+      std::prev(pos)->GetRange().Contains(load_addr)) {
+    region = *std::prev(pos);
+    return Status();
+  }
+
+  if (pos == m_memory_regions->begin())
+    region.GetRange().SetRangeBase(0);
+  else
+    region.GetRange().SetRangeBase(std::prev(pos)->GetRange().GetRangeEnd());
+
+  if (pos == m_memory_regions->end())
+    region.GetRange().SetRangeEnd(UINT64_MAX);
+  else
+    region.GetRange().SetRangeEnd(pos->GetRange().GetRangeBase());
+
+  region.SetReadable(MemoryRegionInfo::eNo);
+  region.SetWritable(MemoryRegionInfo::eNo);
+  region.SetExecutable(MemoryRegionInfo::eNo);
+  region.SetMapped(MemoryRegionInfo::eNo);
   return Status();
 }
 
-Status ProcessMinidump::GetMemoryRegions(
-    lldb_private::MemoryRegionInfos &region_list) {
-  region_list = m_minidump_parser->GetMemoryRegions();
+Status ProcessMinidump::GetMemoryRegions(MemoryRegionInfos &region_list) {
+  BuildMemoryRegions();
+  region_list = *m_memory_regions;
   return Status();
 }
 
diff --git a/lldb/source/Plugins/Process/minidump/ProcessMinidump.h b/lldb/source/Plugins/Process/minidump/ProcessMinidump.h
index 22dc24af7c0..750164cf8aa 100644
--- a/lldb/source/Plugins/Process/minidump/ProcessMinidump.h
+++ b/lldb/source/Plugins/Process/minidump/ProcessMinidump.h
@@ -111,6 +111,9 @@ private:
   const minidump::ExceptionStream *m_active_exception;
   lldb::CommandObjectSP m_command_sp;
   bool m_is_wow64;
+  llvm::Optional<MemoryRegionInfos> m_memory_regions;
+
+  void BuildMemoryRegions();
 };
 
 } // namespace minidump