Skip to content

PPC: Reimplement pooled data reference calculation #167

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

Merged
merged 7 commits into from
Mar 5, 2025

Conversation

LagoLunatic
Copy link
Contributor

#140 PPC: Display data values on hover for pools as well
image

Also adds a new option to disable this calculation for performance (it defaults to calculating them):
image

Also fixes a bug in how extern relocation addends were being treated that was first introduced in #153, but which only became very noticeable when addend diffing was added in #158.

Reimplements encounter#140

The relocations are now generated when the object is first read in `parse`, right after the real relocations are read.

`resolve_relocation` was changed to take `obj.symbols` instead of `obj` as an argument, because `obj` itself doesn't exist yet at the time the relocations are being read.
…addend

This is a regression that was introduced by encounter#158 diffing addends in addition to symbol names. But it's not really a bug in that PR, rather it seems like I simply never added the offset into the addend when creating a fake pool relocation for an extern symbol. So this commit fixes that root issue instead.
@@ -64,10 +64,10 @@ fn reloc_eq(

#[inline]
pub fn resolve_relocation<'obj>(
obj: &'obj Object,
symbols: &'obj [Symbol],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure, does this 'obj lifetime stuff need updating since obj: &Object isn't being passed to this function now?

Copy link
Owner

Choose a reason for hiding this comment

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

This is fine, the lifetime name still makes sense too.

@@ -194,6 +194,13 @@
"name": "Register '$' prefix",
"description": "Display MIPS register names with a '$' prefix."
},
{
"id": "ppc.calculatePoolRelocations",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now I made this a PPC arch specific option since it only affects PPC, though I'd still like to implement this for ARM too in the future, so I guess this can be renamed to a general option "calculatePoolRelocations" later on once it exists for more arches. Alternatively we could make it general right now.

Copy link
Owner

Choose a reason for hiding this comment

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

Let’s leave it specific to PPC for now

@encounter
Copy link
Owner

Looks great, thanks!

@encounter encounter merged commit cf5fc54 into encounter:main Mar 5, 2025
21 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