From 0664f0bad653ca750d320e513f685ab0c852f359 Mon Sep 17 00:00:00 2001 From: Miquel Sabaté Solà Date: Wed, 8 Jul 2026 21:13:00 +0200 Subject: Add a warning for unknown cross-mapping references MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Some segments, like the 'vectors' one, will reference code that is outside of its mapping. But in some other configurations, segments cannot make these cross-mapping references so happily. Imagine: .segment "SWAPPABLE" .proc foo rts .endproc .segment "FIXED" jsr foo Here the assembler will properly detect the address of 'foo' in the context of the 'SWAPPABLE' segment. But what this assembler doesn't know is that this segment is swappable (e.g. UNROM chip). Hence, if the bank being mapped right now is not the one containing the 'SWAPPABLE' segment, then the address computed for 'foo' and used in that 'jsr' instruction will point to something else entirely. This would be similar to a use-after-free bug. This is something that can only be inspected at runtime, and so the assembler cannot be of much help here. Hence, this commit adds a warning so the programmer can understand the potentially dangerous operation. All of that being said, this commit also adds support for "asan:safe" or "check:safe", which is a magic comment that the programmer can write to re-assure the assembler that this operation is fine (e.g. there is a guarantee that the mapped bank is that one we are expecting). Hence, the code above could now be written like so: jsr foo ; check:safe Signed-off-by: Miquel Sabaté Solà --- crates/nasm/README.md | 45 ++++++++++++++- lib/xixanta/src/assembler.rs | 84 +++++++++++++++++++++++++++- lib/xixanta/src/node.rs | 2 + lib/xixanta/src/object.rs | 5 ++ lib/xixanta/src/parser.rs | 15 +++++ scripts/test-e2e.sh | 7 +++ tests/expected/unrom.nes | Bin 0 -> 131088 bytes tests/expected/unrom.txt | 1 + tests/unrom.s | 129 +++++++++++++++++++++++++++++++++++++++++++ 9 files changed, 286 insertions(+), 2 deletions(-) create mode 100644 tests/expected/unrom.nes create mode 100644 tests/expected/unrom.txt create mode 100644 tests/unrom.s diff --git a/crates/nasm/README.md b/crates/nasm/README.md index 9a44eaa..2d24011 100644 --- a/crates/nasm/README.md +++ b/crates/nasm/README.md @@ -246,7 +246,7 @@ fit in a byte. Hence, you could have a code like follows: If you compile the code with `-D PAL=1`, then the first branch will be taken instead of the second one. -### Unused code +## Unused code This assembler will also issue a warning whenever it finds unreferenced variables or labels. Hence, if you have something like: @@ -269,6 +269,49 @@ an error, as `nasm` can rightly identify that this is dead code. This check can be skipped by providing the `--allow-unused` flag on nasm. +## Cross-mapping references + +This assembler will issue a warning whenever you are referencing an object which +is defined into a segment from another mapping. Some segments, like the +'vectors' one, will reference code that is outside of its mapping. But in some +other configurations, segments cannot make these cross-mapping references so +happily. Imagine that we have an UNROM chip configuration, where "SWAPPABLE" is +a segment that can be swapped according to the specification of this mapper +chip. Then, you could have code like this: + +```asm +.segment "SWAPPABLE" + +.proc foo + rts +.endproc + +.segment "FIXED" + +jsr foo +``` + +Here the assembler will properly detect the address of 'foo' in the context of +the 'SWAPPABLE' segment. But what this assembler doesn't know is that this +segment is swappable. Hence, if the bank being mapped right now is not the one +containing the 'SWAPPABLE' segment, then the address computed for 'foo' and used +in that 'jsr' instruction will point to something else entirely. This would be +similar to a use-after-free bug. + +This is something that can only be inspected at runtime, and so the assembler +cannot be of much help here. Hence, this commit adds a warning so the programmer +can understand the potentially dangerous operation. + +All of that being said, this assembler also adds support for "asan:safe" or +"check:safe", which is a magic comment that the programmer can write to +re-assure the assembler that this operation is fine (e.g. there is a guarantee +that the mapped bank is that one we are expecting). Hence, the code above could +now be written like so: + +```asm +jsr foo ; check:safe +``` + ## Address sanitizer This assembler comes with a set of tools that builds up an "address diff --git a/lib/xixanta/src/assembler.rs b/lib/xixanta/src/assembler.rs index 5b72fb5..98ae19d 100644 --- a/lib/xixanta/src/assembler.rs +++ b/lib/xixanta/src/assembler.rs @@ -156,6 +156,11 @@ struct Assembler<'a> { // sanitizer. asan_next_ignore: bool, + // Whether the next bundle should be marked as safe. That is, whether the + // checker can skip some of the checks for the bundle being produced from + // the next node. + asan_next_safe: bool, + // The amount of bytes to reserve for the next variable assignment. asan_next_reserve: usize, @@ -172,6 +177,13 @@ struct Assembler<'a> { // Stack of pending definitions. The last element from this list is the list // to be set to the next PendingNode push. pending_defines: Vec>, + + // List of objects that have been visited while evaluating a node. Reset + // this vector before calling evaluate_node(), and check the results + // afterwards to check which objects are evaluated along the way. The item + // is a tuple where the first member is the String identifier of the + // variable/address/label/etc. that is being stored in the second member. + objects_visited: Vec<(String, Object)>, } /// The result to be given at the end of `assembler::assemble` and @@ -435,10 +447,12 @@ impl<'a> Assembler<'a> { allow_unused: false, asan_enabled: false, asan_next_ignore: false, + asan_next_safe: false, asan_next_reserve: 1, asan_stack_definition: None, macro_context: vec![], pending_defines: vec![], + objects_visited: vec![], } } @@ -846,6 +860,7 @@ impl<'a> Assembler<'a> { affected_on_page: false, resolved: false, negative: false, + safe: false, }, node: None, mapping: self.current_mapping, @@ -879,6 +894,8 @@ impl<'a> Assembler<'a> { self.stage = Stage::Bundling; for node in nodes { + let mut next_safe = false; + match &node.node_type { // Initialize the label to the offset address of the current // segment. Note that this is only the offset from the beginning @@ -929,6 +946,10 @@ impl<'a> Assembler<'a> { next_ignore = true; self.asan_next_ignore = true; } + NodeType::Comment(CommentType::AsanSafe) => { + next_safe = true; + self.asan_next_safe = true; + } NodeType::Instruction => { self.literal_mode = None; match self.evaluate_node(node) { @@ -1001,7 +1022,12 @@ impl<'a> Assembler<'a> { _ => {} } - // Keep `self.asan_next_ignore` to false if possible. + // Keep 'self.asan_next_safe' to false if possible. + if !next_safe { + self.asan_next_safe = false; + } + + // Keep 'self.asan_next_ignore' to false if possible. if next_ignore { next_ignore = false; } else { @@ -1069,12 +1095,54 @@ impl<'a> Assembler<'a> { } } + // Reset the 'objects_visited' list so the evaluate_node() next to + // this can tell us the objects that were actually visited on this + // call. + self.objects_visited = vec![]; + self.literal_mode = None; match self.evaluate_node(&pn.node) { Ok(mut bundle) => { let current = &self.mappings[pn.mapping].segments[pn.segment]; bundle.address = current.bundles[pn.bundle_index].address; + // The first check deals on whether there is any reference + // that crosses the mapping boundary. There are two + // exceptions to this check: + // + // 1. We are currently on the Vector segment, which + // basically references addresses outside of it, and + // that is to be expected and safe. + // 2. If the bundle was marked as "safe" by the programmer + // via the asan:safe/check:safe comment. + // + // Otherwise iterate over the objects that were visited over + // the previous evaluate_node() call, and check for + // potentially dangerous cross references. Since we cannot + // be certain about these references, they are merely + // warnings, not errors. + if !current.bundles[pn.bundle_index].safe + && !matches!(self.mappings[pn.mapping].section_type, SectionType::Vector) + { + for (name, object) in &self.objects_visited { + if pn.mapping != object.mapping { + let reference = + &self.mappings[object.mapping].segments[object.segment].name; + + self.warnings.push(Error { + line: pn.node.value.line, + message: format!( + "referencing '{name}' from '{}', which belongs to the '{reference}' segment", + current.name + ), + source: self.source_for(&pn.node), + global: false, + expanded_from: pn.macro_context.clone(), + }); + } + } + } + // If we are trying to 'jmp'/'jsr' right into the next // instruction, then warn the programmer about it. This // looks silly but in practice it might happen inside of a @@ -1610,6 +1678,10 @@ impl<'a> Assembler<'a> { current.offset += bundle.size as usize; current.segments[self.current_segment].offset += bundle.size as usize; + // Mark this bundle as reference-safe, so the checker afterwards doesn't + // complain on different checks. + bundle.safe = self.asan_next_safe; + if !bundle.resolved { self.pending.push(PendingNode { mapping: self.current_mapping, @@ -1891,6 +1963,7 @@ impl<'a> Assembler<'a> { affected_on_page: false, resolved: false, negative: false, + safe: false, }), Stage::Crunching => { match self.context.get_relative_label( @@ -1977,6 +2050,7 @@ impl<'a> Assembler<'a> { affected_on_page: false, resolved: true, negative: false, + safe: false, }) } @@ -2043,6 +2117,7 @@ impl<'a> Assembler<'a> { affected_on_page: false, resolved: true, negative: false, + safe: false, }) } @@ -2130,6 +2205,7 @@ impl<'a> Assembler<'a> { affected_on_page: false, resolved: true, negative: false, + safe: false, }) } @@ -2641,6 +2717,7 @@ impl<'a> Assembler<'a> { affected_on_page: false, resolved: true, negative: false, + safe: false, }) } @@ -2892,6 +2969,11 @@ impl<'a> Assembler<'a> { fn evaluate_variable(&mut self, node: &PNode) -> Result { match self.context.get_variable(&node.value, &self.mappings) { Ok(mut value) => { + // Push this variable into the "visited objects" list. This list + // is to be cleared out or consumed by the caller. + self.objects_visited + .push((node.value.value.clone(), value.clone())); + // If the given variable has a zero value AND we are resolving // pending nodes AND the variable has a node object in it, then // chances are that this is a macro call with an argument to be diff --git a/lib/xixanta/src/node.rs b/lib/xixanta/src/node.rs index d91177e..4f25158 100644 --- a/lib/xixanta/src/node.rs +++ b/lib/xixanta/src/node.rs @@ -275,6 +275,7 @@ pub enum CommentType { AsanReserve(usize), AsanStack(Range), AsanIgnore, + AsanSafe, } /// The PNode type. @@ -378,6 +379,7 @@ impl fmt::Display for NodeType { CommentType::AsanReserve(_) => write!(f, ";; asan:reserve"), CommentType::AsanStack(_) => write!(f, ";; asan:stack"), CommentType::AsanIgnore => write!(f, ";; asan:ignore"), + CommentType::AsanSafe => write!(f, ";; asan:safe"), }, } } diff --git a/lib/xixanta/src/object.rs b/lib/xixanta/src/object.rs index 64d4298..b263469 100644 --- a/lib/xixanta/src/object.rs +++ b/lib/xixanta/src/object.rs @@ -32,6 +32,10 @@ pub struct Bundle { /// used for internal purposes only. pub resolved: bool, + /// Whether the programmer assured that the line that resulted into this + /// Bundle was safe to perform cross-mapping references. + pub safe: bool, + /// Whether we have to consider the bundle to contain a negative number. /// This comes from the fact that we just have a bunch of signednessless /// bytes, but using the negative unary operator will give us a hint on how @@ -67,6 +71,7 @@ impl Bundle { cycles: 0, affected_on_page: false, resolved: true, + safe: false, negative: false, } } diff --git a/lib/xixanta/src/parser.rs b/lib/xixanta/src/parser.rs index e004fd9..03039f1 100644 --- a/lib/xixanta/src/parser.rs +++ b/lib/xixanta/src/parser.rs @@ -249,6 +249,21 @@ impl Parser { source: self.current_source, }); } + "check:safe" | "asan:safe" => { + self.nodes.last_mut().unwrap().push(PNode { + node_type: NodeType::Comment(CommentType::AsanSafe), + value: PString { + value: cmd, + line: self.line, + start, + end: offset - 1, + }, + left: None, + right: None, + args: None, + source: self.current_source, + }); + } &_ => {} } diff --git a/scripts/test-e2e.sh b/scripts/test-e2e.sh index 071df71..43572b0 100755 --- a/scripts/test-e2e.sh +++ b/scripts/test-e2e.sh @@ -126,6 +126,13 @@ exit_code=$((exit_code + $?)) diff tests/out/unused_macro_arg.nes tests/expected/unused_macro_arg.nes exit_code=$((exit_code + $?)) +echo "test: custom => unrom.nes" +./target/debug/nasm -c unrom tests/unrom.s -o tests/out/unrom.nes 2>tests/out/unrom.txt +diff tests/out/unrom.txt tests/expected/unrom.txt +exit_code=$((exit_code + $?)) +diff tests/out/unrom.nes tests/expected/unrom.nes +exit_code=$((exit_code + $?)) + ## # code.nes diff --git a/tests/expected/unrom.nes b/tests/expected/unrom.nes new file mode 100644 index 0000000..06959e0 Binary files /dev/null and b/tests/expected/unrom.nes differ diff --git a/tests/expected/unrom.txt b/tests/expected/unrom.txt new file mode 100644 index 0000000..6ce1cd9 --- /dev/null +++ b/tests/expected/unrom.txt @@ -0,0 +1 @@ +warning: referencing 'hello_bank0' from 'FIXED', which belongs to the 'BANK0' segment (unrom.s: line 108) diff --git a/tests/unrom.s b/tests/unrom.s new file mode 100644 index 0000000..3761029 --- /dev/null +++ b/tests/unrom.s @@ -0,0 +1,129 @@ +.segment "HEADER" + .byte 'N', 'E', 'S', $1A + .byte $08 ; 128KB of PRG-ROM (8 x 16KB) + .byte $00 ; No CHR-ROM. Games using this chip used RAM instead. + + ;; On the 6th byte we have to set the lower nibble of the mapper (#%0010 for + ;; UNROM). + .byte $20 + + ;; Forcing iNES 2.0 format, which will help us for the next bytes. + .byte $08 + + ;; And now iNES 2.0-specific thingies. + .byte $00 ; No submapper + .byte $00 ; PRG ROM not 4 MiB or larger + .byte $00 ; No PRG RAM + .byte $07 ; 8192 (64 * 2^7) bytes CHR RAM, no battery + .byte $00 ; NTSC; use $01 for PAL + .byte $00 ; No special PPU + +.segment "VECTORS" + .addr nmi, reset, irq + +.segment "BANK0" + +hello_bank0: + lda #2 + sta $10 + rts + +.segment "BANK1" + +hello_bank1: + lda #3 + sta $11 + rts + +.segment "BANK2" +.byte $00 + +.segment "BANK3" +.byte $00 + +.segment "BANK4" +.byte $00 + +.segment "BANK5" +.byte $00 + +.segment "BANK6" +.byte $00 + +.segment "FIXED" + +banktable: + .byte $00, $01, $02, $03, $04, $05, $06 + +zp_current_bank = $00 + +bankswitch: + sty zp_current_bank + tya + sta banktable, y + rts + +.proc reset + sei + cld + ldx #$40 + stx $4017 + + ldx #$FF + txs + + inx + stx $2000 + stx $2001 + stx $4010 + + bit $2002 +@vblankwait1: + bit $2002 + bpl @vblankwait1 + + ldx #0 + lda #0 +@ram_reset_loop: + sta $000, x + sta $100, x + sta $200, x + sta $300, x + sta $400, x + sta $500, x + sta $600, x + sta $700, x + inx + bne @ram_reset_loop + +@vblankwait2: + bit $2002 + bpl @vblankwait2 + + ;;; + ;; NOTE: configuration/reset is done, the code below is our actual program :D + + ldy #0 + jsr bankswitch + jsr hello_bank0 + + ldy #1 + jsr bankswitch + jsr hello_bank1 ; check:safe + + lda $10 + clc + adc $11 + sta $12 + +@loop: + jmp @loop +.endproc + +.proc nmi + rti +.endproc + +.proc irq + rti +.endproc -- cgit v1.2.3