[llvm] r176408 - recommit r172363 & r171325 (reverted in r172756)

[Arnold Schwaighofer]

On Mar 4, 2013, at 12:33 PM, Nuno Lopes <nunoplopes at sapo.pt> wrote:

>> On Mar 4, 2013, at 11:27 AM, Nuno Lopes <nunoplopes at sapo.pt> wrote:
>> 
>>>>> Shuxin: I'm not breaking any assumption here.
>>>> If you don't think you break the assumption,  tell us your immediate reaction to
>>>> the class name "ObjectSizeOffsetVisitor".
>>>> 
>>>> It seem so odd to me that  xxx_ObjectWhatever() return a meaningful value however we have no idea which
>>>> object it is talking about.
>>>> 
>> 
>> Nuno,
>> 
>> I think, the issue is mostly one of naming things as precisely as possible or at least have the documentation describe it as accurate as possible.
>> 
>> If we call a function "getObjectSize" vs. "getPtrAdjustedSizeOfPointedToByObject(s)", this can make a big difference. I claim it could have prevented the basicaa bug we had: I think it was probably due to the misconception that getObjectSize will always return the size of the pointed to underlying object - as it turns out this was not the case  - or it has evolved over time not to be the case anymore. Either way, by naming things more precisely we can avoid future bugs.
> 
> Well, I see that the naming may not be the best, but your proposed name is way too big, IMHO :)   And "adjusted size" doesn't really mean much.
> LLVM is just following gcc's convention here (from __builtin_object_size).

Dan convinced me that we don't need to talk about "Objects" so I will retract this part. I was wrong here, that there is a need for distinction. Dynamically there will always be objects (whether there is a phi or not) but for one instance of the execution of an instruction you can talk about an object. I was looking at this the wrong way.

Given that you already created:
  getObjectSize
  getUnderlyingObjectSize

I am good and retract almost all of my reservations ;)

I must say however that in:

/// \brief Compute the size of the object pointed by Ptr. Returns true and the
/// object size in Size if successful, and false otherwise.
/// If RoundToAlign is true, then Size is rounded up to the aligment of allocas,
/// byval arguments, and global variables.
bool llvm::getObjectSize

It is not very clear what the "object pointed by Ptr" is and that the function could be misconstrued to have the same semantics of getUnderlyingObjectSize even after reading the doc string. You have to read its implementation to know what it is doing. Hence my proposal for "getPointerAdjustedObjectSize". I agree that "getPointerAdjusted…" might also not be very clear, maybe "getSizeFromPtrToEndOfObject". Or at least including it in the documentation string  (Personally, I have a slight preference for talking function names than doc strings).

/// \brief Compute the size of the object pointed by Ptr. Returns true and the
/// object size in Size if successful, and false otherwise. In this context by object we mean
/// the region of memory starting at Ptr to the end of the underlying object pointed to by Ptr.
/// If RoundToAlign is true, then Size is rounded up to the aligment of allocas,
/// byval arguments, and global variables.


Best,
Arnold