Page MenuHomePhabricator

Add MemoryRegionInfo to SB API
ClosedPublic

Authored by hhellyer on May 24 2016, 5:57 AM.

Details

Summary

This adds new SB API calls and classes to allow a user of the SB API to obtain a full list of memory regions accessible within the process. Adding this to the API makes it possible use the API for tasks like scanning memory for blocks allocated with a header and footer to track down memory leaks, otherwise just inspecting every address is impractical especially for 64 bit processes.

These changes only add the API itself and a base implementation of GetMemoryRegions() to lldb_private::Process::GetMemoryRegions.
I will submit separate patches to fill in lldb_private::Process::GetMemoryRegionInfoList and GetMemoryRegionInfo for individual platforms.

The original discussion about this is here:
http://lists.llvm.org/pipermail/lldb-dev/2016-May/010203.html

Event Timeline

hhellyer updated this revision to Diff 58225.May 24 2016, 5:57 AM
hhellyer retitled this revision from to Add MemoryRegionInfo to SB API.
hhellyer updated this object.
hhellyer added a reviewer: clayborg.
hhellyer added a subscriber: lldb-commits.
clayborg requested changes to this revision.May 24 2016, 9:52 AM
clayborg edited edge metadata.

Few minor changes, but looks great overall. Thanks for doing this.

include/lldb/Target/MemoryRegionInfo.h
18–19 ↗(On Diff #61253)

We don't need to inherit from std::enable_shared_from_this do we? Is there a future patch where you intend on using this? If not, lets leave it out for now and add it later if we need to.

source/API/SBProcess.cpp
1487 ↗(On Diff #61253)

Please name "region_info_sp". All variables that are shared pointers should have "_sp" suffixes.

This revision now requires changes to proceed.May 24 2016, 9:52 AM
hhellyer updated this revision to Diff 58771.May 27 2016, 5:21 AM
hhellyer edited edge metadata.

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.

clayborg requested changes to this revision.May 27 2016, 9:58 AM
clayborg edited edge metadata.

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. Then the constructors always create an object and the copy constructor and assignment operators would just copy their contents from one to another instead of sharing the data. If you have code like:

SBMemoryRegionInfo region;
if (process.GetMemoryRegionInfo(0x123, region).Success())
{
    SBMemoryRegionInfo region2(region);
    region2.SetSomething(xxxxx);
}

Would you expect that the variable "region" now has the "SetSomething(xxxxx)" applied to it? I would expect not.

So I propose the following changes:

  • make both SBMemoryRegionInfo and SBMemoryRegionInfoList contain std::unique_ptr objects
  • have their constructors always create a default object
  • change copy constructors to copy the objects instead of sharing a reference
  • change assignment operators to copy the objects instead of sharing a reference
This revision now requires changes to proceed.May 27 2016, 9:58 AM
hhellyer marked 2 inline comments as done.May 31 2016, 5:20 AM

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.

I’m not sure if there’s ever going to be a need for any setter methods on the SBMemoryRegionInfo classes. Can you suggest any use cases? In my mind they just reflect the state of the memory in the process, I'm not sure if they allow that state to be changed.

For example a user wouldn’t ever directly create a new memory region by creating an SBMemoryRegionInfo. The process would allocate memory, memory map a file or load a module and a new one (might) be created. Other functions in lldb might cause the process to allocate or free memory and indirectly create or extend a memory region. Similarly changing a regions properties (e.g. making a region read only or changing it’s size) doesn’t necessarily make sense as those changes won't be reflected back in the process.

If SBMemoryRegion had setters, what the desired behaviour would be? The original purpose of SBMemoryInfoList to take a snap shot of memory regions (instead of using GetNumRegions and GetRegionAtIndex which would give unreliable results if the process was allowed to continue) so I'm not sure how it would all fit together if memory regions could be created or updated especially if the process continued.

I guess the other case is setting something on the memory region that changes the behaviour of lldb with respect to it or using an SBMemoryRegionInfo as an argument to another method. Then you might want to create a custom region and there may be some of those that make more sense.

So I propose the following changes:

  • make both SBMemoryRegionInfo and SBMemoryRegionInfoList contain std::unique_ptr objects
  • have their constructors always create a default object
  • change copy constructors to copy the objects instead of sharing a reference
  • change assignment operators to copy the objects instead of sharing a reference

I'll make the changes and upload a new patch, it certainly doesn't hurt to enable it as a possibility, I'm just not sure what the behaviour would be with modifiable memory regions. I can always go back to the shared pointer version. (Sorry for the delay in replying, it was a bank holiday here yesterday.)

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.

I’m not sure if there’s ever going to be a need for any setter methods on the SBMemoryRegionInfo classes. Can you suggest any use cases? In my mind they just reflect the state of the memory in the process, I'm not sure if they allow that state to be changed.

For example a user wouldn’t ever directly create a new memory region by creating an SBMemoryRegionInfo. The process would allocate memory, memory map a file or load a module and a new one (might) be created. Other functions in lldb might cause the process to allocate or free memory and indirectly create or extend a memory region. Similarly changing a regions properties (e.g. making a region read only or changing it’s size) doesn’t necessarily make sense as those changes won't be reflected back in the process.

If SBMemoryRegion had setters, what the desired behaviour would be? The original purpose of SBMemoryInfoList to take a snap shot of memory regions (instead of using GetNumRegions and GetRegionAtIndex which would give unreliable results if the process was allowed to continue) so I'm not sure how it would all fit together if memory regions could be created or updated especially if the process continued.

It would be if we expose allocating/deallocating memory regions. User would create a memory region, set read/write/execute and a size, then call:

SBError SBProcess::AllocateMemory(SBMemoryRegionInfo &region);

I guess the other case is setting something on the memory region that changes the behaviour of lldb with respect to it or using an SBMemoryRegionInfo as an argument to another method. Then you might want to create a custom region and there may be some of those that make more sense.

Yes, it is in case we enable calls that would require users to fill out memory regions. If we somehow exposed mmap through the public API, you can also request a certain address for the allocation in some calls.

So I propose the following changes:

  • make both SBMemoryRegionInfo and SBMemoryRegionInfoList contain std::unique_ptr objects
  • have their constructors always create a default object
  • change copy constructors to copy the objects instead of sharing a reference
  • change assignment operators to copy the objects instead of sharing a reference

I'll make the changes and upload a new patch, it certainly doesn't hurt to enable it as a possibility, I'm just not sure what the behaviour would be with modifiable memory regions. I can always go back to the shared pointer version. (Sorry for the delay in replying, it was a bank holiday here yesterday.)

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.

hhellyer updated this revision to Diff 59205.Jun 1 2016, 5:29 AM
hhellyer edited edge metadata.

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

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.

That's reasonable, the objects themselves aren't so large or numerous that not sharing them will make much difference even if we don't ever need to modify them.
SBMemoryRegionInfoList was already using a unique_ptr to hold a reference to its impl object so that didn't need updating.

clayborg requested changes to this revision.Jun 1 2016, 9:53 AM
clayborg edited edge metadata.

One last thing: we should default construct a MemoryRegionInfo in all SBMemoryRegionInfo::SBMemoryRegionInfo() constructors so we don't have a make sure to call a non-const ref() method before it the pointer is valid. I know there is other code that does this, but any new code we have been updating to always default constructing an object into the std::unique_ptr just to be safe and avoid potential crashes.

This revision now requires changes to proceed.Jun 1 2016, 9:53 AM
hhellyer updated this revision to Diff 59352.Jun 2 2016, 2:38 AM
hhellyer edited edge metadata.

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

clayborg requested changes to this revision.Jun 2 2016, 9:52 AM
clayborg edited edge metadata.

So a bit more cleanup. We should always have a valid object inside of a SBMemoryRegionInfo or SBMemoryRegionInfoList so there is no need to ever check the m_opaque_ap to see if it contains a valid pointer so all if statements that are doing:

if (m_opaque_ap.get())
    m_opaque_ap->DoSomething();

Can be just:

m_opaque_ap->DoSomething();

Also, no functions should ever call "m_opaque_ap->reset();". All the changes should be easy after that.

source/API/SBMemoryRegionInfo.cpp
46–49 ↗(On Diff #61253)

These 4 lines should just be:

ref() = rhs.ref();

We never want m_opaque_ap to contain an invalid pointer or we can crash.

61 ↗(On Diff #61253)

We shouldn't check for NULL since the auto pointer should never be invalid. This should just call through and ask the underlying object:

return m_opaque_ap->IsValid();

If there is no IsValid() function on the underlying object, this function can be removed.

67 ↗(On Diff #61253)

We should never clear the underlying object, this should just call through:

m_opaque_ap->Clear();
73–75 ↗(On Diff #61253)

This should compare the objects:

return ref() == rhs.ref();
81–83 ↗(On Diff #61253)

This should compare the objects:

return ref() != rhs.ref();
89–90 ↗(On Diff #61253)

You should remove these lines now since m_opaque_ap will never contain an invalid object.

102–105 ↗(On Diff #61253)

No need to check the object

111–114 ↗(On Diff #61253)

No need to check the object, just call the accessor

source/API/SBMemoryRegionInfoList.cpp
109 ↗(On Diff #61253)

No need to check the object, just call the accessor. Remove the if and the other return.

137 ↗(On Diff #61253)

No need to check the object, just call the accessor. Remove the if statement.

144 ↗(On Diff #61253)

No need to check m_opaque_ap since it will always be valid.

151 ↗(On Diff #61253)

No need to check m_opaque_ap since it will always be valid.

158 ↗(On Diff #61253)

No need for an IsValid() since the list is always valid, this function can actually be removed.

This revision now requires changes to proceed.Jun 2 2016, 9:52 AM
hhellyer updated this revision to Diff 59524.Jun 3 2016, 3:32 AM
hhellyer edited edge metadata.
hhellyer marked an inline comment as done.

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

The change to the == operator on SBMemoryRegionInfo required an additional change. It also required adding the == and != operators to MemoryRegionInfo.h, that shouldn't be a problem as far as behaviour goes as nothing can have been comparing MemoryRegionInfo's with == until now. It does mean that SBMemoryRegionInfo's == operator behaves differently to all the other SB* classes operators which do pointer comparison via get() which might not be ideal.

hhellyer updated this revision to Diff 60517.Jun 13 2016, 7:08 AM
hhellyer edited edge metadata.

Updated to rebase on the latest changes to MemoryRegionInfo.h

clayborg accepted this revision.Jun 13 2016, 1:27 PM
clayborg edited edge metadata.
This revision is now accepted and ready to land.Jun 13 2016, 1:27 PM
hhellyer added a comment.EditedJun 14 2016, 6:15 AM

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.

(Or is there a "Land this" button on phabricator I'm missing?)

hhellyer marked 12 inline comments as done.Jun 20 2016, 7:24 AM
hhellyer updated this revision to Diff 61253.Jun 20 2016, 7:59 AM
hhellyer edited edge metadata.

Updating to ensure I have the latest changes.

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?

hhellyer closed this revision.Jun 23 2016, 1:42 AM
This revision was automatically updated to reflect the committed changes.