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

Action to rename functions in the decompiler context menu #2286

Merged

Conversation

NirmalManoj
Copy link
Member

@NirmalManoj NirmalManoj commented Jul 11, 2020

Your checklist for this pull request

Detailed description
This PR implements an action to rename functions in the decompiler context menu. With this PR, a user should be able to right-click a function's name in the decompiler widget and be able to rename the function. (See GIF below)

rename_function_small

Test plan (required)

  • Checkout into the decompiler-refactoring branch in radare2 and r2ghidra-dec and compile them before compiling this PR.
  • Test if the action is working as it's supposed to

Note: We have some issues with the renaming of the functions as described in #2119 and fixing it is outside the scope of this PR. @megabeets told me he will be fixing it in the future. For now, in order to overcome this problem temporarily, you have to disable asm.flags.real.

Closing issues

@NirmalManoj NirmalManoj requested review from ITAYC0HEN and karliss Jul 11, 2020
@NirmalManoj NirmalManoj self-assigned this Jul 11, 2020
.appveyor.yml Outdated
@@ -54,6 +54,10 @@ before_build:

# Build config
build_script:
- cmd: if defined QMAKE ( cd radare2 )
Copy link
Member

@karliss karliss Jul 11, 2020

Choose a reason for hiding this comment

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

Instead of adding this in all the CI scripts you should be able to just change the which commit git submodule points to.

Copy link
Member Author

@NirmalManoj NirmalManoj Jul 11, 2020

Choose a reason for hiding this comment

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

Are you saying I should push the radare2 submodule after checking out decompiler-refactoring?

Copy link
Member

@karliss karliss Jul 12, 2020

Choose a reason for hiding this comment

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

The process is exactly the same as when you update r2 submodule previously #2217 but instead of latest master you need to checkout latest version of decompiler-refactoring branch.

Copy link
Member

@karliss karliss Jul 12, 2020

Choose a reason for hiding this comment

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

Two more places remaining.

Copy link
Member Author

@NirmalManoj NirmalManoj Jul 12, 2020

Choose a reason for hiding this comment

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

I didn't understand what you meant by two more places. QMake build in appveyor works, however the meson build is still failing. Can you take a look here.

Copy link
Member

@karliss karliss Jul 13, 2020

Choose a reason for hiding this comment

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

Just got the same in r2 graph branch. I guess that it might be related to latest meson version. Will need to be fixed in cutter-master.

Copy link
Member

@karliss karliss Jul 15, 2020

Choose a reason for hiding this comment

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

Meson fix merged you might need to rebase decompiler-refactoring branch on top of latest master..

Copy link
Member Author

@NirmalManoj NirmalManoj Jul 15, 2020

Choose a reason for hiding this comment

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

I have tried to rebase and I am not sure if I messed something up. It seems to be working fine. Please check.

.appveyor.yml Outdated Show resolved Hide resolved
src/menus/DecompilerContextMenu.cpp Outdated Show resolved Hide resolved
src/widgets/DecompilerWidget.cpp Outdated Show resolved Hide resolved
src/widgets/DecompilerWidget.cpp Show resolved Hide resolved
@NirmalManoj
Copy link
Member Author

NirmalManoj commented Jul 11, 2020

I have fixed Travis CI by making it use the decompiler-refactoring branch in radare2. I'm still trying to fix GitHub workflow and appveyor. CI fails because it fetches the master from r2 and r2ghidra-dec.

src/core/Cutter.cpp Outdated Show resolved Hide resolved
src/widgets/DisassemblyWidget.cpp Outdated Show resolved Hide resolved
@NirmalManoj NirmalManoj requested a review from karliss Jul 12, 2020
src/menus/DecompilerContextMenu.cpp Outdated Show resolved Hide resolved
QString newName = dialog.getName();
if (!newName.isEmpty()) {
if (type == R_CODE_ANNOTATION_TYPE_FUNCTION_NAME) {
Core()->renameFunction(annotationHere->function_name.offset, newName);
Copy link
Member

@karliss karliss Jul 14, 2020

Choose a reason for hiding this comment

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

Rename action is shown even when there is no function defined in r2 at the specific address. In such situation rename fails. It should instead show an action for defining a function.

Copy link
Member Author

@NirmalManoj NirmalManoj Jul 14, 2020

Choose a reason for hiding this comment

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

I have implemented a define function dialog that shows up when the rename function action is triggered on undefined functions.
Defining_Function_When_Not_Defined

Copy link
Member

@karliss karliss Jul 15, 2020

Choose a reason for hiding this comment

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

It might have been nice to update the label within context menu as well. "Define this function at" sounds a bit weird to me. How about just "Define function at"?

Copy link
Member Author

@NirmalManoj NirmalManoj Jul 15, 2020

Choose a reason for hiding this comment

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

I think having "this" will make it clear that the function someone tried to rename hasn't been defined. Otherwise, we are showing a function in the widget, and on rename action, we are telling them to define a function at an address.

Copy link
Member Author

@NirmalManoj NirmalManoj Jul 15, 2020

Choose a reason for hiding this comment

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

I'd say let's merge as is and decide this later while merging to master.

@NirmalManoj NirmalManoj requested a review from karliss Jul 15, 2020
@NirmalManoj
Copy link
Member Author

NirmalManoj commented Jul 15, 2020

@karliss There are changes to annotations as suggested by you in the currently open PR in r2. I think that shouldn't cause this PR from getting merged. I will open another PR for the show in action and that will refactor code for rename action as well.

Copy link
Member

@karliss karliss left a comment

It's up to you decide if you want do anything about my last comment on defining a function right now or merge as is.

@NirmalManoj NirmalManoj force-pushed the rename-function-action branch 3 times, most recently from 0a57284 to c595141 Compare Jul 15, 2020
@NirmalManoj NirmalManoj requested a review from karliss Jul 16, 2020
@karliss karliss merged commit afce8ec into rizinorg:decompiler-refactoring Jul 16, 2020
7 of 8 checks passed
NirmalManoj added a commit to NirmalManoj/cutter that referenced this pull request Jul 28, 2020
@NirmalManoj NirmalManoj moved this from In progress to Done in Improving Decompiler Widget (GSoC) Jul 29, 2020
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.

None yet

2 participants