I spotted this when looking at why https://reviews.llvm.org/D26676 didn't quite fix reading the z/Linux core dump. Do I need to raise a bug to track this as well or is this sufficient?

@labath - Yes sorry about that. To be safe I did the "honest" thing of using arc to pull down the patch from phabriactor and then arc commit to land it to make sure I committed exactly what was reviewed. Somewhere in that it lost the contents of the core files and just created empty files for them. :-(

I'll land these changes tomorrow morning (UK time) so I can watch the builds and check there's no problems with the tests.

Updating patch to remove executables and create empty targets in the test cases.

@labath If you are happy with the tests and the size of their supporting files I'll land this, otherwise let me know if anything needs changing.

Updated to add the test executables and core files.

Updating the patch to include the test cases and scripts to produce the core dumps.
I haven't included the core dumps and when I do I'll need to update the pid and
tid values in the test scripts. That can wait until we've decided what to do about
the core dumps though.

I'd assumed that gcore would ignore the coredump_filter setting but it turns out it doesn't so I should be able to use that. lldb seems happy enough opening the core files produced with a filter of 0 so it's probably the simplest solution.

I haven't solved that yet! ;-)

This is marked ready to land and I can land it now but I'm still working on the testcases.
I can either:

Update patch due to two missing changes.

Update with fixes from review comments.
I've run clang-format, fixed the use of iterators and added const qualifiers.
I removed the use of the SIGSTOP constant and use GetUnixSignals instead.
I've also moved that block down a few lines until after SetUnixSignals has been called.

The change seems unlikely to pick up many false positives and core dumps from the type of system described in the bug are only going to get more common.

Looks good, thanks.

.arcconfig has been updated.

Arcanist won't upload the patch without the change to .arcconfig yet because of http://lists.llvm.org/pipermail/llvm-dev/2016-July/102305.html but I think this will be patched officially shortly! It should disappear if I update the revision for any reason once that's happened.

Updated patch to export IsMapped in the C++ and Python APIs.
Updated the test case to verify all the regions returned in the list are mapped.

I’ve added a test in TestLinuxCore.py. It found a bug which the change in RangeMap.h resolves.

In D21751#468976, @clayborg wrote:

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

Since there’s a list of memory regions already created in the Process implementations for the core fles I thought it made sense just to use that directly, at least when reading from a core file. It meant there was no requirement to change those GetMemoryRegionInfo implementations if they didn’t behave quite the same. That said it is roughly what I planned to do for the live connections as the behaviour of the qMemoryRegionInfo query is more tightly specified and moving to that model would just push that implementation one layer higher.

Squashing local commits.

I re-updated the patch via arc diff (rather than a manual patch upload). Phabricator now seems to know which revision my changes are based off and has added the "Next Step: arc commit" status. Do I need to request an svn password to run arc commit or can you (or someone) else land this for me?

Updating to ensure I have the latest changes.

Should I be able to deliver these changes now? When I try following the
instructions here: http://llvm.org/docs/Phabricator.html the patch
downloads and applies correctly but whether I use arc via the git or svn
commit methods I'm ultimately prompted for a password for subversion. I'm
not sure if I should have a password or if that means the patch doesn't
match what's expected in some way. I don't know how clever phabricator's
svn commit hooks are and whether they check against the patch before
prompting for authentication.

Updated to rebase on the latest changes to MemoryRegionInfo.h

Fix the latest code review comments to remove unnecessary tests for a valid pointer that will always be initialised.

Made sure an object was always constructed for the std::unique_ptr in SBMemoryRegionInfo.cpp.
Removed unused constructor from SBMemoryRegionInfo.h.

In D20565#444507, @clayborg wrote:

My main reason for making sure we use std::unique_ptr is for API compatibility in the future. The size of std::unique_ptr and std::shared_ptr differs and if anyone wrote an IDE or tool that links against the LLDB public API, then we can't switch the layout of a class without breaking our ABI. So it is safest to go with a std::unique_ptr since the backing class is so simple and isn't expensive to copy.

I've switch to a unique_ptr from a shared_ptr in SBMemoryRegionInfo.

In D20565#442373, @clayborg wrote:

This looks good as long as SBMemoryRegionInfo and SBMemoryRegionInfoList are ready only and will never have any setter functions. If we plan to ever have the SBMemoryRegionInfo or SBMemoryRegionInfoList be modifiable, then we should make each contain a std::unique_ptr instead of a std::shared_ptr.

Removed std::enable_shared_from_this from MemoryRegionInfo.h (which removes the only change from that file)
Renamed region_info to region_info_sp in SBProcess::GetMemoryRegionInfo in SBProcess.cpp
I also corrected the position of the opening { on the functions in that file to match the standard.