Skip to content

fix(boards): 8051 board port has never compiled — its ops table is not a valid C identifier - #106

Draft
srpatcha wants to merge 1 commit into
masterfrom
autofix/board-8051-invalid-identifier
Draft

fix(boards): 8051 board port has never compiled — its ops table is not a valid C identifier#106
srpatcha wants to merge 1 commit into
masterfrom
autofix/board-8051-invalid-identifier

Conversation

@srpatcha

@srpatcha srpatcha commented Sep 4, 2026

Copy link
Copy Markdown
Member

Problem

boards/8051/board_8051.c cannot be compiled by any C compiler. The board port
that CMakeLists.txt:295-296 offers as -DEBLDR_BOARD=8051 has never been
buildable.

Surfaced by the maintenance sweep's health scan for eBoot, section
"C — unused functions and dead code":

boards/8051/board_8051.c:23: error: syntax error: 8051_ops =

Reproduced in this worktree before the fix (recorded below as the superseded
first build-board-8051 run):

boards/8051/board_8051.c:23:30: error: invalid suffix '_ops' on integer constant
   23 | static const eos_board_ops_t 8051_ops = {
boards/8051/board_8051.c:65:13: error: invalid suffix '_ops' on integer constant
   65 |     return &8051_ops;

Root cause

The file names its ops table 8051_ops. A C identifier may not begin with a
digit (C17 §6.4.2.1), so the compiler lexes 8051_ops as an integer constant
with a bogus suffix. Both the definition at line 23 and the reference at line 65
are rejected.

The sibling ports all use <board>_opsavr_ops, arc_ops, m68k_ops — and
that pattern silently produces an invalid identifier for the one board whose
name starts with a digit. Nothing caught it because no CI job configures with
EBLDR_BOARD=8051: the default is none, and the board is only compiled when
someone selects it explicitly.

Fix

Rename the table to mcs8051_ops, matching the MCS8051_* macro prefix this
same file already uses for its memory map, and update the single reference in
board_8051_get_ops().

mcs8051_ops was chosen over board_8051_ops because the file's own vocabulary
is already MCS8051_, and it keeps the <something>_ops shape the other ports
share.

Files changed

  • boards/8051/board_8051.c — two lines: the definition and its one reference.

Expected impact

cmake -DEBLDR_BOARD=8051 can build board_8051 for the first time. No other
board, target or configuration is affected — the file is compiled only when that
option is selected.

Risks and compatibility

Very low. mcs8051_ops has internal linkage (static), so the rename cannot
affect any other translation unit, and the recorded refcheck labels prove
across the whole working root that:

  • 8051_ops appears nowhere outside the file being changed, so nothing was
    depending on the old spelling (it could not have been — it does not compile);
  • mcs8051_ops was not already in use, so the new name collides with nothing.

No public API, ABI, wire format or manifest entry is touched.

Verification I could not run here, and why

The full eboot_core target does not build on master in this worktree,
for a reason unrelated to this change: the _Static_asserts in
include/eos_image.h fail (expression in static assertion is not an integer
at eos_image.h:142). That breakage is already covered by open PRs #94 and #105,
so it is not addressed here and a whole-project build label is deliberately not
recorded rather than recorded as something it is not.

Verification is therefore scoped to the target this change actually affects:
board_8051 compiles, where before it did not. ctest was not run for the same
reason — the test targets depend on eboot_core.

Not covered by this change

The 8051 memory map in this file looks wrong independently of the identifier
bug — MCS8051_FLASH_SIZE is 8 KiB, but MCS8051_SLOT_A is FLASH_BASE + 0x20000 and MCS8051_BOOTCTL computes to address 0, both outside the declared
flash region. Those are values a maintainer has to choose against real STC89C52
part documentation, not something to guess at in a compile fix, so they are
recorded in the sweep backlog instead.

Verification

Executed in an isolated worktree branched from origin/master:

Check Result Duration Command
build-board-8051 pass 1s cmake --build build/8051 --target board_8051
configure pass 0s cmake -B build/8051 -G Ninja -DEBLDR_BOARD=8051
refcheck pass 1s /home/srpatcha/eos/.ai/autoreview/refcheck.sh 8051_ops --exclude boards/8051/board_8051.c
refcheck-newname pass 0s /home/srpatcha/eos/.ai/autoreview/refcheck.sh mcs8051_ops --exclude boards/8051/board_8051.c

Opened by the scheduled autoreview pipeline (model claude-opus-5), branched from origin/master. No human has reviewed this yet. Close it freely if the fix is wrong - a bad automated PR is a bug worth reporting.

…is not a valid C identifier

Opened by the scheduled autoreview pipeline after review of open PRs.
Reviewed against the EmbeddedOS Master Design v2.0.

Files: boards/8051/board_8051.c
@srpatcha srpatcha changed the title fix: fix(boards): 8051 board port has never compiled — its ops table is not a valid C identifier fix(boards): 8051 board port has never compiled — its ops table is not a valid C identifier Sep 4, 2026

@srpatcha srpatcha left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Review — eBoot#106 "fix(boards): 8051 board port has never compiled — its ops table is not a valid C identifier"

head: 1b26378 author: srpatcha ci: fail (4 required checks red — pre-existing on master, not caused by this PR)

Verdict: The change is correct, minimal and well-evidenced: 8051_ops is not a legal
C identifier (C17 §6.4.2.1), the rename to mcs8051_ops is complete, and the recorded
before/after build logs prove the file went from not compiling to compiling. Merge it.
But be clear about what it buys: after this fix boards/8051 compiles and is still
unreachable — board_8051_get_ops() has no caller anywhere in the repo, and neither does
any of the other 81 board ports. The 8051 memory map the PR body flags is also worse than
"looks wrong": on arithmetic, both A/B slot addresses sit outside the declared flash and
the boot-control block lands on address 0.

Findings

# Severity File:line Finding Recommended fix
F1 High boards/8051/board_8051.c:18-21 The memory map is not merely suspect, it is arithmetically incoherent, and this PR makes it compile for the first time. Flash is declared [0x0000, 0x2000) (8 KiB). MCS8051_SLOT_A = 0 + 0x20000 = 0x20000, 128 KiB past the end of flash. MCS8051_SLOT_B = 0 + 8192/3 + 0x20000 = 0x20AAA, likewise outside. MCS8051_BOOTCTL = 0 + 8192 - 0x2000 = 0x0000 — the boot-control block is placed on the reset vector and on flash_base itself. MCS8051_RECOVERY = 0x1555 is inside flash but overlaps whatever slot layout was intended. eBoot is TCB code (.ai/security.md: hold it to a stricter standard, do not round down because a path looks unreachable); a slot address outside the flash region is an out-of-region erase/write target the moment the ops become live. Do not fix by guessing. Either (a) derive real values from STC89C52 part documentation, or (b) make the port refuse to activate — a compile-time _Static_assert that SLOT_A, SLOT_B, RECOVERY and BOOTCTL all fall inside [FLASH_BASE, FLASH_BASE+FLASH_SIZE) would have caught this at the same moment the identifier bug was caught, and would catch it in the other 81 ports too.
F2 Medium boards/8051/board_8051.c:63-66, hal/board_registry.c:16 Pre-existing and repo-wide, but it is what decides whether this PR's "the board can build now" claim means anything. eos_board_register() is called from exactly one place in the tree: tests/unit/test_board_registry.c. No board port registers itself, so board_count is 0 in every real image, eos_board_detect() returns NULL, and eos_board_activate_auto() returns EOS_ERR_NO_IMAGE unconditionally. board_8051_get_ops() is declared in board_8051.h:34 and defined at board_8051.c:63 and referenced by nothing. Grep across all 21 repos confirms it (refcheck log, recorded). Out of scope for this PR — do not widen it. A follow-up should add the registration constructor/table the registry was designed for, and a CI assertion that a selected EBLDR_BOARD ends up registered. Filed as an architecture proposal (see below) because §22 has no rule requiring it.
F3 Medium boards/8051/board_8051.c:31-34 All four ops function pointers — flash_read, flash_write, flash_erase, jump — are NULL. eos_board_activate() (hal/board_registry.c:73-76) checks that get_ops() returned non-NULL but does not check the members, then passes the table straight to eos_hal_init(ops). If this port ever were registered, the first flash operation or the jump to the application would be a NULL call in the bootloader. Validate the required members in eos_board_register() or eos_board_activate() and reject a table with NULL mandatory ops, rather than accepting it and failing at the indirect call. Same one-line-per-field check protects every port.
F4 Medium CI (checks.txt) Four checks are red on this PR: Analyze (C/C++), Build & Test (Linux x86_64), Cross-compile STM32F4, Host Build & Tests. This PR did not cause them. git diff --stat origin/master 1b263789 is exactly `boards/8051/board_8051.c 4 ++--. They are masterbreakage:include/eos_image.hassertsoffsetof(eos_image_header_t, tlv_len) == 62(line 97) andoffsetof(eos_image_header_t, reserved) == 62(line 135) andsizeof(reserved) == 30(line 142) —tlv_lenwas carved out ofreserved` and the later asserts were not updated, so they cannot all hold. Covered by open PRs #94 and #105, exactly as the PR body states.
F5 Low boards/8051/board_8051.c:14-17, 46-60 MCS8051_FLASH_BASE and MCS8051_RAM_BASE are both 0x0000, and both regions are added to the same flat eos_device_table_t. On a real 8051 that is honest — Harvard architecture, separate code and data address spaces — but eos_mem_region_t has no address-space field, so the table records two different 0-based regions and any consumer resolving an address by region lookup will get whichever was added first. Not this PR's problem. Worth an issue: the device-table memory model cannot express a Harvard target, and 8051 is not the only one in boards/ (AVR, PIC16/18/24, MSP430).

Verification I checked

I read state/verify/eBoot__board-8051-invalid-identifier.tsv and all five logs rather
than trusting the body's table. They match it exactly, including the honest recording of
the superseded failing run:

  • configure (-DEBLDR_BOARD=8051) — exit 0.
  • build-board-8051 run 2, exit 1 — the pre-fix reproduction. error: invalid suffix '_ops' on integer constant at board_8051.c:23:30 and :65:13, plus
    expected identifier or '(' before numeric constant. Real compiler output.
  • refcheck 8051_ops --exclude boards/8051/board_8051.c — UNREFERENCED, 0 references
    across 21 repos. Nothing depended on the old spelling.
  • refcheck mcs8051_ops — UNREFERENCED, 0 references. The new name collides with nothing.
  • build-board-8051 run 5, exit 0 — post-fix. Compiles, links libboard_8051.a.

The body's "Verification I could not run here, and why" section declines to record a
whole-project build label and says why. That is the correct call and I am explicitly not
treating the absent ctest as a concealed gap — it is declared, and I verified the stated
reason (F4) is true rather than a convenient excuse.

The rename itself: mcs8051_ops has internal linkage, the definition and its single
reference are both updated, and there are no other uses in the file (I read all 66 lines
at head). Nothing else in the tree spells either name. The fix is complete.

Architecture conformance

Conforms to master design §5.1 and §21. boards/ is the leaf tier — .ai/architect.md:
"A board file is the leaf, never the trunk" — and this change adds no include, link line
or CMake dependency in any direction, let alone upward. eBoot is Tier 1 – Foundation
(§21); the file depends only on eos_hal.h, eos_types.h and eos_device_table.h, all
within eBoot. The TCB surface (§5.1, "eBoot keeps the trusted computing base minimal and
auditable") is not widened by a rename — though F1 and F3 describe TCB code that is
already unsound the moment it becomes reachable.

Where it deviates is not §5.1 but §22, Hardware Support Tiers. The design's lowest tier
is Experimental — "No compatibility promise; may change rapidly" — and §22 closes with
"Do not market all board descriptors as equivalent hardware support." A board port that
has never compiled, is registered by nothing, and carries a memory map that cannot be
right is below every tier the design defines, and §22 provides no name for that state and
no floor that would have caught it. CMakeLists.txt:296-297 nonetheless offers
-DEBLDR_BOARD=8051 as a selectable option alongside working ports. That is the design
being silent, not the code being non-conformant, so it goes to proposals rather than
findings — appended to .ai/autoreview/proposals/2026-09.md.

Proposed changes

  1. Merge this PR as-is. It is correct and the smallest thing that makes the file legal C.
    Do not expand it to cover F1–F3.
  2. Merge #94 (and/or #105) first, or accept merging with four red checks that this
    PR provably did not cause. Nothing else unblocks it — no rebase or new commit here will
    turn those checks green.
  3. Follow-up, in order of value: F1 as a _Static_assert bounds check applied to every
    board port (cheap, catches the same class across all 82); then F3's NULL-member
    validation in the registry; then F2's actual registration path.

Not checked

  • I did not re-run any build. I read the pipeline's recorded logs. NOT RUN by me,
    deliberately — I did not create a worktree outside the sanctioned fix path.
  • ctest was not run, by anyone, for this change. Not by the author (declared, F4
    reason) and not by CI (red for the same reason). The corrected file has been compiled
    and never executed. No test covers board_8051_get_ops() — nothing calls it.
  • I did not verify the 8051 memory map against STC89C52 part documentation. F1's numbers
    are arithmetic on the macros in this file, which is enough to show they are mutually
    inconsistent, but not enough to say what the correct values are. I am not proposing
    values.
  • I did not audit the other 81 board ports for the same identifier bug, NULL ops tables,
    or out-of-range slot addresses. 8051 is the only board whose name starts with a digit,
    so the identifier defect is unique to it; F1 and F3 almost certainly are not, and I have
    not measured how far they spread.
  • I did not confirm which of the four red checks are branch-protection required versus
    merely reported. mergeStateStatus: BLOCKED and reviewDecision: REVIEW_REQUIRED are
    consistent with either.

Automated architecture review of 1b2637892cd7 — 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.

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