Stabilize module pipeline: direct-mode out, sections in, paired-import dedup - #88
Merged
Merged
Conversation
Deploying a816 with
|
| Latest commit: |
5524ea5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://399b63d7.a816.pages.dev |
| Branch Preview URL: | https://feat-experimental-flag-track.a816.pages.dev |
manz
force-pushed
the
feat/experimental-flag-track-register-size
branch
from
May 25, 2026 10:01
6761216 to
985a8d3
Compare
manz
changed the title
`a816/parse/desugar.py` (`desugar_star_eq`) was an attempt to lift
legacy `*= ADDR` to `.alloc at ADDR { body... }` at parse time so
codegen would only see `.alloc`. Body-extent guessing turned out
unreliable: `*=` inside `.if` / `.scope` with mixed inner
placement, `*=` followed by `.include` with its own `*=`, and
trailing unbounded runs to EOF all make the boundary ambiguous
without explicit author input. The pass was never wired into the
parse pipeline (zero callers, 0% coverage) and only paid the
maintenance tax.
Keep the legacy `CodePositionAstNode` -> `CodePositionNode` path
for `*=`. UP001 stays as the manual-migration prompt with an
UNSAFE autofix; the author decides the bound. `placement.py`'s
`is_placement_boundary` is still load-bearing for UP001
container-boundary recursion; only its docstring loses the
reference to the dead pass.
Project required >=80% coverage on changed lines per CLAUDE.md but nothing checked. Added: - `diff-cover` test dependency. - `tests:diffcov` hatch script: runs coverage then `diff-cover coverage.xml --compare-branch=origin/master --fail-under=80`. - `.pre-commit-config.yaml` with two `local` hooks: - `ruff-check` on `pre-commit` (fast, every commit). - `diff-cover` on `pre-push` (full coverage + diff threshold). Run `pre-commit install --hook-type pre-commit --hook-type pre-push` once per clone to wire the hooks. Also added `test_anonymous_in_pool` to cover the `.alloc in POOL` anonymous form added on this branch; lifts changed-line coverage from 82% to 83% so the gate isn't tripping on its own introduction.
`hatch run tests:diffcov` was sitting at 83% (just over the 80%
gate). Add targeted tests that lift it to 91% by exercising:
- `placement.py` boundary recursion (`*=` / `.alloc` inside `.if`,
`.scope`, `{ }`, deeply nested combinations + the negative cases
where containers without inner placement should NOT count) ->
100% file coverage, from 36.7%.
- CLI `_apply_experimental` flag dispatch + unknown-flag warning,
`_apply_a816_toml` `[experimental]` mirror plus CLI-wins-on-overlap
semantics.
- `_synthesize_pinned_pool` rejects `.alloc at ADDR size 0`.
- Object-mode `alias = external_symbol` publishes to the object
writer's alias table; non-object mode raises NodeError.
No production code changes; tests only.
Four SonarQube violations on `3e897e3`: - **CRITICAL S3776** `linker.py:241` `_ingest_symbol` cog 18 -> 15. Extract `_final_address`, `_register_global_symbol`, `_existing_global_address`, `_register_local_symbol`. The body becomes a flat dispatch on `symbol_type`. - **CRITICAL S3776** `parse/ast/placement.py:51` `_block_contains_placement` cog 18 -> 15. Collapse the per-container isinstance cascade into one `_child_bodies` match expression that returns the inner bodies a container exposes; `_node_holds_placement` then runs one any() over those. - **CRITICAL S3776** `program/assemble.py:186` `_export_object_symbols` cog 16 -> 15. Extract `_publish_named_scope_bare_names` and `_should_skip_symbol_export` predicate so the main loop reads as classify + write. - **MAJOR S1172** `parse/codegen/pool.py` `_reject_nested_placement` had an unused `file_info` param (uses `child.file_info` inside the loop). Drop it. All four refactors preserve behaviour; full gate stays at 1170 passed, mypy strict pass, diff-cover 92% on changed lines vs origin/master. Sonar re-analysis: 0 open issues.
The warning fired in `_maybe_warn_register_width_mismatch` during
opcode codegen on any `lda/sta/ldx/...` against a typed-field
operand whose width didn't match the current `a8`/`a16`/`i8`/`i16`
state. Two problems:
- **False positive on memory-mode accesses.** `sta menu.item_ptr`
with a 3-byte `long` field warned "field width (3 bytes) does
not match A register size (8 bits)". For memory ops the field
width is the ADDRESS encoding, not the data the register
transfers; partial reads/writes into a wider field are a
legitimate ROM-hacking idiom (split a long write across two
stores + bank byte).
- **Wrong layer.** This is advisory ("consider `rep #$20` first"),
not a codegen concern. If it lives anywhere it lives in fluff as
a lint rule the author can suppress per-site, not in the hot
emit path where it spams every assemble.
Remove the warning and its helpers (`_opcode_register_width`,
`_typed_field_lookup`, `_register_label`, `_rep_sep_payload`).
Replace the two warn-expecting tests with a single regression
test that asserts memory-mode `sta`/`lda` on a typed field emits
no `field width` warning.
`.map` configured the compiler's resolver bus at codegen time but the linker spun up a fresh Program with the default bus. Custom cartridge mappings (SA-1, ExHiROM, anything beyond the default low_rom) silently vanished and downstream addresses resolved against the wrong bus. Mirror the `pool_decls` pattern: - New `BusMapping` dataclass in `object_file.py`. Bumped ObjectFile VERSION 8 -> 9 to cover the new section. - `ObjectWriter.bus_mappings` populated by `generate_map` in OBJECT mode (no-op in non-object emit paths). - `Linker._merge_bus_mappings` collects across input modules, dedupes paired-import re-emissions on `identifier`, raises on conflicting same-identifier declarations. - `Program.import_linked_symbols` calls a new `_replay_bus_mappings` that re-applies each entry onto its own `resolver.bus` (idempotent: skip identifiers already present). - Test suite: `test_bus_mapping_link_replay.py` covers serialize -> read back, paired-import dedup, conflict error, end-to-end replay onto a fresh Program's bus. Out of scope (idea_mapping_in_a816_toml memo): cartridge mapping is project-scoped, doesn't belong per-`.o`. Long-term it should move to `a816.toml [mapping]` and drop the entire serialize -> dedupe -> replay loop. This commit unblocks the immediate bug; the config-driven approach is the right end state.
Two sibling `.alloc` blocks in the same module both declaring
`_skip:` collided in the module's flat label namespace — the
second `add_label("_skip", ...)` overwrote the first, and a
`bne _skip` in routine_a emitted an offset that pointed inside
routine_b. Same shape for `jmp.w _end`. The skill doc had
already documented `_foo` as module-local; the actual code
didn't enforce per-block scoping.
Open an `AllocBodyScope` (new Scope subclass) around each alloc
body at codegen time. Scope-chain lookup then finds the local
`_skip` first; siblings can't reach in. Non-underscore body
labels still bubble back to the parent on PopScope so
cross-alloc public references (`jsr.w do_thing`) keep working.
Mechanism:
- `AllocBodyScope` marker subclass of `Scope` so `_export_name`
can opt out of the `__sc<idx>__` mangle (cross-alloc public
refs and `.extern` need to see bare names).
- `Resolver.append_alloc_body_scope()` mirrors `append_scope`.
- `_bubble_anon_exportables` lifts non-underscore labels to the
parent on scope restore so public refs survive PopScope.
- `restore_scope(exports=True)` and `PopScopeNode._apply` both
call the new bubble for anon-into-anon (the codegen-time and
emit-time paths must agree).
- `generate_alloc` wraps the body in
`ScopeNode(AllocBodyScope) ... PopScopeNode(exports=True)`.
Regression test: three cases in `test_alloc_body_scope.py`:
- two `_skip:` siblings → each `bne _skip` resolves to own block.
- two `_end:` siblings → `jmp.w _end` targets demo's own `_end`.
- cross-alloc public ref (`jsr.w do_thing`) still resolves.
Triggered by an off-tree repro at /tmp/a816-repros/01_local_label_collision.s
from the cacheguard project; same misbehaviour observed there.
Sort topo-sort deps (was set-iterated, non-deterministic per PYTHONHASHSEED) and rebuild objects when any tracked input changes via a per-object .deps sidecar. Both surfaced as upstream reports building cacheguard.
The next token is often a trailing comment on the following line, which made the formatter fold that comment onto the directive (producing a .table line the assembler then mis-parses).
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Bare LOCAL names (underscore labels, alloc-body locals) collide in the linker's flat symbol map, so jmp.w to a pool-local label could bind to a same-named label in another module: a layout-dependent wild branch. Also guard the object cache against same module name from a different source.
|
This was referenced May 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Branch started as the
--experimental track_register_sizeflag (PR81 regression escape hatch) and grew to absorb the wider stabilization needed to delete the direct build path. Single PR because the pieces interlock: paired-import dedup, linker symbol classification, and placement-boundary fixes all had to land beforebuild_with_imports_directcould go without breaking ff4 / kintsuki / integration ROMs.It then picked up a batch of correctness + build-driver fixes surfaced while building cacheguard on top of this assembler — same module/link surface, so they belong here rather than on a fork of an already-large branch.
Net result: module pipeline handles every source uniformly through parse-time
*=desugar + per-node import classifier; one build path, no fallback; reproducible, dependency-aware incremental builds.ff4 migration (manz/ff4#31): 90/162 → 134/162 kintsuki tests passing, within 4 of the a17 baseline.
What
Module pipeline stabilization (1–8)
Resolver.track_register_size, default off. CLI--experimental FLAG(repeatable),a816.toml [experimental]mirror,build_with_imports(experimental=[...])plumbing..externreaches sub-symbols..externdeclaration when both exist in the same compile unit.*=body boundary.IfAstNode/ScopeAstNode/CompoundAstNodecontaining a*=/.alloc/.relocatecount as boundaries so inner placement doesn't get swallowed into the outer alloc and emit at the wrong physical address._synthesize_pinned_poolidempotent on duplicate pool name (same source location).build_with_imports_direct,--preludeCLI flag,a816.tomlprelude key,AssemblyMode.DIRECTplumbing onassemble*,LinkedModuleNode._register_imported_poolsshim,_object_has_pool_allocsgate. Stop overwriting linker-emitted.adbgwith lineless version. Migrate three tests, tighten four. Docs updated..alloc; add anonymous.alloc in POOL. Codegen errors on.alloc/.relocate/*=nested inside an alloc body with both source locations named. Fluff ruleST001flags same pattern at lint time. Parser accepts.alloc in POOL { body }(no NAME) for asset blobs.@=(kintsuki tests, ff6 ROM-to-WRAM copy) intentionally NOT in forbidden set.Placement + quality follow-ups (9–15)
*=->.alloc atdesugar pass. Dead since the parse-time desugar in (4)/(7) took over; removed to keep one path.diff-cover --fail-under=80vsorigin/masterruns before push..w-masked address operands; size inference owns this now..mapdirectives into.o; linker dedupes and replays at link. Cartridge layout survives separate compilation; identical maps from multiple objects collapse..allocbody labels so underscore-privates don't collide. Opens anAllocBodyScopeper body; two sibling allocs both declaring_skipno longer cross-resolve a branch target.Correctness + build-driver fixes from cacheguard (16–19)
cmp <n>,s($C3) and the,sALU family modelled the offset as an accumulator-sized operand, so.a16rejected the byte offset (cmp does not supports size (b)). Now a single-byte offset likelda <n>,s.lbl - baseinside a relocatable.alloc … in POOLshipped as 0. Two stacked bugs: relocation/alias renamer mangledAllocBodyScopelabels (which export bare) to__sc<idx>__, and single-object builds skipped the linker entirely so the expression relocation never ran. Now resolves both inline and via a hoistedOFF = lbl - baseconstant.set→ compilation/placement order (and the emitted ROM) varied withPYTHONHASHSEED; now sorted. Incremental rebuilds track every input (source,.include,.incbin,.table, imported-module constants) via a per-object.depssidecar instead of the module's own mtime alone — editing an include no longer leaves dependents stale..incbin/.tablenodes to the directive line, not the next token. They recordedp.current()(the token after the path string, often a trailing comment on the next line) as source location, which made the formatter fold that comment onto the directive and produce a.tableline the assembler then mis-parses.jmp.w _loopto a pool-local label could bind to a same-named_loopin another module — a layout-dependent wild branch that masqueraded as random memory corruption. The linker now resolves a relocation's LOCAL operands against the object that emitted it. Also guards the object cache against reusing a__main__.obuilt from a different source path.Test plan
.alloc atfrom two paired-imports stays accepted..if FEATURE { *= 0x9000 ... }inside outer*= 0x8000body emits conditional bytes at $9000, not $8000..allocinside another.allocerrors at codegen with both source lines named;ST001flags same source at lint time..alloc in pool { .incbin "asset.bin" }parses and places into pool.--experimental track_register_sizeoff: ff4 baseline assembles like a17. On: PR81 inference behaviour returns.--preludeflag gone;a816.toml prelude=ignored; explicit.import "preamble"in main is the migration path.cmp 0x03, sunder.a16assembles (c3 03) instead of erroring.lbl - baseinside.alloc … in POOLresolves to the byte distance, both inline and viaOFF = lbl - base; same value as the pinned.alloc atform.PYTHONHASHSEED..included constant, an.incbinasset, or an imported module rebuilds the dependent withoutrm -rf build/obj.formatleaves a comment on the line after.table "x"standalone; output re-parses to the same token stream._loopandjmp.w _loopeach target their own copy, regardless of placement order.Out of scope
@=into.alloc bind BIND_ADDR { ... }syntax — deferred until kintsuki + ff6 migrate off@=.rep/sepinference scope boundary when the experimental flag is on — tracked separately; default-off is enough to ungate ff4..res, overlap-checked) — next PR; the remaining open cacheguard report.