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

Add comment on load opcodes with the loaded value #3289

Merged
merged 1 commit into from
Jan 15, 2023

Conversation

bucienator
Copy link
Contributor

@bucienator bucienator commented Jan 13, 2023

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

First time user of Rizin here, I was doing some analysis on Xtensa code. (esp8266 firmware.) I found that in that architecture 32 bit constant values are loaded into registers by a PC relative load operation, loading the constant from the code segment.

For example:
0x4000bf52 l32r a5, 0x400003f4

As 0x400003f4 is in the ROM as well, the loaded value is known at analysis time, and is usually needed to understand the analyzed function. So I was looking for ways to have the actual value shown in the disassembly, and ultimately came up with the change in this PR.

This modifies the disassembly output by adding a comment to it:

0x4000bf52 l32r a5, 0x400003f4 ; refval: 0x0000ff00

Actually I'm not yet happy with the change though, there are some things to refine on it:

  • Check if the address is indeed in non-writable memory area, and so is useful in static analysis
  • The loaded address is always treated as 32 bits, which is true for the Xtensa case, but not necessarily for other platform's load operations. I'll see how the size of the data could be determined
  • I'm not sure if "refval" is the right term for the loaded value, maybe there's a more commonly known name for it
  • I think it doesn't handle target endianness correctly
  • And lastly, there might be better ways to have a similar outcome, maybe even without a code change.

I'm looking for some feedback if this change is deemed worthy for further improvements, or I should just drop it in favor of a different approach.

Test plan

(Tested manually so far, I'll look for unit tests for the disassebly functionality.)

librz/core/disasm.c Outdated Show resolved Hide resolved
@XVilka
Copy link
Member

XVilka commented Jan 14, 2023

Please add a test, especially in light of the future migration of the Xtensa support to capstone engine:

@bucienator
Copy link
Contributor Author

Please add a test, especially in light of the future migration of the Xtensa support to capstone engine:

I was considering that. My understanding is that I need to add a test binary to the rizin-testbins project, and then the test can refer to that. Is that the way? Should I add a binary only, or source code and build instructions too?

@bucienator
Copy link
Contributor Author

I've added a test case. Is that a good way to test this change?

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.

@bucienator looks good, thank you!

By the way, next time, I recommend using a different branch name. Usually, it's recommended to create a new "feature branch" for each PR separately.

@XVilka XVilka merged commit ca3c77e into rizinorg:dev Jan 15, 2023
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.

3 participants