From a3989fb9c5424f2f93c67669102ee8cb61303c24 Mon Sep 17 00:00:00 2001 From: Miquel Sabaté Solà Date: Tue, 2 Sep 2025 22:23:19 +0200 Subject: Add a warning for each unused variable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Miquel Sabaté Solà --- .gitignore | 1 + lib/xixanta/src/assembler.rs | 66 ++++++++++++++++++++++++++------------ lib/xixanta/src/object.rs | 32 ++++++++++++------ scripts/test-e2e.sh | 6 ++++ tests/expected/unused-warning.txt | 1 + tests/expected/unused.nes | Bin 0 -> 22 bytes tests/unused.s | 18 +++++++++++ 7 files changed, 93 insertions(+), 31 deletions(-) create mode 100644 tests/expected/unused-warning.txt create mode 100644 tests/expected/unused.nes create mode 100644 tests/unused.s diff --git a/.gitignore b/.gitignore index 4e70798..d80deba 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,5 @@ /target /*.nes tests/out/*.nes +tests/out/*.txt TODO diff --git a/lib/xixanta/src/assembler.rs b/lib/xixanta/src/assembler.rs index bdc2e9d..f63e785 100644 --- a/lib/xixanta/src/assembler.rs +++ b/lib/xixanta/src/assembler.rs @@ -397,6 +397,7 @@ impl<'a> Assembler<'a> { object_type: ObjectType::Value, asan_ignore: false, asan_reserve: 1, + accessed: 0, }; if let Err(err) = self.context.set_variable(&var_name, &var_value, false) { @@ -504,6 +505,7 @@ impl<'a> Assembler<'a> { object_type: ObjectType::Value, asan_ignore: self.asan_next_ignore, asan_reserve: self.asan_next_reserve, + accessed: 0, }, false, ) { @@ -674,6 +676,7 @@ impl<'a> Assembler<'a> { object_type: ObjectType::Address, asan_ignore: false, asan_reserve: 1, + accessed: 0, }; if !node.value.is_empty() { @@ -852,31 +855,50 @@ impl<'a> Assembler<'a> { continue; } + // If it doesn't have the proper prefix, skip as well. + if !name.starts_with("zp_") && !name.starts_with("m_") && !name.starts_with("wr_") { + continue; + } + + // Build up the memory range object for this bundle. + let val = bundle.bundle.value() as usize; + let range = MemoryRange { + range: (val..val + bundle.asan_reserve as usize), + name: name.clone(), + }; + + // Check if this variable was ever accessed and warn about + // it. Even if later it conflicts with another memory range, the + // fact that it's not used is not a danger and hence a conflict + // does not have to be reported. Hence, skip after issueing the + // warning. + if bundle.accessed == 0 { + self.warnings.push(Error { + line: 0, + message: format!("variable {range} is unused"), + source: self.sources[0].clone(), + global: true, + }); + continue; + } + // Evaluate if the given object conflicts with an existing // range. - if name.starts_with("zp_") || name.starts_with("m_") || name.starts_with("wr_") { - let val = bundle.bundle.value() as usize; - let range = MemoryRange { - range: (val..val + bundle.asan_reserve as usize), - name: name.clone(), - }; - - for existing in &memory.memory_ranges { - if (range.range.start >= existing.range.start - && range.range.start < existing.range.end) - || (range.range.end > existing.range.start - && range.range.end < existing.range.end) - { - errors.push(Error { - line: 0, - global: true, - message: format!("The variable {range} conflicts with {existing}",), - source: self.sources[0].clone(), - }); - } + for existing in &memory.memory_ranges { + if (range.range.start >= existing.range.start + && range.range.start < existing.range.end) + || (range.range.end > existing.range.start + && range.range.end < existing.range.end) + { + errors.push(Error { + line: 0, + global: true, + message: format!("The variable {range} conflicts with {existing}",), + source: self.sources[0].clone(), + }); } - memory.memory_ranges.push(range); } + memory.memory_ranges.push(range); // Increase the counters for memory usage on either RAM slot and // check for bounds. @@ -1041,6 +1063,7 @@ impl<'a> Assembler<'a> { object_type: ObjectType::Value, asan_ignore: false, asan_reserve: 1, + accessed: 0, }; // Note that we overwrite the variable value from previous @@ -1841,6 +1864,7 @@ impl<'a> Assembler<'a> { asan_ignore: false, object_type: ObjectType::Value, asan_reserve: 1, + accessed: 0, }, true, ) { diff --git a/lib/xixanta/src/object.rs b/lib/xixanta/src/object.rs index 920a4f8..049798f 100644 --- a/lib/xixanta/src/object.rs +++ b/lib/xixanta/src/object.rs @@ -139,6 +139,9 @@ pub struct Object { /// Amount of bytes reserved for this object on the address sanitizer. pub asan_reserve: u8, + + /// Number of times this object was accessed. Incremented by `get_variable`. + pub accessed: usize, } impl Object { @@ -152,6 +155,7 @@ impl Object { object_type, asan_ignore: false, asan_reserve: 1, + accessed: 0, } } } @@ -198,7 +202,7 @@ impl Context { /// this `id` can be scoped or not, and this function will try to pick the /// variable from the right scope. The value itself will be resolved if the /// type is ObjectType::Address. - pub fn get_variable(&self, id: &PString, mappings: &[Mapping]) -> Result { + pub fn get_variable(&mut self, id: &PString, mappings: &[Mapping]) -> Result { // First of all, figure out the name of the scope and the real name of // the variable. If this was not scoped at all (None case when trying to // rsplit by the "::" operator), then we assume on the current scope. @@ -207,25 +211,32 @@ impl Context { None => (self.name(), id.value.as_str()), }; - self.get_variable_in_scope(scope_name, var_name, mappings) + let scope_name_obj = scope_name.to_string(); + self.get_variable_in_scope(&scope_name_obj, var_name, mappings) } // Get the `var_name` variable on the `scope_name` scope (or parents). For // further context, take the `mappings` into consideration when resolving // labels, and `line` when producing context errors. fn get_variable_in_scope( - &self, - scope_name: &str, + &mut self, + scope_name: &String, var_name: &str, mappings: &[Mapping], ) -> Result { // And with that, the only thing left is to find the scope and the // variable in it. - match self.map.get(scope_name) { - Some(scope) => match scope.get(var_name) { + match self.map.get_mut(scope_name) { + Some(scope) => match scope.get_mut(var_name) { Some(var) => match var.object_type { - ObjectType::Value => Ok(var.clone()), - ObjectType::Address => Ok(self.resolve_label(mappings, var)?), + ObjectType::Value => { + var.accessed += 1; + Ok(var.clone()) + } + ObjectType::Address => { + let var_to_resolve = var.clone(); + Ok(self.resolve_label(mappings, &var_to_resolve)?) + } }, None => { // If it cannot be found, then we have to move up through @@ -248,8 +259,9 @@ impl Context { // error from the recursive call, preserve the original // error so it better reflects the original scope where // this was first attempted. - let parent = self.parent(scope_name); - if let Ok(object) = self.get_variable_in_scope(parent, var_name, mappings) { + let parent = self.parent(scope_name).to_string(); + if let Ok(object) = self.get_variable_in_scope(&parent, var_name, mappings) + { Ok(object) } else { err diff --git a/scripts/test-e2e.sh b/scripts/test-e2e.sh index 844aee0..9da3a31 100755 --- a/scripts/test-e2e.sh +++ b/scripts/test-e2e.sh @@ -49,6 +49,12 @@ echo "test: custom => defines-two.nes" diff tests/out/defines-two.nes tests/expected/defines-two.nes exit_code=$((exit_code + $?)) +echo "test: custom => unused.nes" +./target/debug/nasm -c empty --asan -o tests/out/unused.nes tests/unused.s 2>tests/out/unused-warning.txt +diff tests/out/unused-warning.txt tests/expected/unused-warning.txt +diff tests/out/unused.nes tests/expected/unused.nes +exit_code=$((exit_code + $?)) + ## # code.nes diff --git a/tests/expected/unused-warning.txt b/tests/expected/unused-warning.txt new file mode 100644 index 0000000..fa97d8d --- /dev/null +++ b/tests/expected/unused-warning.txt @@ -0,0 +1 @@ +warning: variable 'zp_unused' ($00) is unused (unused.s) diff --git a/tests/expected/unused.nes b/tests/expected/unused.nes new file mode 100644 index 0000000..055b6ec Binary files /dev/null and b/tests/expected/unused.nes differ diff --git a/tests/unused.s b/tests/unused.s new file mode 100644 index 0000000..c26ac42 --- /dev/null +++ b/tests/unused.s @@ -0,0 +1,18 @@ +.segment "HEADER" + .byte 'N', 'E', 'S', $1A + .byte $02 + .byte $01 + .byte $00 + .byte $00 + +.segment "CODE" + +zp_unused = $00 +zp_used = $01 +zp_another = $02 ; asan:ignore +zp_more = $03 +zp_yet = $04 + +lda zp_used +lda (zp_more), y +lda #.lobyte(zp_yet) -- cgit v1.2.3