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

[Greg Clayton via Phabricator via lldb-commits]

clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.

Thanks for all of the changes! Looks great.



================
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) {
----------------
Aj0SK wrote:
> Aj0SK wrote:
> > 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!
> One thing, that could be a little problematic with the new approach is, if the possibility to save full/dirty pages dump would be requested... In this case, we still need to go through all of the sections...
Correct, we would just iterate over all memory regions and get the next region by asking for the region that starts at the end address of the current region.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108233