Match static function variables (#530)

* Match static function variables

* IsleApp::Tick static variables

CMakeLists.txt

@@ -415,8 +415,8 @@ set_property(TARGET lego1 PROPERTY SUFFIX ".DLL")
 if (ISLE_BUILD_APP)
   add_executable(isle WIN32
     ISLE/res/isle.rc
-    ISLE/isleapp.cpp
     ISLE/define.cpp
+    ISLE/isleapp.cpp
   )
 
   target_compile_definitions(isle PRIVATE ISLE_APP)

ISLE/define.cpp

@@ -32,9 +32,3 @@ int g_targetDepth = 16;
 
 // GLOBAL: ISLE 0x410064
 int g_reqEnableRMDevice = 0;
-
-// GLOBAL: ISLE 0x4101bc
-int g_startupDelay = 200;
-
-// GLOBAL: ISLE 0x4101c0
-MxLong g_lastFrameTime = 0;

ISLE/define.h

@@ -21,7 +21,5 @@ extern int g_targetWidth;
 extern int g_targetHeight;
 extern int g_targetDepth;
 extern int g_reqEnableRMDevice;
-extern int g_startupDelay;
-extern MxLong g_lastFrameTime;
 
 #endif // DEFINE_H

ISLE/isleapp.cpp

@@ -765,6 +765,12 @@ void IsleApp::LoadConfig()
 // FUNCTION: ISLE 0x402c20
 inline void IsleApp::Tick(BOOL sleepIfNotNextFrame)
 {
+	// GLOBAL: ISLE 0x4101c0
+	static MxLong g_lastFrameTime = 0;
+
+	// GLOBAL: ISLE 0x4101bc
+	static int g_startupDelay = 200;
+
 	if (!this->m_windowActive) {
 		Sleep(0);
 		return;

tools/isledecomp/isledecomp/compare/core.py

@@ -134,7 +134,9 @@ def _load_cvdump(self):
                 except UnicodeDecodeError:
                     pass
 
-            self._db.set_recomp_symbol(addr, sym.node_type, sym.name(), sym.size())
+            self._db.set_recomp_symbol(
+                addr, sym.node_type, sym.name(), sym.decorated_name, sym.size()
+            )
 
         for lineref in cv.lines:
             addr = self.recomp_bin.get_abs_addr(lineref.section, lineref.offset)
@@ -168,7 +170,12 @@ def _load_markers(self):
                 self._db.skip_compare(fun.offset)
 
         for var in codebase.iter_variables():
-            self._db.match_variable(var.offset, var.name)
+            if var.is_static and var.parent_function is not None:
+                self._db.match_static_variable(
+                    var.offset, var.name, var.parent_function
+                )
+            else:
+                self._db.match_variable(var.offset, var.name)
 
         for tbl in codebase.iter_vtables():
             self._db.match_vtable(tbl.offset, tbl.name)

tools/isledecomp/isledecomp/compare/db.py

@@ -12,6 +12,7 @@
         orig_addr int,
         recomp_addr int,
         name text,
+        decorated_name text,
         size int,
         should_skip int default(FALSE)
     );
@@ -65,12 +66,13 @@ def set_recomp_symbol(
         addr: int,
         compare_type: Optional[SymbolType],
         name: Optional[str],
+        decorated_name: Optional[str],
         size: Optional[int],
     ):
         compare_value = compare_type.value if compare_type is not None else None
         self._db.execute(
-            "INSERT INTO `symbols` (recomp_addr, compare_type, name, size) VALUES (?,?,?,?)",
+            "INSERT INTO `symbols` (recomp_addr, compare_type, name, decorated_name, size) VALUES (?,?,?,?,?)",
-            (addr, compare_value, name, size),
+            (addr, compare_value, name, decorated_name, size),
         )
 
     def get_unmatched_strings(self) -> List[str]:
@@ -214,6 +216,56 @@ def match_vtable(self, addr: int, name: str) -> bool:
 
         return did_match
 
+    def match_static_variable(self, addr: int, name: str, function_addr: int) -> bool:
+        """Matching a static function variable by combining the variable name
+        with the decorated (mangled) name of its parent function."""
+
+        cur = self._db.execute(
+            """SELECT name, decorated_name
+            FROM `symbols`
+            WHERE orig_addr = ?""",
+            (function_addr,),
+        )
+
+        if (result := cur.fetchone()) is None:
+            logger.error("No function for static variable: %s", name)
+            return False
+
+        # Get the friendly name for the "failed to match" error message
+        (function_name, decorated_name) = result
+
+        # Now we have to combine the variable name (read from the marker)
+        # and the decorated name of the enclosing function (the above variable)
+        # into a LIKE clause and try to match.
+        # For example, the variable "g_startupDelay" from function "IsleApp::Tick"
+        # has symbol: "?g_startupDelay@?1??Tick@IsleApp@@QAEXH@Z@4HA"
+        # The function's decorated name is: "?Tick@IsleApp@@QAEXH@Z"
+        cur = self._db.execute(
+            """UPDATE `symbols`
+            SET orig_addr = ?
+            WHERE name LIKE '%' || ? || '%' || ? || '%'
+            AND orig_addr IS NULL
+            AND (compare_type = ? OR compare_type = ? OR compare_type IS NULL)""",
+            (
+                addr,
+                name,
+                decorated_name,
+                SymbolType.DATA.value,
+                SymbolType.POINTER.value,
+            ),
+        )
+
+        did_match = cur.rowcount > 0
+
+        if not did_match:
+            logger.error(
+                "Failed to match static variable %s from function %s",
+                name,
+                function_name,
+            )
+
+        return did_match
+
     def match_variable(self, addr: int, name: str) -> bool:
         did_match = self._match_on(SymbolType.DATA, addr, name) or self._match_on(
             SymbolType.POINTER, addr, name

tools/isledecomp/isledecomp/parser/error.py

@@ -47,6 +47,10 @@ class ParserError(Enum):
     # to ignore things like string literal that are not variables.
     GLOBAL_NOT_VARIABLE = 111
 
+    # WARN: A marked static variable inside a function needs to have its
+    # function marked too, and in the same module.
+    ORPHANED_STATIC_VARIABLE = 112
+
     # This code or higher is an error, not a warning
     DECOMP_ERROR_START = 200
 

tools/isledecomp/isledecomp/parser/node.py

@@ -50,6 +50,7 @@ def is_nameref(self) -> bool:
 @dataclass
 class ParserVariable(ParserSymbol):
     is_static: bool = False
+    parent_function: Optional[int] = None
 
 
 @dataclass

tools/isledecomp/isledecomp/parser/parser.py

@@ -13,6 +13,7 @@
 )
 from .marker import (
     DecompMarker,
+    MarkerCategory,
     match_marker,
     is_marker_exact,
 )
@@ -53,6 +54,9 @@ def insert(self, marker: DecompMarker) -> bool:
         self.markers[key] = marker
         return False
 
+    def query(self, category: MarkerCategory, module: str) -> Optional[DecompMarker]:
+        return self.markers.get((category, module))
+
     def iter(self) -> Iterator[DecompMarker]:
         for _, marker in self.markers.items():
             yield marker
@@ -305,6 +309,23 @@ def _variable_done(
                     )
                 )
             else:
+                parent_function = None
+                is_static = self.state == ReaderState.IN_FUNC_GLOBAL
+
+                # If this is a static variable, we need to get the function
+                # where it resides so that we can match it up later with the
+                # mangled names of both variable and function from cvdump.
+                if is_static:
+                    fun_marker = self.fun_markers.query(
+                        MarkerCategory.FUNCTION, marker.module
+                    )
+
+                    if fun_marker is None:
+                        self._syntax_warning(ParserError.ORPHANED_STATIC_VARIABLE)
+                        continue
+
+                    parent_function = fun_marker.offset
+
                 self._symbols.append(
                     ParserVariable(
                         type=marker.type,
@@ -312,7 +333,8 @@ def _variable_done(
                         module=marker.module,
                         offset=marker.offset,
                         name=self.curly.get_prefix(variable_name),
-                        is_static=self.state == ReaderState.IN_FUNC_GLOBAL,
+                        is_static=is_static,
+                        parent_function=parent_function,
                     )
                 )
 

tools/isledecomp/tests/test_parser.py

@@ -419,8 +419,7 @@ def test_static_variable(parser):
     """We can detect whether a variable is a static function variable
     based on the parser's state when we detect it.
     Checking for the word `static` alone is not a good test.
-    Static class variables are filed as S_GDATA32, same as regular globals.
+    Static class variables are filed as S_GDATA32, same as regular globals."""
-    Only function statics are filed as S_LDATA32."""
 
     parser.read_lines(
         [
@@ -436,7 +435,7 @@ def test_static_variable(parser):
             "// FUNCTION: TEST 0x5555",
             "void test_function() {",
             "// GLOBAL: TEST 0x8888",
-            "int g_internal = 0;",
+            "static int g_internal = 0;",
             "}",
         ]
     )
@@ -629,3 +628,70 @@ def test_match_qualified_variable(parser):
     assert len(parser.variables) == 1
     assert parser.variables[0].name == "MxTest::g_count"
     assert len(parser.alerts) == 0
+
+
+def test_static_variable_parent(parser):
+    """Report the address of the parent function that contains a static variable."""
+
+    parser.read_lines(
+        [
+            "// FUNCTION: TEST 0x1234",
+            "void test()",
+            "{",
+            "   // GLOBAL: TEST 0x5555",
+            "   static int g_count = 0;",
+            "}",
+        ]
+    )
+
+    assert len(parser.variables) == 1
+    assert parser.variables[0].is_static is True
+    assert parser.variables[0].parent_function == 0x1234
+
+
+@pytest.mark.xfail(
+    reason="""Without the FUNCTION marker we don't know that we are inside a function,
+    so we do not identify this variable as static."""
+)
+def test_static_variable_no_parent(parser):
+    """If the function that contains a static variable is not marked, we
+    cannot match it with cvdump so we should skip it and report an error."""
+
+    parser.read_lines(
+        [
+            "void test()",
+            "{",
+            "   // GLOBAL: TEST 0x5555",
+            "   static int g_count = 0;",
+            "}",
+        ]
+    )
+
+    # No way to match this variable so don't report it
+    assert len(parser.variables) == 0
+    assert len(parser.alerts) == 1
+    assert parser.alerts[0].code == ParserError.ORPHANED_STATIC_VARIABLE
+
+
+def test_static_variable_incomplete_coverage(parser):
+    """If the function that contains a static variable is marked, but
+    not for each module used for the variable itself, this is an error."""
+
+    parser.read_lines(
+        [
+            "// FUNCTION: HELLO 0x1234",
+            "void test()",
+            "{",
+            "   // GLOBAL: HELLO 0x5555",
+            "   // GLOBAL: TEST 0x5555",
+            "   static int g_count = 0;",
+            "}",
+        ]
+    )
+
+    # Match for HELLO module
+    assert len(parser.variables) == 1
+
+    # Failed for TEST module
+    assert len(parser.alerts) == 1
+    assert parser.alerts[0].code == ParserError.ORPHANED_STATIC_VARIABLE