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

Fix "return" key in disassembler widget (#3090) and graph jumps #3146

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

whol-hoopa
Copy link

@whol-hoopa whol-hoopa commented Mar 5, 2023

Fixed the return key in disassembler
Fix graph jumps

Your checklist for this pull request

Detailed description

I fixed the problem with the disassembler not jumping when the instruction is a jump conditional
The graph also had a similar problem with not jumping when the instruction is a conditional jump so I also fixed the graph jumps since it also used similar functions to when the disassembler wants to jump.

Test plan (required)

The test can be performed on "jne" instruction and other conditonal jump instructions.
The test should also be performed on the disassembler graph.

Closing issues

closes #3140
closes #3090

@whol-hoopa whol-hoopa changed the title Fix "return" key in disassembler widget (#3090) Fix "return" key in disassembler widget (#3090) and graph jumps Mar 5, 2023
@XVilka

This comment was marked as resolved.

@XVilka
Copy link
Member

XVilka commented Mar 9, 2023

@whol-hoopa the failure is unrelated, it's something changed on Rizin side, see #3147
Just please remove updating the Rizin from the PR, and it should build just fine.

@whol-hoopa whol-hoopa force-pushed the whola-graph_jumps_disassembly_jumps branch from c7f5ad5 to 16b195a Compare March 9, 2023 06:26
@whol-hoopa
Copy link
Author

Should this pull request be broken down to smaller pull requests? It changes the graph and disassembly.

@XVilka
Copy link
Member

XVilka commented Mar 30, 2023

@whol-hoopa should be fine as is. @thestr4ng3r could you please take a look?

@XVilka XVilka requested a review from ITAYC0HEN March 30, 2023 01:58
@wargio
Copy link
Member

wargio commented Mar 30, 2023

@karliss could you give a look?

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.

Looks good from the code PoV, will test later today

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.

Can not follow conditional branching targets (JNE/JNZ)
5 participants