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

[Carlos O'Donell <carlos_odonell at mentor dot com>]

On 9/24/2012 11:04 AM, H.J. Lu wrote:
>> 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.

Does the alignment always have to be taken into account when looking
at the return of chunck_at_offset? If yes, then why doesn't he macro
itself change; is that too intrusive a change to validate easily?

Either way, one option I would suggest:

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

Or wrap this into a new macro that takes MALLOC_ALIGNMENT into account:

     p = fencepost_at_offset (prev_heap, prev_heap->size - (MINSIZE-2*SIZE_SZ));

Use it when working with fencposts.

I'm happy with either.

Cheers,
Carlos.
-- 
Carlos O'Donell
Mentor Graphics / CodeSourcery
carlos_odonell@mentor.com
carlos@codesourcery.com
+1 (613) 963 1026