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

refactor: use parse_address whenever appropriate #723

Merged
merged 5 commits into from Sep 22, 2021

Conversation

theguy147
Copy link
Collaborator

refactor: use parse_address whenever appropriate

Description/Motivation/Screenshots

This small refactoring PR replaces the following occurrences with parse_address for consistency and easier maintainability:

  • to_unsigned_long(gdb.parse_and_eval(...))
  • int(gdb.parse_and_eval(...))

How Has This Been Tested?

Architecture Yes/No Comments
x86-32 ✔️
x86-64 ✔️
ARM ✖️
AARCH64 ✖️
MIPS ✖️
POWERPC ✖️
SPARC ✖️
RISC-V ✖️
make test ✔️

Checklist

  • My PR was done against the dev branch, not master.
  • My code follows the code style of this project.
  • My change includes a change to the documentation, if required.
  • My change adds tests as appropriate.
  • I have read and agree to the CONTRIBUTING document.

@theguy147
Copy link
Collaborator Author

theguy147 commented Sep 21, 2021

the only place of them I am actually not a 100% sure if it's a good idea changing int(gdb.parse_and_eval(...)) to parse_address(...) is in __get_register_for_selected_frame (and thereby get_register).

EDIT:
nevermind. with get_register there should never be a difference between converting it to int or unsigned_long. the calling function should always use something like long_to_twos_complement in RISCV class or similar if negative values are to be expected.

@hugsy hugsy added this to the Release: 2021.10 milestone Sep 21, 2021
Copy link
Owner

@hugsy hugsy left a comment

Choose a reason for hiding this comment

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

Woohoo Ctrl+H FTW

Copy link
Collaborator

@Grazfather Grazfather left a comment

Choose a reason for hiding this comment

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

Nice work :)

I like changes like this, but they are also scary. I hope you're able to at least test each command that would be affected by these changes.

@theguy147
Copy link
Collaborator Author

theguy147 commented Sep 22, 2021

Nice work :)

I like changes like this, but they are also scary. I hope you're able to at least test each command that would be affected by these changes.

Apart from some usages of get_register (total of 46 usages across gef.py - I tested around half of them) I have checked all commands, functions and structs manually on a x86_64 machine and didn't get any errors and the tests are running successfully. I also didn't test ida-interact because I don't have IDA or Binja installed on this machine. I can check it out in a few days when I'm back home from traveling.

@hugsy
Copy link
Owner

hugsy commented Sep 22, 2021

I also didn't test ida-interact because I don't have IDA or Binja installed on this machine. I can check it out in a few days when I'm back home from traveling.

Don't bother, ida-interact is being moved to gef-extras

@hugsy hugsy merged commit 1df157e into hugsy:dev Sep 22, 2021
@theguy147 theguy147 deleted the parse_address branch September 22, 2021 19:57
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.

None yet

3 participants