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

Use relocation types name defined by each bin format. #4695

Merged
merged 5 commits into from
Nov 4, 2024

Conversation

Roeegg2
Copy link
Contributor

@Roeegg2 Roeegg2 commented Nov 2, 2024

Your checklist for this pull request

  • I've read the guidelines for contributing to this repository
  • I made sure to follow the project's coding style
  • I've documented or updated the documentation of every function and struct this PR changes. If not so I've explained why.
  • I've added tests that prove my fix is effective or that my feature works (if possible)
  • I've updated the rizin book with the relevant information (if needed)

Detailed description

  • Modified bin_reloc_type_name to return correct naming if possible, otherwise return the old ADD/SET naming.
  • Add print_name field to RzBinReloc

Every relocation is being assigned a type based off whether it needs an addend added to it or not (SET for no addend, ADD for with). This is what RzBinReloc.additive was for.
RzBinReloc.type has just been the bit size of the relocation
And the final name of the relocation printed was just a combination of the both (which is wrong)

In order to not break all the other relocation types, I haven't removed the old code completely yet. When all the other executable formats support the new printing I'll remove it completely.
...

Test plan

All tests green
...

Closing issues

...

Copy link
Member

@Rot127 Rot127 left a comment

Choose a reason for hiding this comment

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

Very much like these changes!
Please address the comments though and don't delete half of the PR description template. It is ok to fill out All tests green in the testing paragraph. If this is actually sufficient to test the changes (as it does here).

librz/bin/p/bin_elf.inc Outdated Show resolved Hide resolved
librz/bin/p/bin_elf.inc Outdated Show resolved Hide resolved
librz/bin/p/bin_elf.inc Show resolved Hide resolved
librz/core/cbin.c Outdated Show resolved Hide resolved
Copy link
Member

@wargio wargio left a comment

Choose a reason for hiding this comment

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

looks so much better. wonderful job.

@wargio wargio changed the title Reloc naming Use relocation types name defined by each bin format. Nov 3, 2024
@Roeegg2 Roeegg2 force-pushed the reloc-naming branch 3 times, most recently from d89eb12 to a9853a5 Compare November 3, 2024 12:53
@Roeegg2
Copy link
Contributor Author

Roeegg2 commented Nov 3, 2024

@Rot127 @wargio did I miss anything?

librz/core/cbin.c Outdated Show resolved Hide resolved
librz/include/rz_bin.h Outdated Show resolved Hide resolved
* Modified `bin_reloc_type_name` to return correct naming if possible,
  otherwise return the old ADD/SET naming.
* Add `print_name` field to `RzBinReloc`
* Fixed the relocation names in the regression tests
@Roeegg2
Copy link
Contributor Author

Roeegg2 commented Nov 3, 2024

@Rot127 done

Copy link
Member

@wargio wargio left a comment

Choose a reason for hiding this comment

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

LGTM

Co-authored-by: Rot127 <45763064+Rot127@users.noreply.github.com>
librz/core/cbin.c Outdated Show resolved Hide resolved
librz/include/rz_bin.h Outdated Show resolved Hide resolved
@wargio
Copy link
Member

wargio commented Nov 4, 2024

@XVilka muon seems broken?

@XVilka
Copy link
Member

XVilka commented Nov 4, 2024

Copy link
Member

@XVilka XVilka left a comment

Choose a reason for hiding this comment

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

LGTM

@XVilka XVilka merged commit 8b90795 into rizinorg:dev Nov 4, 2024
43 of 44 checks passed
@Roeegg2 Roeegg2 deleted the reloc-naming branch November 5, 2024 02:19
@Roeegg2 Roeegg2 restored the reloc-naming branch November 5, 2024 02:20
@Roeegg2 Roeegg2 deleted the reloc-naming branch November 5, 2024 02:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants