[llvm] r217708 - llvm-profdata: Avoid undefined behaviour when reading raw profiles

[Justin Bogner]

David Blaikie <dblaikie at gmail.com> writes:
>>>>> Any chance of changing this to not rely on UB (this code looks
>>>>> like it's violating C++ aliasing rules pretty consistently) and
>>>>> instead using memcpy?
>>>>
>>>> It's always casting from char *, which IIUC doesn't break aliasing
>>>> rules, so I *think* that this isn't relying on UB.
>>>>
>>> [+Richard for some language lawyering]
>>>
>>> I believe the rules are:
>>>
>>> You can take a pointer to any entity, cast it to a char* then back,
>>> and continue using it as the original type. But this only works if
>>> the pointer was originally a pointer to that type you're
>>> dereferencing by, which isn't the case here.
>>>
>>> You can take a pointer to T (where T is a standard layout(?)), cast
>>> to pointer to char, read the bytes of the object then write them
>>> back into some other object (via a char* that actually points to a
>>> T - derived the same way as the original) you can do that safely.
>>>
>>> But none of this allows taking a char* that wasn't derived from a
>>> T*, casting it to a T* and reading.
>>>
>>> But this is just my high-level understanding, I don't actually know
>>> where the words are in the standard to support this position.
>>
>> C++11 has them in the bullet point list in section 3.10.
>
> Ah, thanks 3.10p10 specifically. Huh, didn't know you could read an unsigned
> int as an int, for example... that's a bit weird, but OK.
>
>> Sorry about this, looks like I missed that subtlety :/.
>>    
>>> Hoping Richard can fill us in/confirm/deny and/or I can go hunting.
>>>
>>> - David
>>>
>>>
>>>> Assuming I'm right about that, the current design avoids unnecessarily
>>>> copying from the file to an auxiliary structure just to read the data
>>>> and throw it away.
>> 
>> I think the optimizer will see through the `memcpy`s anyway, so this
>> probably isn't a big deal.
>
> That's certainly the goal - file bugs whenever this doesn't work. 

Well then, I guess I should fix this to use memcpy. I've filed a bug so
I don't forget. http://llvm.org/bugs/show_bug.cgi?id=21049

Thanks for looking that up Duncan.