This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
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