From 264b9e815be0e3c78aac9c9f23d9f737363dc2d9 Mon Sep 17 00:00:00 2001 From: MS Date: Mon, 5 Feb 2024 06:43:13 -0500 Subject: [PATCH] Match static function variables (#530) * Match static function variables * IsleApp::Tick static variables --- CMakeLists.txt | 2 +- ISLE/define.cpp | 6 -- ISLE/define.h | 2 - ISLE/isleapp.cpp | 6 ++ tools/isledecomp/isledecomp/compare/core.py | 11 ++- tools/isledecomp/isledecomp/compare/db.py | 56 ++++++++++++++- tools/isledecomp/isledecomp/parser/error.py | 4 ++ tools/isledecomp/isledecomp/parser/node.py | 1 + tools/isledecomp/isledecomp/parser/parser.py | 24 ++++++- tools/isledecomp/tests/test_parser.py | 72 +++++++++++++++++++- 10 files changed, 167 insertions(+), 17 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 53963915..cd2e2cd0 100644 --- a/CMakeLists.txt +++ b/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) diff --git a/ISLE/define.cpp b/ISLE/define.cpp index c1f020fa..48fd907e 100644 --- a/ISLE/define.cpp +++ b/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; diff --git a/ISLE/define.h b/ISLE/define.h index 3f92244b..6d1a59bb 100644 --- a/ISLE/define.h +++ b/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 diff --git a/ISLE/isleapp.cpp b/ISLE/isleapp.cpp index c4897a2b..1e61fdfb 100644 --- a/ISLE/isleapp.cpp +++ b/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; diff --git a/tools/isledecomp/isledecomp/compare/core.py b/tools/isledecomp/isledecomp/compare/core.py index 4e1a1f70..74352ba1 100644 --- a/tools/isledecomp/isledecomp/compare/core.py +++ b/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) diff --git a/tools/isledecomp/isledecomp/compare/db.py b/tools/isledecomp/isledecomp/compare/db.py index 2723b3ce..83447ff5 100644 --- a/tools/isledecomp/isledecomp/compare/db.py +++ b/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 (?,?,?,?)", - (addr, compare_value, name, size), + "INSERT INTO `symbols` (recomp_addr, compare_type, name, decorated_name, size) VALUES (?,?,?,?,?)", + (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 diff --git a/tools/isledecomp/isledecomp/parser/error.py b/tools/isledecomp/isledecomp/parser/error.py index 81123381..b838a394 100644 --- a/tools/isledecomp/isledecomp/parser/error.py +++ b/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 diff --git a/tools/isledecomp/isledecomp/parser/node.py b/tools/isledecomp/isledecomp/parser/node.py index 71d4ebdd..eeaed713 100644 --- a/tools/isledecomp/isledecomp/parser/node.py +++ b/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 diff --git a/tools/isledecomp/isledecomp/parser/parser.py b/tools/isledecomp/isledecomp/parser/parser.py index 4d8beace..c3c9cde3 100644 --- a/tools/isledecomp/isledecomp/parser/parser.py +++ b/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, ) ) diff --git a/tools/isledecomp/tests/test_parser.py b/tools/isledecomp/tests/test_parser.py index 7dcc24b1..1abae670 100644 --- a/tools/isledecomp/tests/test_parser.py +++ b/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. - Only function statics are filed as S_LDATA32.""" + Static class variables are filed as S_GDATA32, same as regular globals.""" 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