[Lldb-commits] [PATCH] D108233: WIP: Add minidump save-core functionality to ELF object files

[Andrej Korman via Phabricator via lldb-commits]

Aj0SK added inline comments.


================
Comment at: lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp:549
+    bool is_interesting = false;
+    for (size_t interesting_address : interesting_addresses)
+      if (interesting_address >= addr && interesting_address < addr + size) {
----------------
clayborg wrote:
> Shouldn't this be the main loop that would replace the "while (range_info.GetRange().GetRangeBase() != LLDB_INVALID_ADDRESS)" loop? I would think that the main loop wiould be something like:
> 
> ```
> std::set<addr_t> visited_region_base_addresses;
> for (size_t interesting_address : interesting_addresses) {
>   error = process_sp->GetMemoryRegionInfo(interesting_address, range_info);
>   // Skip failed memory region requests or any regions with no permissions.
>   if (error.Fail() || range_info.GetPermissions() == 0)
>     continue;
>   const addr_t addr = range_info.GetRange().GetRangeBase();
>   // Skip any regions we have already saved out.
>   if (visited_region_base_addresses.insert(addr).second == false)
>     continue;
>   const addr_t size = range_info.GetRange().GetByteSize();
>   if (size == 0)
>     continue;
>   auto data_up = std::make_unique<DataBufferHeap>(size, 0);
>   const size_t bytes_read = process_sp->ReadMemory(addr, data_up->GetBytes(), size, error);
>   if (bytes_read == 0)
>     continue;
>   // We have a good memory region with valid bytes to store.
>   LocationDescriptor memory_dump;
>   memory_dump.DataSize = static_cast<llvm::support::ulittle32_t>(size);
>   memory_dump.RVA =static_cast<llvm::support::ulittle32_t>(GetCurrentDataEndOffset());
>   MemoryDescriptor memory_desc;
>   memory_desc.StartOfMemoryRange = static_cast<llvm::support::ulittle64_t>(addr);
>   memory_desc.Memory = memory_dump;
>   mem_descriptors.push_back(memory_desc);
>   m_data.AppendData(data_up->GetBytes(), bytes_read);
> }
> ```
> 
> 
This kind of an approach was something I also thought about and in fact, I was mainly inspired by this [[ https://github.com/llvm-mirror/lldb/blob/d01083a850f577b85501a0902b52fd0930de72c7/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp#L6051 | approach ]]. Also, the previous comment about the need of saving the loaded modules addresses in a set doesn't apply to my solution, am I correct? Just to clear it a little bit, I like your solution though and it's better when I think about it. Thanks for the suggestion!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108233/new/

https://reviews.llvm.org/D108233