[PATCH] score: Split _Terminate and call _Terminate_CPU_Fatal_halt.

[Chris Johns]

On 28/04/2014 4:18 pm, Sebastian Huber wrote:
> On 2014-04-28 01:48, Chris Johns wrote:
>> On 27/04/2014 10:30 pm, Sebastian Huber wrote:
>>> On 04/27/2014 02:13 PM, Chris Johns wrote:
>>>> Splitting the call to _CPU_Fatal_halt out into a separate function
>>>> allows the rtems-test gdb support the ability to halt once the
>>>> _Terminate function has completed it's work.
>>>>
>>>> This change allows the BeagleBoard xM BSP to pass a number of
>>>> important tests.
>>>
>>> I don't find a BeagleBord BSP in the tree.
>>>
>>
>> Ben has nicely answered this. Thanks Ben.
>>
>>> In case the BSP has to do fancy things during termination, why don't you
>>> do this in bsp_reset() or bsp_fatal_extension()?
>>>
>>> I guess the bsp.h has no:
>>>
>>> #include <bsp/default-initial-extension.h>
>>>
>>
>> I do not know and from a testing point of view I do not think it
>> should it matter.
>>
>> The _Terminate function needs to be split so we can set a break point
>> and have
>> it hit in the default case. Setting a break point based on a line
>> number in a
>> file is far too fragile. We need the extensions to run so the END
>> marker is
>> output in a few tests.
>
> You can set a break point to bsp_fatal_extension() (works with every BSP
> in the tree) or you can set a break point to
> rtems_test_fatal_extension() (works with every test in the tree, if not,
> then this test is broken).
>

The split lets me execute as much code as possible and it is something 
we should encourage with testing.

>>
>> I added the weak part because it costs nothing to add. Maybe we should
>> consider
>> the patch as 2 parts.
>>
>>> The way to control the termination sequence is via user extensions and
>>> not weak functions.
>>
>> Pros and cons ... If the point is having only one way, that is a valid
>> comment.
>> On the other hand weak functions cost little if anything and it lets
>> the BSP
>> designer have control over adding the required BSP specialisation. Weak
>> functions can be abused and that should be avoided.
>
> I see no point in adding another can of worms for just one BSP if you
> can easily avoid this with the existing solution (see above).
>

Happy to drop the weak attribute from the change.

>>
>> I have now taken a closer look at this default extension approach. The
>> <bsp/default-initial-extension.h> fights the cpukit and bsp separation
>> and adds
>> new rules to the use of confdefs.h that I do not think are being
>> enforced.
>
> If you don't use confdefs.h, then you should know what you are doing.
>
>> If a
>> new user to RTEMS uses a BSP and creates an app using confdefs.h and
>> does not
>> first include bsp.h or default-initial-extension.h the resulting BSP code
>> linked in is not what the BSP designer intended and as far as I can
>> see there
>> is not indication something is wrong.
>
> If you use confdefs.h, then everything is fine:
>
> [...]
> #ifdef CONFIGURE_DISABLE_BSP_SETTINGS
>    #undef BSP_DEFAULT_UNIFIED_WORK_AREAS
>    #undef BSP_IDLE_TASK_BODY
>    #undef BSP_IDLE_TASK_STACK_SIZE
>    #undef BSP_INITIAL_EXTENSION
>    #undef BSP_INTERRUPT_STACK_SIZE
>    #undef BSP_MAXIMUM_DEVICES
>    #undef BSP_ZERO_WORKSPACE_AUTOMATICALLY
>    #undef CONFIGURE_BSP_PREREQUISITE_DRIVERS
>    #undef CONFIGURE_MALLOC_BSP_SUPPORTS_SBRK
> #else
>    #include <bsp.h>
> #endif
> [...]
>
> This missing include of <bsp.h> was indeed a problem in the past and
> resulted in quite a lot service requests and confused users (e.g. no
> power saving idle thread).
>

I missed this and Joel pointed it out to me. This is fine.

Chris