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/24/2012 10:29 AM, Carlos O'Donell 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]
Note that this actually doesn't matter. I misread the code there, the 2*SIZE_SZ
is consistently added and subtracted to get at the right pointer. And on x32
2*SIZE_SZ != MALLOC_ALIGNMENT anyway.
> * 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.
>