Support pointers to non-exported data in `datacmp` by jonschz · Pull Request #543 · isledecomp/reccmp

jonschz · 2026-09-06T10:08:50Z

The current approach is flawed: In the LEGO1 example, we find a _DIOBJECTDATAFORMAT * and deduce that it points to some instance, but it actually points to an array (_DIOBJECTDATAFORMAT[0x100]). This cannot be reconstructed from the PDB.

Tasks:

Support annotating the type of a // GLOBAL
Revert deducing the type from the recomp parent type (since e.g. a pointer and an array are ambiguous)
Add test coverage
- missing: get_by_name

jonschz · 2026-09-06T16:01:52Z

The following will work on LEGO1:

// GLOBAL: LEGO1 0x10098f80
// c_dfDIKeyboard

// GLOBAL: LEGO1 0x10097f80 TYPE=_DIOBJECTDATAFORMAT[256] NO_RECOMP_SYMBOL
// c_rgodfDIKeyboard

// If you enable this one, there will be no errors on LEGO1.
// If you disable it, you can see that the mismatches on `c_rgodfDIKeyboard` are displayed correctly.
// // GLOBAL: LEGO1 0x100db700
// _GUID_Key

@madebr do you want to take a look?

jonschz · 2026-09-06T18:07:21Z

    marker = match_marker("// VTABLE: TEST 0x1234 S p a c e s")
    assert marker is not None
-    assert marker.extra == "S p a c e s"
+    assert marker.extras == ("S", "p", "a", "c", "e", "s")


Could this be a problem in some way? a key-value approach might need a way of escaping spaces, but that could also be overkill

disinvite · 2026-09-06T18:12:51Z

Does the compiler drop unused types from the PDB? If not, users could just create whatever type/shape is needed:

struct cKeyboardStruct {
    _DIOBJECTDATAFORMAT x[0x100]
};

Building a struct-name to leaf-id map doesn't add too much performance overhead. I ran a quick test yesterday with an extra regex search on each TYPES leaf for the class name = ... string.

jonschz · 2026-09-06T20:32:08Z

Does the compiler drop unused types from the PDB?

Unfortunately, yes. The following makes it to the PDB:

struct cKeyboardStruct {
    _DIOBJECTDATAFORMAT x[0x100];
};

static cKeyboardStruct* keyboardStruct = NULL;

but the last line is necessary, which can have an impact on binary matching.

jonschz · 2026-09-06T20:45:58Z

But even if we could make the "define your own struct in C++" approach work, it would get rid of the array type creation logic at most. The remainder of this change would still be needed.

disinvite · 2026-09-06T21:48:19Z

Rough idea I have not thought through:

// VTABLE: HELLO 0x1000 KEYVAL
// VTABLE: HOWDY 0x2000 KEYVAL
// VTABLE: GREET 0x3000 KEYVAL
// symbol = asdadfafadfadf
// name = Pizza:`vftable'{for XYZ}
// base_class = XYZ

Basically using the "extra" field to indicate that we're parsing the following comments differently.

disinvite · 2026-09-06T21:50:59Z

But even if we could make the "define your own struct in C++" approach work, it would get rid of the array type creation logic at most. The remainder of this change would still be needed.

True. I'm just looking for any way to reduce the surface area. And if we need to do this for a variable that has no PDB struct to attach to, we are stuck.

jonschz · 2026-09-08T11:24:49Z

Rough idea I have not thought through:

This approach looks promising to me. Some thoughts:

Do we even need the KEYVAL? We could always try to detect an arbitrary number of key-value type comments below (regex draft: \s*\/\/\s*(?P<key>\w+) =\s*(?P<value>(?<= )\S.*\S|\S|)\s*$), and continue the existing parser after the first mismatch.
- We could even start emitting deprecation warnings for the existing VTABLE pattern that they should migrate to a key-value based approach
Is there a risk of a symbol annotation being misdetected as a key-value? This could happen if a \s=\s could be part of a symbol, which I don't think is possible. If there is an issue, though, the KEYVAL annotation in the first line won't save us (since we can't tell if the current line already is the symbol or just another key-value annotation)
The new pattern would effectively mean "boolean flags are set in the same line as the annotation, key-value flags are set in comments below". Looks good to me, just wanted to state this explicitly so we can all agree/disagree on this pattern.

Edit: I read your comment again and now I think I understand where you are going with the KEYVAL. I'd prefer to keep the key-value per-annotation, even if that can lead to a bit more boilerplate. Sharing key-value attributes between multiple annotations feels more error-prone and harder to parse to me.

Edit 2: I wouldn't provide the symbol via key-value since that is against the general annotation pattern we have established:

// <ANNOTATION>
<TARGET>

where <TARGET> is either an actual C++ target or a comment of a target or symbol. If we introduced a // symbol = ... key-value annotation, we would have to conditionally parse the <TARGET> below, which I don't like.

disinvite · 2026-09-08T18:43:21Z

We don't need KEYVAL if it we can distinguish a comment "completion token" from a comment with a key/value pair with perfect accuracy. Another option is to use an indicator character, as in:

// FUNCTION: TEST 0x1234
// @test = 100
// @emote = 😎
// NameOf::TheFunction

but we know @ can appear in symbols, and it can be the first character.

But what if there is no "completion token"? Are these still valid annotations?

//// Completed by attributes only:
// GLOBAL: TEST 0x2000
// @name = MyGlobalVariable

//// Attribute and comment completion token in conflict:
// GLOBAL: TEST 0x3000
// @name = OtherVariable
// RealNameOfTheVariable

//// Attribute and code completion token in conflict:
// GLOBAL: TEST 0x3000
// @name = OtherOtherVariable
int CodeVariable = 5;

So it may be that I've introduced something that will take more effort to solve than the specific ask for the DirectInput keyboard struct.

jonschz · 2026-09-08T20:00:26Z

Going full circle, how about one-line key-value with double quotes and backslash escaping, similar to JSON? (I have briefly considered proper JSON, but I think it's overkill and adds noise). This could look like

// the new thing
// GLOBAL: LEGO1 0x10097f80 TYPE="_DIOBJECTDATAFORMAT[256]" NO_RECOMP_SYMBOL
// c_rgodfDIKeyboard

// legacy vtable
// VTABLE: LEGO1 0x10097f80 MyBaseClass
class ChildClass: public MyBaseClass {};

// modern vtable
// VTABLE: LEGO1 0x10097f80 BASE_CLASS="MyBaseClass"
class ChildClass: public MyBaseClass {};

// hypothetical quote escape
// GLOBAL: TEST 0x1234 KEY_WITH_QUOTES_AND_SPACES="I said \"hello\""

Main benefit: This keeps syntax and parser complexity lower and is capable of representing all strings. We could use existing JSON logic to parse only the string, then we would already have support for other escape sequences like \n (if we ever need them, which I seriously doubt)

jonschz · 2026-09-12T21:35:14Z

I implemented the JSON key-value approach. datacmp on LEGO1 will now pass with the following addition:

// GLOBAL: LEGO1 0x10098f80
// c_dfDIKeyboard

// this one can be disabled to verify that c_rgodfDIKeyboard is being compared correctly
// GLOBAL: LEGO1 0x100db700
// _GUID_Key

// GLOBAL: LEGO1 0x10097f80 TYPE="_DIOBJECTDATAFORMAT[256]" NO_RECOMP_SYMBOL
// c_rgodfDIKeyboard

jonschz · 2026-09-12T21:35:48Z

+# TODO: Discuss if we want to split the file instead, and if so, what can/should be pulled out
+# pylint:disable=too-many-lines
+


Needs opinion

I think we can easily extract all the read_(leaf_type) functions and their companion regex strings into their own module.

jonschz · 2026-09-13T06:06:27Z

-    marker = match_marker("// VTABLE: TEST 0x1234 S p a c e s")
-    assert marker is not None
-    assert marker.extra == "S p a c e s"


semantics no longer relevant or desired. New semantics are covered by other tests.

jonschz · 2026-09-13T06:06:35Z

-    # Trailing spaces removed
-    marker = match_marker("// VTABLE: TEST 0x8888 spaces    ")
-    assert marker is not None
-    assert marker.extra == "spaces"


Covered by other tests

jonschz · 2026-09-13T06:09:51Z

drive-by: better type safety. Functionality wise, this was only adapted to the signature change of match_variables.

jonschz · 2026-09-13T06:14:52Z

+                logger.warning(
+                    'Legacy VTABLE base class annotation used above %s:%i. Change to `// VTABLE: %s 0x%x BASE_CLASS="%s"`.',
+                    self.filename.name,
+                    self.line_number,
+                    marker.module,
+                    marker.offset,
+                    base_class,
+                )


We can move this to a second PR if you prefer, or even split into three (implement key-value, implement orig type annotation, change VTABLE annotation with legacy support).

If we keep everything in here, we should change the PR title.

I think a split makes sense if it's not too much trouble.

jonschz · 2026-09-13T06:23:10Z

+        for match in SINGLE_MARKER_EXTRA_REGEX.finditer(raw_extra):
+            raw_value = match.group("value")
+            if raw_value is None:
+                extra_flags.add(match.group("key"))
+            else:
+                try:
+                    value = json.loads(raw_value)
+                    assert isinstance(
+                        value, str
+                    ), "This assertion should never fail since the regex checks the presence of double quotes"
+                    extra_strings.append((match.group("key"), value))
+                except JSONDecodeError as e:
+                    logging.warning(
+                        "Invalid JSON in extra (last part) of annotation '%s'",
+                        raw_extra,
+                        exc_info=e,
+                    )
+                    return None


Not sure if this would rather belong into match_msvc.py architecturally. Benefit would be that we could make decomplint failures out of these. The current approach has the benefit that all regex shenanigans are localised to this file.

In the LEGO1 example, we find a `_DIOBJECTDATAFORMAT *` and deduce that it points to some instance, but it actually points to an array (`_DIOBJECTDATAFORMAT[0x100]`). This cannot be reconstructed from the PDB.

jonschz · 2026-09-13T12:22:45Z

 }


+@dataclass


generates __eq__ which I need in a test

disinvite · 2026-09-15T01:18:33Z

+        orig_addr: int,
+        report: ReccmpReportProtocol = reccmp_report_nop,


I think we should just use the existing logger and not drag the report protocol into here. reccmp-cvdump fails with a circular import because you can't import from reccmp.compare and not get .core and all its dependencies.

disinvite · 2026-09-15T01:19:47Z

+        expected_leaf_fragment = f"class name = {name},"
+        potential_hits = [
+            self.get(key)
+            for key, (leaf, _) in self._raw.items()
+            if expected_leaf_fragment in leaf
+        ]


We should cache this structure so we only pay for it once. Or not at all, if there are no name lookups.

disinvite · 2026-09-15T01:28:52Z

+        # The same entry may appear multiple times (e.g. due to forward refs), so we deduplicate by key
+        # and also filter again by the name just to be sure
+        actual_hits = dict((hit.key, hit) for hit in potential_hits if hit.name == name)


We don't currently do anything with the "WARNING: UDT mismatch" reports, but would that explain why we have some duplicate struct names? Maybe we could treat these cases (where the leaf does not have the FORWARD REF flag but the leaf id does not match the embedded UDT) as forward refs. Not sure how correct an approach that is.

The forward refs were the reason I introduced this code since the string match catches them. I don't have any new insight about the UDT mismatch. The warning about double matches is merely a precaution. I'd keep the other handling as it is.

disinvite · 2026-09-15T01:30:41Z

+    ) -> TypeInfo | None:
+        """
+        Searches the type database for `name`.
+        Also supports arrays with decimal length (e.g. `MyType[20]`);


Maybe this should be two functions: one to get the CvdumpTypeKey from the name, and one to create a synthetic array type where you pass in the key and number of elements you want. That would shift the user-provided type string parsing back to the caller.

disinvite · 2026-09-15T02:12:51Z

Since we are adding new options to the markers anyway, would it be better to have "create synthetic matches from this variable" as one of the options and do it in the Compare pipeline instead of in the variable comparator? To match the current PR behavior, we would need to create synthetic matches recursively, although this is perhaps not what the user intends in every situation.

jonschz · 2026-09-17T10:37:51Z

Since we are adding new options to the markers anyway, would it be better to have "create synthetic matches from this variable" as one of the options and do it in the Compare pipeline instead of in the variable comparator?

The difficulty is that we only find out the recomp address of the synthetic match when datacmp is run on the normal match that references a synthetic one. We'd have to annotate
c_dfDIKeyboard that it contains synthetic matches, and then always run a datacmp-like comparison on all entities with that annotation. Not sure if this is what you meant. Sounds like a lot of effort to me, especially since some datacmp-like logic will have to be duplicated or generalised to the Compare step. Might be the less hacky approach, what I don't like is the two new annotation types needed for that.

jonschz · 2026-09-18T07:00:25Z

Maybe a better alternative:

// GLOBAL: LEGO1 0x10097f80 TYPE="_DIOBJECTDATAFORMAT[256]" SYNTHETIC_MATCH_FROM_POINTER="0x10098f94"
// c_rgodfDIKeyboard

The requirement would be that there is a different, valid global covering the SYNTHETIC_MATCH_FROM_POINTER address. We could then introduce a compare step that

goes over all globals with that flag,
searches for another matched global covering the referenced address
get the recomp address from the match
create a new entity based on that

We could then repeat this step until the number of matched globals no longer increases, which would cover recursive structures.

jonschz force-pushed the datacmp-global-pointer-detection branch from 09a2327 to fc3f917 Compare September 6, 2026 16:00

jonschz commented Sep 6, 2026

View reviewed changes

Comment thread reccmp/compare/variables.py Outdated

jonschz commented Sep 6, 2026

View reviewed changes

Comment thread reccmp/parser/parser.py Outdated

jonschz commented Sep 6, 2026

View reviewed changes

jonschz marked this pull request as ready for review September 12, 2026 21:35

jonschz marked this pull request as draft September 12, 2026 21:37

jonschz commented Sep 12, 2026

View reviewed changes

jonschz commented Sep 13, 2026

View reviewed changes

jonschz added 12 commits September 13, 2026 14:25

WIP: Works, but the approach is flawed

e3daeec

In the LEGO1 example, we find a `_DIOBJECTDATAFORMAT *` and deduce that it points to some instance, but it actually points to an array (`_DIOBJECTDATAFORMAT[0x100]`). This cannot be reconstructed from the PDB.

WIP: save progress

767ec4d

fix recursion

fbf3638

fix test regressions

14300a9

general cleanup; tests still need to be adapted

39f7f2e

fix tests

3f2d6b2

refactor annotation parameter structure

27d0d6b

fix test failures

0f7cde1

Fix another test failure

eda2180

cleanup from self-review

549f0da

add tests for new features

1d75d31

add test coverage for get_by_name

7de69a2

jonschz force-pushed the datacmp-global-pointer-detection branch from 60f3b57 to 7de69a2 Compare September 13, 2026 12:25

jonschz marked this pull request as ready for review September 13, 2026 12:25

jonschz changed the title ~~[Draft] Support pointers to non-exported data in datacmp~~ Support pointers to non-exported data in datacmp Sep 13, 2026

jonschz commented Sep 13, 2026

View reviewed changes

Comment thread reccmp/compare/db.py

}

@dataclass

jonschz Sep 13, 2026

Copy link
Copy Markdown

Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generates __eq__ which I need in a test

jonschz requested a review from disinvite September 13, 2026 12:27

disinvite reviewed Sep 15, 2026

View reviewed changes

		# TODO: Discuss if we want to split the file instead, and if so, what can/should be pulled out
		# pylint:disable=too-many-lines

		orig_addr: int,
		report: ReccmpReportProtocol = reccmp_report_nop,

Conversation

jonschz commented Sep 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonschz commented Sep 6, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

disinvite commented Sep 6, 2026

Uh oh!

jonschz commented Sep 6, 2026

Uh oh!

jonschz commented Sep 6, 2026

Uh oh!

disinvite commented Sep 6, 2026

Uh oh!

disinvite commented Sep 6, 2026

Uh oh!

jonschz commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

disinvite commented Sep 8, 2026

Uh oh!

jonschz commented Sep 8, 2026

Uh oh!

jonschz commented Sep 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonschz Sep 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

disinvite commented Sep 15, 2026

Uh oh!

jonschz commented Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonschz commented Sep 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jonschz commented Sep 6, 2026 •

edited

Loading

jonschz commented Sep 8, 2026 •

edited

Loading

jonschz Sep 13, 2026 •

edited

Loading

jonschz commented Sep 17, 2026 •

edited

Loading