Aj0SK added inline comments.
================
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);
----------------
clayborg wrote:
> 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);
> }
> ```
>
Thank you for the suggestion. I will add it to the next revision.
================
Comment at: lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp:519
+lldb_private::Status
+MinidumpFileBuilder::AddMemoryList(const lldb::ProcessSP &process_sp) {
+ Status error;
----------------
clayborg wrote:
> 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.
I agree that this code should take care of a CoreStyle but it poses a
significant problem to this implementation as it was mainly designed to save
smaller Minidumps. This solution stores all of the information in memory before
dumping them (this eases an implementation quite a bit because of the way how
Minidump pointers (offsets) are working). This implementation, on my machine,
exhausted memory before the minidump could be written in case of a full memory
dump. At this time, we don't plan to reimplement this solution in the way it
would allow to store all the data on disc at the time of minidump creation so
there are two possible solutions that I see:
1. If the type of the CoreStyle is full or dirty, this plugin will return false
from the SaveCore function. Then maybe the default CoreStyle for this plugin
should be changed to "eSaveCoreStackOnly".
2. To the best of my knowledge, it should be theoretically possible to
reimplement Dump() method to sort of take special care of a MemoryListStream,
dumping also the memory information at the end of the minidump file as I think
the memory dump is the most stressful for the memory and otherwise there is no
problem with this.
================
Comment at: lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp:667
+ if (memory_buffer) {
+ size_t size = memory_buffer->getBufferSize();
+ AddDirectory(stream, size);
----------------
clayborg wrote:
> Do we need to make sure size is not zero?
I can add this. The idea was that even if the file is "empty", we at least give
the user opening the minidump the information that we have been able to find
this file and add it to the minidump instead of only not providing any
information at all.
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
