diff options
| -rw-r--r-- | crates/nasm/README.md | 45 | ||||
| -rw-r--r-- | lib/xixanta/src/assembler.rs | 84 | ||||
| -rw-r--r-- | lib/xixanta/src/node.rs | 2 | ||||
| -rw-r--r-- | lib/xixanta/src/object.rs | 5 | ||||
| -rw-r--r-- | lib/xixanta/src/parser.rs | 15 | ||||
| -rwxr-xr-x | scripts/test-e2e.sh | 7 | ||||
| -rw-r--r-- | tests/expected/unrom.nes | bin | 0 -> 131088 bytes | |||
| -rw-r--r-- | tests/expected/unrom.txt | 1 | ||||
| -rw-r--r-- | tests/unrom.s | 129 |
9 files changed, 286 insertions, 2 deletions
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<Vec<PendingDefine>>, + + // 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<Bundle, Error> { 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<usize>), 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 Binary files differnew file mode 100644 index 0000000..06959e0 --- /dev/null +++ b/tests/expected/unrom.nes 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 |