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

[Andrej Korman via Phabricator via lldb-commits]

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