casaroli commented on code in PR #21514:
URL: https://github.com/apache/nuttx/pull/21514#discussion_r4495715943
##########
arch/Kconfig:
##########
@@ -1460,12 +1460,11 @@ config ARCH_HAVE_INTERRUPTSTACK
config ARCH_INTERRUPTSTACK
int "Interrupt Stack Size"
depends on ARCH_HAVE_INTERRUPTSTACK
- default 0
+ range 128 2147483647
Review Comment:
With no `default`, `ARCH_INTERRUPTSTACK` always has a value when
`ARCH_HAVE_INTERRUPTSTACK=y`: kconfig sets it to the range minimum, 128. So a
config can no longer run without an interrupt stack, and every config that had
0 now gets a 128 B interrupt stack. CI shows it: the normalize step adds
`CONFIG_ARCH_INTERRUPTSTACK=128` to freedom-k64f:nsh and the other defconfigs,
AVR fails with `unknown pseudo-op: .bss`, MIPS with `undefined reference to
g_intstackalloc`, armv6-m clang in `arm_initialstate.c:184`, and qemu-intel64
loses 16128 B of IRQ stacks (0x2000 to 128 B each).
Fix: keep `default 0` with 0 meaning "no interrupt stack" (and the `> 0`
checks), or add a separate bool option that enables the interrupt stack and set
it in every defconfig that has a size now.
##########
sched/misc/assert.c:
##########
@@ -252,9 +253,10 @@ static void dump_stacks(FAR struct tcb_s *rtcb, uintptr_t
sp)
int cpu = rtcb->cpu;
#else
int cpu = this_cpu();
+
UNUSED(cpu);
#endif
-#if CONFIG_ARCH_INTERRUPTSTACK > 0
+#ifdef CONFIG_ARCH_INTERRUPTSTACK
Review Comment:
This `#ifdef` is always true: lines 75-76 still define
`CONFIG_ARCH_INTERRUPTSTACK` to 0 when it is not set. The same fallback stays
in `arm_internal.h:89`, `arm64_internal.h:89`, `x86_64_internal.h:93` and the
avr, hc, mips, or1k, renesas, sparc, tricore, x86 and xtensa internal headers,
so every new `#ifdef CONFIG_ARCH_INTERRUPTSTACK` after them is true. sim and
qemu-i486 fail in CI with `implicit declaration of function
'up_get_intstackbase'` and `division by zero` in this file, and the new
`#ifndef` at `x86_64_internal.h:114` can never select 0x2000.
Fix: remove the `#define CONFIG_ARCH_INTERRUPTSTACK 0` fallbacks together
with the switch to `#ifdef`.
##########
arch/arm/src/tlsr82/chip/b87/boot/cstartup_flash.S:
##########
@@ -39,7 +39,7 @@
#ifndef __IRQ_STK_SIZE__
#define __IRQ_STK_SIZE__
CONFIG_ARCH_INTERRUPTSTACK
-#if CONFIG_ARCH_INTERRUPTSTACK <= 128
+#ifndef CONFIG_ARCH_INTERRUPTSTACK
Review Comment:
The old check stopped the build for a size `<= 128`, as the message says.
`#ifndef` never fires, and 128 (the new range minimum) now passes.
Fix: keep `#if CONFIG_ARCH_INTERRUPTSTACK <= 128`.
##########
arch/arm/src/common/arm_internal.h:
##########
@@ -89,7 +90,7 @@
# define CONFIG_ARCH_INTERRUPTSTACK 0
#endif
-#define INTSTACK_SIZE (CONFIG_ARCH_INTERRUPTSTACK & ~STACKFRAME_ALIGN_MASK)
+#define INTSTACK_SIZE STACK_ALIGN_DOWN(CONFIG_ARCH_INTERRUPTSTACK)
Review Comment:
`STACK_ALIGN_DOWN()` aligns to `TLS_STACK_ALIGN` (8 KiB by default) when
`CONFIG_TLS_ALIGNED=y`, which is the default in protected and kernel builds and
is set in qemu-armv7a:smp. Then `INTSTACK_SIZE` is `2048 & ~8191 = 0`, while
the assembly still allocates `CONFIG_ARCH_INTERRUPTSTACK & ~7`. On
qemu-armv7a:smp `g_irqstack_alloc` is 0 bytes (0x2000 on master) and the board
prints nothing at boot; on master it passes ostest. On mps2-an500:knsh
`arm_initialize_stack()` puts MSP at the bottom of the interrupt stack and QEMU
locks up after `nx_start: Entry` (master passes ostest). The same happens in
`arm64_internal.h:153` (qemu-armv8a:knsh: `g_interrupt_stack` is 0 bytes, but
`arm64_vectors.S` sets SP to `g_interrupt_stack + CONFIG_ARCH_INTERRUPTSTACK`;
it panics at boot with a data abort on 0xdeaddeaddeaddead) and in the risc-v,
xtensa, avr, or1k, tricore and sparc color/check sizes.
Fix: keep `STACKFRAME_ALIGN_DOWN()` (the old `& ~STACKFRAME_ALIGN_MASK`) for
the interrupt stack size; the interrupt stack has no TLS block.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
