mirror of
https://github.com/imjasonh/nescript
synced 2026-07-10 17:52:51 +00:00
codereview: address four residual concerns from the hardware review
- Analyzer: new `W0108` warning when an array's byte size exceeds 256. The codegen lowers `arr[i]` to `LDA base,X` and the 6502's X register is 8 bits, so elements past byte 255 are unreachable. The old debug bounds check silently skipped arrays in that range; it now clamps the compare to 255 and the analyzer diagnoses the declaration up front. - UxROM `__bank_select`: the routine previously wrote the bank number to a fixed `$FFF0`, which works on emulators that don't simulate bus conflicts (jsnes, Mesen permissive) but is broken on real hardware because a single ROM byte can't match every possible bank number. Fixed by `TAX; STA __bank_select_table,X` — the store lands at `table + bank_num`, whose ROM byte is exactly `bank_num`, so CPU bus = A = ROM = no conflict. New `LabelAbsoluteX` addressing-mode variant in the assembler resolves the table's base address through the existing fixup pass. The two existing UxROM example ROMs shift a few bytes but their goldens still match (jsnes is bus-conflict-permissive). - Source maps: new `source_map_survives_aggressive_peephole_folding` regression test. The reviewer was worried peephole could drop `__src_<N>` labels and silently leave stale source-map entries. Peephole actually treats labels as block boundaries and never deletes them — the test pins that down by compiling a program tailored to trip every peephole fold and asserting every codegen-recorded source marker survives into the final linker label table. - Frame-overrun counter: new `debug_frame_overrun_counter_reads_back_from_user_code` end-to-end test that proves the contract works: NMI emits `INC $07FF`, user `peek(0x07FF)` lowers to `LDA $07FF`, and the RAM allocator doesn't hand out `$07FF` to a user variable. https://claude.ai/code/session_01MaNVcDmK9gsspRkdxowQAM
This commit is contained in:
parent
9b54ff83c0
commit
c5297567d2
11 changed files with 354 additions and 20 deletions
|
|
@ -915,6 +915,34 @@ impl Analyzer {
|
|||
.map(|(n, l)| (n.clone(), l.size))
|
||||
.collect();
|
||||
let size = type_size_with(&var.var_type, &struct_sizes);
|
||||
|
||||
// Warn on arrays whose byte size exceeds 256: the codegen
|
||||
// lowers `arr[i]` to `LDA base,X` (or `ZeroPageX`), and the
|
||||
// 6502's X register is 8 bits, so elements whose byte
|
||||
// offset is >= 256 are unreachable. For a `u8` array the
|
||||
// safe max count is 256; for a `u16` array it's 128
|
||||
// (since the codegen doesn't scale the index by element
|
||||
// width — see the note in `emit_bounds_check`). This
|
||||
// diagnostic replaces the previous silent-skip in the
|
||||
// debug-mode bounds checker.
|
||||
if let NesType::Array(_, _) = &var.var_type {
|
||||
if size > 256 {
|
||||
self.diagnostics.push(
|
||||
Diagnostic::warning(
|
||||
ErrorCode::W0108,
|
||||
format!(
|
||||
"array '{}' has byte size {size}, but the 6502's 8-bit X index can only reach the first 256 bytes — elements past that are unreachable",
|
||||
var.name
|
||||
),
|
||||
var.span,
|
||||
)
|
||||
.with_help(
|
||||
"shrink the array, split it across multiple smaller arrays, or use separate fields for each element".to_string(),
|
||||
),
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
let Some(address) = self.allocate_ram(size, var.span) else {
|
||||
// Allocation failed (E0301 already emitted) — still add the
|
||||
// symbol so that later references don't cascade into E0502,
|
||||
|
|
|
|||
|
|
@ -1661,3 +1661,77 @@ fn analyze_fast_var_underscore_exempt() {
|
|||
result.diagnostics
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn analyze_oversized_array_warns_w0108() {
|
||||
// A u8 array with 300 elements has byte size 300 > 256. The
|
||||
// codegen lowers `arr[i]` to `LDA base,X` with X 8-bit, so
|
||||
// elements 256..299 are unreachable. W0108 should fire.
|
||||
let result = analyze_ok(
|
||||
r#"
|
||||
game "T" { mapper: NROM }
|
||||
var big: u8[300]
|
||||
on frame {
|
||||
big[0] = 0
|
||||
wait_frame
|
||||
}
|
||||
start Main
|
||||
"#,
|
||||
);
|
||||
assert!(
|
||||
result
|
||||
.diagnostics
|
||||
.iter()
|
||||
.any(|d| d.code == ErrorCode::W0108),
|
||||
"oversized u8 array should emit W0108, got: {:?}",
|
||||
result.diagnostics
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn analyze_boundary_size_256_array_ok() {
|
||||
// A u8[256] exactly fills the 8-bit X register — every element
|
||||
// is reachable. No W0108.
|
||||
let result = analyze_ok(
|
||||
r#"
|
||||
game "T" { mapper: NROM }
|
||||
var big: u8[256]
|
||||
on frame {
|
||||
big[0] = 0
|
||||
wait_frame
|
||||
}
|
||||
start Main
|
||||
"#,
|
||||
);
|
||||
assert!(
|
||||
!result
|
||||
.diagnostics
|
||||
.iter()
|
||||
.any(|d| d.code == ErrorCode::W0108),
|
||||
"u8[256] should not emit W0108, got: {:?}",
|
||||
result.diagnostics
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn analyze_small_array_never_warns_w0108() {
|
||||
let result = analyze_ok(
|
||||
r#"
|
||||
game "T" { mapper: NROM }
|
||||
var small: u8[16]
|
||||
on frame {
|
||||
small[0] = 0
|
||||
wait_frame
|
||||
}
|
||||
start Main
|
||||
"#,
|
||||
);
|
||||
assert!(
|
||||
!result
|
||||
.diagnostics
|
||||
.iter()
|
||||
.any(|d| d.code == ErrorCode::W0108),
|
||||
"small array should not emit W0108, got: {:?}",
|
||||
result.diagnostics
|
||||
);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -182,6 +182,24 @@ impl Assembler {
|
|||
self.output.push(0); // placeholder
|
||||
}
|
||||
}
|
||||
AddressingMode::LabelAbsoluteX(name) => {
|
||||
// `STA label,X` style indexed store with a label-
|
||||
// resolved base address. Encodes like `absolute,X`
|
||||
// but the 16-bit address is patched in by the
|
||||
// fixup pass, same as plain `Label` fixups.
|
||||
if let Some(op) = opcodes::encode(inst.opcode, &AddressingMode::AbsoluteX(0)) {
|
||||
self.output.push(op);
|
||||
self.fixups.push(Fixup {
|
||||
offset: self.output.len(),
|
||||
label: name.clone(),
|
||||
kind: FixupKind::Absolute,
|
||||
});
|
||||
self.output.push(0); // placeholder low byte
|
||||
self.output.push(0); // placeholder high byte
|
||||
} else {
|
||||
panic!("opcode {:?} cannot target an absolute,X label", inst.opcode);
|
||||
}
|
||||
}
|
||||
AddressingMode::SymbolLo(name) => {
|
||||
if let Some(op) = opcodes::encode(inst.opcode, &AddressingMode::Immediate(0)) {
|
||||
self.output.push(op);
|
||||
|
|
|
|||
|
|
@ -79,6 +79,13 @@ pub enum AddressingMode {
|
|||
// Pre-resolution symbolic forms
|
||||
Label(String),
|
||||
LabelRelative(String),
|
||||
/// Absolute-X indexed form targeting a named label — resolves
|
||||
/// to the 16-bit address of `label` at fix-up time, with the
|
||||
/// instruction encoded as `absolute,X`. Used by `UxROM`'s
|
||||
/// `__bank_select` to write into the bus-conflict table
|
||||
/// (`STA __bank_select_table,X`) where the target address has
|
||||
/// to be resolved by the linker.
|
||||
LabelAbsoluteX(String),
|
||||
SymbolLo(String),
|
||||
SymbolHi(String),
|
||||
|
||||
|
|
@ -103,7 +110,11 @@ impl AddressingMode {
|
|||
| Self::IndirectY(_)
|
||||
| Self::Relative(_) => 1,
|
||||
Self::Absolute(_) | Self::AbsoluteX(_) | Self::AbsoluteY(_) | Self::Indirect(_) => 2,
|
||||
Self::Label(_) | Self::LabelRelative(_) | Self::SymbolLo(_) | Self::SymbolHi(_) => 0,
|
||||
Self::Label(_)
|
||||
| Self::LabelRelative(_)
|
||||
| Self::LabelAbsoluteX(_)
|
||||
| Self::SymbolLo(_)
|
||||
| Self::SymbolHi(_) => 0,
|
||||
// `Bytes` is the full emitted payload — the assembler
|
||||
// skips the usual opcode byte for `NOP+Bytes` and writes
|
||||
// the raw vector, so the whole thing is operand.
|
||||
|
|
@ -125,7 +136,11 @@ impl AddressingMode {
|
|||
Self::Absolute(v) | Self::AbsoluteX(v) | Self::AbsoluteY(v) | Self::Indirect(v) => {
|
||||
v.to_le_bytes().to_vec()
|
||||
}
|
||||
Self::Label(_) | Self::LabelRelative(_) | Self::SymbolLo(_) | Self::SymbolHi(_) => {
|
||||
Self::Label(_)
|
||||
| Self::LabelRelative(_)
|
||||
| Self::LabelAbsoluteX(_)
|
||||
| Self::SymbolLo(_)
|
||||
| Self::SymbolHi(_) => {
|
||||
vec![]
|
||||
}
|
||||
Self::Bytes(v) => v.clone(),
|
||||
|
|
|
|||
|
|
@ -452,13 +452,17 @@ impl<'a> IrCodeGen<'a> {
|
|||
if size == 0 {
|
||||
return;
|
||||
}
|
||||
// Anything >= 256 would overflow the u8 immediate; skip
|
||||
// the check rather than emit a bogus compare. A proper
|
||||
// 16-bit bounds check would need a two-byte compare
|
||||
// against the high byte too.
|
||||
let Ok(size_u8) = u8::try_from(size) else {
|
||||
// Sizes > 256 skip the bounds check because the X register
|
||||
// is 8 bits — any index the codegen can actually produce is
|
||||
// already in-bounds for a byte count that large. The
|
||||
// analyzer's W0108 warning fires at declaration time so the
|
||||
// user knows those elements are unreachable. Size == 256
|
||||
// fits in a `CMP #$00` (trivially true), so we clamp to
|
||||
// 255 — the tightest useful check — and treat 256+ the same.
|
||||
let size_u8 = u8::try_from(size.min(255)).unwrap_or(255);
|
||||
if size_u8 == 0 {
|
||||
return;
|
||||
};
|
||||
}
|
||||
// Use a short BCC over an unconditional JMP instead of a
|
||||
// plain `BCS __debug_halt`. A single BCS can only span 127
|
||||
// bytes, and `__debug_halt` is emitted at the very end of
|
||||
|
|
|
|||
|
|
@ -42,6 +42,7 @@ pub enum ErrorCode {
|
|||
W0105, // palette sub-palette universal mismatch (mirror collision)
|
||||
W0106, // implicit drop of non-void function return value
|
||||
W0107, // `fast` variable rarely accessed (wastes zero-page slot)
|
||||
W0108, // array elements past byte 255 unreachable via 8-bit X index
|
||||
}
|
||||
|
||||
impl fmt::Display for ErrorCode {
|
||||
|
|
@ -68,6 +69,7 @@ impl fmt::Display for ErrorCode {
|
|||
Self::W0105 => "W0105",
|
||||
Self::W0106 => "W0106",
|
||||
Self::W0107 => "W0107",
|
||||
Self::W0108 => "W0108",
|
||||
};
|
||||
write!(f, "{code}")
|
||||
}
|
||||
|
|
@ -82,7 +84,8 @@ impl ErrorCode {
|
|||
| Self::W0104
|
||||
| Self::W0105
|
||||
| Self::W0106
|
||||
| Self::W0107 => Level::Warning,
|
||||
| Self::W0107
|
||||
| Self::W0108 => Level::Warning,
|
||||
_ => Level::Error,
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1455,11 +1455,28 @@ pub fn gen_bank_select(mapper: Mapper) -> Vec<Instruction> {
|
|||
out.push(Instruction::implied(RTS));
|
||||
}
|
||||
Mapper::UxROM => {
|
||||
// UxROM: write bank number to any address in $8000-$FFFF.
|
||||
// We use $FFF0 so the write lands in the fixed bank's
|
||||
// tail area where the linker can back it with a matching
|
||||
// bank-table byte to avoid bus conflicts.
|
||||
out.push(Instruction::new(STA, AM::Absolute(0xFFF0)));
|
||||
// UxROM: write the bank number to any address in
|
||||
// $8000-$FFFF. On boards with bus conflicts the CPU's
|
||||
// write and the ROM byte at that address are ANDed on
|
||||
// the data bus, so we must write to an address whose
|
||||
// ROM byte already equals the bank number. The linker
|
||||
// splices a 256-byte table (`__bank_select_table`,
|
||||
// bytes 0..255) into the fixed bank, and we index into
|
||||
// it with X = bank number: `STA __bank_select_table, X`
|
||||
// stores A (= bank number) at
|
||||
// `__bank_select_table + X`, whose ROM byte is exactly
|
||||
// X, so bus = A = X = ROM — no conflict.
|
||||
//
|
||||
// Previously this wrote to a fixed `$FFF0`, which
|
||||
// happens to work on emulators that don't simulate bus
|
||||
// conflicts (jsnes, Mesen permissive) but would glitch
|
||||
// on real hardware because a single ROM byte can't
|
||||
// match every possible bank number.
|
||||
out.push(Instruction::implied(TAX));
|
||||
out.push(Instruction::new(
|
||||
STA,
|
||||
AM::LabelAbsoluteX("__bank_select_table".into()),
|
||||
));
|
||||
out.push(Instruction::implied(RTS));
|
||||
}
|
||||
Mapper::MMC3 => {
|
||||
|
|
|
|||
|
|
@ -674,14 +674,32 @@ fn bank_select_mmc1_serializes_five_bits_to_e000() {
|
|||
}
|
||||
|
||||
#[test]
|
||||
fn bank_select_uxrom_writes_fff0() {
|
||||
// UxROM bank-select writes A to $FFF0, which lives in the
|
||||
// fixed bank's bus-conflict-safe table.
|
||||
fn bank_select_uxrom_uses_bus_conflict_table() {
|
||||
// UxROM bank-select has to write to a ROM address whose byte
|
||||
// already equals the bank being selected. The routine does
|
||||
// `TAX; STA __bank_select_table, X` so the store goes to
|
||||
// `table + bank_num`, whose ROM byte is `bank_num` — making
|
||||
// the bus write match the ROM byte regardless of which bank
|
||||
// is being requested. We assert both pieces here so a
|
||||
// regression back to the broken `STA $FFF0` form fails
|
||||
// loudly.
|
||||
let sel = gen_bank_select(Mapper::UxROM);
|
||||
let has_write = sel
|
||||
let has_tax = sel.iter().any(|i| i.opcode == TAX && i.mode == AM::Implied);
|
||||
assert!(has_tax, "UxROM bank-select must TAX before the store");
|
||||
let has_indexed_store = sel.iter().any(|i| {
|
||||
i.opcode == STA && matches!(&i.mode, AM::LabelAbsoluteX(n) if n == "__bank_select_table")
|
||||
});
|
||||
assert!(
|
||||
has_indexed_store,
|
||||
"UxROM bank-select must `STA __bank_select_table,X`"
|
||||
);
|
||||
let writes_fff0 = sel
|
||||
.iter()
|
||||
.any(|i| i.opcode == STA && i.mode == AM::Absolute(0xFFF0));
|
||||
assert!(has_write, "UxROM bank-select must write to $FFF0");
|
||||
assert!(
|
||||
!writes_fff0,
|
||||
"UxROM bank-select must not fall back to the bus-conflict-unsafe STA $FFF0 form"
|
||||
);
|
||||
assert_eq!(sel.last().unwrap().opcode, RTS);
|
||||
}
|
||||
|
||||
|
|
@ -723,7 +741,15 @@ fn bank_select_stashes_bank_number_in_zp() {
|
|||
#[test]
|
||||
fn bank_select_assembles_for_every_mapper() {
|
||||
for m in [Mapper::NROM, Mapper::MMC1, Mapper::UxROM, Mapper::MMC3] {
|
||||
let sel = gen_bank_select(m);
|
||||
let mut sel = gen_bank_select(m);
|
||||
// UxROM `gen_bank_select` references `__bank_select_table`
|
||||
// via `AM::LabelAbsoluteX`; give the assembler something
|
||||
// to resolve against in this standalone unit test. Real
|
||||
// linking appends the table in the linker pass, so the
|
||||
// label always resolves there.
|
||||
if m == Mapper::UxROM {
|
||||
sel.extend(super::gen_uxrom_bank_table());
|
||||
}
|
||||
let result = asm::assemble(&sel, 0xC000);
|
||||
assert!(
|
||||
!result.bytes.is_empty(),
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue