[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:519
+lldb_private::Status
+MinidumpFileBuilder::AddMemoryList(const lldb::ProcessSP &process_sp) {
+  Status error;
----------------
Aj0SK wrote:
> 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.
To state my preference, I would rather stick to the first option and landed a minimum viable product. The only reason for this is that I have this version way better tested and also I am not entirely sure about how minidump deals with the whole memory dumps as it can be a little problematic for a big memory chunks... Then, I would rather add the necessary changes in the next patch...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108233