Skip to content

Assembled the module ports, which no check had ever compiled - #672

Open
fdesbiens wants to merge 1 commit into
eclipse-threadx:devfrom
fdesbiens:fix/module-port-asm-coverage
Open

Assembled the module ports, which no check had ever compiled#672
fdesbiens wants to merge 1 commit into
eclipse-threadx:devfrom
fdesbiens:fix/module-port-asm-coverage

Conversation

@fdesbiens

Copy link
Copy Markdown
Contributor

scripts/check_clang.sh:214 globbed ports_module/*/gnu/src, which does not exist — the module ports keep their assembly in module_manager/src. The [ -d ] guard skipped it in silence, so 116 assembly files across nine Arm module ports were assembled by no check, with either compiler — in the script whose own comments state three times that "a port that is simply absent from the count reads as covered".

The feature-macro stage had the same gap for the same reason.

stage before after
1 — assembly of every Arm gnu port 724 of 724 840 of 840
2 — assembly behind feature macros 412 files 469 files

What the corrected path exposed

Five defects. Only one of them was a build failure; the other four assembled cleanly and did the wrong thing, because GAS runs the C preprocessor on .S and not on .s. In a .s file every # line is a comment, so a #define constant is never substituted, an #ifdef block is assembled whatever the macro says, and an #if/#else pair emits both branches.

ports_smp/cortex_a7_smp/gnu/src/tx_thread_smp_unprotect.s — the worst of the five, and it is not a module port at all. The only .s in a directory of twenty-one .S. It ignored all four of its own feature macros: wrote the caller's LR into the protection structure on every unprotect (a store guarded by TX_MPCORE_DEBUG_ENABLE), sent an unconditional SEV, disabled FIQ whether or not TX_ENABLE_FIQ_SUPPORT was set, and returned through both BX lr and MOV pc, lr. Its cortex_a5_smp and cortex_a9_smp siblings are .S.

ports_module/cortex_m33/.../tx_thread_stack_build.s — emitted both arms of #ifdef TX_SINGLE_MODE_SECURE, so the non-secure LR value overwrote the secure one and a secure build got the wrong initial frame.

ports_module/cortex_m23/.../tx_thread_context_{save,restore}.S — carried the POP {r0, lr} that check_clang.sh's own FEATURE_MACROS comment cites as the reason that stage exists. The 16-bit Thumb POP takes r0–r7 and pc only. The identical fix, comment and all, already sits in ports/cortex_m23/gnu/src; the module copy never got it because nothing scanned it.

ports_module/cortex_m23/.../tx_thread_secure_stack_initialize.SMOV rather than MOVS for an 8-bit immediate, latent behind TX_SINGLE_MODE_SECURE. Both siblings in the same directory already use MOVS. GCC quietly widened it to a 4-byte movw; LLVM rejects it.

ports_module/cortex_a7/gnu/module_manager/src — the one that actually failed to assemble, on GCC 14.3 as well as on LLVM: #define SYS_MODE 0x1F was never expanded, so #SYS_MODE reached the assembler as an undefined symbol. The Cortex-A7 GNU module manager has never assembled with the project's own default compiler.

The fix, and why renaming rather than converting the #defines

Twenty-nine .s files under gnu trees become .S.

Every one of them is already named .S by the build scripts that compile it, so this repairs those scripts rather than churning them. ports_module/cortex_a7/gnu/example_build/build_threadx.bat names all eighteen module_manager/src files with a capital S, and works today only on a case-insensitive filesystem.

Converting the #defines to GNU = assignments — the other candidate fix — would have made two files assemble and left twenty-seven silently ignoring their macros. The #ifdef blocks are the larger half of the defect and only the rename addresses them.

Files carrying no preprocessor directives are left as .s: they are not broken. check_ports.sh gains a fourth check that keeps them that way, in the file whose header says each check exists because a real defect reached the repository through it. Only the gnu trees are checked — the IAR, Arm Compiler 5 and Keil assemblers preprocess .s themselves, and around three hundred files here rely on that.

Verification

Both toolchains, same tree:

  • check_clang.sh with ATfE 22.1.0: 840 of 840 assembled, 469 macro-guarded files, 185 of 185 C sources for each of nine cores, 42 of 42 script example builds, 5 of 5 CMake images. All five stages green.
  • The same stage-1 walk with arm-gnu-toolchain 14.3.rel1 (arm-none-eabi and aarch64-none-elf): 840 of 840.
  • check_ports.sh green including the reproducibility check, which is what proves none of the renamed files is generated from ports_arch/.
  • The new check was shown to fail, by planting a copy of the file it was written for: it names the file, the line and the directive.

Regression tests

None accompanies this, deliberately. The assembly it covers is executed by no host test. The check going from 724 files to 840 is the coverage AGENTS.md asks for, together with the new check_ports.sh section — which is what stops the class recurring rather than just this instance.

Why now

scripts/check_gcc.sh is next, and it mirrors this script stage for stage. Written against the old glob it would inherit the same hole; written against the corrected one it fails on the Cortex-A7 pair on its first run. Landing this first makes its stage 1 green by construction. PR #639 is also a Cortex-R52 module port — it lands in ports_module/ and, as things stood, would have been assembled by nothing at all.

scripts/check_clang.sh globbed ports_module/*/gnu/src, which does not exist --
the module ports keep their assembly in module_manager/src. The [ -d ] guard
skipped it in silence, so 116 assembly files across nine Arm module ports were
assembled by no check, with either compiler, in the script whose own comments
state three times that "a port that is simply absent from the count reads as
covered". Stage 1 goes from 724 of 724 to 840 of 840; the feature-macro stage
had the same gap and goes from 412 files to 469.

Correcting the path exposed five defects, and only one of them was a build
failure. The other four assembled cleanly and did the wrong thing, because GAS
runs the C preprocessor on .S and not on .s:

  ports_smp/cortex_a7_smp/gnu/src/tx_thread_smp_unprotect.s, the only .s in a
  directory of twenty-one .S, ignored all four of its own feature macros. It
  wrote the caller's LR into the protection structure on every unprotect -- a
  store guarded by TX_MPCORE_DEBUG_ENABLE -- sent an unconditional SEV, and
  returned through both BX lr and MOV pc, lr. Its cortex_a5_smp and
  cortex_a9_smp siblings are .S.

  ports_module/cortex_m33/.../tx_thread_stack_build.s emitted both arms of an
  #ifdef TX_SINGLE_MODE_SECURE, so the non-secure LR value overwrote the secure
  one and the secure build got the wrong frame.

  ports_module/cortex_m23/.../tx_thread_context_{save,restore}.S carried the
  POP {r0, lr} that check_clang.sh's own comment describes as the reason the
  feature-macro stage exists. The 16-bit Thumb POP takes r0-r7 and pc only.
  The identical fix already sits in ports/cortex_m23/gnu/src; the module copy
  never got it because nothing scanned it.

  ports_module/cortex_m23/.../tx_thread_secure_stack_initialize.S used MOV
  rather than MOVS for an 8-bit immediate, latent behind TX_SINGLE_MODE_SECURE.
  Both siblings in the same directory already use MOVS.

  ports_module/cortex_a7/gnu/module_manager/src is the one that failed to
  assemble, on GCC 14.3 as well as on LLVM: #define SYS_MODE was never
  expanded, so #SYS_MODE reached the assembler as an undefined symbol.

Twenty-nine .s files under gnu trees are renamed to .S. Every one of them is
already named .S by the build scripts that compile it, so this repairs those
scripts rather than churning them -- ports_module/cortex_a7's build_threadx.bat
names all eighteen with a capital S, and works today only on a case-insensitive
filesystem. Renaming rather than converting the #defines to GNU assignments is
what fixes the #ifdef blocks as well as the constants; the assignments would
have fixed two files and left twenty-seven silently ignoring their macros.

Files with no preprocessor directives are left as .s: they are not broken, and
check_ports.sh gains a check that keeps them that way. Only the gnu trees are
checked there -- the IAR, Arm Compiler 5 and Keil assemblers preprocess .s
themselves, and about three hundred files in this repository rely on that.

Verified with both toolchains on the same tree: 840 of 840 assembled by
ATfE 22.1.0 and by arm-gnu-toolchain 14.3.rel1, all five stages of
check_clang.sh green, and check_ports.sh green including the reproducibility
check. The new check was shown to fail by planting a copy of the file it was
written for.

No regression test accompanies this. The assembly it covers is executed by no
host test, and the check itself going from 724 files to 840 is the coverage
AGENTS.md asks for -- together with the new check_ports.sh section, which is
what stops the class recurring.

Assisted-by: Claude Code (Opus 5) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant