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

[Nuno Lopes]

>>> 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.

I see, it makes perfect sense to improve that comment to clarify what the 
function does.  Would you mind commiting your change, please?

Danke,
Nuno