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/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.
> 


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