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.

Sign up for GitHub

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

Jump to bottom

Display jumping fix useless decompilation #2351

Merged
merged 12 commits into from Aug 2, 2020
Merged

Display jumping fix useless decompilation #2351

merged 12 commits into from Aug 2, 2020

Conversation

NirmalManoj
Copy link
Member

Your checklist for this pull request

Detailed description

The important modifications and additions in this PR are:

  1. Saves the previous function's address and if the newly decompiled function is the same as the previous one, the scroll position will remain the same.
  2. Modified signal breakpointsChanged() to breakpointsChanged(RVA offset). Removed emit instructionChanged from all functions that emit breakpointsChanged.

Display jumps while toggling breakpoint (Before)
BeforeFixingDisplayJumping
Display position doesn't change while toggling breakpoint (After)
AfterFixingDisplayJumping

You can notice the same change in other actions also, e.g. Add/Rename/Remove names, Refresh, Add/Delete/Modify comments, etc.

Test plan (required)

  1. Test to make sure that the breakpoints are toggled and highlighted properly in the decompiler and ensure if the display position is getting changed.
  2. See if the display position is changing when the view is refreshed using the refresh button.
  3. Notice the changes in the display that happens when an instruction is modified. For example, change an if condition from je address to jne address and see if the display changes in an undesirable way.
  4. Open the following three widgets in sync (Decompiler, Disassembly, Graph). Toggle breakpoints and make sure that every single change is reflected properly in all the widgets.

Closing issues
closes #2270

@NirmalManoj NirmalManoj requested review from karliss, ITAYC0HEN and thestr4ng3r and removed request for ITAYC0HEN July 30, 2020 15:16
@NirmalManoj NirmalManoj self-assigned this Jul 30, 2020
@NirmalManoj NirmalManoj added the Decompiler Issues and feature requests related to the decompiler in Cutter label Jul 30, 2020
@NirmalManoj NirmalManoj linked an issue Jul 30, 2020 that may be closed by this pull request
Decompiler view jumps when toggling a breakpoint or adding a comment #2270
Closed
3 tasks
@NirmalManoj NirmalManoj force-pushed the display-jumping-fix-useless-decompilation branch from ef14282 to e1775ef Compare July 30, 2020 18:51
karliss
karliss requested changes Jul 31, 2020
View reviewed changes
Copy link
Member

@karliss karliss left a comment

Choose a reason for hiding this comment

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

I sometimes noticed a situation where I select a line in decompiler, add a breakpoint and breakpoint appears in disassembly but not the decompiler. It's hard to tell if it's an actual problem or just r2ghidra returning bad offset<->line mapping.

src/core/Cutter.cpp Outdated Show resolved Hide resolved
@ITAYC0HEN
Copy link
Member

Does this PR is using functionalities from the decompiler-refactoring? if not, maybe this can be pushed to maste as it is a bug-fix\improvement which is small and can be enjoyed by our users. What do you think?

@NirmalManoj
Copy link
Member Author

@ITAYC0HEN Yes, we have improved "DecompilerWidget" and this uses some improvements and features in decompiler-refactoring (e.g. setInfoForBreakpoints()). It shouldn't take more than 10-12 days to get decompiler-refactoring ready to get merged to master.

@ITAYC0HEN
Copy link
Member

Thanks! <3

@NirmalManoj NirmalManoj merged commit a54b57a into rizinorg:decompiler-refactoring Aug 2, 2020
@NirmalManoj NirmalManoj moved this from In progress to Done in Improving Decompiler Widget (GSoC) Aug 2, 2020
NirmalManoj added a commit that referenced this pull request Aug 6, 2020
@NirmalManoj
Display jumping fix useless decompilation (#2351)
cf324e7 
* save scroll position and reset to that if the newly decompiled function is the same as the previous one

* emit instructionChanged signal replaced completely by breakpointsChanged in toggle/add breakpoint functions.

* removed addbreakpoint(QString) and toggleBreakpoint(QString)
NirmalManoj added a commit that referenced this pull request Aug 13, 2020
@NirmalManoj
Display jumping fix useless decompilation (#2351)
95e12d8 
* save scroll position and reset to that if the newly decompiled function is the same as the previous one

* emit instructionChanged signal replaced completely by breakpointsChanged in toggle/add breakpoint functions.

* removed addbreakpoint(QString) and toggleBreakpoint(QString)
NirmalManoj added a commit that referenced this pull request Aug 14, 2020
@NirmalManoj
Display jumping fix useless decompilation (#2351)
9102590 
* save scroll position and reset to that if the newly decompiled function is the same as the previous one

* instructionChanged signal replaced completely by breakpointsChanged in toggle/add breakpoint functions.

* removed addbreakpoint(QString) and toggleBreakpoint(QString)
NirmalManoj added a commit that referenced this pull request Aug 14, 2020
@NirmalManoj
Display jumping fix useless decompilation (#2351)
13112d4 
* save scroll position and reset to that if the newly decompiled function is the same as the previous one

* instructionChanged signal replaced completely by breakpointsChanged in toggle/add breakpoint functions.

* removed addbreakpoint(QString) and toggleBreakpoint(QString)
@NirmalManoj NirmalManoj mentioned this pull request Aug 14, 2020
Merged
3 tasks
NirmalManoj added a commit that referenced this pull request Aug 14, 2020
@NirmalManoj
Display jumping fix useless decompilation (#2351)
2146124 
* save scroll position and reset to that if the newly decompiled function is the same as the previous one

* instructionChanged signal replaced completely by breakpointsChanged in toggle/add breakpoint functions.

* removed addbreakpoint(QString) and toggleBreakpoint(QString)
NirmalManoj added a commit that referenced this pull request Aug 15, 2020
@NirmalManoj
Display jumping fix useless decompilation (#2351)
e3e1fb1 
* save scroll position and reset to that if the newly decompiled function is the same as the previous one

* instructionChanged signal replaced completely by breakpointsChanged in toggle/add breakpoint functions.

* removed addbreakpoint(QString) and toggleBreakpoint(QString)
NirmalManoj added a commit that referenced this pull request Aug 15, 2020
@NirmalManoj
Display jumping fix useless decompilation (#2351)
19c9580 
* save scroll position and reset to that if the newly decompiled function is the same as the previous one

* instructionChanged signal replaced completely by breakpointsChanged in toggle/add breakpoint functions.

* removed addbreakpoint(QString) and toggleBreakpoint(QString)
NirmalManoj added a commit that referenced this pull request Aug 18, 2020
@NirmalManoj
Display jumping fix useless decompilation (#2351)
b42a19e 
* save scroll position and reset to that if the newly decompiled function is the same as the previous one

* instructionChanged signal replaced completely by breakpointsChanged in toggle/add breakpoint functions.

* removed addbreakpoint(QString) and toggleBreakpoint(QString)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karliss karliss karliss approved these changes

@thestr4ng3r thestr4ng3r Awaiting requested review from thestr4ng3r

@ITAYC0HEN ITAYC0HEN Awaiting requested review from ITAYC0HEN

Assignees

@NirmalManoj NirmalManoj

Labels
Decompiler Issues and feature requests related to the decompiler in Cutter
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Decompiler view jumps when toggling a breakpoint or adding a comment
3 participants
@NirmalManoj @ITAYC0HEN @karliss