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

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to