This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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?

OK with that change.

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]