Small default stacks for idle and usb_pump threads Topic is solved

[hercek]

Well, this may not be a real bug report but I would expect that when a ChibiOS application is compiled with all chconf.h _DBG_ options activated using GCC -Og then it should not crash with stack overflows. So this is more of a proposal that the default stack sizes should be a bit bigger.

ChibiOS stable_17.6.x 8d1d9337216

Compiler GCC 7.2.0-1
g++ -std=c++14 -fno-rtti -fno-exceptions -Og -ggdb -fomit-frame-pointer -falign-functions=16
All ChibiOS debug defines active except CH_DBG_STATISTICS. Fast interrupts were used. Hardware FPU was used.

Platform: STM32F405

Board: Olimex STM32-H405

Nature of the problem, failure mode:
Stacks of the idle thread and usb_pump threads randomly overflowed and overwrote nearby data. In my case, it was the bottom of the process stack and some USB driver data.

If we agree that the default stack sizes should be at least as big so that there are no problems to compile with ChibiOS _DBG_ settings activated and -Og then the default stack sizes should be at least:

#define PORT_IDLE_THREAD_STACK_SIZE         96
/* preferably at least 128 to have some safety margin */
#define STM32_USB_OTG_THREAD_STACK_SIZE     128
/* well here the default was 128 but I would put it at 192 at least */

[Giovanni]

Hi,

I haven't tried version GCC 7.x yet.

PORT_IDLE_THREAD_STACK_SIZE represents the stack frame of the idle thread, are you sure it should be 96? I never seen it so large. Could you post the asm listing of idle thread?

Giovanni

[hercek]

Well, I'm not sure now whether 96 is not way too much. The problem is I did not record the exact way I computed it when I was writing the original message. I'm sure I was computing it from the memory dump of ch_idle_thread_wa by looking at which values of 0x55555555 were overwritten and then adding there some safety margin (maybe 32 ???). I did not record the original content of the idle thread stack from which the computation was done.

Anyway the real problem is not the idle thread itself (see the gdb-log.txt attachment containing its disassembly). The problem is the code which executes on the process stack and which was added by ChibiOS debugging macros, plus compiling with -Og, and use of fast interrupts which add some context to the stack too. Hardware FPU was used too.

The proper way of fixing this needs somewhat more complicated PORT_WA_SIZE macro which currently defined only like this:

Code: Select all

// chcore_v7m.h:397
#define PORT_WA_SIZE(n) ((size_t)PORT_GUARD_PAGE_SIZE +                     \
                         sizeof (struct port_intctx) +                      \
                         sizeof (struct port_extctx) +                      \
                         (size_t)(n) +                                      \
                         (size_t)PORT_INT_REQUIRED_STACK)

By somewhat more complicated I mean a way which takes CH_DBG_* into account. The ChibiOS debug overhead is probably meant to be included in PORT_INT_REQUIRED_STACK (regardless whether ChibiOS debug macros CH_DBG_* are enabled or not). If that is right then a simple approach is just to increase PORT_INT_REQUIRED_STACK. It is "conservatively" set to 64 (chcore_v7m.h:107) currently. But I think it is smaller than required by at least 32 + (256-232) - 16 + 16 = 56 bytes.
The reasoning:

• 32 is PORT_GUARD_PAGE_SIZE which was enabled in my setup but it should work even when the guard page is disabled.
• (256-232) is the space overwritten on my idle stack (see the gdb-log.txt attachment). This is a new idle stack I produced now (not the one I used when writing the original message). PORT_IDLE_THREAD_STACK_SIZE was redefined to 256 in my chconf.h. 232 is the offset of the byte with the smallest address which was still overwritten on the stack.
• -16 is the value to which PORT_IDLE_THREAD_STACK_SIZE is defined by default.
• +16 ... well we actually should not consume the stack space allocated to _idle_thread routine in PORT_INT_REQUIRED_STACK.

It would be good to have some safety margin there too.

Hell, I cannot attach it. The web site complains the extension is wrong. OK, here it is pasted directly into this message:

(gdb) run
adapter speed: 4000 kHz
target halted due to debug-request, current mode: Thread 
xPSR: 0x01000000 pc: 0x08000210 msp: 0x20000400
^C
Program received signal SIGINT, Interrupt.
_idle_thread (p=0x0) at ChibiOS/os/rt/src/chsys.c:72
72	static void _idle_thread(void *p) {
(gdb) bt
#0  _idle_thread (p=0x0) at ChibiOS/os/rt/src/chsys.c:72
#1  0x0800034e in _port_thread_start ()
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
(gdb) disassem _idle_thread
Dump of assembler code for function _idle_thread:
=> 0x08000620 <+0>:	b.n	0x8000620 <_idle_thread>
End of assembler dump.
(gdb) up
#1  0x0800034e in _port_thread_start ()
(gdb) disassem _port_thread_start
Dump of assembler code for function _port_thread_start:
   0x08000340 <+0>:	bl	0x8000820 <_dbg_check_unlock>
   0x08000344 <+4>:	movs	r3, #0
   0x08000346 <+6>:	msr	BASEPRI, r3
   0x0800034a <+10>:	mov	r0, r5
   0x0800034c <+12>:	blx	r4
=> 0x0800034e <+14>:	movs	r0, #0
   0x08000350 <+16>:	bl	0x80011b0 <chThdExit>
End of assembler dump.
(gdb) x/157wx ch_idle_thread_wa
0x20000820 <ch_idle_thread_wa>:	0x55555555	0x55555555	0x55555555	0x55555555
0x20000830 <ch_idle_thread_wa+16>:	0x55555555	0x55555555	0x55555555	0x55555555
0x20000840 <ch_idle_thread_wa+32>:	0x55555555	0x55555555	0x55555555	0x55555555
0x20000850 <ch_idle_thread_wa+48>:	0x55555555	0x55555555	0x55555555	0x55555555
0x20000860 <ch_idle_thread_wa+64>:	0x55555555	0x55555555	0x55555555	0x55555555
0x20000870 <ch_idle_thread_wa+80>:	0x55555555	0x55555555	0x55555555	0x55555555
0x20000880 <ch_idle_thread_wa+96>:	0x55555555	0x55555555	0x55555555	0x55555555
0x20000890 <ch_idle_thread_wa+112>:	0x55555555	0x55555555	0x55555555	0x55555555
0x200008a0 <ch_idle_thread_wa+128>:	0x55555555	0x55555555	0x55555555	0x55555555
0x200008b0 <ch_idle_thread_wa+144>:	0x55555555	0x55555555	0x55555555	0x55555555
0x200008c0 <ch_idle_thread_wa+160>:	0x55555555	0x55555555	0x55555555	0x55555555
0x200008d0 <ch_idle_thread_wa+176>:	0x55555555	0x55555555	0x55555555	0x55555555
0x200008e0 <ch_idle_thread_wa+192>:	0x55555555	0x55555555	0x55555555	0x55555555
0x200008f0 <ch_idle_thread_wa+208>:	0x55555555	0x55555555	0x55555555	0x55555555
0x20000900 <ch_idle_thread_wa+224>:	0x55555555	0x55555555	0x20000a50	0x20002430
0x20000910 <ch_idle_thread_wa+240>:	0x20001170	0x20000974	0x20001b50	0x08000d8f
0x20000920 <ch_idle_thread_wa+256>:	0x08000338	0x8100a200	0x55555555	0x55555555
0x20000930 <ch_idle_thread_wa+272>:	0x55555555	0x55555555	0x55555555	0x55555555
0x20000940 <ch_idle_thread_wa+288>:	0x55555555	0x55555555	0x20000a50	0x20002430
0x20000950 <ch_idle_thread_wa+304>:	0x20001010	0x20000974	0x000001b9	0x08000d8f
0x20000960 <ch_idle_thread_wa+320>:	0x0800033c	0x81000200	0x20000a50	0x200037f0
0x20000970 <ch_idle_thread_wa+336>:	0x20000a50	0x55555555	0x55555555	0x55555555
0x20000980 <ch_idle_thread_wa+352>:	0x20000a50	0x200037f0	0x00000000	0x55555555
0x20000990 <ch_idle_thread_wa+368>:	0x20001b50	0x08000361	0x08000362	0x61000000
0x200009a0 <ch_idle_thread_wa+384>:	0x55555555	0x55555555	0x55555555	0x55555555
0x200009b0 <ch_idle_thread_wa+400>:	0x55555555	0x20000a90	0x20000a50	0x55555555
0x200009c0 <ch_idle_thread_wa+416>:	0x55555555	0x55555555	0x55555555	0x55555555
0x200009d0 <ch_idle_thread_wa+432>:	0x55555555	0x08000f4d	0x55555555	0x08000621
0x200009e0 <ch_idle_thread_wa+448>:	0x55555555	0x08000361	0x00000000	0x20003070
0x200009f0 <ch_idle_thread_wa+464>:	0x00000000	0x00000000	0x20001a48	0x0800034f
0x20000a00 <ch_idle_thread_wa+480>:	0x08000620	0x61000000	0x3e99999a	0x00000000
0x20000a10 <ch_idle_thread_wa+496>:	0x00000000	0x00000000	0x00000000	0x00000000
0x20000a20 <ch_idle_thread_wa+512>:	0x00000000	0x00000000	0x00000000	0x00000000
0x20000a30 <ch_idle_thread_wa+528>:	0x00000000	0x3f800000	0x3f800000	0x45160000
0x20000a40 <ch_idle_thread_wa+544>:	0x3f800000	0x7fc00000	0x30000010	0x08000345
0x20000a50 <ch_idle_thread_wa+560>:	0x20000a90	0x20000a90	0x00000001	0x20000974
0x20000a60 <ch_idle_thread_wa+576>:	0x20002430	0x200012d0	01x0800947c	0x20000820
0x20000a70 <ch_idle_thread_wa+592>:	0x55010001	0x006434cf	0x00000000	0x20000a7c
0x20000a80 <ch_idle_thread_wa+608>:	0x20000a7c	0x00000000	0x00000000	0x00000001
0x20000a90 <ch>:	0x20000a90
(gdb) cht 7
Thread state codes:
00 READY   01 CURRENT  02 WTSTART 03 SUSPEND 04 QUEUED  
05 WTSEM   06 WTMTX    07 WTCOND  08 SLEEP   09 WTEXIT  
10 WTOREVT 11 WTANDEVT 12 SNDMSGQ 13 SNDMSG  14 WTMSG   15 FINAL
Trace buffer:
type: 1, state: 10, rtstamp: 12746475, time: 8212462, thread: idle, waitObj: 0x1
type: 2, state: 00, rtstamp: 12760876, time: 8212462, enterIsr: SysTick_Handler
type: 3, state: 00, rtstamp: 12761263, time: 8212463, exitIsr: SysTick_Handler
type: 2, state: 00, rtstamp: 12763196, time: 8212463, enterIsr: Vector88 /*ADC*/
type: 3, state: 00, rtstamp: 12763617, time: 8212463, exitIsr: Vector88
/* There is a fast interrupt somewhere here which is not logged by ChibiOS. */
type: 1, state: 00, rtstamp: 12763971, time: 8212463, thread: foc, waitObj: 0x0
type: 1, state: 10, rtstamp: 12766694, time: 8212463, thread: idle, waitObj: 0x1
Most recent trace buffer index is 55 (in range 0..127).
Current thread name: idle
(gdb)

[Giovanni]

Hi,

USB pump has been removed, I think this is no more relevant.

Giovanni

[faisal]

Hi,

USB pump has been removed, I think this is no more relevant.

Giovanni

Is there no issue with using CH_DBG_* options? Is any additional stack size required by the idle thread taken into account already?

[Giovanni]

The default of PORT_INT_REQUIRED_STACK is quite generous.

Giovanni

[apmorton]

Just something I thought I would archive here in case someone ends up in this thread from search results.
Everything I am about to say applies to ChibiOS 17.x and 18.x - things may be different in the future.

Its worth noting that PORT_IDLE_THREAD_STACK_SIZE can be important when you are using the hooks below:

• CH_CFG_CONTEXT_SWITCH_HOOK
  - when otp is the idle thread, this hook is executed from the idle thread stack
• CH_CFG_IDLE_LEAVE_HOOK
  - this hook is always executed from the idle thread stack
• CH_CFG_IDLE_LOOP_HOOK
  - this hook is always executed from the idle thread stack

It's also worth noting that CH_CFG_IDLE_ENTER_HOOK is never executed from the idle thread stack, but instead is always executed from the stack of the thread being switched _from_.

If you are using any of these hooks in a way that might result in stack usage, the default idle stack size could bite you.
An example from personal experience was adding the hooks required for SEGGER SystemView.

[Giovanni]

Good points, in general hooks are executed before the switch operation if there is a switch involved.

Giovanni

[hercek]

Well, I had the overflow in both usb_pump an idle threads. I did not use the HOOKs.
Anyway I do not really mind if the default sizes are not increased. People can increase it themselves in debug builds.

Could this be also related to the fact that I did not use heap? Maybe stack overflow checks (during task switch) did not work when heap is not available (CH_CFG_USE_HEAP was FALSE)? I caught it because the program was crashing and it was not detected by CH_DBG_ENABLE_STACK_CHECK (it was set to TRUE). Well it may not be implemented for STM32F4.

[Giovanni]

Hi,

Heap is unrelated to stacks. Stack overflow detection is implemented on F4, a guard page is placed at each stack base.

Giovanni