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

Show relocation diffs in function view when the data's content differs #153

Merged
merged 19 commits into from
Jan 18, 2025

Conversation

LagoLunatic
Copy link
Contributor

This implements #152. Relocations to data will be highlighted as a diff in the function view if the contents of the data are not the same on both sides:
image

I also added a new option called "Relax shifted data diffs". When this is checked, data relocations that have their contents matching but are at the wrong address will not show up as a diff. This can be helpful for float literals, as they all get shifted around when the number or order of any of the float literals in the TU are wrong.


In order to support diffing fake pool relocations (#140), I have added them to the end of the line visually, as a fake argument. Without this, objdiff would not check if the fake relocation matches on each side or not, as none of the real arguments reference this fake relocation.
image
This makes it easy to tell a glance when the value of a float literal is wrong, even when the offset into the pool is correct, without needing to hover over every single float load in the function to view the value in the tooltip.

This also works for stringBase pools:
image
In order to get this working, I had to switch it from comparing the bytes of the data to instead comparing the string representation of the data that gets shown on hover. This is because if the bytes are compared directly, then any single string in the entire stringBase being wrong would result in every reference to stringBase in the object being shown as wrong, because stringBase's bytes includes every string, even the ones not being referenced by the current instruction.


Note: When I initially was implementing this PR by comparing bytes directly, this resulted in all functions that use pools suddenly showing a false diff due to the dtk issue where the first symbol in a pool is shown instead of the pool symbol itself:
image
To get around this, I initially added a hack where a diff will be ignored if the right side's symbol name starts with the string "...", which wasn't ideal.

But later, after I switched it from comparing bytes to comparing display strings, I found a better way to fix this false positive. All I had to do was remove a separate hack I had implemented in #140, specifically the one where all addi opcodes are assumed to be referencing strings and it tries to show the data as a string, but creates many false positives. Once that was removed I could also remove the "..." hack, because then neither the left nor the right side were trying to display any visual representation of the pool symbol's data.
I don't think removing the addi hack from the previous PR has much downside to it - the main reason it was originally added actually became obsolete by the time the previous PR was finished, but I forgot to remove that hack at the time.


This PR still needs more testing to check for possible false positive diffs it could have introduced, I only tested it on 3-4 TWW TUs so far.

@LagoLunatic
Copy link
Contributor Author

I've changed this to be opt-in. The option is now a dropdown:
image

"Address only" is objdiff's current behavior on main, where objdiff diffs relocation addresses but ignores the data's value.
"Value only" is the new name for "Relax shifted data diffs" described in the original PR.
"Value and address" is the default behavior from the original PR, where a difference for either the relocation address or its value (for data symbols) will cause it to show as a diff in the function view.

@encounter encounter merged commit 2876be3 into encounter:main Jan 18, 2025
17 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.

2 participants