[llvm] r216973 - BumpPtrAllocator: use uintptr_t when aligning addresses to avoid undefined behaviour
David Majnemer
david.majnemer at gmail.com
Tue Sep 2 15:45:35 PDT 2014
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.
I would also change this to be:
if ((uintptr_t)End - AlignedAddr >= Size)
Consider AlignedAddr + Size s.t. it wraps *past* End and results in being
really small.
> + char *AlignedPtr = (char*)AlignedAddr;
> + CurPtr = AlignedPtr + Size;
> // Update the allocation point of this memory block in
> MemorySanitizer.
> // Without this, MemorySanitizer messages for values originated
> from here
> // will point to the allocation of the entire slab.
> - __msan_allocated_memory(Ptr, Size);
> - return Ptr;
> + __msan_allocated_memory(AlignedPtr, Size);
> + return AlignedPtr;
> }
>
> // If Size is really big, allocate a separate slab for it.
> @@ -228,19 +229,22 @@ public:
> void *NewSlab = Allocator.Allocate(PaddedSize, 0);
> CustomSizedSlabs.push_back(std::make_pair(NewSlab, PaddedSize));
>
> - Ptr = alignPtr((char *)NewSlab, Alignment);
> - assert((uintptr_t)Ptr + Size <= (uintptr_t)NewSlab + PaddedSize);
> - __msan_allocated_memory(Ptr, Size);
> - return Ptr;
> + uintptr_t AlignedAddr = alignAddr(NewSlab, Alignment);
> + assert(AlignedAddr + Size <= (uintptr_t)NewSlab + PaddedSize);
> + char *AlignedPtr = (char*)AlignedAddr;
> + __msan_allocated_memory(AlignedPtr, Size);
> + return AlignedPtr;
> }
>
> // Otherwise, start a new slab and try again.
> StartNewSlab();
> - Ptr = alignPtr(CurPtr, Alignment);
> - CurPtr = Ptr + Size;
> - assert(CurPtr <= End && "Unable to allocate memory!");
> - __msan_allocated_memory(Ptr, Size);
> - return Ptr;
> + AlignedAddr = alignAddr(CurPtr, Alignment);
> + assert(AlignedAddr + Size <= (uintptr_t)End &&
> + "Unable to allocate memory!");
> + char *AlignedPtr = (char*)AlignedAddr;
> + CurPtr = AlignedPtr + Size;
> + __msan_allocated_memory(AlignedPtr, Size);
> + return AlignedPtr;
> }
>
> // Pull in base class overloads.
> @@ -373,7 +377,7 @@ public:
> /// all memory allocated so far.
> void DestroyAll() {
> auto DestroyElements = [](char *Begin, char *End) {
> - assert(Begin == alignPtr(Begin, alignOf<T>()));
> + assert(Begin == (char*)alignAddr(Begin, alignOf<T>()));
> for (char *Ptr = Begin; Ptr + sizeof(T) <= End; Ptr += sizeof(T))
> reinterpret_cast<T *>(Ptr)->~T();
> };
> @@ -382,7 +386,7 @@ public:
> ++I) {
> size_t AllocatedSlabSize = BumpPtrAllocator::computeSlabSize(
> std::distance(Allocator.Slabs.begin(), I));
> - char *Begin = alignPtr((char *)*I, alignOf<T>());
> + char *Begin = (char*)alignAddr(*I, alignOf<T>());
> char *End = *I == Allocator.Slabs.back() ? Allocator.CurPtr
> : (char *)*I +
> AllocatedSlabSize;
>
> @@ -392,7 +396,7 @@ public:
> for (auto &PtrAndSize : Allocator.CustomSizedSlabs) {
> void *Ptr = PtrAndSize.first;
> size_t Size = PtrAndSize.second;
> - DestroyElements(alignPtr((char *)Ptr, alignOf<T>()), (char *)Ptr +
> Size);
> + DestroyElements((char*)alignAddr(Ptr, alignOf<T>()), (char *)Ptr +
> Size);
> }
>
> Allocator.Reset();
>
> Modified: llvm/trunk/include/llvm/Support/MathExtras.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/MathExtras.h?rev=216973&r1=216972&r2=216973&view=diff
>
> ==============================================================================
> --- llvm/trunk/include/llvm/Support/MathExtras.h (original)
> +++ llvm/trunk/include/llvm/Support/MathExtras.h Tue Sep 2 16:51:35 2014
> @@ -550,16 +550,15 @@ inline uint64_t MinAlign(uint64_t A, uin
> return (A | B) & (1 + ~(A | B));
> }
>
> -/// \brief Aligns \c Ptr to \c Alignment bytes, rounding up.
> +/// \brief Aligns \c Addr to \c Alignment bytes, rounding up.
> ///
> /// Alignment should be a power of two. This method rounds up, so
> -/// AlignPtr(7, 4) == 8 and AlignPtr(8, 4) == 8.
> -inline char *alignPtr(char *Ptr, size_t Alignment) {
> +/// alignAddr(7, 4) == 8 and alignAddr(8, 4) == 8.
> +inline uintptr_t alignAddr(void *Addr, size_t Alignment) {
> assert(Alignment && isPowerOf2_64((uint64_t)Alignment) &&
> "Alignment is not a power of two!");
>
> - return (char *)(((uintptr_t)Ptr + Alignment - 1) &
> - ~(uintptr_t)(Alignment - 1));
> + return (((uintptr_t)Addr + Alignment - 1) & ~(uintptr_t)(Alignment -
> 1));
> }
>
> /// NextPowerOf2 - Returns the next power of two (in 64-bits)
>
> Modified: llvm/trunk/unittests/Support/AllocatorTest.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/AllocatorTest.cpp?rev=216973&r1=216972&r2=216973&view=diff
>
> ==============================================================================
> --- llvm/trunk/unittests/Support/AllocatorTest.cpp (original)
> +++ llvm/trunk/unittests/Support/AllocatorTest.cpp Tue Sep 2 16:51:35 2014
> @@ -130,7 +130,7 @@ public:
> void *MemBase = malloc(Size + Alignment - 1 + sizeof(void*));
>
> // Find the slab start.
> - void *Slab = alignPtr((char *)MemBase + sizeof(void *), Alignment);
> + void *Slab = (void *)alignAddr((char*)MemBase + sizeof(void *),
> Alignment);
>
> // Hold a pointer to the base so we can free the whole malloced block.
> ((void**)Slab)[-1] = MemBase;
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140902/a8c58a69/attachment.html>
More information about the llvm-commits
mailing list