From ecb60fb4d65231dd7d1f4ea4dd2818fd119e9c7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Desbiens?= Date: Wed, 12 Aug 2026 16:34:30 -0400 Subject: [PATCH] Assembled the code behind feature macros, and fixed the POP it found scripts/check_clang.sh assembled every source with default flags, so the preprocessor discarded each #ifdef block before the assembler saw it. Nothing in the tree had ever assembled a guarded path. That covers the VFP context save and restore in ten ports, and 218 files carrying TX_LOW_POWER or TX_ENABLE_EXECUTION_CHANGE_NOTIFY. Turning those on found a defect. The Cortex-M0 and Cortex-M23 execution-profile paths bracket their call with PUSH {r0, lr} BL _tx_execution_isr_enter POP {r0, lr} and the last of those is invalid on Armv6-M and Armv8-M Baseline, where the 16-bit Thumb POP takes r0-r7 and pc and nothing else. GNU rejects it as well -- "cannot honor width suffix" -- so TX_ENABLE_EXECUTION_CHANGE_NOTIFY and TX_EXECUTION_PROFILE_ENABLE have never been buildable on either port with either toolchain. Four files, all the same shape. The fix pops into a scratch register and moves it, MOV to a high register being permitted where POP is not. r1 is free: the BL may clobber r0-r3, which is the reason r0 is saved in the first place. Disassembling the result gives push {r0, lr} / bl / pop {r0, r1} / mov lr, r1 / bx lr, one 16-bit instruction more than before and otherwise the same. Two findings that were not defects, recorded in the script so they are not rediscovered: Cortex-R4 needs an -mfpu to assemble its VFP path, because its FPU is an option rather than part of the core. GNU fails identically without one, so this is a flags requirement and not a toolchain divergence. The A profile ports must not be given one. Adding -mfpu=vfpv3-d16 uniformly broke 28 files with "register expected", because those ports save D16-D31 and a -d16 FPU does not have those registers. Their defaults were already right. The new stage runs under --asm-only as well, needing no target C library, and reports 37 of 37 VFP files, 8 of 8 TX_LOW_POWER and 218 of 218 TX_ENABLE_EXECUTION_CHANGE_NOTIFY. Restoring the POP for one run makes it fail with 217 of 218 and name the file and the error, so the stage is not vacuous. The other four stages are unchanged: 711 of 711 assembled, 185 of 185 common C sources for each of nine cores, 42 of 42 script-driven examples and 5 of 5 CMake images. The fixed code is verified to assemble with both toolchains and to encode as intended. It is not verified running: there is no Cortex-M0 or Cortex-M23 model here, and these are context save and restore paths, so that gap is worth stating. Assisted-by: Claude Code (Opus 5) --- .../gnu/src/tx_thread_context_restore.S | 9 ++- .../gnu/src/tx_thread_context_save.S | 9 ++- .../gnu/src/tx_thread_context_restore.S | 9 ++- .../gnu/src/tx_thread_context_save.S | 9 ++- scripts/check_clang.sh | 72 +++++++++++++++++-- 5 files changed, 99 insertions(+), 9 deletions(-) diff --git a/ports/cortex_m0/gnu/src/tx_thread_context_restore.S b/ports/cortex_m0/gnu/src/tx_thread_context_restore.S index 968548671..68fd52517 100644 --- a/ports/cortex_m0/gnu/src/tx_thread_context_restore.S +++ b/ports/cortex_m0/gnu/src/tx_thread_context_restore.S @@ -85,7 +85,14 @@ _tx_thread_context_restore: /* Call the ISR exit function to indicate an ISR is complete. */ PUSH {r0, lr} // Save return address BL _tx_execution_isr_exit // Call the ISR exit function - POP {r0, lr} // Recover return address + POP {r0, r1} // Recover r0 and the return address + MOV lr, r1 // This core cannot POP into LR: the + // 16-bit Thumb POP takes r0-r7 and + // pc only. MOV to a high register + // is permitted, so restore LR from + // a scratch register. r1 is free -- + // the BL above may clobber r0-r3, + // which is why r0 is saved at all. #endif BX lr @} diff --git a/ports/cortex_m0/gnu/src/tx_thread_context_save.S b/ports/cortex_m0/gnu/src/tx_thread_context_save.S index 9866da139..e62a28711 100644 --- a/ports/cortex_m0/gnu/src/tx_thread_context_save.S +++ b/ports/cortex_m0/gnu/src/tx_thread_context_save.S @@ -76,7 +76,14 @@ _tx_thread_context_save: /* Call the ISR enter function to indicate an ISR is starting. */ PUSH {r0, lr} // Save return address BL _tx_execution_isr_enter // Call the ISR enter function - POP {r0, lr} // Recover return address + POP {r0, r1} // Recover r0 and the return address + MOV lr, r1 // This core cannot POP into LR: the + // 16-bit Thumb POP takes r0-r7 and + // pc only. MOV to a high register + // is permitted, so restore LR from + // a scratch register. r1 is free -- + // the BL above may clobber r0-r3, + // which is why r0 is saved at all. #endif /* Context is already saved - just return. */ diff --git a/ports/cortex_m23/gnu/src/tx_thread_context_restore.S b/ports/cortex_m23/gnu/src/tx_thread_context_restore.S index 6f2b453cf..e8ddac054 100644 --- a/ports/cortex_m23/gnu/src/tx_thread_context_restore.S +++ b/ports/cortex_m23/gnu/src/tx_thread_context_restore.S @@ -73,7 +73,14 @@ _tx_thread_context_restore: /* Call the ISR exit function to indicate an ISR is complete. */ PUSH {r0, lr} // Save return address BL _tx_execution_isr_exit // Call the ISR exit function - POP {r0, lr} // Recover return address + POP {r0, r1} // Recover r0 and the return address + MOV lr, r1 // This core cannot POP into LR: the + // 16-bit Thumb POP takes r0-r7 and + // pc only. MOV to a high register + // is permitted, so restore LR from + // a scratch register. r1 is free -- + // the BL above may clobber r0-r3, + // which is why r0 is saved at all. #endif BX lr diff --git a/ports/cortex_m23/gnu/src/tx_thread_context_save.S b/ports/cortex_m23/gnu/src/tx_thread_context_save.S index 84bf99083..136c6391f 100644 --- a/ports/cortex_m23/gnu/src/tx_thread_context_save.S +++ b/ports/cortex_m23/gnu/src/tx_thread_context_save.S @@ -73,7 +73,14 @@ _tx_thread_context_save: /* Call the ISR enter function to indicate an ISR is starting. */ PUSH {r0, lr} // Save return address BL _tx_execution_isr_enter // Call the ISR enter function - POP {r0, lr} // Recover return address + POP {r0, r1} // Recover r0 and the return address + MOV lr, r1 // This core cannot POP into LR: the + // 16-bit Thumb POP takes r0-r7 and + // pc only. MOV to a high register + // is permitted, so restore LR from + // a scratch register. r1 is free -- + // the BL above may clobber r0-r3, + // which is why r0 is saved at all. #endif BX lr diff --git a/scripts/check_clang.sh b/scripts/check_clang.sh index c0cae4014..c7ebed79a 100755 --- a/scripts/check_clang.sh +++ b/scripts/check_clang.sh @@ -14,11 +14,11 @@ # SPDX-License-Identifier: MIT and CC0-1.0 ############################################################################## -# Builds the Arm ports with an LLVM based toolchain, in four stages: assemble -# every assembly source of every Arm gnu port, compile the common C sources for -# one core per architecture profile, link the example builds that have a script -# driver, then link those driven by CMake. Only the two linking stages need a -# target C library. +# Builds the Arm ports with an LLVM based toolchain, in five stages: assemble +# every assembly source of every Arm gnu port, assemble again the parts guarded +# by feature macros, compile the common C sources for one core per architecture +# profile, then link the example builds, both the script-driven ones and those +# driven by CMake. Only the linking stages need a target C library. # # scripts/check_clang.sh # clang from PATH # scripts/check_clang.sh --clang /path/to/clang @@ -140,6 +140,28 @@ declare -A PORT_TARGET=( [cortex_a78_smp]="aarch64-none-elf cortex-a78" ) +# Assembly guarded by a feature macro is invisible to the stage above, which +# assembles with default flags and so lets the preprocessor discard every #ifdef +# block before the assembler sees it. These are the macros a user can turn on; +# each file carrying one is assembled again with it defined. +# +# This is not hypothetical. It is where "POP {r0, lr}" was found in the Cortex-M0 +# and Cortex-M23 execution-profile paths: invalid on Armv6-M and Armv8-M +# Baseline, where the 16-bit POP takes r0-r7 and pc only, and rejected by GNU as +# well as by LLVM. Turning the feature on had never once been tried. +FEATURE_MACROS="TX_ENABLE_VFP_SUPPORT TX_LOW_POWER TX_ENABLE_EXECUTION_CHANGE_NOTIFY" + +# Extra flags for the VFP paths, per core, needed only where -mcpu alone cannot +# assemble them. Cortex-R4's FPU is an option rather than part of the core, so +# both toolchains reject its VFP code without an -mfpu. +# +# Do not extend this to the A profile ports. They save D16-D31, which exists only +# on a 32-register FPU, so naming a -d16 FPU takes those registers away and turns +# 28 working files into "register expected". Their defaults are already correct. +declare -A VFP_EXTRA=( + [cortex_r4]="-mfpu=vfpv3-d16 -mfloat-abi=softfp" +) + # One core per architecture profile for the C sources. Compiling all of them # for every core would multiply the run time without adding coverage, since the # port headers differ by profile rather than by core. @@ -200,6 +222,46 @@ if [ -n "$skipped" ]; then say " not Arm, skipped:$(echo $skipped | tr ' ' '\n' | sort -u | tr '\n' ' ')" fi +# -------------------------------------------------------------------------- +say "" +say "== Assembly behind feature macros ==" + +for macro in $FEATURE_MACROS; do + macro_total=0 + macro_bad=0 + for src in $(grep -rl "$macro" ports/*/gnu/src/*.S ports_smp/*/gnu/src/*.S \ + 2>/dev/null | sort); do + core="$(echo "$src" | cut -d/ -f2)" + spec="${PORT_TARGET[$core]:-}" + [ -n "$spec" ] || continue + # shellcheck disable=SC2086 + set -- $spec + target="$1"; cpu="$2"; shift 2; extra="$*" + + # The FPU flags apply to the VFP paths only; the other macros guard no + # floating-point code and do not need them. + fpu="" + if [ "$macro" = "TX_ENABLE_VFP_SUPPORT" ]; then + fpu="${VFP_EXTRA[$core]:-}" + fi + + macro_total=$((macro_total + 1)) + output="$("$CC" --target="$target" -mcpu="$cpu" $extra $fpu \ + -D"$macro" -c "$src" -o /dev/null 2>&1)" + if [ -n "$output" ]; then + fail "$src with -D$macro" + echo "$output" | grep "error:" | head -3 | sed 's/^/ /' + macro_bad=$((macro_bad + 1)) + failures=$((failures + 1)) + fi + done + if [ "$macro_total" -eq 0 ]; then + say " $macro: no assembly is guarded by it" + else + say " $macro: $((macro_total - macro_bad)) of $macro_total assembled" + fi +done + # -------------------------------------------------------------------------- if [ "$asm_only" -eq 0 ]; then say ""