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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
