Decomp linter warning for offset reuse (#342)

* Linter warning for offset reuse in codebase

* Fix repeated offset problems

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;

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

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()

LEGO1/legoworld.cpp

@@ -122,7 +122,7 @@ void LegoWorld::FUN_10073400()
 {
 }
 
-// STUB: LEGO1 0x10073400
+// STUB: LEGO1 0x10073430
 void LegoWorld::FUN_10073430()
 {
 }

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";
 	}
 

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";
 	}
 

LEGO1/mxdsselectaction.h

@@ -49,7 +49,4 @@ class MxDSSelectAction : public MxDSParallelAction {
 // FUNCTION: LEGO1 0x100cbd00
 // MxStringListCursor::~MxStringListCursor
 
-// TEMPLATE: LEGO1 0x100cc450
-// MxListEntry<MxString>::GetValue
-
 #endif // MXDSSELECTACTION_H

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";
 	}
 

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";
 	}
 

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()

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