Fix ghidra global pointer import

reccmp/cvdump/types.py

@@ -198,7 +198,7 @@ class CvdumpTypesParser:
     )
 
     LF_FIELDLIST_ENUMERATE = re.compile(
-        r"list\[\d+\] = LF_ENUMERATE,.*value = (?P<value>\d+), name = '(?P<name>[^']+)'"
+        r"list\[\d+\] = LF_ENUMERATE,.*value = (?:\([\w_]*\)\s)?(?P<value>-?\d+)(?:\([\w_]*\))?, name = '(?P<name>[^']+)'"
     )
 
     LF_ARRAY_RE = re.compile(
@@ -356,8 +356,17 @@ def get(self, type_key: CvdumpTypeKey) -> TypeInfo:
         if obj is None:
             raise CvdumpKeyError(type_key)
 
-        if obj.get("type") == "LF_POINTER":
-            return self.get(CVInfoTypeEnum.T_32PVOID)
+        obj_type = obj.get("type")
+
+        if obj_type == "LF_POINTER":
+            # FIXME: This introduces regressions on datacmp and also fails some unit tests.
+            # Need to reconsider if there is a better approach for preserving full type information.
+            # It may make the most sense to adapt our domain types to our needs.
+
+            element_type_key = obj.get("element_type")
+            assert element_type_key is not None
+            pointee_type = self.get(element_type_key)
+            return TypeInfo(key=type_key, size=4, name=f"{pointee_type.name} *")
 
         if obj.get("is_forward_ref", False):
             # Get the forward reference to follow.
@@ -370,18 +379,21 @@ def get(self, type_key: CvdumpTypeKey) -> TypeInfo:
             return self.get(forward_ref)
 
         # These type references are just a wrapper around a scalar
-        if obj.get("type") == "LF_ENUM":
+        if obj_type == "LF_ENUM":
             underlying_type = obj.get("underlying_type")
             if underlying_type is None:
                 raise CvdumpKeyError(f"Missing 'underlying_type' in {obj}")
 
             return self.get(underlying_type)
 
         # Else it is not a forward reference, so build out the object here.
-        if obj.get("type") == "LF_ARRAY":
+        if obj_type == "LF_ARRAY":
             members = self._mock_array_members(obj)
-        else:
+        elif obj_type in ("LF_CLASS", "LF_STRUCTURE", "LF_UNION", "LF_FIELDLIST"):
             members = self._get_field_list(obj)
+        else:
+            # e.g. obj_type == "LF_PROCEDURE"
+            members = None
 
         return TypeInfo(
             key=type_key,

reccmp/ghidra/importer/ghidra_helper.py

@@ -148,7 +148,7 @@ def set_ghidra_label(api: FlatProgramAPI, address: int, label_with_namespace: st
             and existing_label_name == name
         ):
             logger.debug(
-                "Label '%s' at 0x%s already exists", label_with_namespace, address_hex
+                "Label '%s' at %s already exists", label_with_namespace, address_hex
             )
         else:
             logger.debug(

reccmp/ghidra/importer/importer.py

@@ -91,10 +91,6 @@ def _do_execute_import(
 ):
     pdb_functions = extraction.get_function_list()
 
-    if api is None:
-        logger.info("Completed the dry run outside Ghidra.")
-        return
-
     # pylint: disable=possibly-used-before-assignment
     type_importer = PdbTypeImporter(api, extraction, ignore_types=ignore_types)
 

reccmp/ghidra/importer/pdb_extraction.py

@@ -31,7 +31,6 @@ class CppRegisterSymbol(CppStackOrRegisterSymbol):
 
 @dataclass
 class FunctionSignature:
-    original_function_symbol: SymbolsEntry
     call_type: str
     arglist: list[CvdumpTypeKey]
     return_type: CvdumpTypeKey
@@ -119,7 +118,6 @@ def get_func_signature(self, fn: SymbolsEntry) -> FunctionSignature | None:
         this_adjust = function_type.get("this_adjust", 0)
 
         return FunctionSignature(
-            original_function_symbol=fn,
             call_type=call_type,
             arglist=arg_list_pdb_types,
             return_type=function_type["return_type"],

reccmp/ghidra/importer/type_importer.py

@@ -20,7 +20,6 @@
     TypedefDataType,
     ComponentOffsetSettingsDefinition,
 )
-from ghidra.util.task import ConsoleTaskMonitor
 
 from reccmp.cvdump.types import (
     CvdumpParsedType,
@@ -85,7 +84,7 @@ def import_pdb_type_into_ghidra(
         """
         Recursively imports a type from the PDB into Ghidra.
         @param type_index Either a scalar type like `T_INT4(...)` or a PDB reference like `0x10ba`
-        @param slim_for_vbase If true, the current invocation
+        @param slim_for_vbase If `True`, the current invocation
             imports a superclass of some class where virtual inheritance is involved (directly or indirectly).
             This case requires special handling: Let's say we have `class C: B` and `class B: virtual A`. Then cvdump
             reports a size for B that includes both B's fields as well as the A contained at an offset within B,
@@ -137,7 +136,11 @@ def _import_scalar_type(self, type_key: CvdumpTypeKey) -> DataType:
         cvtype = CvdumpTypeMap[type_key]
 
         if cvtype.pointer is None:
-            return get_scalar_ghidra_type(type_key)
+            # Scalars need to be added to the database explicitly since there can be multiple
+            # non-identical instances of the same scalar. See the failing unit tests if you remove the wrapper.
+            return add_data_type_or_reuse_existing(
+                self.api, get_scalar_ghidra_type(type_key)
+            )
 
         points_to = get_scalar_ghidra_type(cvtype.pointer)
         return get_or_add_pointer_type(self.api, points_to)
@@ -564,9 +567,7 @@ def _delete_and_recreate_struct_data_type(
         category_path = category_path_of(sanitized_name.namespace_path)
 
         assert (
-            self.api.getCurrentProgram()
-            .getDataTypeManager()
-            .remove(existing_data_type, ConsoleTaskMonitor())
+            self.api.getCurrentProgram().getDataTypeManager().remove(existing_data_type)
         ), f"Failed to delete and re-create data type {sanitized_name}"
         data_type: DataType = StructureDataType(
             category_path, str(sanitized_name), class_size

tests/conftest.py

@@ -8,7 +8,11 @@
 from reccmp.formats import NEImage, PEImage, detect_image
 
 from .binfiles_test_setup import BINFILE_ISLE, BINFILE_LEGO1, BINFILE_SKI, TestBinfile
-from .ghidra_integration_test_setup import ghidra_integration_test_program
+from .ghidra_integration_test_setup import (
+    GhidraFunctionTestHelper,
+    GhidraTypeTestHelper,
+    ghidra_integration_test_program,
+)
 
 # Suppress linter warnings related to the fact that the header support for Ghidra is limited
 # and that we cannot import Ghidra classes before Ghidra has been loaded
@@ -19,6 +23,7 @@
 if TYPE_CHECKING:
     from ghidra.program.flatapi import FlatProgramAPI
     from ghidra.program.model.listing import Program
+    from reccmp.ghidra.importer.type_importer import PdbTypeImporter
 
 
 REQUIRE_BINFILES_OPTION = "--require-binfiles"
@@ -143,3 +148,17 @@ def fixture_ghidra_program(ghidra_program: "Program") -> "Iterator[FlatProgramAP
     finally:
         # Revert all side effects of the test that just ran
         transaction.abort()
+
+
+@pytest.fixture(name="type_helper", scope="function")
+def ghidra_type_helper_fixture(
+    ghidra: "FlatProgramAPI",
+) -> Iterator[GhidraTypeTestHelper]:
+    yield GhidraTypeTestHelper(ghidra)
+
+
+@pytest.fixture(name="function_helper", scope="function")
+def ghidra_function_helper_fixture(
+    ghidra: "FlatProgramAPI",
+) -> Iterator[GhidraFunctionTestHelper]:
+    yield GhidraFunctionTestHelper(ghidra)

tests/ghidra_integration_test_setup.py

@@ -1,11 +1,17 @@
 from contextlib import contextmanager
 from pathlib import Path
 from typing import TYPE_CHECKING, Iterator, cast
-
-
+from unittest.mock import Mock
 import pytest
+
 from pyghidra import HeadlessPyGhidraLauncher  # type: ignore[import-untyped]
 
+from reccmp.compare.core import Compare
+from reccmp.compare.ingest import load_cvdump_types
+from reccmp.cvdump.analysis import CvdumpAnalysis
+from reccmp.cvdump.parser import CvdumpParser
+from .raw_image import RawImage
+
 # Suppress linter warnings related to the fact that the header support for Ghidra is limited
 # and that we cannot import Ghidra classes before Ghidra has been loaded
 
@@ -14,7 +20,8 @@
 
 if TYPE_CHECKING:
     from ghidra.program.model.listing import Program
-
+    from ghidra.program.flatapi import FlatProgramAPI
+    from reccmp.ghidra.importer.type_importer import PdbTypeImporter
 
 GHIDRA_PROJECT_NAME = "ghidra-integration-test"
 GHIDRA_FOLDER_NAME = "/"
@@ -41,6 +48,17 @@ def ghidra_integration_test_program(
 ) -> "Iterator[Program]":
     assert request.config.cache is not None
 
+    try:
+        # Gets rid of spurious stack traces from a misunderstanding between pytest and jpype.
+        # See https://jpype.readthedocs.io/en/latest/userguide.html#errors-reported-by-python-fault-handler
+        import faulthandler
+
+        faulthandler.enable()
+        faulthandler.disable()
+    # pylint: disable-next=broad-exception-caught # This is fine to fail, we don't need to handle it
+    except Exception:
+        pass
+
     HeadlessPyGhidraLauncher().start()
 
     # pylint: disable-next=import-error
@@ -126,3 +144,75 @@ def ghidra_integration_test_program(
 
         finally:
             ghidra_project.getProject().close()
+
+
+class GhidraTypeTestHelper:
+    def __init__(self, ghidra: "FlatProgramAPI"):
+        from reccmp.ghidra.importer.type_importer import (
+            PdbTypeImporter,
+            PdbFunctionExtractor,
+        )
+
+        self.ghidra = ghidra
+        self.compare = Compare(
+            RawImage.from_memory(), RawImage.from_memory(), Mock(), "TEST"
+        )
+        self.type_importer = PdbTypeImporter(
+            ghidra, PdbFunctionExtractor(self.compare), set()
+        )
+
+    def set_up_cvdump_types(self, cvdump_types: str):
+        parser = CvdumpParser()
+        parser.read_section("TYPES", cvdump_types)
+        analysis = CvdumpAnalysis(parser)
+        load_cvdump_types(analysis, self.compare.types)
+
+
+class GhidraFunctionTestHelper:
+    ORIG_FN_TO_OVERWRITE_PRIMARY = 0x00402880  # readIntFromRegistry()
+    ORIG_FN_TO_OVERWRITE_SECONDARY = 0x00402C20  # IsleApp::Tick()
+    ORIG_DATA_TO_OVERWRITE = 0x00410040 # g_windowRect
+
+    def __init__(self, ghidra: "FlatProgramAPI"):
+        self.ghidra = ghidra
+        self.orig_address = self.ORIG_FN_TO_OVERWRITE_PRIMARY
+        self.address_ghidra = ghidra.getAddressFactory().getAddress(
+            hex(self.orig_address)
+        )
+        self.ghidra_function = self.ghidra.getFunctionContaining(self.address_ghidra)
+        assert (
+            self.ghidra_function is not None
+        ), f"No Ghidra function at address {self.address_ghidra}"
+
+    def overwrite_example_function(self, data: bytes):
+        from jpype import JArray, JByte  # type: ignore[import-untyped]
+
+        assert len(data) > 0
+
+        # Clear the existing decompiled code so we can overwrite it
+        listing = self.ghidra.getCurrentProgram().getListing()
+        end_addr = self.address_ghidra.add(len(data) - 1)
+        listing.clearCodeUnits(self.address_ghidra, end_addr, False)
+
+        # Overwrite the memory
+        self.ghidra.getCurrentProgram().getMemory().setBytes(
+            self.address_ghidra, JArray.of(data, JByte)
+        )
+
+    def assert_c_code(self, code: str):
+        from ghidra.app.decompiler import DecompInterface
+        from ghidra.util.task import TaskMonitor
+
+        iface = DecompInterface()
+        iface.openProgram(self.ghidra.getCurrentProgram())
+
+        res = iface.decompileFunction(self.ghidra_function, 5, TaskMonitor.DUMMY)
+        assert res.decompileCompleted(), "Decompilation failed"
+
+        # The line endings returned by the API differ between Windows and Linux
+        decompiled_c_code = res.getDecompiledFunction().getC().replace("\r\n", "\n")
+
+        # This print statement is suppressed when the test passes, but is helpful if the test fails
+        print(decompiled_c_code)
+
+        assert decompiled_c_code == code

tests/helpers.py

@@ -0,0 +1,9 @@
+from typing import TypeVar
+
+T = TypeVar("T")
+
+
+def assert_instance(value: object, expected_class: type[T]) -> T:
+    """Type narrowing does not work well in the IDE for some reason, this makes it explicit"""
+    assert isinstance(value, expected_class)
+    return value

tests/test_cvdump_types.py

@@ -1018,7 +1018,7 @@ def test_this_adjust_hex(empty_parser: CvdumpTypesParser):
     # codespell:ignore-begin
     empty_parser.read_all("""\
 0x657a : Length = 26, Leaf = 0x1009 LF_MFUNCTION
-    Return type = T_VOID(0003), Class type = 0x15ED, This type = 0x15EE, 
+    Return type = T_VOID(0003), Class type = 0x15ED, This type = 0x15EE,
     Call type = ThisCall, Func attr = none
     Parms = 3, Arg list type = 0x6579, This adjust = 24""")
     # codespell:ignore-end
@@ -1028,22 +1028,22 @@ def test_this_adjust_hex(empty_parser: CvdumpTypesParser):
 
 BIG_TYPE_KEY_SAMPLE = """
 0x1023 : Length = 70, Leaf = 0x1505 LF_STRUCTURE
-    # members = 0,  field list type 0x0000, FORWARD REF, 
+    # members = 0,  field list type 0x0000, FORWARD REF,
     Derivation list type 0x0000, VT shape type 0x0000
     Size = 0, class name = threadlocaleinfostruct, unique name = Uthreadlocaleinfostruct@@, UDT(0x00011738)
 
 0x00011736 : Length = 14, Leaf = 0x1503 LF_ARRAY
     Element type = 0x00011735
     Index type = T_ULONG(0022)
     length = 96
-    Name = 
+    Name =
 
 0x00011737 : Length = 418, Leaf = 0x1203 LF_FIELDLIST
     list[0] = LF_MEMBER, public, type = 0x00011736, offset = 72
         member name = 'lc_category'
 
 0x00011738 : Length = 70, Leaf = 0x1505 LF_STRUCTURE
-    # members = 18,  field list type 0x11737, 
+    # members = 18,  field list type 0x11737,
     Derivation list type 0x0000, VT shape type 0x0000
     Size = 216, class name = threadlocaleinfostruct, unique name = Uthreadlocaleinfostruct@@, UDT(0x00011738)
 """
@@ -1074,6 +1074,39 @@ def test_enum_keys_over_ffff(empty_parser: CvdumpTypesParser):
     assert empty_parser.keys[TK(0x105DA)]["name"] == "e_STATE_t"
 
 
+ENUM_WITH_DATA_TYPES_SAMPLES = """
+0x4e63 : Length = 102, Leaf = 0x1203 LF_FIELDLIST
+	list[0] = LF_ENUMERATE, public, value = 1, name = 'c_act1'
+	list[1] = LF_ENUMERATE, public, value = 2, name = 'c_imain'
+	list[2] = LF_ENUMERATE, public, value = 16, name = 'c_ielev'
+	list[3] = LF_ENUMERATE, public, value = 32, name = 'c_iisle'
+	list[4] = LF_ENUMERATE, public, value = (LF_USHORT) 32768, name = 'c_act2'
+	list[5] = LF_ENUMERATE, public, value = (LF_ULONG) 65536, name = 'c_act3'
+
+0x5695 : Length = 50, Leaf = 0x1203 LF_FIELDLIST
+	list[0] = LF_ENUMERATE, public, value = (LF_CHAR) -1(0xFF), name = 'c_unknownminusone'
+	list[1] = LF_ENUMERATE, public, value = 8, name = 'c_unknown8'
+"""
+
+
+def test_enum_with_data_types(empty_parser: CvdumpTypesParser):
+    """Make sure we can read an LF_FIELDLIST of an enum with a negative value (GH #380)"""
+    empty_parser.read_all(ENUM_WITH_DATA_TYPES_SAMPLES)
+    assert empty_parser.keys[TK(0x4E63)]["variants"] == [
+        EnumItem(name="c_act1", value=1),
+        EnumItem(name="c_imain", value=2),
+        EnumItem(name="c_ielev", value=16),
+        EnumItem(name="c_iisle", value=32),
+        EnumItem(name="c_act2", value=32768),
+        EnumItem(name="c_act3", value=65536),
+    ]
+
+    assert empty_parser.keys[TK(0x5695)]["variants"] == [
+        EnumItem(name="c_unknownminusone", value=-1),
+        EnumItem(name="c_unknown8", value=8),
+    ]
+
+
 ARRAY_WITH_UNKNOWN_ELEMENT = """
 0x1000 : Length = 14, Leaf = 0x1503 LF_ARRAY
     Element type = ???(0555)