[llvm-commits] [llvm] r77023 - /llvm/trunk/include/llvm/Support/MemoryObject.h

[Sean Callanan]

Wow, that logic error escaped me completely (that was essentially  
untested code because the subclasses always extended it).
Anyway, I'll fix that and the other problems you saw.  Thanks, Daniel!

Sean

On 2009/07/24, at 21:59, Daniel Dunbar wrote:

> Hi Sean,
>
> Cool! A few minor comments:
>
> On Fri, Jul 24, 2009 at 5:30 PM, Sean Callanan<scallanan at apple.com>  
> wrote:
>> +/// MemoryObject - Abstract base class for contiguous addressable  
>> memory.
>> +///   Necessary for cases in which the memory is in another  
>> process, in a
>> +///   file, or on a remote machine.
>> +class MemoryObject {
>> +public:
>> +  /// Constructor     - Override as necessary.
>> +  MemoryObject() {
>> +  }
>
> Since the class is abstract, we generally either make the constructor
> protected or just leave it off if it is unnecessary. A virtual
> destructor is needed, however.
>
>> +
>> +  /// getBase         - Returns the lowest valid address in the  
>> region.
>> +  ///
>> +  /// @result         - The lowest valid address.
>> +  virtual uint64_t getBase() const = 0;
>
> Should this be uintptr_t instead of uint64_t?
>
>> +  /// getExtent       - Returns the size of the region in bytes.   
>> (The region is
>> +  ///                   contiguous, so the highest valid address  
>> of the region
>> +  ///                   is getBase() + getExtent() - 1).
>> +  ///
>> +  /// @result         - The size of the region.
>> +  virtual uint64_t getExtent() const = 0;
>> +
>> +  /// readByte        - Tries to read a single byte from the region.
>> +  ///
>> +  /// @param address  - The address of the byte, in the same space  
>> as getBase().
>> +  /// @param ptr      - A pointer to a byte to be filled in.  Must  
>> be non-NULL.
>> +  /// @result         - 0 if successful; -1 if not.  Failure may  
>> be due to a
>> +  ///                   bounds violation or an implementation- 
>> specific error.
>> +  virtual int readByte(uint64_t address, uint8_t* ptr) const = 0;
>
> Is there a particular reason to provide this? It seems like clients
> could just pass size=1 if they only want a byte. And it seems like
> most implementations will want to override readBytes anyway?
>
>> +  /// readByte        - Tries to read a contiguous range of bytes  
>> from the
>> +  ///                   region, up to the end of the region.
>> +  ///                   You should override this function if there  
>> is a quicker
>> +  ///                   way than going back and forth with  
>> individual bytes.
>> +  ///
>> +  /// @param address  - The address of the first byte, in the same  
>> space as
>> +  ///                   getBase().
>> +  /// @param size     - The maximum number of bytes to copy.
>
> Is it true that address + size should never overflow?
>
>> +  /// @param buf      - A pointer to a buffer to be filled in.   
>> Must be non-NULL
>> +  ///                   and large enough to hold size bytes.
>> +  /// @result         - The number of bytes copied if successful;  
>> (uint64_t)-1
>> +  ///                   if not.
>> +  ///                   Failure may be due to a bounds violation  
>> or an
>> +  ///                   implementation-specific error.
>
> Is there a reason to return the number of bytes copied, it is always
> the size. Then it could just return true/false on success.
>
>> +  virtual uint64_t readBytes(uint64_t address,
>> +                             uint64_t size,
>> +                             uint8_t* buf) const {
>> +    uint64_t current = address;
>> +    uint64_t limit = getBase() + getExtent();
>> +    uint64_t ret = 0;
>> +
>> +    while(current - address < size && current < limit) {
>> +      if(readByte(current, &buf[(current - address)]))
>> +        return (uint64_t)-1;
>> +
>> +      ret++;
>> +    }
>> +
>> +    return ret;
>> +  }
>
> This isn't quite right (current never changes). If most
> implementations will override this it might be nicer to have this just
> be a pure abstract base class. But if not,
> --
> for (uint64_t i = 0; i != size; ++i)
>  if (readByte(address + i, buf + i))
>    return (uint64_t)-1;
> return size;
> --
> is simpler.
>
> - Daniel
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits