Re: PING: PATCH: [BZ #14562] Properly handle fencepost with MALLOC_ALIGN_MASK

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Mon, Sep 24, 2012 at 7:29 AM, Carlos O'Donell
<carlos_odonell@mentor.com> wrote:
> On 9/20/2012 10:37 AM, Carlos O'Donell wrote:
>> On 9/20/2012 9:44 AM, H.J. Lu wrote:
>>> On Tue, Sep 18, 2012 at 12:02 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>> On Fri, Sep 14, 2012 at 4:42 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>> On Fri, Sep 14, 2012 at 3:05 PM, Roland McGrath <roland@hack.frob.com> wrote:
>>>>>> The change is probably fine but I really don't understand this code at all.
>>>>>> Carlos did the main review of the changes to which this is a follow-on.
>>>>>> So he is probably the best person to review this too.
>>>>>
>>>>> Hi Carlos,
>>>>>
>>>>> Can you take a look at
>>>>>
>>>>> http://sourceware.org/ml/libc-alpha/2012-09/msg00240.html
>>>>>
>>>>> Thanks.
>>>>>
>>>>> --
>>>>> H.J.
>>>>
>>>> PING.
>>>>
>>>
>>> PING
>>
>> I applaud your aggressive pinging :-)
>>
>> I'll review this before the end of the day.
>
> Review:
>
> * File copyright date is correct [OK]
> * Has a BZ, and BZ is in ChangeLog [OK]
> * Tested on 3 core targets [OK]
> * Review malloc/malloc.c (sysmalloc):
>   Code aligns old_top size at MALLOC_ALIGNMENT, then subtracts 2*SIZE_SZ,
>   but that's equivalent to MALOC_ALIGNMENT, so the fencepost stays
>   aligned. [OK]
> * Review malloc/arena.c (heap_trim):
>   Code computes new p from prev_p and an assumption about the size.
>   The size assumption doesn't take into account bytes from misalignment.
>   New patch adjusts p based on extra alignment bytes. [OK]
>
> Comments:
>
> Your change to malloc/arena.c (heap_trim) breaks the abstraction created
> by the macro chunk_at_offset.
>
> Is it possible to compute alignment bytes and feed that into the single
> macro call to chunk_at_offset instead of adjusting p directly?
>
> I'd be happier with that change.
>
> e.g.
>
> +    /* fencepost must be properly aligned.  */
> +    misalign = <Compute it>
> -    p = chunk_at_offset(prev_heap, prev_heap->size - (MINSIZE-2*SIZE_SZ));
> +    p = chunk_at_offset(prev_heap, prev_heap->size - (MINSIZE-2*SIZE_SZ+misalign));
>
> Does that make sense?
>

The patched code is

    p = chunk_at_offset(prev_heap, prev_heap->size - (MINSIZE-2*SIZE_SZ));
    /* fencepost must be properly aligned.  */
    misalign = ((long) p) & MALLOC_ALIGN_MASK;
    p = (mchunkptr)(((unsigned long) p) & ~MALLOC_ALIGN_MASK);

We use chunk_at_offse to compute "misalign",  We can do

    p = chunk_at_offset(prev_heap, prev_heap->size - (MINSIZE-2*SIZE_SZ));
    /* fencepost must be properly aligned.  */
    misalign = ((long) p) & MALLOC_ALIGN_MASK;
    p = chunk_at_offset(prev_heap, prev_heap->size -
(MINSIZE-2*SIZE_SZ+misalign));

But I think it is less clear than

   p = (mchunkptr)(((unsigned long) p) & ~MALLOC_ALIGN_MASK);

But I can go either way.  Please let me know which one you prefer.

Thanks.

-- 
H.J.