cleaner & greener (was Re: A patch for RTEMS4.10.0 PowerPC heap space initialization)
Kate Feng
feng1 at bnl.gov
Wed May 11 14:40:00 UTC 2011
Can you or anyone see if the attached patch is acceptable, please ?
It does not propagate information via global variables and
there is no need to add an argument in any global function, either.
My understanding is that only PPC port needs the
rtems_malloc_sbrk_helpers.
I am thinking to submit a PR if there is no further question, feedback,
or suggestion after 24 hours.
Thanks,
Kate
Till Straumann wrote:
> I agree with Kate that ppc/sbrk is currently broken in 4.10
> (bsp_libc_init() is always called with a zero sbrk_amount).
>
> I also suggest that this should be fixed but I'm not sure
> the proposed approach (propagating information via global
> variables) is the best solution.
>
> It may be cleaner to add an additional argument to
> bsp_get_work_area() and bootcard_bsp_libc_helper()
> to propagate the sbrk_amount.
>
> -- Till
>
>
> On 05/10/2011 05:40 PM, Kate Feng wrote:
>> Sebastian Huber wrote:
>>> Hello,
>>>
>>> I have some questions about this patch.
>> Can you or anyone see if the attached new patch acceptable, please ?
>> I am thinking to submit a PR by tomorrow afternoon if there is
>> no further question, feedback or suggestion. It seems a critical bug.
>>
>> Cheers,
>> Kate
>>> On 05/07/2011 02:44 PM, Kate Feng wrote:
>>>> Hi Joel,
>>>>
>>>> There are bugs in RTEMS4.10.0 and its CVS for the PowerPC heap space
>>>> allocation. All PPC boards are limited to 32M bytes of memory space
>>>> due
>>>> to the change in RTEMS4.10 for its work space. The attached patch
>>>> fixed
>>>> this.
>>>> I consider that it is critical. Thus, I will submit a PR.
>>>>
>>>> Cheers,
>>>> Kate Feng
>>>>
>>>>
>>>> rtems-4.10.0-powerpc-heapspace.diff
>>>>
>>>>
>>>> diff -Naur rtems-4.10.0.orig/c/src/lib/libbsp/powerpc/ChangeLog
>>>> rtems-4.10.0/c/src/lib/libbsp/powerpc/ChangeLog
>>>> --- rtems-4.10.0.orig/c/src/lib/libbsp/powerpc/ChangeLog 2011-03-04
>>>> 11:56:23.000000000 -0500
>>>> +++ rtems-4.10.0/c/src/lib/libbsp/powerpc/ChangeLog 2011-05-07
>>>> 08:15:17.000000000 -0400
>>>> @@ -1,3 +1,8 @@
>>>> +2011-05-07 Kate Feng <feng at bnl.gov>
>>>> +
>>>> + * shared/startup/bspgetworkarea.c : Moved _bsp_sbrk_init to +
>>>> c/src/lib/libbsp/shared/bootcard.c
>>>> +
>>>> 2011-03-04 Joel Sherrill <joel.sherrilL at OARcorp.com>
>>>>
>>>> * shared/start/start.S: Remove conflict markers in comment.
>>>> diff -Naur
>>>> rtems-4.10.0.orig/c/src/lib/libbsp/powerpc/shared/startup/bspgetworkarea.c
>>>>
>>>> rtems-4.10.0/c/src/lib/libbsp/powerpc/shared/startup/bspgetworkarea.c
>>>> ---
>>>> rtems-4.10.0.orig/c/src/lib/libbsp/powerpc/shared/startup/bspgetworkarea.c
>>>>
>>>> 2009-11-29 23:29:47.000000000 -0500
>>>> +++
>>>> rtems-4.10.0/c/src/lib/libbsp/powerpc/shared/startup/bspgetworkarea.c
>>>> 2011-05-07 08:21:08.000000000 -0400
>>>> @@ -31,15 +31,12 @@
>>>> )
>>>> {
>>>> uintptr_t work_size;
>>>> - uintptr_t spared;
>>>> uintptr_t work_area;
>>>>
>>>> work_area = (uintptr_t)&__rtems_end +
>>>> rtems_configuration_get_interrupt_stack_size();
>>>> work_size = (uintptr_t)BSP_mem_size - work_area;
>>>>
>>>> - spared = _bsp_sbrk_init( work_area, &work_size );
>>>> -
>>>> *work_area_start = (void *)work_area,
>>>> *work_area_size = work_size;
>>>> *heap_start = BSP_BOOTCARD_HEAP_USES_WORK_AREA;
>>>> @@ -54,7 +51,7 @@
>>>> void *sp = __builtin_frame_address(0);
>>>> void *end = *work_area_start + *work_area_size;
>>>> printk(
>>>> - "work_area_start = 0x%p\n"
>>>> + "work_area_start = 0x%08\n"
>>>
>>> What about 64-bit architectures?
>>>
>>>> "work_area_size = %d 0x%08x\n"
>>>> "end = 0x%p\n"
>>>> "heap_start = 0x%p\n"
>>>> diff -Naur rtems-4.10.0.orig/c/src/lib/libbsp/shared/bootcard.c
>>>> rtems-4.10.0/c/src/lib/libbsp/shared/bootcard.c
>>>> --- rtems-4.10.0.orig/c/src/lib/libbsp/shared/bootcard.c 2009-10-12
>>>> 16:22:18.000000000 -0400
>>>> +++ rtems-4.10.0/c/src/lib/libbsp/shared/bootcard.c 2011-05-07
>>>> 07:58:07.000000000 -0400
>>>> @@ -77,8 +77,10 @@
>>>> uintptr_t heap_size
>>>> )
>>>> {
>>>> - if ( !rtems_unified_work_area &&
>>>> - heap_start == BSP_BOOTCARD_HEAP_USES_WORK_AREA) {
>>>> + uintptr_t spared;
>>>> +
>>>> + if ( (!rtems_unified_work_area) &&
>>>> + (heap_start == BSP_BOOTCARD_HEAP_USES_WORK_AREA)) {
>>>
>>> Why the extra parentheses?
>>>
>>>> uintptr_t work_space_size = rtems_configuration_get_work_space_size();
>>>>
>>>> heap_start = (char *) work_area_start + work_space_size;
>>>> @@ -89,8 +91,13 @@
>>>> heap_size = heap_size_default;
>>>> }
>>>> }
>>>> + else /* Kate Feng: if (rtems_unified_work_area) */ {kkk
>>>
>>> What is the purpose of this comment?
>>>
>>>> + heap_start = (char *) work_area_start ;
>>>
>>> What is the purpose of this cast? Why the extra ' ' before the ';'?
>>>
>>>> + heap_size = work_area_size;
>>>> + }
>>>> + spared = _bsp_sbrk_init( (uintptr_t) heap_start , &heap_size );
>>>
>>> Here we force every BSP to provide this sbrk() mechanic. I don't think
>>> this is
>>> good. This _bsp_sbrk_init() only exists for the PowerPC at the moment.
>>>
>>>>
>>>> - bsp_libc_init(heap_start, heap_size, 0);
>>>> + bsp_libc_init((void *) heap_start, heap_size, spared);
>>>
>>> What is the purpose of this cast?
>>>
>>>> }
>>>>
>>>> /*
>>>
>>
>>
>>
>> _______________________________________________
>> rtems-users mailing list
>> rtems-users at rtems.org
>> http://www.rtems.org/mailman/listinfo/rtems-users
>
> _______________________________________________
> rtems-users mailing list
> rtems-users at rtems.org
> http://www.rtems.org/mailman/listinfo/rtems-users
-------------- next part --------------
A non-text attachment was scrubbed...
Name: rtems-4.10.0-heapspace_05112011.diff
Type: text/x-patch
Size: 6347 bytes
Desc: not available
URL: <http://lists.rtems.org/pipermail/users/attachments/20110511/5191a939/attachment-0001.bin>
More information about the users
mailing list