[llvm] r216973 - BumpPtrAllocator: use uintptr_t when aligning addresses to avoid undefined behaviour

[Hans Wennborg]

On Tue, Sep 2, 2014 at 3:45 PM, David Majnemer <david.majnemer at gmail.com> wrote:
> On Tue, Sep 2, 2014 at 2:51 PM, Hans Wennborg <hans at hanshq.net> wrote:
>>
>> Author: hans
>> Date: Tue Sep  2 16:51:35 2014
>> New Revision: 216973
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=216973&view=rev
>> Log:
>> BumpPtrAllocator: use uintptr_t when aligning addresses to avoid undefined
>> behaviour
>>
>> In theory, alignPtr() could push a pointer beyond the end of the current
>> slab, making
>> comparisons with that pointer undefined behaviour. Use an integer type to
>> avoid this.
>>
>> Modified:
>>     llvm/trunk/include/llvm/Support/Allocator.h
>>     llvm/trunk/include/llvm/Support/MathExtras.h
>>     llvm/trunk/unittests/Support/AllocatorTest.cpp
>>
>> Modified: llvm/trunk/include/llvm/Support/Allocator.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/Allocator.h?rev=216973&r1=216972&r2=216973&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/include/llvm/Support/Allocator.h (original)
>> +++ llvm/trunk/include/llvm/Support/Allocator.h Tue Sep  2 16:51:35 2014
>> @@ -210,16 +210,17 @@ public:
>>      BytesAllocated += Size;
>>
>>      // Allocate the aligned space, going forwards from CurPtr.
>> -    char *Ptr = alignPtr(CurPtr, Alignment);
>> +    uintptr_t AlignedAddr = alignAddr(CurPtr, Alignment);
>>
>>      // Check if we can hold it.
>> -    if (Ptr + Size <= End) {
>> -      CurPtr = Ptr + Size;
>> +    if (AlignedAddr + Size <= (uintptr_t)End) {
>
>
> I would somehow assert that the alignment doesn't wrap End such that it is
> smaller than CurPtr.

Right, alignPtr should never overflow. I'll add an assert there.

> I would also change this to be:
> if ((uintptr_t)End - AlignedAddr >= Size)

That relies on End being greater than AlignedAddr though, which isn't
guaranteed. I'll add a test that exercises this.

> Consider AlignedAddr + Size s.t. it wraps *past* End and results in being
> really small.

Right, AlignedAddr + Size could overflow uintptr_t, and then my code
breaks. How about we take your code, but do a signed comparison,
something like:

if ((ptrdiff_t)((uintptr_t)End - AlignedAddr) >= (ptrdiff_t)Size)) {

Hrm, I don't like how complicated this becomes :/ Maybe we could just
assert that AlignedAddr + Size doesn't overflow?