[Lldb-commits] [PATCH] D20565: Add MemoryRegionInfo to SB API

[Greg Clayton via lldb-commits]

clayborg added a comment.

In http://reviews.llvm.org/D20565#444223, @hhellyer wrote:

> In http://reviews.llvm.org/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.
>
>
> 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.


http://reviews.llvm.org/D20565