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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Very nice! Structure is good now. I found a few other issue we should fix as well.



================
Comment at: lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp:225
+    m.SizeOfImage = static_cast<llvm::support::ulittle32_t>(
+        mod->GetObjectFile()->GetByteSize());
+    m.Checksum = static_cast<llvm::support::ulittle32_t>(0);
----------------
I am not sure if the object file's full size is the right value here. I think the right value would be the last contiguous loaded address range for a given file. This information is going to be used to set the load addresses of any sections in the object file when it is loaded, and if we have a huge ELF file that has tons of debug info, this is going to make the loaded address range way too big.

So the algorithm I would suggest is:
1 - grab the section that contains base address:
2 - in a loop, grab the next section that starts  at the end of this section, and as long as the addresses are contiguous, increase the SizeOfImage:

So something like:
```
lldb::SectionSP sect_sp = mod->GetObjectFile()->GetBaseAddress().GetSection();
uint64_t SizeOfImage = 0;
if (sect_sp) {
  lldb::addr_t sect_addr = sect_sp->GetLoadAddress(&target);
  // Use memory size since zero fill sections, like ".bss", will be smaller on disk.
  lldb::addr_t sect_size = sect_sp->MemorySize(); 
  // This will usually be zero, but make sure to calculate the BaseOfImage offset.
  const lldb::addr_t base_sect_offset = m.BaseOfImage - sect_addr;
  SizeOfImage = sect_size - base_sect_offset;
  lldb::addr_t next_sect_addr = sect_addr + sect_size;
  Address sect_so_addr;
  target.ResolveLoadAddress(next_sect_addr, sect_so_addr);
  lldb::SectionSP next_sect_sp = sect_so_addr.GetSections();
  while (next_sect_sp && next_sect_sp->GetLoadBaseAddress(&target) == next_sect_addr)) {
    sect_size = sect_sp->MemorySize(); 
    SizeOfImage += sect_size;
    next_sect_addr += sect_size;
    target.ResolveLoadAddress(next_sect_addr, sect_so_addr);
    next_sect_sp = sect_so_addr.GetSections();
  }
  m.SizeOfImage = static_cast<llvm::support::ulittle32_t>(SizeOfImage);
} else {
  m.SizeOfImage = static_cast<llvm::support::ulittle32_t>(sect_size);
}
```



================
Comment at: lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp:519
+lldb_private::Status
+MinidumpFileBuilder::AddMemoryList(const lldb::ProcessSP &process_sp) {
+  Status error;
----------------
This should take in the CoreStyle and only emit what was asked for.

If style is set to "eSaveCoreStackOnly", then grab only the memory regions for each thread stack pointer and save only those. 

We can't tell from LLDB APIs if a page is dirty, so if we get "eSaveCoreDirtyOnly", then we will need to save all memory regions that have write permissions.

If it is either "eSaveCoreFull"  or "eSaveCoreUnspecified" then save everything.


================
Comment at: lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp:667
+    if (memory_buffer) {
+      size_t size = memory_buffer->getBufferSize();
+      AddDirectory(stream, size);
----------------
Do we need to make sure size is not zero?


================
Comment at: lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h:1-3
+//===-- MinidumpFileBuilder.h ---------------------------------- -*- C++
+//-*-===//
+//
----------------
Fix this header comment line to be on one line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108233