Edit/Rename Variables Actions for function variables #2357

NirmalManoj · 2020-08-01T11:58:05Z

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 updated the documentation with the relevant information (if needed)

Detailed description
This PR implements edit variable action and rename variable action for function variables(function parameters and local variables).

See GIFs below.
1. Both actions working for variables that are defined in radare2.

2. Disabled actions and tooltips for them. Actions are disabled when the variable represented by the annotation is not present in radare2.

3. When the rename variable action is triggered by shortcut for a variable that doesn't exist in radare2.

4. When the edit variable action is triggered by shortcut for a variable that doesn't exist in radare2.

Test plan (required)

Fetch PR Annotator for local variables and function paramters rz-ghidra#128 ~~and checkout commit rizinorg/rz-ghidra@ac829cb~~. Compile it.
Load functions in Cutter and find variables and arguments that are present in the disassembly. See GIF 1.
1. Check if rename action and edit variable action are working properly in both ways. That is, using the context menu and by using the shortcut.
Find variables and arguments in the binary loaded that are not present in the disassembly.
1. Right-click and in the context menu, make sure these actions are disabled and the tooltips are shown when you hover over the action. Refer to GIF 2.
2. Click on these variables and try to use action for renaming variable and editing variables using shortcuts. Both should show a dialog. Refer to GIF 3 and 4.
Try using the rename action for references and make sure this PR didn't cause any regressions.

Closing issues

src/menus/DecompilerContextMenu.cpp

Surendrajat · 2020-08-02T05:11:04Z

Is a popup saying can't do it really any better than doing nothing? How about showing this info on the status bar instead? I find it a bit annoying to move the mouse to close it every time I press N by mistake.

NirmalManoj · 2020-08-02T06:48:03Z

Is a popup saying can't do it really any better than doing nothing? How about showing this info on the status bar instead? I find it a bit annoying to move the mouse to close it every time I press N by mistake.

The idea is to show this warning so that the user will know that it is the expected behavior. Otherwise, I think it might look like a broken feature. How often do people press N by mistake while the cursor is over a variable? Also, in order to close the warning dialog, you can press Enter instead of moving the mouse.

About the status bar, I think this is a bit too verbose to be shown in the status bar. As of now, you can see this dialog only if you press N or Y on keyboard, and it can be closed by Enter. These actions are disabled in the context menu and a tooltip is shown instead when the variable doesn't exist in radare2.

karliss · 2020-08-02T10:18:30Z

src/core/Cutter.cpp

@@ -676,6 +676,11 @@ void CutterCore::renameFlag(QString old_name, QString new_name)
    emit flagsChanged();
 }

+void CutterCore::renameFunctionVariable(QString newName, QString oldName, RVA offset){
+    cmdRawAt(QString("afvn %1 %2").arg(newName).arg(oldName), offset);


There will be problems if variable name contains special symbols especially space.

Looks like the relevant part of afvn command is straight forward. Get function at offset, get function variable with old name, call rename on old variable. Don't forget about CORE_LOCK.

I have tried to use r2 API in the recently pushed changes. Please check.

This is still incomplete and shows a slightly weird behavior where this stops working at times. Then I try to rename variable from disassembly, it works. And when I come back to the decompiler widget, that also starts to work. I get this message in the console.
WARNING: r_anal_function_get_var_byname: assertion 'fcn && name' failed (line 235)

The assert is quite clear. The function expected that fcn and name are not null pointers but you passed a null pointer. You just need to follow the basic process of debugging - if something complains about bad value think did you expected to have a bad value there, if not where did the bad value came from, why did the code from which the bad value came returned a bad value, did you fulfill all the input requirements of function that returned you bad value, repeat the process until you locate the place where you made the mistake.

I fixed this problem. I was using r2 API in the wrong manner. I thought r_anal_get_function_at() would give us the function from any offset inside the function.

R2 allows functions to overlap and the function range analysis isn't guaranteed to be identical between r2 and ghidra. I have seen both cases r2 splitting code into multiple smaller functions than ghidra and ghidra not being able to fully analyze a large switch statement when r2 could. To better handle such cases I strongly advice:

pass the function addres to renameFunctionVariable thus allowing caller to decide if they have more precise information available than getFunctionStart/r_anal_get_functions_in. In the case of decompiler widget you already know the address of function which was decompiled.

don't call getFunctionStart in renameFunctionVariable and update the documentation explaining that it expects the address of function instead of any address inside the function

I will make this change.

In the case of decompiler widget you already know the address of function which was decompiled.

If you meant decompiledFunctionAddr, is it also not fetched in the same manner?

decompiledFunctionAddr = Core()->getFunctionStart(addr); dec->decompileAt(addr); ```

That's not exactly the since after the function has been decompiled you can click on any address decompiler thought belongs to the function not only the ones that would be chosen when deciding which function to decompile.

Changed as suggested.

ITAYC0HEN · 2020-08-02T15:14:35Z

src/menus/DecompilerContextMenu.cpp

+    actionEditFunctionVariables.setToolTip(tr("Can't edit this variable."
+                                              "<br>Only local variables defined in disassembly can be edited."));


Suggested change

actionEditFunctionVariables.setToolTip(tr("Can't edit this variable."

"<br>Only local variables defined in disassembly can be edited."));

actionEditFunctionVariables.setToolTip(tr("Can't edit this variable.<br>"

"Only local variables defined in the disassembly can be edited."));

src/menus/DecompilerContextMenu.cpp

not context sensitive yet

tooltip for disabled action left

changes to editvariabledialog's title

and update submodule

* Edit Function Variables Action * Rename Function Variables Action * CutterCore::renameFunctionVariable

NirmalManoj commented Aug 1, 2020

View reviewed changes

src/menus/DecompilerContextMenu.cpp Outdated Show resolved Hide resolved

NirmalManoj self-assigned this Aug 1, 2020

NirmalManoj added the Decompiler Issues and feature requests related to the decompiler in Cutter label Aug 1, 2020

NirmalManoj added this to In progress in Improving Decompiler Widget (GSoC) Aug 1, 2020

NirmalManoj requested review from ITAYC0HEN, karliss and thestr4ng3r August 1, 2020 12:17

NirmalManoj changed the title ~~Retype variables action~~ Edit/Rename Variables Actions for function variables Aug 1, 2020

karliss requested changes Aug 3, 2020

View reviewed changes

ITAYC0HEN requested changes Aug 3, 2020

View reviewed changes

NirmalManoj requested review from karliss and ITAYC0HEN August 4, 2020 19:04

NirmalManoj added 13 commits August 5, 2020 16:21

skelton

480a838

not context sensitive yet

more context

ff9a93f

tooltip for disabled action left

Warning dialog for non-mapped variables implemented

0cbe53d

formatted warning dialog text

10b1a5d

Rename for function variables

8a90abc

CutterCore::renameFunctionVariable + rename action for function vars

b908d63

changes to editvariabledialog's title

Tool tip

ce0f347

Final clean up before PR

99b9932

use r2 api for renaming

8c02f29

and update submodule

suggested change - <br>

93364b4

use r_annotation_is_variable()

0bb5245

.data() -> constData()

b242577

Core()->getFunctionStart(offset))

699b26a

NirmalManoj force-pushed the retype-variables-action branch from 5b67dc8 to 699b26a Compare August 5, 2020 11:05

NirmalManoj added 2 commits August 5, 2020 17:15

Remove unnecessary Core()->

cfc08e6

pass exact function address to renameFunctionVariable()

f4b88b8

karliss approved these changes Aug 6, 2020

View reviewed changes

NirmalManoj merged commit b8a3e95 into rizinorg:decompiler-refactoring Aug 6, 2020

NirmalManoj added a commit that referenced this pull request Aug 6, 2020

Edit/Rename Variables Actions for function variables (#2357)

b56001b

* Edit Function Variables Action * Rename Function Variables Action * CutterCore::renameFunctionVariable

NirmalManoj moved this from In progress to Done in Improving Decompiler Widget (GSoC) Aug 10, 2020

NirmalManoj added a commit that referenced this pull request Aug 13, 2020

Edit/Rename Variables Actions for function variables (#2357)

f3a3e62

* Edit Function Variables Action * Rename Function Variables Action * CutterCore::renameFunctionVariable

NirmalManoj added a commit that referenced this pull request Aug 14, 2020

Edit/Rename Variables Actions for function variables (#2357)

4af1041

* Edit Function Variables Action * Rename Function Variables Action * CutterCore::renameFunctionVariable

NirmalManoj added a commit that referenced this pull request Aug 14, 2020

Edit/Rename Variables Actions for function variables (#2357)

556c3f6

* Edit Function Variables Action * Rename Function Variables Action * CutterCore::renameFunctionVariable

NirmalManoj added a commit that referenced this pull request Aug 14, 2020

Edit/Rename Variables Actions for function variables (#2357)

c04b524

* Edit Function Variables Action * Rename Function Variables Action * CutterCore::renameFunctionVariable

NirmalManoj mentioned this pull request Aug 14, 2020

Enhanced Decompiler Widget with new Dedicated Context Menu and Actions #2391

Merged

3 tasks

NirmalManoj added a commit that referenced this pull request Aug 14, 2020

Edit/Rename Variables Actions for function variables (#2357)

d1eca1d

* Edit Function Variables Action * Rename Function Variables Action * CutterCore::renameFunctionVariable

NirmalManoj added a commit that referenced this pull request Aug 15, 2020

Edit/Rename Variables Actions for function variables (#2357)

32257d8

* Edit Function Variables Action * Rename Function Variables Action * CutterCore::renameFunctionVariable

NirmalManoj added a commit that referenced this pull request Aug 15, 2020

Edit/Rename Variables Actions for function variables (#2357)

1c7f420

* Edit Function Variables Action * Rename Function Variables Action * CutterCore::renameFunctionVariable

NirmalManoj added a commit that referenced this pull request Aug 17, 2020

Edit/Rename Variables Actions for function variables (#2357)

bd550f0

* Edit Function Variables Action * Rename Function Variables Action * CutterCore::renameFunctionVariable

NirmalManoj added a commit that referenced this pull request Aug 18, 2020

Edit/Rename Variables Actions for function variables (#2357)

1c86f54

* Edit Function Variables Action * Rename Function Variables Action * CutterCore::renameFunctionVariable

thestr4ng3r removed this from Done in Improving Decompiler Widget (GSoC) Dec 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Edit/Rename Variables Actions for function variables #2357

Edit/Rename Variables Actions for function variables #2357

NirmalManoj commented Aug 1, 2020 •

edited

Surendrajat commented Aug 2, 2020

NirmalManoj commented Aug 2, 2020 •

edited

karliss Aug 2, 2020

NirmalManoj Aug 3, 2020

NirmalManoj Aug 3, 2020

karliss Aug 5, 2020

NirmalManoj Aug 5, 2020

karliss Aug 5, 2020

NirmalManoj Aug 5, 2020

NirmalManoj Aug 5, 2020

karliss Aug 5, 2020

NirmalManoj Aug 5, 2020 •

edited

ITAYC0HEN Aug 2, 2020

NirmalManoj Aug 3, 2020

		actionEditFunctionVariables.setToolTip(tr("Can't edit this variable."
		"<br>Only local variables defined in disassembly can be edited."));

Edit/Rename Variables Actions for function variables #2357

Edit/Rename Variables Actions for function variables #2357

Conversation

NirmalManoj commented Aug 1, 2020 • edited

Surendrajat commented Aug 2, 2020

NirmalManoj commented Aug 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NirmalManoj Aug 5, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NirmalManoj commented Aug 1, 2020 •

edited

NirmalManoj commented Aug 2, 2020 •

edited

NirmalManoj Aug 5, 2020 •

edited