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

[Hans Wennborg]

On Tue, Sep 2, 2014 at 4:40 PM, Hans Wennborg <hans at chromium.org> wrote:
> 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?

Duncan had the idea of extracting the adjustment required for aligning
first, and then checking that size + adjustment fits in the slab,
which seems like an easier approach. The discussion is on the patch
thread: http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140901/233553.html