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

[Joel Sherrill]

On 05/13/2011 12:41 PM, Kate Feng wrote:
> I agree so.  More comments regarding the attached patch before the
> PR ?
>
This is very close for me.  I still believe the bsp.h has to define
CONFIGURE_MALLOC_BSP_SUPPORTS_SBRK.  The BSP is the
only place that knows for sure sbrk() can work.  The goal
is to avoid having ANY code related to sbrk() in a BSP which
doesn't support it.

If the bsp.h does what it is supposed to, then bootcard.c
can conditionally compile the code that calls bsp_sbrk_init().
This results in bsp_sbrk_init() only being required when the
BSP really supports it.   In addition, if the BSP doesn't support
sbrk(), then the current simple code can be used.

Either a BSP supports sbrk() or it doesn't.  We need to avoid
code bloat in BSPs.  It is a continuous battle.  sbrk() is an
optional feature and when not supported by a BSP, we should
use the simplest possible code.

I know that is hard to see when all you work on is what are
the largest RTEMS targets.  But there are plenty of smaller
ones and flight validated where extra code is undesirable.

This should be a minor change.  Mostly centered around bootcard.c
> Thanks,
> Kate
>
> Gedare Bloom wrote:
>> +static uintptr_t heap_sbrk_spared=0;
>> Why is this declared as static global? It looks to me like it is only
>> used in one function, can it go on that function's local variables?
>>
>> _bsp_sbrk_init()
>> Should be renamed as bsp_sbrk_init() to be consistent with other bsp functions.
>>
>> On Fri, May 13, 2011 at 11:08 AM, Kate Feng<feng1 at bnl.gov>  wrote:
>>
>>> Sebastian Huber wrote:
>>>
>>>> On 05/13/2011 04:13 PM, Kate Feng wrote:
>>>>
>>>>
>>>>> +++ rtems-4.10.0/c/src/lib/libbsp/shared/bootcard.c     2011-05-13
>>>>> 09:53:27.000000000 -0400
>>>>> @@ -53,6 +53,7 @@
>>>>>   #include<bsp/bootcard.h>
>>>>>   #include<rtems/bspIo.h>
>>>>> +#include<rtems/malloc.h>
>>>>>   /*
>>>>>   *  At most a single pointer to the cmdline for those target
>>>>> @@ -65,6 +66,9 @@
>>>>>   */
>>>>>   extern bool rtems_unified_work_area;
>>>>>   +static uintptr_t heap_sbrk_spared=0;
>>>>> +extern uintptr_t _bsp_sbrk_init(uintptr_t, uintptr_t*);
>>>>>
>>>>>
>>>> The _bsp_sbrk_init() function is only available on PowerPC.
>>>>
>>>>
>>>>
>>> Thanks for your feedback.  How about the attached ?
>>> I added _bsp_sbrk_init() for other BSPs at
>>> c/src/lib/libbsp/shared/sbrk.c.  According to Joel, it would be needed
>>> in the future.  The name seems to be OK at this level, unless Joel comes
>>> up with a different name. One could fill in the function when the memory
>>> protection is implemented.  It seems that other
>>> non-PPC BSPs all use the sbrk() from c/src/lib/libbsp/shared/sbrk.c.
>>>
>>> More comments before PR ?
>>>
>>> Thanks,
>>> Kate
>>>
>>>
>>> _______________________________________________
>>> 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