cleaner & greener (was Re: A patch for RTEMS4.10.0 PowerPC heap space initialization)

Wed May 11 15:03:32 UTC 2011

On 05/11/2011 09:40 AM, Kate Feng wrote:
> 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.
>
This seems inordinately complicated and is breaking a couple
of patterns.

+ bsp.h is supposed to define CONFIGURE_MALLOC_BSP_SUPPORTS_SBRK
and NOT set the sbrk helper pointer in code.

+ I agree that we shouldn't pass 0 all the time to bsp_libc_init() as the
sbrk amount.  This is clearly an oversight.  If you added another macro
required to be set by the bsp.h when it defines
CONFIGURE_MALLOC_BSP_SUPPORTS_SBRK, then it could be the name
of a variable or constant to pass to bsp_libc_init() as the sbrk() amount.

If the above is addressed, then I fail to see why this call in
RTEMS_Malloc_Initialize() isn't sufficient to let the BSP's
sbrk() work:

   if ( rtems_malloc_sbrk_helpers != NULL ) {
     heap_begin = (*rtems_malloc_sbrk_helpers->initialize)(
       heap_begin,
       sbrk_amount
     );
     heap_size  = (uintptr_t) sbrk_amount;
   }

The problem is that the sbrk amount is never flowed into the
code flow at the top and you are going to great pains to inject
it later.

If the sbrk() plugin flow is wrong, let's fix that not work around it.

--joel

> 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

-- 
Joel Sherrill, Ph.D.             Director of Research&  Development
joel.sherrill at OARcorp.com        On-Line Applications Research
Ask me about RTEMS: a free RTOS  Huntsville AL 35805
    Support Available             (256) 722-9985