Safe handling of unexpected exceptions Topic is solved

[Spym]

Hello everyone,

I am concerned about the way ChibiOS/RT handles unexpected exceptions. The current approach is to simply drop into an infinite loop (e.g. see /os/common/startup/ARMCMx/compilers/GCC/vectors.c for Cortex Ms). This can be quite dangerous, as the application may need to perform some emergency safe de-activation protocol in the event of such a catastrophic failure. The existing failure hook CH_CFG_SYSTEM_HALT_HOOK handles this well. However, the current approach to handling unexpected interrupts bypassess that mechanism. This surprised me in an unpleasant way while I was debugging my application, since I expected that the system halt hook will be invoked for all failure modes recognized by the OS. Besides, since the unexpected exception handler doesn't disable other interrupts, the MCU may still continue to perform some functions, which can further aggravate the consequences.

I propose to apply a trivial modification to the unexpected exception hook as follows (assuming ARM Cortex M in this example, I expect the fix to be identical for all other architectures):

Code: Select all

--- a/os/common/startup/ARMCMx/compilers/GCC/vectors.c
+++ b/os/common/startup/ARMCMx/compilers/GCC/vectors.c
@@ -45,9 +45,13 @@
 /*lint -save -e9075 [8.4] All symbols are invoked from asm context.*/
 void _unhandled_exception(void) {
 /*lint -restore*/
-
-  while (true) {
-  }
+  chSysHalt("UNDEFINED IRQ");
 }


Would this be accepted upstream?

[Giovanni]

Hi,

The default exception handler is not related to RT, it is part of the default startup files. Startup files have no knowledge of upper layers so the only possible "default" handling is that infinite loop. Calling chSysHalt() would put a dependency on the RTOS which is an optional element in the SW stack.

About safe exception handling, note that all symbols in the vectors table are "weak" so you can redefine them in you application and do proper handling of all undefined/unused exceptions/IRQs, for example by calling chSysHalt(). You don't need to redefine each single one of them, GCC allows to create aliases of functions using function attributes. You may create a single handler for all them or several handlers depending on your requirements.

The general idea is that the application should be responsible for handling the unexpected, that handler is just meant to be a default. The default is not meant to be a general solution.

Giovanni

[Tabulous]

here is what ive used, maybe we can add something like this to the HAL ?

Code: Select all

/*
 * except.c
 *
 *  Created on: Aug 30, 2016
 */

#include "main.h"

#if defined(ENABLE_EXCPT_DUMP)
#include <stdint.h>
#include <ch.h>
#include <string.h>

/**
 * Executes the BKPT instruction that causes the debugger to stop.
 * If no debugger is attached, this will be ignored
 */
#define bkpt() __asm volatile("BKPT #0\n")

void NMI_Handler(void) {
    //TODO
    while(1);
}

//See http://infocenter.arm.com/help/topic/com.arm.doc.dui0552a/BABBGBEC.html
typedef enum  {
    Reset = 1,
    NMI = 2,
    HardFault = 3,
    MemManage = 4,
    BusFault = 5,
    UsageFault = 6,
} FaultType;

char exceptionstr[50];

void sendchar(char c)
{

    while (!(USART2->SR & USART_SR_TXE));
    USART2->DR = ( c );
}

void exception_dump( char *s )
{
    do
    {
        sendchar( *s );
    }
    while( *s++ );
}

void faulttype( FaultType type )
{

    switch( type )
    {
        case NMI:
          exception_dump( "NMI" );
          break;

        case Reset:
          exception_dump( "Reset" );
          break;

        case BusFault:
          exception_dump( "Bus Fault" );
          break;

        case HardFault:
          exception_dump( "Hard Fault" );
          break;

        case UsageFault:
          exception_dump( "Usage Fault" );
          break;

        case MemManage:
          exception_dump( "Memory Manager" );
          break;
    }
}


void hex2string( uint32_t hex )
{
uint8_t i;
char ascii = 0x0;
uint32_t divider = 0x10000000;


    exception_dump("0x");

    for ( i=0; i<8; i++ )
    {
        ascii = hex / divider;
        hex -= (ascii * divider);
        divider /= 0x10;

        switch (ascii)
        {
            case 0xf:
                sendchar('F');
                break;

            case 0xe:
                sendchar('E');
                break;

            case 0xd:
                sendchar('D');
                break;

            case 0xc:
                sendchar('C');
                break;

            case 0xb:
                sendchar('B');
                break;

            case 0xa:
                sendchar('A');
                break;

            default:
                ascii += 0x30;
                sendchar(ascii);
                break;
        }
    }
}

void HardFault_Handler(void) {
    //Copy to local variables (not pointers) to allow GDB "i loc" to directly show the info
    //Get thread context. Contains main registers including PC and LR
    struct port_extctx ctx;
    memcpy(&ctx, (void*)__get_PSP(), sizeof(struct port_extctx));
    (void)ctx;
    //Interrupt status register: Which interrupt have we encountered, e.g. HardFault?
    FaultType faultType = (FaultType)__get_IPSR();
    (void)faultType;
    //For HardFault/BusFault this is the address that was accessed causing the error
    uint32_t faultAddress = SCB->BFAR;
    (void)faultAddress;
    //Flags about hardfault / busfault
    //See http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0552a/Cihdjcfc.html for reference
    bool isFaultPrecise = ((SCB->CFSR >> SCB_CFSR_BUSFAULTSR_Pos) & (1 << 1) ? true : false);
    bool isFaultImprecise = ((SCB->CFSR >> SCB_CFSR_BUSFAULTSR_Pos) & (1 << 2) ? true : false);
    bool isFaultOnUnstacking = ((SCB->CFSR >> SCB_CFSR_BUSFAULTSR_Pos) & (1 << 3) ? true : false);
    bool isFaultOnStacking = ((SCB->CFSR >> SCB_CFSR_BUSFAULTSR_Pos) & (1 << 4) ? true : false);
    bool isFaultAddressValid = ((SCB->CFSR >> SCB_CFSR_BUSFAULTSR_Pos) & (1 << 7) ? true : false);
    (void)isFaultPrecise;
    (void)isFaultImprecise;
    (void)isFaultOnUnstacking;
    (void)isFaultOnStacking;
    (void)isFaultAddressValid;
    //Output some debug info about the expection
    exception_dump( "********** Exception Dump **********\r\n" );

    if( isFaultPrecise )
    {
        exception_dump( "Fault Precise     : TRUE\r\n" );
    }
    else
    {
        exception_dump( "Fault Precise     : FALSE\r\n" );
    }

    if( isFaultImprecise )
    {
        exception_dump( "Fault Imprecise   : TRUE\r\n" );
    }
    else
    {
        exception_dump( "Fault Imprecise   : FALSE\r\n" );
    }

    if( isFaultOnStacking )
    {
        exception_dump( "Fault Onstacking  : TRUE\r\n" );
    }
    else
    {
        exception_dump( "Fault Onstacking  : FALSE\r\n" );
    }

    if( isFaultOnUnstacking )
    {
        exception_dump( "Fault Unstacking  : TRUE\r\n" );
    }
    else
    {
        exception_dump( "Fault Unstacking  : FALSE\r\n" );
    }

    if( isFaultAddressValid )
    {
        exception_dump( "Fault Valid Addr  : TRUE\r\n" );
    }
    else
    {
        exception_dump( "Fault Valid Addr  : FALSE\r\n" );
    }

    exception_dump( "Fault Addr        : " );
    hex2string( faultAddress );
    exception_dump( "\r\n" );

    exception_dump( "Fault Type        : " );
    faulttype( faultType );
    exception_dump( "\r\n" );

    exception_dump( "Context PC        : " );
    hex2string( ( uint32_t )ctx.pc );
    exception_dump( "\r\n" );

    exception_dump( "Context Thread    : " );
    hex2string( ( uint32_t )ctx.lr_thd );
    exception_dump( "\r\n" );


    //Cause debugger to stop. Ignored if no debugger is attached
    bkpt();
    NVIC_SystemReset();
}

void BusFault_Handler(void) __attribute__((alias("HardFault_Handler")));

void UsageFault_Handler(void) {
    //Copy to local variables (not pointers) to allow GDB "i loc" to directly show the info
    //Get thread context. Contains main registers including PC and LR
    struct port_extctx ctx;
    memcpy(&ctx, (void*)__get_PSP(), sizeof(struct port_extctx));
    (void)ctx;
    //Interrupt status register: Which interrupt have we encountered, e.g. HardFault?
    FaultType faultType = (FaultType)__get_IPSR();
    (void)faultType;
    //Flags about hardfault / busfault
    //See http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0552a/Cihdjcfc.html for reference
    bool isUndefinedInstructionFault = ((SCB->CFSR >> SCB_CFSR_USGFAULTSR_Pos) & (1 << 0) ? true : false);
    bool isEPSRUsageFault = ((SCB->CFSR >> SCB_CFSR_USGFAULTSR_Pos) & (1 << 1) ? true : false);
    bool isInvalidPCFault = ((SCB->CFSR >> SCB_CFSR_USGFAULTSR_Pos) & (1 << 2) ? true : false);
    bool isNoCoprocessorFault = ((SCB->CFSR >> SCB_CFSR_USGFAULTSR_Pos) & (1 << 3) ? true : false);
    bool isUnalignedAccessFault = ((SCB->CFSR >> SCB_CFSR_USGFAULTSR_Pos) & (1 << 8) ? true : false);
    bool isDivideByZeroFault = ((SCB->CFSR >> SCB_CFSR_USGFAULTSR_Pos) & (1 << 9) ? true : false);
    (void)isUndefinedInstructionFault;
    (void)isEPSRUsageFault;
    (void)isInvalidPCFault;
    (void)isNoCoprocessorFault;
    (void)isUnalignedAccessFault;
    (void)isDivideByZeroFault;
    bkpt();
    NVIC_SystemReset();
}

void MemManage_Handler(void) {
    //Copy to local variables (not pointers) to allow GDB "i loc" to directly show the info
    //Get thread context. Contains main registers including PC and LR
    struct port_extctx ctx;
    memcpy(&ctx, (void*)__get_PSP(), sizeof(struct port_extctx));
    (void)ctx;
    //Interrupt status register: Which interrupt have we encountered, e.g. HardFault?
    FaultType faultType = (FaultType)__get_IPSR();
    (void)faultType;
    //For HardFault/BusFault this is the address that was accessed causing the error
    uint32_t faultAddress = SCB->MMFAR;
    (void)faultAddress;
    //Flags about hardfault / busfault
    //See http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0552a/Cihdjcfc.html for reference
    bool isInstructionAccessViolation = ((SCB->CFSR >> SCB_CFSR_MEMFAULTSR_Pos) & (1 << 0) ? true : false);
    bool isDataAccessViolation = ((SCB->CFSR >> SCB_CFSR_MEMFAULTSR_Pos) & (1 << 1) ? true : false);
    bool isExceptionUnstackingFault = ((SCB->CFSR >> SCB_CFSR_MEMFAULTSR_Pos) & (1 << 3) ? true : false);
    bool isExceptionStackingFault = ((SCB->CFSR >> SCB_CFSR_MEMFAULTSR_Pos) & (1 << 4) ? true : false);
    bool isFaultAddressValid = ((SCB->CFSR >> SCB_CFSR_MEMFAULTSR_Pos) & (1 << 7) ? true : false);
    (void)isInstructionAccessViolation;
    (void)isDataAccessViolation;
    (void)isExceptionUnstackingFault;
    (void)isExceptionStackingFault;
    (void)isFaultAddressValid;
    bkpt();
    NVIC_SystemReset();
}
#endif


[Giovanni]

Hi,

That module is more like a debug support module, I think the initial point was about handling into the final code for safety reasons.

Anyway, it would be a good idea to include something like this somewhere but not in HAL, it is Cortex-specific, perhaps it could go under /os/various/debug_support.

Giovanni

[Spym]

Hi Giovanni,

Apologies about the delayed response, forum notifications ended up not being delivered for some reason.

I did consider to redefine the weak handlers; however, this approach is quite error prone as well: my compiler (which is GCC, although I expect the same holds for most compilers) does not allow to deterministically provide multiple weak definitions for the same function in the absence of any strong one. This requires one to very carefully, manually check that the application provides strong overrides to every unused handler (possibly in the form of aliases, it doesn't matter). While an extra handler definition is easy to detect (since in this case the linker will fail, referring to redundant definitions), the opposite case is impossible to detect automatically. By all means, please correct me if I'm mistaken.

Therefore, I can see only the following two reliable solutions:

1. As I described in my first post, invoke chSysHalt(). I do understand that a compile-time dependency on the OS is unfeasible, but this can be circumvented somehow by forward-declaring the halt handler, for instance:

Code: Select all

extern void chSysHalt(const char*);
void _unhandled_exception(void) {
  chSysHalt("UNDEFINED IRQ");
}


Not sure if it fits the architectural standards of ChibiOS though...

2. Make the _unhandled_exception() handler weak. This approach will enable the application to easily implement custom behaviors with no extra hassle. Unless I'm missing something here.

Thanks!

[Giovanni]

Hi,

I will look into making __unhandled_exception a weak symbol.

Giovanni

[Matthias]

Hi Giovanni,

this is another vote for making _unhandled_exception a weak symbol.

We have got the requirement to log occurence of unexpected exceptions. Currently we solved the problem by our own version of the vector table, defining _unhandled_exception as a weak symbol and overwrite it in our local code.
It would be nice, if this little change make it into ChibiOS.

As a side node: For some controller families (ARM, e200) the unhandled exception handler is already a weak symbol, but not for the ARM Cortex-M Controller (ARMCMx), that we are use.

Regards
Matthias

[Giovanni]

Hi,

Made it weak in both trunk and 16.x.

Giovanni

[Spym]

Thanks, Giovanni, this is much appreciated. Shall we expect the 17.x release anytime soon?

[Giovanni]

I was trying today but too many bugs open in this forum.... next weekend likely.

If it is urgent for you please let me know.

Giovanni