[Lldb-commits] [PATCH] D21751: Implement GetMemoryRegions() for Linux and Mac OSX core files.

[Howard Hellyer via lldb-commits]

hhellyer added a comment.

In http://reviews.llvm.org/D21751#468976, @clayborg wrote:

> We should document the Process::GetMemoryRegionInfo() and what we expect of it so that all plug-ins implement it correctly.


I'll add a full comment in Process.h and I'll make sure the description matches the behaviour of qMemoryRegionInfo.

> > I will push a revision which follows the approach you suggest, if we find that throws up it’s own problems then I can always revert. As I mentioned the remote connections should (hopefully) all work with that pattern so GetMemoryRegions implemented in fewer commits with that approach.

> 

> 

> Indeed. Since we already expect a behavior from Process::GetMemoryRegionInfo(), I would say lets enforce these requirements by making Process::GetMemoryRegions() just use that API as required. The only time we should see an error from Process::GetMemoryRegionInfo() is when it isn't implemented.


Ok, I’ve looked at the implementation on a couple of Mac and Linux cores, at the moment it will return an error if the function is unimplemented or you request a load_addr past the last mapped memory range. After the change it will only return an error when unimplemented. As GetMemoryRegionInfo (on Mac and Linux cores) doesn't return an error if you ask for unmapped memory ranges in the middle of the address space this is probably more consistent as the MemoryRange will always be populated for any address that’s queried, there's nothing magic about addresses beyond the last mapped region it should behave the same for those. I’ll go through and check the behaviour elsewhere and update the patch. I will have to update the existing implementations to set the flag that says whether a memory region is mapped or not but that doesn't look complicated.

> > Is PAGEZERO actually included in Mac core dumps? I can see it as load command 0 when I run otool against an executable but when I look at a core dump from the same executable the first load command looks like load command 1 in the executable. There doesn’t seem to be a load command in the core for address 0. I might just be missing an flag on otool though.

> 

> 

> It might not be, but that brings up another thing I was thinking of is that we could start supplementing the Process::GetMemoryRegionInfo() info by also looking in any sections that were found in core file memory that might not be fully described by the core file itself. There is no requirements that if two sections are adjacent in a core file that they show up as different mapped regions, so you might find a mapped section of memory from 0x1000-0x8000 that is actually 0x1000-0x4000 for ".data" and 0x4000-0x8000 that is ".bss". The memory region was mapped read+write, but would it be nice to know about these different regions separately? We don't need to do this now, but this is one place where we could figure out that 0x0-0x100000000 is __PAGEZERO even though it isn't mapped into the current mach-o file's memory regions. Also there are different makers of core files: lldb, the kernel, possibly others. They don't always follow the same rules when making mach-o core files. So it is nice to be flexible.


Another possibility, instead of splitting regions, is providing a way to iterate over all the sections a region contains. I'm not sure which is better. Do you want me to add a "backed" flag, possibly just for cores, that identifies whether there is any data for this region actually in the core dump. (For example a core might be truncated because of a ulimit or have omitted regions because of /proc/[pid]/coredump_filter settings on Linux.)

In http://reviews.llvm.org/D21751#469903, @labath wrote:

> Please also add a test for this in `TestLinuxCore.py`.
>
> There shouldn't be a need to add a new core file, you can just add an additional check to the `do_test` method that verifies the right memory regions (which you can pass in the same way as the pid). You can pass in bogus (empty) values for the i386 and s390x test cases, as they are not working now anyway.


Ok, I'll add that into this patch unless you prefer test updates as separate commits.


http://reviews.llvm.org/D21751