[Patch] Use uintptr_t instead of ptr comparisons in BumpPtrAllocator (Was: Re: [llvm] r215841 - BumpPtrAllocator: remove 'no slabs allocated yet' check)

[Hans Wennborg]

On Wed, Sep 3, 2014 at 9:47 AM, Chandler Carruth <chandlerc at google.com> wrote:
> As long as 'alignAddr' can move a pointer more than one past the end of the
> allocation, it doesn't really work IMO. But this is all such a technicality,
> I'm not sure how much continued hacking is worth here.

I'm not sure I'm following here. alignAddr doesn't work with a
pointer, it's just rounding up a uintptr_t. Are you concerned about
comparing that uintptr_t to another uintptr_t?

> You need to calculate both the alignment addition and the requested size,
> entirely as an integer delta without moving the pointer in order to avoid
> moving the pointer more thane one past the end I think...

I thought that's what my patch was doing?

> On Wed, Sep 3, 2014 at 9:36 AM, Hans Wennborg <hans at chromium.org> wrote:
>>
>> On Wed, Sep 3, 2014 at 7:20 AM, Duncan P. N. Exon Smith
>> <dexonsmith at apple.com> wrote:
>> >
>> >> On 2014 Sep 2, at 21:34, Chandler Carruth <chandlerc at google.com> wrote:
>> >>
>> >>> On Tue, Sep 2, 2014 at 10:44 AM, Hans Wennborg <hans at chromium.org>
>> >>> wrote:
>> >>> I'm attaching a patch that should make this UB-proof. Let me know what
>> >>> you think.
>> >>
>> >> The reason I didn't suggest this originally is that it doesn't actually
>> >> make anything UB-proof. You're still potentially walking a pointer far past
>> >> the end of the allocated array and comparing it with a pointer one past the
>> >> end of the array. You need to check every single time prior to adding that
>> >> there is space within the allocated array.
>> >
>> > I think the attached/committed patch is an incremental improvement.
>> > The pointer isn't walking anywhere; all addition and comparison is
>> > done as `uintptr_t`.  So it removed the "common case" reliance on UB.
>> >
>> >> As an example of how I think this could fail, imagine that your system
>> >> maps pointers to uintptr_t such that 'End' becomes
>> >> std::numeric_limits<uintptr_t>::max() and either the aligning or the size
>> >> addition wrap.
>> >
>> > Overflowing `uintptr_t` is the only UB left.  Is this a practical
>> > concern?  I assumed not, but maybe that was naive?
>> >
>> >> If you want to go this route, at least use intptr_t so we get a UBSan
>> >> failure if this state ever arises. Better would be to just reformulate the
>> >> entire thing in terms that are guaranteed to only use pointers within the
>> >> allocated array.
>> >
>> > It should be sufficient to check that the final `uintptr_t` is `>=`
>> > the unmodified `Ptr` before converting back to `char*`.
>> >
>> > Reformulating might be better anyway, though.  I wouldn't mind
>> > something like this:
>> >
>> >     uintptr_t Adjustment = getAlignmentAdjustment(Ptr, Alignment);
>> >     assert(Adjustment + Size >= Size && "overflow");
>> >     if (Adjustment + Size <= uintptr_t(End - Ptr)) {
>> >       Ptr += Adjustment;
>> >
>> >       // allocate in current slab.
>> >       return;
>> >     }
>> >     // allocate in new slab.
>>
>> I like this idea. How about the attached patch?