From 994d17a85e2fb610bb860ee7458d89fc6ae52a43 Mon Sep 17 00:00:00 2001 From: MS Date: Sun, 17 Dec 2023 17:45:33 -0500 Subject: [PATCH] Decomp linter warning for offset reuse (#342) * Linter warning for offset reuse in codebase * Fix repeated offset problems --- LEGO1/isle.cpp | 2 +- LEGO1/legogamestate.cpp | 1 - LEGO1/legoomni.cpp | 4 +-- LEGO1/legoworld.cpp | 2 +- LEGO1/motorcycle.h | 2 +- LEGO1/mxdsbuffer.h | 2 +- LEGO1/mxdsselectaction.h | 3 -- LEGO1/mxramstreamcontroller.h | 2 +- LEGO1/score.h | 2 +- tools/isledecomp/isledecomp/parser/linter.py | 36 +++++++++++++++++--- tools/isledecomp/tests/test_linter.py | 29 ++++++++++++++++ 11 files changed, 68 insertions(+), 17 deletions(-) diff --git a/LEGO1/isle.cpp b/LEGO1/isle.cpp index cefd6009..2d1d856a 100644 --- a/LEGO1/isle.cpp +++ b/LEGO1/isle.cpp @@ -176,7 +176,7 @@ void Isle::VTable0x68(MxBool p_add) // TODO } -// STUB: LEGO1 0x10031820 +// STUB: LEGO1 0x100327a0 MxLong Isle::HandleTransitionEnd() { return 0; diff --git a/LEGO1/legogamestate.cpp b/LEGO1/legogamestate.cpp index 29f539ac..62a30db4 100644 --- a/LEGO1/legogamestate.cpp +++ b/LEGO1/legogamestate.cpp @@ -43,7 +43,6 @@ ColorStringStruct g_colorSaveData[43] = { // NOTE: This offset = the end of the variables table, the last entry // in that table is a special entry, the string "END_OF_VARIABLES" -// GLOBAL: LEGO1 0x100f3e50 extern const char* g_endOfVariables; // FUNCTION: LEGO1 0x10039550 diff --git a/LEGO1/legoomni.cpp b/LEGO1/legoomni.cpp index ff695c63..e401f51a 100644 --- a/LEGO1/legoomni.cpp +++ b/LEGO1/legoomni.cpp @@ -112,8 +112,8 @@ const char* g_current = "current"; // GLOBAL: LEGO1 0x101020e8 void (*g_omniUserMessage)(const char*, int); -// GLOBAL: LEGO1 0x100f4c54 -MxBool g_isWorldActive; +// GLOBAL: LEGO1 0x100f4c58 +MxBool g_isWorldActive = TRUE; // FUNCTION: LEGO1 0x10015700 LegoOmni* Lego() diff --git a/LEGO1/legoworld.cpp b/LEGO1/legoworld.cpp index 11467954..48c95c4f 100644 --- a/LEGO1/legoworld.cpp +++ b/LEGO1/legoworld.cpp @@ -122,7 +122,7 @@ void LegoWorld::FUN_10073400() { } -// STUB: LEGO1 0x10073400 +// STUB: LEGO1 0x10073430 void LegoWorld::FUN_10073430() { } diff --git a/LEGO1/motorcycle.h b/LEGO1/motorcycle.h index 3b847873..a698fcd8 100644 --- a/LEGO1/motorcycle.h +++ b/LEGO1/motorcycle.h @@ -13,7 +13,7 @@ class Motorcycle : public IslePathActor { // FUNCTION: LEGO1 0x10035840 inline virtual const char* ClassName() const override // vtable+0x0c { - // GLOBAL: LEGO1 0x10035840 + // GLOBAL: LEGO1 0x100f38e8 return "Motorcycle"; } diff --git a/LEGO1/mxdsbuffer.h b/LEGO1/mxdsbuffer.h index 1a37ba2a..af9b466f 100644 --- a/LEGO1/mxdsbuffer.h +++ b/LEGO1/mxdsbuffer.h @@ -27,7 +27,7 @@ class MxDSBuffer : public MxCore { // FUNCTION: LEGO1 0x100c6500 inline virtual const char* ClassName() const override // vtable+0x0c { - // GLOBAL: LEGO1 0x100f0568 + // GLOBAL: LEGO1 0x101025b8 return "MxDSBuffer"; } diff --git a/LEGO1/mxdsselectaction.h b/LEGO1/mxdsselectaction.h index e4f09433..187e3a72 100644 --- a/LEGO1/mxdsselectaction.h +++ b/LEGO1/mxdsselectaction.h @@ -49,7 +49,4 @@ class MxDSSelectAction : public MxDSParallelAction { // FUNCTION: LEGO1 0x100cbd00 // MxStringListCursor::~MxStringListCursor -// TEMPLATE: LEGO1 0x100cc450 -// MxListEntry::GetValue - #endif // MXDSSELECTACTION_H diff --git a/LEGO1/mxramstreamcontroller.h b/LEGO1/mxramstreamcontroller.h index df7f12a9..008fe458 100644 --- a/LEGO1/mxramstreamcontroller.h +++ b/LEGO1/mxramstreamcontroller.h @@ -14,7 +14,7 @@ class MxRAMStreamController : public MxStreamController { // FUNCTION: LEGO1 0x100b9430 inline virtual const char* ClassName() const override // vtable+0xc { - // GLOBAL: LEGO1 0x10102130 + // GLOBAL: LEGO1 0x10102118 return "MxRAMStreamController"; } diff --git a/LEGO1/score.h b/LEGO1/score.h index 03a20a0a..89d9608d 100644 --- a/LEGO1/score.h +++ b/LEGO1/score.h @@ -18,7 +18,7 @@ class Score : public LegoWorld { // FUNCTION: LEGO1 0x100010c0 inline virtual const char* ClassName() const override // vtable+0x0c { - // GLOBAL: LEGO1 0x100f0058 + // GLOBAL: LEGO1 0x100f0050 return "Score"; } diff --git a/tools/isledecomp/isledecomp/parser/linter.py b/tools/isledecomp/isledecomp/parser/linter.py index 99c8e272..54406ce9 100644 --- a/tools/isledecomp/isledecomp/parser/linter.py +++ b/tools/isledecomp/isledecomp/parser/linter.py @@ -1,6 +1,7 @@ from typing import List, Optional from .parser import DecompParser from .error import ParserAlert, ParserError +from .node import ParserSymbol def get_checkorder_filter(module): @@ -14,16 +15,39 @@ def __init__(self) -> None: self._parser = DecompParser() self._filename: str = "" self._module: Optional[str] = None + # Set of (str, int) tuples for each module/offset pair seen while scanning. + # This is _not_ reset between files and is intended to report offset reuse + # when scanning the entire directory. + self._offsets_used = set() - def reset(self): + def reset(self, full_reset: bool = False): self.alerts = [] self._parser.reset() self._filename = "" self._module = None + if full_reset: + self._offsets_used.clear() + def file_is_header(self): return self._filename.lower().endswith(".h") + def _load_offsets_from_list(self, marker_list: List[ParserSymbol]): + """Helper for loading (module, offset) tuples while the DecompParser + has them broken up into three different lists.""" + for marker in marker_list: + value = (marker.module, marker.offset) + if value in self._offsets_used: + self.alerts.append( + ParserAlert( + code=ParserError.DUPLICATE_OFFSET, + line_number=marker.line_number, + line=f"0x{marker.offset:08x}", + ) + ) + else: + self._offsets_used.add(value) + def _check_function_order(self): """Rules: 1. Only markers that are implemented in the file are considered. This means we @@ -55,8 +79,9 @@ def _check_function_order(self): last_offset = fun.offset def _check_offset_uniqueness(self): - # TODO - pass + self._load_offsets_from_list(self._parser.functions) + self._load_offsets_from_list(self._parser.vtables) + self._load_offsets_from_list(self._parser.variables) def _check_byname_allowed(self): if self.file_is_header(): @@ -75,7 +100,7 @@ def check_lines(self, lines, filename, module=None): """`lines` is a generic iterable to allow for testing with a list of strings. We assume lines has the entire contents of the compilation unit.""" - self.reset() + self.reset(False) self._filename = filename self._module = module @@ -84,9 +109,10 @@ def check_lines(self, lines, filename, module=None): self._parser.finish() self.alerts = self._parser.alerts[::] + self._check_offset_uniqueness() + if self._module is not None: self._check_byname_allowed() - self._check_offset_uniqueness() if not self.file_is_header(): self._check_function_order() diff --git a/tools/isledecomp/tests/test_linter.py b/tools/isledecomp/tests/test_linter.py index cb3b8f27..84ce6495 100644 --- a/tools/isledecomp/tests/test_linter.py +++ b/tools/isledecomp/tests/test_linter.py @@ -70,6 +70,7 @@ def test_module_isolation(linter): ] assert linter.check_lines(lines, "test.cpp", "TEST") is True + linter.reset(True) assert linter.check_lines(lines, "test.cpp", "ALPHA") is True @@ -81,5 +82,33 @@ def test_byname_headers_only(linter): ] assert linter.check_lines(lines, "test.h", "TEST") is True + linter.reset(True) assert linter.check_lines(lines, "test.cpp", "TEST") is False assert linter.alerts[0].code == ParserError.BYNAME_FUNCTION_IN_CPP + + +def test_duplicate_offsets(linter): + """The linter will retain module/offset pairs found until we do a full reset.""" + lines = [ + "// FUNCTION: TEST 0x1000", + "// FUNCTION: HELLO 0x1000", + "// MyClass::~MyClass", + ] + + # Should not fail for duplicate offset 0x1000 because the modules are unique. + assert linter.check_lines(lines, "test.h", "TEST") is True + + # Simulate a failure by reading the same file twice. + assert linter.check_lines(lines, "test.h", "TEST") is False + + # Two errors because offsets from both modules are duplicated + assert len(linter.alerts) == 2 + assert all(a.code == ParserError.DUPLICATE_OFFSET for a in linter.alerts) + + # Partial reset will retain the list of seen offsets. + linter.reset(False) + assert linter.check_lines(lines, "test.h", "TEST") is False + + # Full reset will forget seen offsets. + linter.reset(True) + assert linter.check_lines(lines, "test.h", "TEST") is True