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 8:21 AM, Carlos O'Donell
<carlos_odonell@mentor.com> wrote:
> 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.
>

This is the patch I am checking in.   It is also needed for 2.16.
OK for 2.16 branch?

Thanks.

-- 
H.J.
---
2012-09-24  H.J. Lu  <hongjiu.lu@intel.com>

	[BZ #14562]
	* malloc/arena.c (heap_trim): Properly get fencepost and adjust
	new chunk size with MALLOC_ALIGN_MASK.

diff --git a/malloc/arena.c b/malloc/arena.c
index 97c0b90..f24e76c 100644
--- a/malloc/arena.c
+++ b/malloc/arena.c
@@ -655,15 +655,19 @@ heap_trim(heap_info *heap, size_t pad)
   unsigned long pagesz = GLRO(dl_pagesize);
   mchunkptr top_chunk = top(ar_ptr), p, bck, fwd;
   heap_info *prev_heap;
-  long new_size, top_size, extra;
+  long new_size, top_size, extra, prev_size, misalign;

   /* Can this heap go away completely? */
   while(top_chunk == chunk_at_offset(heap, sizeof(*heap))) {
     prev_heap = heap->prev;
-    p = chunk_at_offset(prev_heap, prev_heap->size - (MINSIZE-2*SIZE_SZ));
+    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);
     assert(p->size == (0|PREV_INUSE)); /* must be fencepost */
     p = prev_chunk(p);
-    new_size = chunksize(p) + (MINSIZE-2*SIZE_SZ);
+    new_size = chunksize(p) + (MINSIZE-2*SIZE_SZ) + misalign;
     assert(new_size>0 && new_size<(long)(2*MINSIZE));
     if(!prev_inuse(p))
       new_size += p->prev_size;