Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Alert to ambiguous name match #29

Merged
merged 4 commits into from
Dec 8, 2024
Merged

Conversation

disinvite
Copy link
Collaborator

@disinvite disinvite commented Dec 7, 2024

Resolves #28.

We use the existing logger to display the warning, so brace yourself for more messages. Sample:

[WARNING] Ambiguous match 0x10058980 on name '_Construct' to '?_Construct@@YAXPAPAVLegoAnimPresenter@@ABQAV1@@Z'
[WARNING] Ambiguous match 0x1004a780 on name '_Construct' to '?_Construct@@YAXPAPAULegoPathCtrlEdge@@ABQAU1@@Z'
[ERROR] Failed to find function symbol with annotation 0x10022360 and name '?_Construct@@YAXPAPAVMxCore@@ABQAV1@@Z'
[WARNING] Ambiguous match 0x10010c00 on name 'Mx3DPointFloat::operator=' to '??4Mx3DPointFloat@@QAEAAV0@ABV0@@Z'
[WARNING] Ambiguous match 0x10064b20 on name 'Mx4DPointFloat::operator=' to '??4Mx4DPointFloat@@QAEAAV0@ABV0@@Z'
[ERROR] Failed to find function symbol with annotation 0x100c7ef0 and name 'list<MxNextActionDataStart *>::insert'
[ERROR] Failed to find function symbol with annotation 0x100c1bc0 and name 'list<MxDSAction *,allocator<MxDSAction *> >::insert'

Other notes:

  • I dropped the _find_potential_match method in favor of two more general purpose search methods (on name/type and symbol). These return MatchInfo objects, both matched and unmatched, so it's up to the caller to check before writing to the DB with a call to set_pair.
  • We now build indices at the moment they are needed in the two aforementioned methods. This should improve performance (but perhaps only slightly)
  • We could very easily create a generic search function that would take any combination of keywords. The problem I found in testing is that SQLite will not engage the indices if you parameterize the key extracted from the JSON string. For example:
    SELECT cols FROM symbols WHERE json_extract(kvstore,?) = ?
    The workaround is to use string concat instead of a parameter, but this of course leads to the dreaded SQL injection problem. We probably don't need to worry about this too much considering the lifespan and purpose of the data. I'll add this in a separate PR later when we need it.

@disinvite disinvite requested a review from jonschz December 7, 2024 19:57
Copy link
Collaborator

@jonschz jonschz left a comment

Choose a reason for hiding this comment

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

Looks great overall! Feel free to merge after responding to the suggestions I made.

Just curious: What else would you like to see/implement in order to consider #28 solved? In my opinion, this PR is good enough.

reccmp/isledecomp/compare/db.py Show resolved Hide resolved
reccmp/isledecomp/compare/db.py Show resolved Hide resolved
reccmp/isledecomp/compare/db.py Outdated Show resolved Hide resolved
reccmp/isledecomp/compare/db.py Show resolved Hide resolved
reccmp/isledecomp/compare/db.py Outdated Show resolved Hide resolved
@disinvite
Copy link
Collaborator Author

I think that's everything if you want to give a final OK @jonschz. I guess we can close the issue after all because this solves the reported problem. I'll just add more enhancement issues for the follow-up items.

@jonschz
Copy link
Collaborator

jonschz commented Dec 8, 2024

Looks good, feel free to merge 👍

@disinvite disinvite merged commit 34068b5 into isledecomp:master Dec 8, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Alert to ambiguous name match
2 participants