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

[Kate Feng]

Joel Sherrill wrote:
>
> 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.
Yes, I agree so.  This patch will not work for PPC boards if
CONFIGURE_MALLOC_BSP_SUPPORTS_SBRK is not defined.
I actually had it defined in the bsp.h initially.  Now it is still there.
> 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. 
For this patch, bsp_sbrk_init() & sbrk() will not be called if the bsp.h 
does
not define CONFIGURE_MALLOC_BSP_SUPPORTS_SBRK.
The reason why bsp_sbrk_init() is created is because it is needed
to be declared to avoid a warning message.
> 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.  
It seems that all the non-powerpc BSPs call the libbsp/sbrk.c,
which really does nothing at this point.
> 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
To save code for smaller target, I would have to use the
#ifdef  CONFIGURE_MALLOC_BSP_SUPPORTS_SBRK
at bootcard.c,  libbsp/sbrk.c and rtems/bspIo.h to hide
bsp_sbrk_init().  If this is OK with you, I will do so.

Thanks,
Kate
>> 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
>>>>
>>>>
>>>>
>
>