Skip to content

fix(toolchain): master cannot link a Cortex-M4 executable — nosys.specs is passed twice - #126

Merged
srpatcha merged 6 commits into
embeddedos-org:masterfrom
Kartikey1306:fix/arm-m4-duplicate-specs
Sep 8, 2026
Merged

srpatcha merged 6 commits into
embeddedos-org:masterfrom
Kartikey1306:fix/arm-m4-duplicate-specs

Conversation

@Kartikey1306

@Kartikey1306 Kartikey1306 commented Sep 2, 2026 •

Copy link
Copy Markdown
Contributor

master cannot link a Cortex-M4 executable. toolchains/arm-cortex-m4.cmake
supplies -specs=nosys.specs twice — once on CMAKE_C_FLAGS_INIT, once on
CMAKE_EXE_LINKER_FLAGS_INIT — and CMake links through the compiler driver, so
both reach the link line:

arm-none-eabi-gcc: fatal error: .../nosys.specs: attempt to rename spec
'link_gcc_c_sequence' to already defined spec 'nosys_link_gcc_c_sequence'

This was originally written up as a cleanup. It is not. @srpatcha verified it
end to end on a host with arm-none-eabi-gcc 14.2.1 and newlib: with master's
toolchain file a three-line add_executable() project fails to build; with this
one it links. The earlier framing invited reviewers to deprioritise it, so it is
corrected here.

Why CI never noticed, and still would not

Every ARM toolchain here sets CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY
and no cross target links an executable, so Cross-compile ARM Cortex-M4
passes identically with the duplicate present or absent — it passed on the
unfixed commit. The defect surfaced only by accident, when #118 briefly put
cmd/eos into the cross build; #118 has since gated that to host builds, which
removed the accident too.

So the signal is blind to this class, which is why the fix ships with a test.

The change

-set(CMAKE_C_FLAGS_INIT "${CPU_FLAGS} -specs=nosys.specs -ffunction-sections …")
+set(CMAKE_C_FLAGS_INIT "${CPU_FLAGS} -ffunction-sections …")

Removing it from the compile line is provably a no-op: the whole of
nosys.specs is %rename link_gcc_c_sequence plus three *link_* entries —
no *cc1, no *cpp — so it cannot affect an object file.
arm-none-eabi-stm32f4.cmake already did it this way.

arm-none-eabi-r5.cmake moves the same flag from CMAKE_C_FLAGS_INIT to
CMAKE_EXE_LINKER_FLAGS_INIT. Behaviour-preserving — it reached the link line
once through the driver before and reaches it once directly now — and it is the
file that would reproduce this bug the moment anyone adds the linker entry that
looks missing.

A trap for later edits: nosys.specs's *nosys_libc chooses -lc vs
-lc_nano by inspecting the command line for specs=nano.specs. The two specs
must therefore stay on the same line; neither can be moved to compile flags
"for symmetry".

The test

tests/unit/test_toolchain_specs.py parses every toolchains/*.cmake and
*.yaml and fails if a spec file reaches the link line more than once, or is
declared on compile flags at all. Modelled on
test_cmake_test_registration.py: fail on the class, not the instance. Needs no
ARM toolchain and runs in the existing pytest job.

It walks each file in order with a variable table, so a spec reaches the checks
however it is written — set() quoted or not, string(APPEND …), or hidden
inside an interpolated ${CPU_FLAGS}, which is how both Cortex-M4 files build
their flag strings.

Parser output on the current tree:

arm-cortex-m4.cmake         CMAKE_EXE_LINKER_FLAGS_INIT: [nosys.specs, nano.specs]
arm-none-eabi-r5.cmake      CMAKE_EXE_LINKER_FLAGS_INIT: [nosys.specs]
arm-none-eabi-stm32f4.cmake CMAKE_EXE_LINKER_FLAGS_INIT: [nano.specs, nosys.specs]

Mutations caught: the original duplicate re-introduced; a spec hidden in
CPU_FLAGS; and a parser that sees nothing at all
(test_every_arm_toolchain_is_actually_parsed, added after my first regex
silently failed to read stm32f4's three-line set() and every check passed
vacuously for that file).

Known limit

The strongest test would configure a throwaway add_executable() and inspect
the generated link line — it would have caught this directly rather than by
pattern. It is not here because it needs arm-none-eabi-gcc on the runner,
which the pytest job does not install, so it would skip on CI and run only where
someone already has the toolchain. A test that is green because it did not run
is worse than the pattern check. Worth a follow-up that installs the toolchain.

CI

Two reds, both inherited, neither from this branch:

An earlier revision of this branch carried #127's registration hunk, which made
that check green here and is why the 2026-09-02 comments read as they do. It has
been removed: folding a master repair into a toolchain PR to get a green tick is
how a red master becomes everyone's problem to route around.

Verification

item result
pytest tests/unit/test_toolchain_specs.py PASS — 7 passed
ctest PASS — 38/38
link an executable with master's toolchain FAILS (verified by @srpatcha, gcc 14.2.1 + newlib)
link the same with this branch's PASS

@Kartikey1306

Copy link
Copy Markdown
Contributor Author

Both failing checks here (Run Python tests and Build & Test (Linux x86_64)) are inherited from master, not caused by this change. They share one root cause:

these suites exist in tests/ but no add_executable() in tests/CMakeLists.txt
builds them, so they never run: ['test_crypto_ed25519_loworder.c']

3aa8644 (#93, "remove duplicate test targets") removed the only registration of test_crypto_ed25519_loworder — there was exactly one before that commit, so it was not a duplicate. The suite guarding #101's low-order key rejection has not been built since.

Fixed in #127, which restores those four lines verbatim. Once that merges, rebase this branch and both checks should go green without any change to the toolchain fix itself.

Nothing to do on this PR.

@Kartikey1306

Copy link
Copy Markdown
Contributor Author

This red is inherited from master, not caused by this PR. #128 fixes it.

The failing check is Run Python tests on Build & Test (Linux x86_64):

FAILED tests/unit/test_cmake_test_registration.py::test_every_c_suite_is_built_or_listed
AssertionError: these suites exist in tests/ but no add_executable() in
tests/CMakeLists.txt builds them, so they never run:
['test_crypto_ed25519_loworder.c']

This PR changes one file, toolchains/arm-cortex-m4.cmake, and does not touch
tests/ at all — so it cannot have caused a missing test registration, and
cannot fix it either.

Cause: #93 removed the test_crypto_ed25519_loworder target. Its base predated
#101, which had registered it, so it read as one more duplicate to delete. It
was the only non-duplicate in that diff. #128 restores it; master goes green
and this PR should go green on a rerun once that lands.

Nothing to change here. Worth a rerun after #128 merges rather than
investigating the toolchain change.

Two asides on this PR's own subject, since I was in toolchains/ anyway:

  • Cross-compile ARM Cortex-M4 passed on this commit, so the duplicate
    nosys.specs was not breaking the link — the change is a cleanup, which is
    worth saying explicitly in the description so a reviewer does not go looking
    for the failure it fixed.
  • If you want evidence for the description, arm-none-eabi-gcc -v output shows
    the spec file being read twice; capturing that before/after would make the
    change self-evidencing. I do not have an ARM toolchain on this machine, so I
    cannot produce it for you.

@srpatcha srpatcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — eos#126 "fix(toolchain): stop passing nosys.specs twice on a Cortex-M4 link"

head: 41280da author: Kartikey1306 ci: pass

Verdict: The toolchain change is correct — -specs= belongs on the link line only,
arm-none-eabi-stm32f4.cmake already does it that way, and the dup.specs reproduction is
real evidence rather than reasoning. But the head no longer changes one file: it also
carries the test_crypto_ed25519_loworder registration, which the body does not mention
and which directly contradicts this PR's own comment of 2026-09-02. And the defect being
fixed still has nothing that would catch it coming back.

Findings

# Severity File:line Finding Recommended fix
1 Medium tests/CMakeLists.txt:123-129 Undisclosed second change. files.txt for this head is 7+ 0- tests/CMakeLists.txt and 14+ 1- toolchains/arm-cortex-m4.cmake; the body describes only the second. This also makes the PR's own comment at 2026-09-02T14:06 false against the head it now points at — "This PR changes one file, toolchains/arm-cortex-m4.cmake, and does not touch tests/ at all — so it cannot have caused a missing test registration, and cannot fix it either." It now does both. The hunk is the fourth open copy of that registration (#125, #127, #118, and per your comment #128), inserted at the #125 position rather than the original position #127 restores, so it will conflict with #127. Drop the hunk and let #127 land. Your own comment already prescribes this — "worth a rerun after #128 merges rather than investigating the toolchain change" — and folding a master repair into a toolchain PR to get a green tick is the thing that makes a red master everyone's problem to route around. If it stays, say so in the body and mark the stale comment.
2 Medium toolchains/arm-cortex-m4.cmake:19 Nothing will catch this coming back. The PR's own analysis is why: CMAKE_TRY_COMPILE_TARGET_TYPE is STATIC_LIBRARY and nothing in the cross build links an executable, so Cross-compile ARM Cortex-M4 passes identically with the duplicate present or absent — it passed on the unfixed commit, as noted in your 2026-09-02 comment. The defect was found by accident when #118 briefly put cmd/eos into the cross build, and #118 has since gated that to host builds, which removes the accident too. So after this merges, the repository is back to having no way to notice. This repo already has the right pattern for exactly this: tests/unit/test_cmake_test_registration.py parses a build file and fails on a class of mistake. Add a sibling — parse every toolchains/*.cmake, extract each -specs=<file>, and fail if the same specs file appears in more than one variable that reaches a link line (CMAKE_C_FLAGS_INIT, CMAKE_CXX_FLAGS_INIT, CMAKE_EXE_LINKER_FLAGS_INIT). Roughly twenty lines, needs no ARM toolchain, and runs in the existing pytest job.
3 Low toolchains/arm-none-eabi-r5.cmake:21-23 After this PR, two of the three ARM bare-metal toolchain files agree that -specs= lives on the link line; the third still does not. arm-none-eabi-r5.cmake puts --specs=nosys.specs in CMAKE_C_FLAGS_INIT (:21), copies it into CMAKE_CXX_FLAGS_INIT (:22), and leaves CMAKE_EXE_LINKER_FLAGS_INIT (:23) with no specs at all. That is not broken today — it reaches the link line exactly once, via the same "CMake links through the compiler driver" coupling the body describes — but it is the file that reproduces this bug the moment anyone adds a -specs= to its linker flags, which is the natural thing to do when an executable first fails to link. Move it in the same PR: CMAKE_C_FLAGS_INIT loses --specs=nosys.specs, CMAKE_EXE_LINKER_FLAGS_INIT gains -specs=nosys.specs. Same one-line move, same justification, and it makes finding 2's check trivially green across all three files.

The toolchain fix itself is correct and I checked the surrounding files rather than taking
the body's word: arm-none-eabi-stm32f4.cmake:36-38 does keep both -specs= entries in
CMAKE_EXE_LINKER_FLAGS_INIT and nowhere else, so "this matches it" is accurate. The
dup.specs before/after reproduction is the right kind of evidence for a change whose real
target cannot be exercised locally, and the caveat — Homebrew arm-none-eabi-gcc ships no
newlib, so the full cross build could not run — is stated plainly instead of being papered
over. Two of the asks in the 2026-09-02 review comment (say plainly that this is a cleanup
rather than a fix for an observed failure; capture compiler output) are answered in the
current body.

No check is weakened: nothing disabled, no assertion removed, no || true added. All 26
required checks pass in checks.txt for this head — note that the two reds the earlier
comments discuss are green here because of the undisclosed hunk in finding 1, not
because master was repaired.

Architecture conformance

§21 — conforms. toolchains/ is target adaptation; .ai/architect.md names
toolchains/ in the reference layout and nothing here crosses a tier or changes a
dependency direction. §9.2's "reproducible lockfiles/manifests for production builds" and
"per-target flash/RAM reports" both depend on the cross link working at all, so this sits
underneath them rather than against them.

§22 Hardware support tiers — relevant, and worth naming. Cortex-M4 is one of the
architectures the MVP targets (§25.1, "three excellent reference hardware targets"), and
§22 makes even the Community tier require "build status". A toolchain file whose
executable link path has never been exercised by CI does not meet that, and finding 2 is
the cheap partial answer. The real answer is an executable in the cross build.

No master-design proposal from this PR; §22 already says what is needed here.

Proposed changes

  1. Drop the tests/CMakeLists.txt hunk; let #127 land and rebase.
  2. Move --specs=nosys.specs in arm-none-eabi-r5.cmake from :21 to :23.
  3. Add tests/unit/test_toolchain_specs.py asserting no -specs=<file> appears in more
    than one link-reaching flag variable per toolchain file. Land it after 1 and 2 so it is
    green on arrival.

All three are independent of each other and none changes what the toolchain fix does.

Not checked

  • I did not run a cross build, and could not: doing so would leave the repository dirty,
    which this run must not do. The dup.specs reproduction and the compile-command table
    are the author's.
  • The fix is unverified end to end by anyone, including CI — that is finding 2, not an
    omission of this review. Cross-compile ARM Cortex-M4 passing on this head proves the
    cross build still configures and compiles; it cannot prove anything about the link line,
    because it links no executable.
  • I read arm-cortex-m4.cmake, arm-none-eabi-stm32f4.cmake and arm-none-eabi-r5.cmake.
    I did not read aarch64-linux-gnu.cmake, arm-linux-gnueabihf.cmake or
    riscv64-linux-gnu.cmake; grep -n "specs=" toolchains/*.cmake returns nothing for
    those three, so they are not affected by this specific defect, but I did not review them
    for anything else.
  • Whether removing -specs=nosys.specs from the compile line changes any diagnostic or
    built-in define for the M4 target. nosys.specs supplies link-time stubs, so it should
    not, but I did not diff the preprocessor output before and after.

Automated architecture review of 41280da7aaf0 — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.

Kartikey1306 added a commit to Kartikey1306/eos that referenced this pull request Sep 3, 2026
…ARM file

Answers the review on embeddedos-org#126.

Finding 1 (Medium) -- the head carried an undisclosed second change, the
test_crypto_ed25519_loworder registration, which contradicted this PR's own
2026-09-02 comment ("this PR changes one file ... and does not touch tests/").
It was also the fourth open copy of that registration, at a different position
from embeddedos-org#127's. Dropped: the branch is rebased to the toolchain change alone, and
embeddedos-org#127 is the one to take for the registration -- it restores the four lines to
their original position, keeps the crypto suites grouped, and is the position
embeddedos-org#122 is already written against.

Finding 2 (Medium) -- nothing would catch this coming back. Every ARM
toolchain here sets CMAKE_TRY_COMPILE_TARGET_TYPE to STATIC_LIBRARY and no
cross build links an executable, so `Cross-compile ARM Cortex-M4` passed
identically with the duplicate present and absent; it passed on the unfixed
commit. The defect surfaced by accident when embeddedos-org#118 briefly put cmd/eos into the
cross build, and embeddedos-org#118 has since gated that to host builds -- removing the
accident too.

Adds tests/unit/test_toolchain_specs.py, modelled on
test_cmake_test_registration.py: parse every toolchains/*.cmake, extract each
-specs=<file>, and fail if one appears in more than one variable that reaches
a link line (CMake links through the compiler driver, so C/CXX flags do). A
second test asserts specs are declared on the linker flags rather than the
compiler flags, which is the arrangement that turns into a duplicate the
moment someone adds the linker entry that looks missing. No ARM toolchain
needed; runs in the existing pytest job.

Finding 3 (Low) -- arm-none-eabi-r5.cmake put --specs=nosys.specs in
CMAKE_C_FLAGS_INIT with nothing in CMAKE_EXE_LINKER_FLAGS_INIT. Not broken
today, and one added linker -specs= away from the same failure. Moved, so all
three ARM bare-metal toolchains state it the same way and the new guard is
green across all of them.

Verified:
  pytest tests/unit/test_toolchain_specs.py    2 passed
  pytest tests/                                all pass
  discrimination: reintroducing the duplicate in arm-cortex-m4.cmake gives
      arm-cortex-m4.cmake: -specs=nosys.specs is set in CMAKE_C_FLAGS_INIT
      and CMAKE_EXE_LINKER_FLAGS_INIT, so it reaches the link line more than
      once
    on both tests, so the guard catches the original defect rather than
    restating the fix.
  No ARM toolchain on this machine, so the cross link itself is NOT verified
  here; that remains CI's to show.

Refs embeddedos-org#126
@Kartikey1306
Kartikey1306 force-pushed the fix/arm-m4-duplicate-specs branch from 41280da to 3cf4eaf Compare September 3, 2026 09:59
Kartikey1306 added a commit to Kartikey1306/eos that referenced this pull request Sep 3, 2026
…ARM file

Answers the review on embeddedos-org#126.

Finding 1 (Medium) -- the head carried an undisclosed second change, the
test_crypto_ed25519_loworder registration, which contradicted this PR's own
2026-09-02 comment ("this PR changes one file ... and does not touch tests/").
It was also the fourth open copy of that registration, at a different position
from embeddedos-org#127's. Dropped: the branch is rebased to the toolchain change alone, and
embeddedos-org#127 is the one to take for the registration -- it restores the four lines to
their original position, keeps the crypto suites grouped, and is the position
embeddedos-org#122 is already written against.

Finding 2 (Medium) -- nothing would catch this coming back. Every ARM
toolchain here sets CMAKE_TRY_COMPILE_TARGET_TYPE to STATIC_LIBRARY and no
cross build links an executable, so `Cross-compile ARM Cortex-M4` passed
identically with the duplicate present and absent; it passed on the unfixed
commit. The defect surfaced by accident when embeddedos-org#118 briefly put cmd/eos into the
cross build, and embeddedos-org#118 has since gated that to host builds -- removing the
accident too.

Adds tests/unit/test_toolchain_specs.py, modelled on
test_cmake_test_registration.py: parse every toolchains/*.cmake, extract each
-specs=<file>, and fail if one appears in more than one variable that reaches
a link line (CMake links through the compiler driver, so C/CXX flags do). A
second test asserts specs are declared on the linker flags rather than the
compiler flags, which is the arrangement that turns into a duplicate the
moment someone adds the linker entry that looks missing. No ARM toolchain
needed; runs in the existing pytest job.

Finding 3 (Low) -- arm-none-eabi-r5.cmake put --specs=nosys.specs in
CMAKE_C_FLAGS_INIT with nothing in CMAKE_EXE_LINKER_FLAGS_INIT. Not broken
today, and one added linker -specs= away from the same failure. Moved, so all
three ARM bare-metal toolchains state it the same way and the new guard is
green across all of them.

Verified:
  pytest tests/unit/test_toolchain_specs.py    2 passed
  pytest tests/                                all pass
  discrimination: reintroducing the duplicate in arm-cortex-m4.cmake gives
      arm-cortex-m4.cmake: -specs=nosys.specs is set in CMAKE_C_FLAGS_INIT
      and CMAKE_EXE_LINKER_FLAGS_INIT, so it reaches the link line more than
      once
    on both tests, so the guard catches the original defect rather than
    restating the fix.
  No ARM toolchain on this machine, so the cross link itself is NOT verified
  here; that remains CI's to show.

Refs embeddedos-org#126
@Kartikey1306
Kartikey1306 force-pushed the fix/arm-m4-duplicate-specs branch from 3cf4eaf to 126b0cd Compare September 3, 2026 09:59
…removed

`CI — eos` and `Python Unit Tests` are red on master (769191a), and both
fail for one reason:

    these suites exist in tests/ but no add_executable() in
    tests/CMakeLists.txt builds them, so they never run:
    ['test_crypto_ed25519_loworder.c']

3aa8644 (embeddedos-org#93, "remove duplicate test targets from tests/CMakeLists.txt")
removed all four lines that build and register
test_crypto_ed25519_loworder. It was not a duplicate — there was exactly
one registration before that commit, and it took it.

What stopped running is the suite that guards embeddedos-org#101: Ed25519 rejecting
low-order public keys. A low-order key makes every term of the
verification equation collapse to the identity regardless of the
message, so a signature of all zeros verifies against any content at
all. The fix is still in services/crypto/src/ed25519_verify.c and still
correct; nothing has been checking it since embeddedos-org#93 merged, and nothing
would have noticed if a later change had removed it too.

Restored verbatim from 3aa8644^, in its original position before
test_crypto_sha512. No other file changes.

The guard that caught this is test_cmake_test_registration.py, added
recently. It did exactly its job — this is the first thing it found.

Verified:
  test_cmake_test_registration.py    -> 5 passed (1 failed on master)
  pytest tests/                      -> 15 passed (14 passed 1 failed on master)
  test_crypto_ed25519_loworder       -> 5/5 tests passed
  ctest                              -> 39/39 passed (master builds 38)

@srpatcha srpatcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — eos#126 "fix(toolchain): stop passing nosys.specs twice on a Cortex-M4 link"

head: 126bd0c9271f9656b2c8951551053799b517c10a author: Kartikey1306 ci: fail (both failures inherited from eBoot master — see CI)

Verdict: Correct, and it is not the cleanup the PR body settles for calling it. This host has arm-none-eabi-gcc 14.2.1 with newlib, so I ran the check the author said they could not: with master's arm-cortex-m4.cmake, linking any executable fails outright; with this PR's, it links. The new test is the right instrument but it closes one CMake idiom rather than the class its docstring claims.

The verification the body says it could not do — done, with the real toolchain

The caveat in the body is "the full cross build could not be run locally — the Homebrew arm-none-eabi-gcc ships no newlib, so nosys.specs and even stdint.h are absent". This host has both:

arm-none-eabi-gcc (15:14.2.rel1-1) 14.2.1 20241119
nosys.specs -> /usr/lib/gcc/arm-none-eabi/14.2.1/../../../arm-none-eabi/lib/nosys.specs

1. The failure reproduces on the real spec file, not a stand-in. The body used a substitute dup.specs; this is nosys.specs itself:

once:   arm-none-eabi-gcc -mcpu=cortex-m4 -mthumb m.c -specs=nosys.specs -o once.elf
        -> links
twice:  arm-none-eabi-gcc -mcpu=cortex-m4 -mthumb -specs=nosys.specs m.c -specs=nosys.specs -o twice.elf
        -> fatal error: .../nosys.specs: attempt to rename spec 'link_gcc_c_sequence'
           to already defined spec 'nosys_link_gcc_c_sequence'

2. End to end through CMake, with the repo's own toolchain files. Same three-line CMake project (add_executable(app main.c)), same compiler, only the toolchain file differs:

master  toolchains/arm-cortex-m4.cmake   configure OK, build:
          arm-none-eabi-gcc: fatal error: .../nosys.specs: attempt to rename spec …
          gmake: *** [Makefile:91: all] Error 2
this PR toolchains/arm-cortex-m4.cmake   configure OK, build:
          [100%] Built target app

So on master the Cortex-M4 toolchain cannot link an executable at all. That is worth putting in the description: the body's "Why it has never bitten" is accurate about this repository's cross build, but the file is broken for anyone who adds an executable, and #118 hitting it was not luck.

3. Removing it from the compile line is provably a no-op, which the body asserts and I can now show. The whole of nosys.specs is:

%rename link_gcc_c_sequence  nosys_link_gcc_c_sequence
*nosys_libgloss:  -lnosys
*nosys_libc:      %{!specs=nano.specs:-lc} %{specs=nano.specs:-lc_nano}
*link_gcc_c_sequence: %(nosys_link_gcc_c_sequence) --start-group %G %(nosys_libc) %(nosys_libgloss) --end-group

Nothing but *link_gcc_c_sequence — no *cc1, no *cpp. It has no effect on a compile line, so dropping it there changes no object file.

4. The arm-none-eabi-r5.cmake change really is behaviour-preserving, as claimed. Both versions link the test executable successfully ([100%] Built target app on each), which is what you would expect: nosys.specs reached the link line through the driver before and reaches it directly now, once either way.

One incidental note from nosys.specs: *nosys_libc switches between -lc and -lc_nano by inspecting the command line for specs=nano.specs. In arm-cortex-m4.cmake both specs sit on CMAKE_EXE_LINKER_FLAGS_INIT together, so -lc_nano is selected, before and after. This PR does not disturb it — worth knowing only because it means the two specs must stay on the same line, so neither can be moved to compile flags "for symmetry" later.

Findings

# Severity File:line Finding Recommended fix
1 Medium tests/unit/test_toolchain_specs.py:41-52 The docstring's aim is to "fail on the class of mistake rather than on one instance of it", and _specs_per_variable recognises exactly one idiom: set(<VAR> "…") with a quoted literal. It cannot see string(APPEND CMAKE_EXE_LINKER_FLAGS_INIT " -specs=…") — the standard way to add to these variables — nor an unquoted set(), nor add_compile_options/add_link_options, nor a spec arriving through an interpolated variable. The last is not hypothetical here: arm-cortex-m4.cmake:32 and arm-none-eabi-stm32f4.cmake:36-38 both build their flag strings from ${CPU_FLAGS}, and stm32f4's CMAKE_EXE_LINKER_FLAGS_INIT is "${CPU_FLAGS} … -specs=nano.specs -specs=nosys.specs -lnosys". Put -specs= into CPU_FLAGS and both new tests pass while both lines carry it. Today's tree is covered — I ran the parser over all six toolchain files and it correctly reports the specs for all three ARM ones, including stm32f4's multi-line set() — so this is about the next edit, not this one. Two options. Cheap: expand ${…} from earlier set()s in the same file before scanning, and add string(APPEND …) to the patterns. Better and idiom-independent: configure each toolchain file against a throwaway add_executable() and assert the generated link line names each spec at most once. As this review demonstrates, that runs on any host with the toolchain installed — and it would have caught the defect directly rather than by pattern.
2 Low tests/unit/test_toolchain_specs.py:27; toolchains/arm-none-eabi.yaml:9, arm-none-eabi-r5.yaml:9 TOOLCHAINS.glob("*.cmake") covers one of the two authorities on these flags. The .yaml toolchain descriptors carry the same information — arm-none-eabi-r5.yaml:9 and arm-none-eabi.yaml:9 both read ldflags: -specs=nosys.specs -specs=nano.specs — and are parsed into EosToolchain.cflags/ldflags by toolchains/src/toolchain.c:60-64. No live duplicate: that is the eos CLI's own toolchain model, a separate consumer from CMake, so nothing combines the two. But it means a .yaml can drift to name a spec in cflags and no test notices, and after this PR nosys.specs for the r5 target is declared in two files that nothing keeps in step. Worth noting in the PR's favour: all five .yaml descriptors already keep specs in ldflags only — the YAML side had the convention right and the .cmake files were the outliers, which is a good argument for the change and a better one for scanning both. Extend the scan to toolchains/*.yaml, treating cflags/ldflags as the equivalent of LINK_REACHING, so both descriptions of the same toolchain are held to one rule.
3 Low tests/unit/test_toolchain_specs.py:33-37 LINK_REACHING omits CMAKE_ASM_FLAGS_INIT, which both arm-cortex-m4.cmake:33 and arm-none-eabi-stm32f4.cmake:30 set from ${CPU_FLAGS}. CMake picks a target's linker language from its sources, so an executable whose sources are all .S links with CMAKE_ASM_FLAGS — a spec named there plus one in CMAKE_EXE_LINKER_FLAGS_INIT is the same duplicate, invisible to the scan. No such target exists today; a bare-metal tree acquires them. Add "CMAKE_ASM_FLAGS_INIT" to LINK_REACHING. One line, and the current tree still passes — verified: no toolchain file names a spec in ASM flags.
4 Low tests/CMakeLists.txt:93-96; comments of 2026-09-02 Two things. The test_crypto_ed25519_loworder registration hunk is carried here, as it is by #118, #119 and #122 — a four-way conflict at one anchor, with #125 being the PR whose job it is. And the two comments name different fixing PRs: 14:05 says "Fixed in #127, which restores those four lines verbatim", 14:06 says "#128 fixes it" and "#128 restores it". Both cannot be right, and whichever is wrong is the pointer the next reader follows. Land #125 (or whichever of #127/#128 is real), then rebase and drop the duplicated hunk here and in #118/#119/#122. Correct or delete the superseded comment — both are already stale, since Run Python tests and Build & Test (Linux x86_64) are green on this head precisely because the branch now carries the registration.

CI

Two red, neither caused by this PR, and both differ from what the 2026-09-02 comments describe — those predate the current head.

  • Cross-compile ARM64 kernel — .github/workflows/eos-simulation.yml checks out embeddedos-org/eBoot@master and builds it. eBoot's master does not compile: eBoot/include/eos_image.h:135,142, error: 'eos_image_header_t' has no member named 'reserved'. Reproduced in a clean worktree; three pre-existing errors from one bad merge, all covered by eBoot#94.
  • Full-stack integration summary — needs: [build-kernel, …], fails downstream. QEMU ARM64 simulation (13 TCs) shows skipping for the same reason.

Run Python tests, Build & Test (Linux x86_64), Cross-compile ARM Cortex-M4, Host Tests (x86_64), QEMU Boot Test, Static Analysis, Analyze (C/C++), CodeQL and the SBOM validation are all green — so the registration failures the comments discuss are already resolved on this head.

Reproduced locally at head 126bd0c:

cmake -S . -B bt -DEOS_BUILD_TESTS=ON && cmake --build bt   # rc=0, no errors
ctest --test-dir bt --no-tests=error                        # 39/39 passed
pytest tests/ -q                                            # 17 passed
plus the four toolchain checks quoted above

Note the author's own aside is confirmed and is the important asymmetry: Cross-compile ARM Cortex-M4 passes identically with the duplicate present and absent, because CMAKE_TRY_COMPILE_TARGET_TYPE is STATIC_LIBRARY in every ARM toolchain here and no cross target links an executable. The CI signal is genuinely blind to this defect, which is exactly why the new test earns its place — and why finding 1 matters, since a pattern-matching test is the only guard and it guards one pattern.

Architecture conformance

Conforms.

  • §9.2, "Reproducible lockfiles/manifests for production builds" and §7.1's separation of architecture/SoC/board concerns — the change moves in the right direction: one statement of a link-time flag, in the variable that reaches the link line, identical across all three ARM bare-metal toolchains. arm-none-eabi-stm32f4.cmake was already the model and the other two now match it. Finding 2 is that the .yaml descriptors are a fourth statement of the same fact.
  • §22 hardware support tiers — not engaged, but relevant in one respect: a toolchain file that cannot link an executable would make any Reference-tier claim for a Cortex-M4 board unsupportable, since §22 requires "maintained debugger/flash path" for that tier. This fix is a precondition for that claim rather than a cleanup.
  • §21 / §5.1 — toolchains/ is Tier 1 build adaptation; nothing points up a tier and no dependency is added or moved.
  • Weakened checks — none. Two tests added, no assertion removed, nothing disabled. The .cmake edits remove a flag from a line where it provably has no effect (evidence above), which is a narrowing of the command line, not of a check.

Proposed changes

  1. Nothing blocks this landing. The toolchain fix is correct and now independently verified against a real newlib toolchain, both directions.
  2. Findings 2 and 3 — two small additions to the new test, no behaviour change, current tree stays green.
  3. Finding 1 — worth doing, and the configure-and-inspect-the-link-line version is the one that would actually have caught this. It can be a follow-up; the pattern test is better than nothing in the meantime.
  4. Finding 4 — fix the #127/#128 pointer, and sort the merge order with #125/#118/#119/#122.
  5. Recommend promoting the body's "Why it has never bitten" from a caveat to the headline: master cannot link a Cortex-M4 executable. The current framing invites a reviewer to deprioritise this, and the evidence above says otherwise.

I opened no fix PR: nothing here is rated High, and every finding is inside this PR's own files.

Not checked

  • The repo's own cross build was not run end to end. I linked a synthetic three-line executable with the repo's toolchain files, which isolates exactly the flag under test, but I did not cross-build eos itself — Cross-compile ARM Cortex-M4 in CI covers the static-library path and, as established, is blind to the link path either way.
  • The Cortex-R5 path was exercised only with the same synthetic executable on arm-none-eabi-gcc 14.2.1. No real R5 target, no -nostartfiles linker script, and arm-none-eabi-r5.yaml was read but never fed to toolchains/src/toolchain.c.
  • -specs=nano.specs was not tested in isolation. I confirmed nosys.specs is link-only by reading it; I did not read nano.specs or check whether it has compile-line effects, which matters if anyone later moves it the way this PR moves nosys.specs.
  • arm-none-eabi-stm32f4.cmake was not modified by this PR and was not built. It is cited as the correct model and the parser confirms it declares its specs on the linker flags only; nothing more was verified about it.
  • Only GCC 14.2.1 was used. The duplicate-spec diagnostic is a GCC driver behaviour and older or vendor toolchains may differ in whether it is fatal; the fix is correct regardless, but "master cannot link" is verified for this compiler version, not asserted for all.
  • No .S-only executable was constructed, so finding 3 is reasoned from CMake's linker-language selection rather than observed.

Automated architecture review of 126bd0c9271f — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.

@Kartikey1306

Copy link
Copy Markdown
Contributor Author

All four findings addressed at 0957ac4. Taking the headline recommendation
first, because it changes what this PR is.

The framing was wrong, and your evidence is what shows it

You ran the check I said I could not: with master's arm-cortex-m4.cmake,
linking any Cortex-M4 executable fails outright. That is not a cleanup.
The body's "Why it has never bitten" was accurate about this repository's cross
build and misleading about the file, and framing it as a tidy-up invited exactly
the deprioritisation you flag. I have rewritten the body to lead with it.

Your reading of nosys.specs is the part I could not produce and it settles the
no-op claim properly — the whole file is %rename link_gcc_c_sequence plus
three *link_* entries, no *cc1, no *cpp, so removing it from the compile
line cannot change an object file. That is a stronger argument than mine, which
was "it should not".

Noting your incidental point in the record because it is a trap for a later
edit: *nosys_libc selects -lc vs -lc_nano by inspecting the command line
for specs=nano.specs, so the two specs must stay on the same line and
neither can be moved to compile flags "for symmetry".

4 — the borrowed hunk

Dropped. You are right that it was doing three bad things at once: making
Run Python tests green on a branch with nothing to do with it, contradicting
this PR's own 2026-09-02 comment, and sitting at a different anchor to #127's so
it would have conflicted.

That check is red again on this head, which is the honest state until #127
lands. #125 is closed and #128 I closed as a duplicate, so #127 is the real
one
— that resolves the contradictory pointer you flagged; the 14:06 comment
naming #128 is superseded.

1 — one idiom, not the class

Correct, and the bypass was live rather than theoretical. The parser now walks
the file in order with a variable table, resolves ${...} against what is in
force at that point, and applies string(APPEND ...). Two tests pin the two
bypasses you named.

Found while verifying rather than assuming, and it is this test's own
failure mode one level up: my first regex required the closing paren on the same
line, so arm-none-eabi-stm32f4.cmake's three-line
set(CMAKE_EXE_LINKER_FLAGS_INIT …) was invisible — and every check passed
vacuously for that file. You had verified the old parser handled it; mine
regressed it. Fixed, and now pinned two ways:

  • test_a_multi_line_set_is_still_seen
  • test_every_arm_toolchain_is_actually_parsed — asserts the parser finds specs
    in all three ARM files, so a parser that silently sees nothing can no longer
    pass.

Parser output now matches what you reported:

arm-cortex-m4.cmake        CMAKE_EXE_LINKER_FLAGS_INIT: [nosys.specs, nano.specs]
arm-none-eabi-r5.cmake     CMAKE_EXE_LINKER_FLAGS_INIT: [nosys.specs]
arm-none-eabi-stm32f4.cmake CMAKE_EXE_LINKER_FLAGS_INIT: [nano.specs, nosys.specs]

Mutations caught: a spec hidden in CPU_FLAGS, and the original duplicate
re-introduced on CMAKE_C_FLAGS_INIT.

On your "better" option — configure a throwaway add_executable() and
inspect the generated link line. I agree it is the version that would have
caught this directly, and I have not done it: it needs arm-none-eabi-gcc on
the runner, which the pytest job does not install, so it would skip on CI and
run only where someone already has the toolchain — a test that is green because
it did not run. Worth doing as a job that installs the toolchain, which is a
larger change than this PR. Flagging rather than silently choosing the weaker
option.

2 and 3 — descriptors and ASM flags

Both done. toolchains/*.yaml are scanned, treating cflags/ldflags as the
equivalent of LINK_REACHING; all five already keep specs in ldflags, so the
tree is green and the rule now holds both descriptions of a toolchain to it.
CMAKE_ASM_FLAGS_INIT added — no toolchain names a spec there today, verified.

Verification

item result
pytest tests/unit/test_toolchain_specs.py PASS — 7 passed
ctest PASS — 38/38
pytest tests/ 1 failed, 21 passed — the failure is the #127 registration, restored to red on purpose

Both CI reds remain inherited: eBoot's master (eos_image.h reserved,
eBoot#94) and this one.

@Kartikey1306 Kartikey1306 changed the title fix(toolchain): stop passing nosys.specs twice on a Cortex-M4 link fix(toolchain): master cannot link a Cortex-M4 executable — nosys.specs is passed twice Sep 3, 2026

@srpatcha srpatcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — eos#126 "fix(toolchain): master cannot link a Cortex-M4 executable — nosys.specs is passed twice"

head: 0957ac4 author: Kartikey1306 ci: fail (four red, two causes, both inherited — see CI)

Verdict: Correct, and the reframing from "cleanup" to "master cannot link" is right — I reproduced it again at this head against arm-none-eabi-gcc 14.2.1: with master's arm-cortex-m4.cmake a three-line add_executable() project dies on the duplicate spec; with this head's it links. All four findings from the 126b0cd3 round are addressed and I checked each by running the parser rather than reading it. The test now catches the class it claims for the idioms it was extended to cover; two spellings that produce the identical duplicate still slip past it.

Findings

# Severity File:line Finding Recommended fix
1 Medium tests/unit/test_toolchain_specs.py:35-40 LINK_REACHING is an exact list of the _INIT spellings, and the non-_INIT ones produce the same failure. A toolchain file may set CMAKE_C_FLAGS and CMAKE_EXE_LINKER_FLAGS directly — that is how a large share of vendor toolchain files are written, and it reaches the link line identically. Measured against this head's parser: set(CMAKE_C_FLAGS "-specs=nosys.specs") plus set(CMAKE_EXE_LINKER_FLAGS "-specs=nosys.specs") returns {} — no variable recorded, both new tests green, and the executable does not link. The per-config forms (CMAKE_C_FLAGS_RELEASE_INIT, CMAKE_EXE_LINKER_FLAGS_DEBUG_INIT) are uncovered for the same reason. Latent, not live: all six current toolchains/*.cmake use the _INIT forms — confirmed by running _specs_per_variable over every one of them. But the docstring's stated aim is to "fail on the class of mistake rather than on one instance of it", and this is the same instance under a different name. Match a pattern instead of a tuple: re.compile(r"^CMAKE_(C|CXX|ASM)_FLAGS(_[A-Z]+)?(_INIT)?$|^CMAKE_EXE_LINKER_FLAGS(_[A-Z]+)?(_INIT)?$"). Keep the compile/link split for test_specs_are_declared_on_the_linker_flags_not_the_compiler_flags by testing the language prefix rather than the exact name. list(APPEND <var> …) is invisible too and can go in the same change, though it is a much less likely spelling for what is a string here.
2 Low tests/unit/test_toolchain_specs.py:110-127, :246-256 The .yaml scan can pass vacuously, and there is no guard against it — which is the exact failure the .cmake side has a test for. Three things, all measured: (a) the block-sequence form is invisible — _yaml_specs_per_key("cflags:\n - -specs=nosys.specs\n") returns {}, because a - item line has no : and is skipped at :117. (b) A trailing comment is read as a live flag — _yaml_specs_per_key("ldflags: -Wl,--gc-sections # -specs=nosys.specs") reports nosys.specs; :116 drops whole-line comments only. Harmless on ldflags, a false failure on cflags. (c) test_every_arm_toolchain_is_actually_parsed (:246) covers .cmake only and hardcodes three filenames, so neither a descriptor that parses to nothing nor a fourth ARM .cmake file is protected from the vacuous pass the test exists to prevent. Today's five descriptors all use the scalar form and parse correctly (arm-none-eabi-r5.yaml and arm-none-eabi.yaml → ldflags: [nosys.specs, nano.specs]; the other three → {}), so nothing is broken now. Replace the hardcoded list with a non-circular guard that covers both file types: for every file under toolchains/ whose raw text contains -specs=, assert the corresponding parser returns a non-empty result. That catches a new toolchain file, a new descriptor, and a parser regression, without naming any file. Separately, strip an unquoted trailing # comment in _yaml_specs_per_key, and handle the - item form.
3 Low tests/CMakeLists.txt:93 The borrowed test_crypto_ed25519_loworder registration was correctly dropped this round, but its trailing blank line stayed: the diff against master is now 1+ 0- on tests/CMakeLists.txt, one added empty line and nothing else. So a toolchain PR still touches the one file that #118, #119, #122 and #127 all edit, at exactly the anchor they edit, for no reason — a free merge conflict, and it contradicts the body's own "It has been removed". git checkout origin/master -- tests/CMakeLists.txt, or delete the stray line. The file should not appear in this PR's diff at all.
4 Low PR body, "CI" section "Two reds, both inherited" — there are four: Build & Test (Linux x86_64), Cross-compile ARM64 kernel, Full-stack integration summary, Run Python tests. The two causes named are correct and do account for all four (Build & Test fails inside its own "Run Python tests" step on the same test_crypto_ed25519_loworder assertion — verified from job 100742191738: 1 failed, 21 passed; Full-stack integration summary is downstream of the ARM64 kernel build — verified from job 100742510470). Worth correcting because a reader who counts red checks against the body will think two are unexplained, and this PR's whole subject is a signal that reported the wrong thing. "Four red, two causes" plus the mapping.

What I verified rather than took on trust

Run at 0957ac49 in a detached worktree; the working checkout was not touched and the worktree was removed afterwards.

  • The headline claim, reproduced end to end on this host (arm-none-eabi-gcc (15:14.2.rel1-1) 14.2.1 20241119, real newlib nosys.specs). Identical three-line CMake project, add_executable(app main.c), only the toolchain file differs:

    arm-cortex-m4   [master] rc=2  arm-none-eabi-gcc: fatal error: …/nosys.specs: attempt to
                                   rename spec 'link_gcc_c_sequence' to already defined spec
                                   'nosys_link_gcc_c_sequence'
    arm-cortex-m4   [head]   rc=0  [100%] Built target app
    arm-none-eabi-r5 [master] rc=0 [100%] Built target app
    arm-none-eabi-r5 [head]   rc=0 [100%] Built target app
    

    So the Cortex-M4 fix is real and the Cortex-R5 move is behaviour-preserving, both measured at this head rather than carried over from the previous round.

  • All seven tests pass on the current tree, executed directly (pytest is not installed on this host, so each test_* function was called and its assertion observed). Parser output over every toolchains/*.cmake:

    arm-cortex-m4.cmake          CMAKE_EXE_LINKER_FLAGS_INIT: [nosys.specs, nano.specs]
    arm-none-eabi-r5.cmake       CMAKE_EXE_LINKER_FLAGS_INIT: [nosys.specs]
    arm-none-eabi-stm32f4.cmake  CMAKE_EXE_LINKER_FLAGS_INIT: [nano.specs, nosys.specs]
    aarch64 / arm-linux-gnueabihf / riscv64: {}
    

    which matches the body's table.

  • Negative control. Re-introducing -specs=nosys.specs into arm-cortex-m4.cmake's CMAKE_C_FLAGS_INIT fails both checks with the right message — -specs=nosys.specs is set in CMAKE_C_FLAGS_INIT and CMAKE_EXE_LINKER_FLAGS_INIT, so it reaches the link line more than once. The test is not one that would pass either way.

  • Last round's findings 1, 2 and 3 are genuinely fixed, not just described as fixed. _resolve follows ${CPU_FLAGS} (probe: a spec hidden in CPU_FLAGS is reported in both CMAKE_C_FLAGS_INIT and CMAKE_EXE_LINKER_FLAGS_INIT); string(APPEND …) is seen; the multi-line set() in arm-none-eabi-stm32f4.cmake parses; CMAKE_ASM_FLAGS_INIT is in LINK_REACHING with the linker-language reasoning written down; toolchains/*.yaml are scanned. Finding 4 last round — the borrowed registration hunk — is dropped, modulo the stray line in finding 3 above.

  • ctest. cmake -DEOS_BUILD_TESTS=ON -DCMAKE_BUILD_TYPE=Release, build rc=0, ctest --no-tests=error → 38/38 passed, which matches the body's figure exactly (39 last round, one fewer now that the borrowed registration is gone).

Architecture conformance

§21 / §5.1 — conforms. toolchains/ is Tier-1 target adaptation and is named in .ai/architect.md's reference layout. Nothing crosses a tier, no dependency direction changes, no board specifics move into hal/.

§22 Hardware support tiers — this is the section that makes the change more than hygiene. §22 requires "build status" even at the Community tier and a "maintained debugger/flash path" at Reference. A Cortex-M4 toolchain file that cannot link an executable makes any tier claim for an M4 board unsupportable, and Cortex-M4 is one of the classes §25.1's "three excellent reference hardware targets" is drawn from. This is a precondition for a §22 claim, not a cleanup.

§9.2, "reproducible … builds" and "per-target flash/RAM reports" — both require a working cross link, so this sits underneath them.

Weakened checks — none. Two tests added, one removed flag that provably has no compile-line effect (nosys.specs contains only %rename link_gcc_c_sequence and three *link_* entries — no *cc1, no *cpp), no assertion removed, nothing disabled.

The gap the body names under "Known limit" — that the strongest test would configure a throwaway executable and inspect the generated link line, but the pytest runner has no ARM toolchain — is stated honestly and is the right call: a test that skips on CI and runs only where someone already has the compiler is the "green because it did not run" shape .ai/security.md warns about. Worth recording that the executable-link test is runnable on a machine with the toolchain, as demonstrated above; the follow-up that installs gcc-arm-none-eabi in the pytest job is a small job and would close finding 1 as a side effect, because it tests behaviour instead of spelling.

No master-design proposal from this PR. §22 already requires what is missing here; the shortfall is in CI coverage, not in the design.

Proposed changes

  1. Finding 3 — one line, and it removes this PR from a four-way conflict.
  2. Finding 1 — pattern instead of tuple in LINK_REACHING, with the language prefix carrying the compile/link split. The current tree stays green; I checked that all six files use _INIT.
  3. Finding 2 — the non-circular "any file containing -specs= must parse to something" guard, plus the two _yaml_specs_per_key fixes.
  4. Finding 4 — correct the CI count in the body.
  5. Nothing blocks this landing once 3 is done. The toolchain fix is correct and independently verified in both directions, twice now, on a real newlib toolchain.
  6. Merge order unchanged: land #127 (the registration repair), then rebase — the two red causes here clear when #127 and eBoot#94/#97 land, and neither has anything to do with this branch.

I opened no fix PR. Nothing here is rated High, and every finding is inside this PR's own files.

CI

Four red, two causes, neither from this branch:

check cause
Run Python tests tests/unit/test_cmake_test_registration.py::test_every_c_suite_is_built_or_listed — test_crypto_ed25519_loworder.c exists in tests/ and no add_executable() builds it. #93 removed the registration; #127 restores it.
Build & Test (Linux x86_64) the same assertion, from this workflow's own "Run Python tests" step. Job log: 1 failed, 21 passed in 0.23s.
Cross-compile ARM64 kernel eos-simulation.yml builds embeddedos-org/eBoot@master, which does not compile. Verified directly against origin/master (22d8f8b): core/ed25519_verify.c defines point_is_identity twice (:281, :338) and include/eos_image.h:135 asserts offsetof(eos_image_header_t, reserved) against a struct with no such member. eBoot#94 and #97 are the repairs; eBoot#84 carries the same repair as undisclosed scope.
Full-stack integration summary downstream. Job log prints Build kernel (ARM64 cross-compile) FAIL / QEMU simulation (13 TCs) FAIL and exits on the QEMU condition.

Green and relevant: Cross-compile ARM Cortex-M4 (58s), Host Tests (x86_64), QEMU Boot Test, Default configuration, Static Analysis, Analyze (C/C++), CodeQL, the SBOM validation and all ten Python legs.

Note the asymmetry the body identifies and I confirmed last round still holds: Cross-compile ARM Cortex-M4 passes identically with the duplicate present or absent, because every ARM toolchain here sets CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY and no cross target links an executable. The green tick on that job is not evidence about this defect in either direction.

Not checked

  • The repo's own cross build was not run end to end. The link reproduction used an isolated three-line executable, which is what isolates the flag; I did not cross-build eos itself.
  • Only GCC 14.2.1. The duplicate-spec diagnostic is a driver behaviour; older or vendor toolchains may differ in whether it is fatal. "master cannot link a Cortex-M4 executable" is verified for this compiler, not asserted for all.
  • nano.specs was not read. I confirmed nosys.specs is link-only by reading it; whether nano.specs has compile-line effects matters if anyone later moves it the way this PR moves nosys.specs, and the body's warning that the two must stay on the same line (because *nosys_libc inspects the command line for specs=nano.specs) is the reason not to try.
  • No .S-only executable was built, so the CMAKE_ASM_FLAGS_INIT reasoning in LINK_REACHING is CMake's documented linker-language selection rather than something I observed.
  • arm-none-eabi-r5.yaml was read but never fed to toolchains/src/toolchain.c. The .yaml side of the r5 target is checked by the new test as text only; the CLI's own parse of it was not exercised.
  • pytest was not run, because it is not installed on this host and the sandbox could not install it. Each test function was imported and called directly and every assertion was observed; that covers the same code but not pytest's collection, fixtures or parametrisation (this file uses none).

Automated architecture review of 0957ac490fda — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.

Kartikey1306 and others added 5 commits September 3, 2026 23:16
arm-cortex-m4.cmake sets -specs=nosys.specs in CMAKE_C_FLAGS_INIT and again
in CMAKE_EXE_LINKER_FLAGS_INIT. CMake links through the compiler driver, so
both reach the link line and the specs file is read twice. A specs file may
only be read once -- the second read tries to re-rename a spec it has
already renamed:

    arm-none-eabi-gcc: fatal error: nosys.specs: attempt to rename spec
    'link_gcc_c_sequence' to already defined spec 'nosys_link_gcc_c_sequence'

It has been latent because CMAKE_TRY_COMPILE_TARGET_TYPE is STATIC_LIBRARY
and nothing in the cross build links an executable, so the duplicate never
reached a link line. It surfaced the moment one did.

The flag belongs on the link line only; arm-none-eabi-stm32f4.cmake already
keeps it there and nowhere else. This matches it. The compile line does not
need it: nosys.specs supplies the syscall stubs the linker wants.

Verified with arm-none-eabi-gcc rather than by reading. The mechanism, using
a specs file this toolchain does ship:

    once:  arm-none-eabi-gcc ... m.o -specs=dup.specs -o a.elf     -> links
    twice: arm-none-eabi-gcc -specs=dup.specs m.o -specs=dup.specs -> fatal error:
           attempt to rename spec 'link_gcc_c_sequence' to already defined
           spec 'dup_link_gcc_c_sequence'

the same error CI reports, from the same cause. And the generated build:
before, every compile command carried -specs=nosys.specs; after, none does
and the exe link flags still carry it once.

The full cross build could not be run here -- the Homebrew arm-none-eabi-gcc
ships no newlib, so nosys.specs and stdint.h are both absent. Configure
succeeds with this change and fails without it on that toolchain, which is
the part it governs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ARM file

Answers the review on embeddedos-org#126.

Finding 1 (Medium) -- the head carried an undisclosed second change, the
test_crypto_ed25519_loworder registration, which contradicted this PR's own
2026-09-02 comment ("this PR changes one file ... and does not touch tests/").
It was also the fourth open copy of that registration, at a different position
from embeddedos-org#127's. Dropped: the branch is rebased to the toolchain change alone, and
embeddedos-org#127 is the one to take for the registration -- it restores the four lines to
their original position, keeps the crypto suites grouped, and is the position
embeddedos-org#122 is already written against.

Finding 2 (Medium) -- nothing would catch this coming back. Every ARM
toolchain here sets CMAKE_TRY_COMPILE_TARGET_TYPE to STATIC_LIBRARY and no
cross build links an executable, so `Cross-compile ARM Cortex-M4` passed
identically with the duplicate present and absent; it passed on the unfixed
commit. The defect surfaced by accident when embeddedos-org#118 briefly put cmd/eos into the
cross build, and embeddedos-org#118 has since gated that to host builds -- removing the
accident too.

Adds tests/unit/test_toolchain_specs.py, modelled on
test_cmake_test_registration.py: parse every toolchains/*.cmake, extract each
-specs=<file>, and fail if one appears in more than one variable that reaches
a link line (CMake links through the compiler driver, so C/CXX flags do). A
second test asserts specs are declared on the linker flags rather than the
compiler flags, which is the arrangement that turns into a duplicate the
moment someone adds the linker entry that looks missing. No ARM toolchain
needed; runs in the existing pytest job.

Finding 3 (Low) -- arm-none-eabi-r5.cmake put --specs=nosys.specs in
CMAKE_C_FLAGS_INIT with nothing in CMAKE_EXE_LINKER_FLAGS_INIT. Not broken
today, and one added linker -specs= away from the same failure. Moved, so all
three ARM bare-metal toolchains state it the same way and the new guard is
green across all of them.

Verified:
  pytest tests/unit/test_toolchain_specs.py    2 passed
  pytest tests/                                all pass
  discrimination: reintroducing the duplicate in arm-cortex-m4.cmake gives
      arm-cortex-m4.cmake: -specs=nosys.specs is set in CMAKE_C_FLAGS_INIT
      and CMAKE_EXE_LINKER_FLAGS_INIT, so it reaches the link line more than
      once
    on both tests, so the guard catches the original defect rather than
    restating the fix.
  No ARM toolchain on this machine, so the cross link itself is NOT verified
  here; that remains CI's to show.

Refs embeddedos-org#126
…orrowed hunk

Addresses findings 1-4 of the review on 126bd0c.

Finding 4 first, because it is the one that was making the branch look better
than it is. The test_crypto_ed25519_loworder registration was carried here
from embeddedos-org#127. It made Run Python tests green on a branch that has nothing to do
with it, contradicted this PR's own 2026-09-02 comment, and sat at a different
anchor to embeddedos-org#127's, so it would have conflicted. Dropped. That check is red
again until embeddedos-org#127 lands, which is the honest state -- folding a master repair
into a toolchain PR to get a green tick is how a red master becomes everyone's
problem to route around.

Finding 1: the parser recognised one CMake idiom, set(VAR "literal"). It could
not see string(APPEND ...), an unquoted set(), or -- the one that matters here
-- a spec arriving through ${CPU_FLAGS}, which is how both Cortex-M4 toolchains
build their flag strings. Putting -specs= there would have reached both
CMAKE_C_FLAGS_INIT and CMAKE_EXE_LINKER_FLAGS_INIT while every check passed.
The file is now walked in order with a variable table, ${...} resolved against
what is in force at that point, and string(APPEND ...) applied. Two tests pin
those two bypasses.

Finding 3: CMAKE_ASM_FLAGS_INIT added to LINK_REACHING. CMake picks a target's
linker language from its sources, so an executable built only from .S files
links with the ASM flags. Both Cortex-M4 files set it from ${CPU_FLAGS}. The
current tree still passes; no toolchain names a spec there today.

Finding 2: the .yaml descriptors are scanned too. They are the second
authority on these flags -- toolchains/src/toolchain.c parses them into
EosToolchain.cflags/ldflags, a separate consumer from CMake -- and all five
already keep specs in ldflags, which is a good argument for this change and a
better one for holding both descriptions to one rule.

Found while verifying rather than assuming: my first regex required the closing
paren on the same line, so arm-none-eabi-stm32f4.cmake's three-line
set(CMAKE_EXE_LINKER_FLAGS_INIT ...) was invisible and every check passed
vacuously for that file. That is this test's own failure mode one level up.
Fixed, and pinned by test_a_multi_line_set_is_still_seen and
test_every_arm_toolchain_is_actually_parsed -- the latter asserts the parser
finds specs in all three ARM files, so a parser that silently sees nothing can
no longer pass.

Verified: the parser reports arm-cortex-m4 [nosys, nano], arm-none-eabi-r5
[nosys], arm-none-eabi-stm32f4 [nano, nosys], all on
CMAKE_EXE_LINKER_FLAGS_INIT. Mutations caught: a spec hidden in CPU_FLAGS, and
the original duplicate re-introduced on CMAKE_C_FLAGS_INIT.

7 toolchain tests pass, ctest 38/38.
Residue from 0957ac4, which dropped the borrowed
test_crypto_ed25519_loworder registration and left the blank line that had
separated it from the block below. It is the only remaining change this PR
makes to tests/CMakeLists.txt, it changes nothing, and it sits at the exact
anchor where embeddedos-org#118, embeddedos-org#119 and embeddedos-org#127 each add the real registration block -- so
it would have conflicted with whichever of those lands first, for no reason
at all.

tests/CMakeLists.txt is now untouched by this branch.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ot a copy of it

Restacked onto embeddedos-org#127. The previous head dropped the registration hunk
entirely, per finding 4, which left Run Python tests red by design until
embeddedos-org#127 landed. Stacking gets the same result without a duplicate: the hunk
comes from embeddedos-org#127, this branch adds none of its own, and the diff against
embeddedos-org#127 is toolchain-only.

Verified locally before pushing: pytest 22 passed (including
test_every_c_suite_is_built_or_listed, the check that was failing), cmake
configure and build clean, ctest 39/39 with test_crypto_ed25519_loworder
among them.
@Kartikey1306
Kartikey1306 force-pushed the fix/arm-m4-duplicate-specs branch from f1dc936 to 4219f6d Compare September 3, 2026 17:47

@srpatcha srpatcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — eos#126 "fix(toolchain): master cannot link a Cortex-M4 executable — nosys.specs is passed twice"

head: 4219f6d author: Kartikey1306 ci: fail (2 of 26 — both cross-repo, not this PR)

Verdict: The fix is correct and I reproduced the defect end to end rather than taking it on report — the author could not, having no ARM toolchain, and relied on someone else's verification. With master's arm-cortex-m4.cmake a three-line add_executable() project fails to link with the exact error quoted; with this branch's it links. Every supporting claim in the body — that removing the flag from the compile line is a no-op, that the nano.specs coupling is a real trap, that the r5 change is behaviour-preserving — also checks out against the actual spec file and toolchain. One finding: the body states that an unrelated hunk was removed from the diff, and it is still there.

Findings

# Severity File:line Finding Recommended fix
1 Medium tests/CMakeLists.txt:93-97 The body says this hunk was removed. It is still in the diff, and it is what makes Run Python tests green here. The body states plainly:
"An earlier revision of this branch carried #127's registration hunk… It has been removed: folding a master repair into a toolchain PR to get a green tick is how a red master becomes everyone's problem to route around."
At head 4219f6d (a force-update over 0957ac4) the diff still contains the four test_crypto_ed25519_loworder registration lines plus a trailing blank, and I confirmed it is byte-identical to the hunk in eos#121 (diff of the two tests/CMakeLists.txt diffs → no output). Two independent corroborations that the body describes a different tree than the head: (a) Run Python tests is not among the failing checks here, whereas the body's CI section says it fails on this registration; (b) the body reports ctest … 38/38, and I measure 39 tests, 39 passed at this head — 38 is master's count, reachable only without the hunk. The stated principle is right; the diff does not follow it.
Either drop the hunk and let Run Python tests be red until #127 lands — which is what the body says was done and is the more defensible choice given the reasoning — or keep it and rewrite that paragraph to say so, noting the collision with #118, #119, #121 and #127, all of which add the identical block at the identical anchor. Then correct 38/38 to 39/39. Do not leave the body asserting the opposite of the diff.
2 Low tests/unit/test_toolchain_specs.py:52-53 (with :141-158) set(... CACHE STRING ...) defeats duplicate detection. SET_RE's quoted alternative requires \s*\) right after the closing quote, so set(VAR "…" CACHE STRING "") falls through to the unquoted alternative [^)\n]+ and captures "…" CACHE STRING "". _unquote strips only the outermost quote pair, leaving an embedded ", and SPECS_RE's \S+ swallows it — so the spec is recorded under a different key and a real duplicate is missed. Executed against this head:
set(CMAKE_EXE_LINKER_FLAGS_INIT "-specs=nosys.specs" CACHE STRING "") + set(CMAKE_C_FLAGS_INIT "-specs=nosys.specs") → parsed as {'CMAKE_EXE_LINKER_FLAGS_INIT': ['nosys.specs"'], 'CMAKE_C_FLAGS_INIT': ['nosys.specs']}, duplicate NOT detected.
The same pair without CACHE is detected correctly. CACHE STRING is a common idiom in CMake toolchain files, and no file under toolchains/ uses it today — so this is latent, not active. It matters because this test's stated purpose is to "fail on the class of mistake rather than on one instance of it", and this is a hole in the class.
Tighten the value capture and strip trailing punctuation from the spec name. Smallest version: add CACHE handling to _unquote's input by making SET_RE's quoted branch not require the immediate ) —
`SET_RE = re.compile(r'set(\s*(\w+)\s+("(?:[^"\]
3 Low tests/unit/test_toolchain_specs.py:154 Cosmetic: when the same spec appears twice in one variable the check correctly fails (verified: -specs=nosys.specs -specs=nosys.specs in one set() → detected), but ' and '.join(sorted(variables)) renders the message as "is set in CMAKE_EXE_LINKER_FLAGS_INIT and CMAKE_EXE_LINKER_FLAGS_INIT". sorted(set(variables)), and add a "twice in the same variable" clause to the message so the diagnosis reads correctly for that case.
4 Low (CI / body) 2 of 26 checks fail, neither from this diff: Cross-compile ARM64 kernel fails building eBoot (eBoot/include/eos_image.h:135:23: error: 'eos_image_header_t' has no member named 'reserved', also :142), and Full-stack integration summary fails as its dependent. Confirmed pre-existing — the same workflow fails on origin/master at feee2726, and eos#121, #129 and #134 report the identical 2 failures. The body's attribution is stale on one point: it names eBoot#94 as the repair, while the later comment thread on eos#121 says eBoot#94's diagnosis is superseded by eBoot#97 (eos_image.h reserved[] vs tlv_len/tlv_hash). Nothing for this author beyond updating the eBoot PR number. I did not verify which eBoot PR is current.

Architecture conformance

Conforms. toolchains/ is target adaptation inside eos — master design §21 Tier 1 — Foundation, and §5.1's "EoS depends only on lower hardware/runtime abstractions". A toolchain file is the lowest layer there is; it introduces no dependency in any direction, and .ai/architect.md's "a board file is the leaf, never the trunk" applies equally to a toolchain description. §21.1 is not in play.

The change does not restructure and change behaviour in the same commit, which .ai/architect.md lists under Do not. The arm-cortex-m4.cmake edit is the behaviour fix; the arm-none-eabi-r5.cmake edit is a consistency move, and I verified it is genuinely behaviour-preserving rather than taking the claim (both toolchains link a test executable successfully — see below). So the two are separable in effect even though they share a commit.

No check was weakened. The diff adds a test and removes none; nothing is skipped or xfailed. The one CI-visible effect of the undisclosed hunk (finding 1) is to make a check pass, which is the opposite direction from a weakened check but is still a change to the evidence that the body does not own.

§9.2's "actionable diagnostics" and §28's evidence model are both served properly here. The author's own "Known limit" section is the right call and I want to record agreement with it: the strongest test would configure a real add_executable() and read the link line, and it is correctly not included because the pytest job installs no ARM toolchain, so it would skip on CI and pass vacuously. "A test that is green because it did not run is worse than the pattern check" is exactly §28's principle. That said — see below — the runner could install the toolchain, and the follow-up they suggest is worth doing, because I ran that stronger check by hand and it is decisive in a way the pattern check is not.

Proposed changes

  1. Finding 1 is the only one I would hold the PR for, and it is a body/diff reconciliation, not a code change.
  2. Finding 2, one regex, plus the CACHE parser test.
  3. Finding 3, sorted(set(...)).
  4. The follow-up the body proposes is worth filing now, because I can confirm it works: install gcc-arm-none-eabi in the pytest job (or a dedicated job) and configure the three-file probe below against each ARM toolchain file, asserting the build succeeds. It is ~15 lines, needs no hardware, and catches this class directly rather than by pattern — including the CACHE case in finding 2 and anything else the parser cannot see:
cmake_minimum_required(VERSION 3.16)
project(linkprobe C)
add_executable(probe main.c)     # main.c: int main(void){return 0;}

configured with -DCMAKE_TOOLCHAIN_FILE=toolchains/<file>.cmake. On master's arm-cortex-m4.cmake this exits 2; on this branch's it exits 0.

Not checked

  • toolchains/*.yaml descriptors. test_descriptors_declare_specs_on_ldflags_only passes, but I did not read the descriptors myself or check that _yaml_specs_per_key's line-wise parse handles the forms they actually use (block scalars, lists, quoted values). Its SPECS_RE runs on the raw post-colon text, so the finding-2 trailing-punctuation issue applies there too and I did not probe it.
  • CMAKE_ASM_FLAGS_INIT in practice. The test includes it, and the reasoning (an executable built only from .S sources links with the ASM flags) is sound, but no such target exists in the tree and I did not construct one.
  • Which eBoot PR repairs the ARM64 failure — #94 per this body, #97 per the eos#121 thread. I did not look at eBoot.
  • arm-none-eabi-stm32f4.cmake I did not link-test; only arm-cortex-m4.cmake and arm-none-eabi-r5.cmake. The test parses it and the body reports its specs on the linker flags only.
  • Run Python tests on CI I read from checks.txt as passing; I did not inspect that job's log to confirm the registration test is what makes it green, though the local ctest count (39 vs master's 38) is consistent with it.
  • What I did run, and what it showed. git archive of the head into a temp directory; arm-none-eabi-gcc 14.2.1 20241119 (Ubuntu 15:14.2.rel1-1) is present on this host, so unlike the author I could test the actual claim:
    • Headline defect reproduced. Same three-line project, same CMake, only the toolchain file differing. With origin/master:toolchains/arm-cortex-m4.cmake: configure exit=0, BUILD EXIT=2, arm-none-eabi-gcc: fatal error: …/arm-none-eabi/lib/nosys.specs: attempt to rename spec 'link_gcc_c_sequence' to already defined spec 'nosys_link_gcc_c_sequence' — character-for-character the error in the body. With this head's file: configure exit=0, BUILD EXIT=0. grep -c "specs=nosys" is 2 on master's file and 1 (excluding comments) on this one. This is a real link failure on master, not a cleanup, and the body's reframing is correct.
    • "Removing it from the compile line is provably a no-op" — confirmed by reading the file. nosys.specs on this host is exactly %rename link_gcc_c_sequence nosys_link_gcc_c_sequence plus *nosys_libgloss, *nosys_libc and *link_gcc_c_sequence. No *cc1, no *cpp — it cannot affect an object file.
    • The nano.specs trap is real and worth keeping in the comment. *nosys_libc: is %{!specs=nano.specs:-lc} %{specs=nano.specs:-lc_nano} — it does inspect the command line for specs=nano.specs, so the two flags must stay on the same line, exactly as the body warns.
    • The r5 change is behaviour-preserving — the same probe links successfully with both origin/master's and this head's arm-none-eabi-r5.cmake (configure=0 build=0 for both).
    • cmake -DEOS_BUILD_TESTS=ON && cmake --build -j4 → exit 0, no errors; ctest --no-tests=error → 39/39 passed; pytest tests/ -q → 22 passed; pytest tests/unit/test_toolchain_specs.py -q → 7 passed.
    • Mutation-checked the new test myself, rather than trusting the table: re-introducing -specs=nosys.specs into arm-cortex-m4.cmake's CMAKE_C_FLAGS_INIT fails test_no_spec_file_reaches_the_link_line_twice and test_specs_are_declared_on_the_linker_flags_not_the_compiler_flags (2 failed, 5 passed). Re-introducing it in the --specs= double-dash spelling into arm-none-eabi-r5.cmake fails the same two — so the parser handles both spellings. The test does what it says.
    • Single configuration: GCC 15.2.0 host, x86_64 Linux, Python 3.12. No cross target other than the hand-built probes, and no hardware.

Automated architecture review of 4219f6d95cec — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.

@srpatcha
srpatcha merged commit 87f2f12 into embeddedos-org:master Sep 8, 2026
26 of 28 checks passed
Kartikey1306 added a commit to Kartikey1306/eos that referenced this pull request Sep 14, 2026
…ARM file

Answers the review on embeddedos-org#126.

Finding 1 (Medium) -- the head carried an undisclosed second change, the
test_crypto_ed25519_loworder registration, which contradicted this PR's own
2026-09-02 comment ("this PR changes one file ... and does not touch tests/").
It was also the fourth open copy of that registration, at a different position
from embeddedos-org#127's. Dropped: the branch is rebased to the toolchain change alone, and
embeddedos-org#127 is the one to take for the registration -- it restores the four lines to
their original position, keeps the crypto suites grouped, and is the position
embeddedos-org#122 is already written against.

Finding 2 (Medium) -- nothing would catch this coming back. Every ARM
toolchain here sets CMAKE_TRY_COMPILE_TARGET_TYPE to STATIC_LIBRARY and no
cross build links an executable, so `Cross-compile ARM Cortex-M4` passed
identically with the duplicate present and absent; it passed on the unfixed
commit. The defect surfaced by accident when embeddedos-org#118 briefly put cmd/eos into the
cross build, and embeddedos-org#118 has since gated that to host builds -- removing the
accident too.

Adds tests/unit/test_toolchain_specs.py, modelled on
test_cmake_test_registration.py: parse every toolchains/*.cmake, extract each
-specs=<file>, and fail if one appears in more than one variable that reaches
a link line (CMake links through the compiler driver, so C/CXX flags do). A
second test asserts specs are declared on the linker flags rather than the
compiler flags, which is the arrangement that turns into a duplicate the
moment someone adds the linker entry that looks missing. No ARM toolchain
needed; runs in the existing pytest job.

Finding 3 (Low) -- arm-none-eabi-r5.cmake put --specs=nosys.specs in
CMAKE_C_FLAGS_INIT with nothing in CMAKE_EXE_LINKER_FLAGS_INIT. Not broken
today, and one added linker -specs= away from the same failure. Moved, so all
three ARM bare-metal toolchains state it the same way and the new guard is
green across all of them.

Verified:
  pytest tests/unit/test_toolchain_specs.py    2 passed
  pytest tests/                                all pass
  discrimination: reintroducing the duplicate in arm-cortex-m4.cmake gives
      arm-cortex-m4.cmake: -specs=nosys.specs is set in CMAKE_C_FLAGS_INIT
      and CMAKE_EXE_LINKER_FLAGS_INIT, so it reaches the link line more than
      once
    on both tests, so the guard catches the original defect rather than
    restating the fix.
  No ARM toolchain on this machine, so the cross link itself is NOT verified
  here; that remains CI's to show.

Refs embeddedos-org#126
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.

2 participants