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

Emit RAnnotatedCode to Cutter #112

Merged
merged 11 commits into from Jun 7, 2020
Merged

Emit RAnnotatedCode to Cutter #112

merged 11 commits into from Jun 7, 2020

Conversation

NirmalManoj
Copy link
Member

@NirmalManoj NirmalManoj commented May 30, 2020

Detailed description
[Update: This was merged to master by PR #114]

In this PR, RAnnotatedCode is emitted from the decompileAt() function called by Cutter. Made changes to make this possible.
There is code duplication in r2ghidra_decompile_annotated_code() and most of it is from Decompile(). I need suggestions in the ways I should refactor both functions to make it more elegant, if possible.
...

Test plan

  1. Get PR #2227 and the latest radare2 version before compiling this PR.
  2. Open the decompiler widget and make sure that the correct decompiled code is being shown while you select offsets from functions and other widgets.
  3. Make sure that the decompiler widget is seeking the correct address while clicking on functions(and other offsets) in the decompiler widget.
  4. Make sure that invalid offsets are not causing Segmentation Fault.
  5. Check the code and make sure that I have not missed any exceptional cases that will cause errors.

...

Closing issues

Notes
Merge all these three PRs together to ensure smooth functioning of all decompilers.
Refactored decompiler widget and R2Dec plugin: Cutter #2227
Refactored R2Ghidra plugin that emits RAnnotatedCode: R2Ghidra-dec #112
Refactored retdec plugin that emits RAnnotatedCode: Florian's fork #1, after this PR is verified, I will send it to retdec's repo.
...

@NirmalManoj NirmalManoj requested a review from thestr4ng3r May 30, 2020
cutter-plugin/R2GhidraDecompiler.cpp Outdated Show resolved Hide resolved
@@ -132,6 +133,87 @@ static void ApplyPrintCConfig(RConfig *cfg, PrintC *print_c)
print_c->setMaxLineSize(cfg_var_linelen.GetInt(cfg));
}

RAnnotatedCode* r2ghidra_decompile_annotated_code(RCore *core){
Copy link
Member

@karliss karliss Jun 2, 2020

Choose a reason for hiding this comment

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

I would prefer if the internal implementation function passed offset explicitly instead of relying on global offset value. If necessary the core->offset can be passed as default position but that should be done at user interface level in the corresponding r2 command handler.

Copy link
Member Author

@NirmalManoj NirmalManoj Jun 2, 2020

Choose a reason for hiding this comment

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

I would prefer if the internal implementation function passed offset explicitly instead of relying on global offset value.

What benefit do we get from this? Is static void Decompile(RCore *core, DecompileMode mode) also not doing the same thing?

Copy link
Member

@karliss karliss Jun 2, 2020

Choose a reason for hiding this comment

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

Yes there are other parts of code doing this. Poor separation of user interface from the implementation of main logic isn't uncommon in r2 and it makes it's API more difficult to use.

There are two aspects to this.

One is that "global variables are evil". core->offset isn't a one from language perspective but from the perspective of data-flow in the whole system it is one, with the same problems. You can find plenty of articles on this topic.

Other aspect is that core->offset is mixing responsibilities of 3 different concepts.

  • core->offset as part of interface - this only makes sense in the r2 command mode, in Cutter and Panel mode there is no single current offset.
  • current offset as implementation detail of some io backends
  • function capable of decompiling code at any point of program

Decompilation function shouldn't know anything about the current interface state. It is the responsibility of user interface tracking which parts you are looking at and to request decompiling them.

Copy link
Member

@ITAYC0HEN ITAYC0HEN Jun 2, 2020

Choose a reason for hiding this comment

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

I am joining @karliss here with these concerns.
@NirmalManoj please suggest better API layer functions to demonstrate the desired changes, show us and then implement if it looks good :)

Copy link
Member Author

@NirmalManoj NirmalManoj Jun 4, 2020

Choose a reason for hiding this comment

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

Can we change RAnnotatedCode* r2ghidra_decompile_annotated_code(RCore *core) to RAnnotatedCode* r2ghidra_decompile_annotated_code(RCore *core, RVA addr)?

With this change, in r2ghidra_decompile_annotated_code we decompile code at the address RVA addr without using core->offset. @karliss @ITAYC0HEN

Copy link
Member Author

@NirmalManoj NirmalManoj Jun 4, 2020

Choose a reason for hiding this comment

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

I have made this change in the new commit @karliss @ITAYC0HEN . Now, cutter's plugin doesn't use core->offset directly or indirectly(as far as I could see:))

Copy link
Member

@karliss karliss Jun 4, 2020

Choose a reason for hiding this comment

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

Looks good.

Copy link
Member Author

@NirmalManoj NirmalManoj Jun 4, 2020

Choose a reason for hiding this comment

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

@thestr4ng3r Should I make similar changes for retdec in the current PR?

Copy link
Member

@thestr4ng3r thestr4ng3r Jun 4, 2020

Choose a reason for hiding this comment

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

Yes please.

Copy link
Member Author

@NirmalManoj NirmalManoj Jun 4, 2020

Choose a reason for hiding this comment

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

@thestr4ng3r Done. Please check if I have missed someplace that fetches offset from the core.

@NirmalManoj NirmalManoj changed the title Emit rannotatedcode to cutter Emit RAnnotatedCode to Cutter Jun 4, 2020
Copy link
Member

@ITAYC0HEN ITAYC0HEN left a comment

The code looks good to me, as well as the actual results when running :)
I would like to wait for all of the reviewers to make this PR green before merging (@thestr4ng3r @karliss )

Also, whoever merges -> make sure to read the notes and merge all the relevant PRs together :)

@NirmalManoj NirmalManoj requested review from ITAYC0HEN and karliss Jun 4, 2020
@@ -132,6 +133,87 @@ static void ApplyPrintCConfig(RConfig *cfg, PrintC *print_c)
print_c->setMaxLineSize(cfg_var_linelen.GetInt(cfg));
}

RAnnotatedCode* r2ghidra_decompile_annotated_code(RCore *core, ut64 addr){
Copy link
Member

@karliss karliss Jun 5, 2020

Choose a reason for hiding this comment

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

r2ghidra_decompile_annotated_code is mostly identical with Decompile it should be be possible to refactor the common part into separate function.

src/core_ghidra.cpp Show resolved Hide resolved
@@ -271,7 +281,9 @@ static void Decompile(RCore *core, DecompileMode mode)
{
PJ *pj = pj_new ();
if(!pj)
{
Copy link
Member

@thestr4ng3r thestr4ng3r Jun 6, 2020

Choose a reason for hiding this comment

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

Copy link
Member Author

@NirmalManoj NirmalManoj Jun 6, 2020

Choose a reason for hiding this comment

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

@thestr4ng3r Please review again

@NirmalManoj NirmalManoj requested review from thestr4ng3r and karliss Jun 6, 2020
Copy link
Member

@thestr4ng3r thestr4ng3r left a comment

Please make sure to closely follow this style in core_ghidra.cpp: https://gist.github.com/thestr4ng3r/d0b1de10852ca3d119e8c83375211e4b
The only exception is the name of r2ghidra_decompile_annotated_code() because it is part of the public interface.

CMakeLists.txt Outdated
@@ -46,7 +46,8 @@ set(CORE_SOURCE
src/R2PrintC.h
src/R2PrintC.cpp
src/RCoreMutex.h
src/RCoreMutex.cpp)
src/RCoreMutex.cpp
src/r2ghidra_annotated_code.h)
Copy link
Member

@thestr4ng3r thestr4ng3r Jun 6, 2020

Choose a reason for hiding this comment

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

Same as in retdec, you can call this file r2ghidra.h.

Copy link
Member Author

@NirmalManoj NirmalManoj Jun 6, 2020

Choose a reason for hiding this comment

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

Done.

src/core_ghidra.cpp Show resolved Hide resolved
DecompilerLock lock;

#ifndef DEBUG_EXCEPTIONS
static void refactored_decompile(RCore *&core, std::stringstream &out_stream,
Copy link
Member

@thestr4ng3r thestr4ng3r Jun 6, 2020

Choose a reason for hiding this comment

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

Better signature:

static void Decompile(RCore *core, ut64 addr, DecompileMode mode, std::stringstream &out_stream, RAnnotatedCode **out_code)

The & for core is unnecessary, for code it's better readable like this (you will have to use *out_code instead of code in the function body).
Also this makes is more clear what the inputs and what the outputs are.

Copy link
Member Author

@NirmalManoj NirmalManoj Jun 6, 2020

Choose a reason for hiding this comment

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

Changed.

}
}

static void Decompile(RCore *core, DecompileMode mode)
Copy link
Member

@thestr4ng3r thestr4ng3r Jun 6, 2020

Choose a reason for hiding this comment

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

Suggested change
static void Decompile(RCore *core, DecompileMode mode)
static void DecompileCmd(RCore *core, DecompileMode mode)

to distinguish it from the regular Decompile() as mentioned above.

Copy link
Member Author

@NirmalManoj NirmalManoj Jun 6, 2020

Choose a reason for hiding this comment

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

Done.


#ifndef DEBUG_EXCEPTIONS
static void refactored_decompile(RCore *&core, std::stringstream &out_stream,
DecompileMode mode, RAnnotatedCode *&code, ut64 addr){
Copy link
Member

@thestr4ng3r thestr4ng3r Jun 6, 2020

Choose a reason for hiding this comment

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

Excessive indent, you can keep this all on a single line.

Copy link
Member Author

@NirmalManoj NirmalManoj Jun 6, 2020

Choose a reason for hiding this comment

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

Changed as requested above.

#include <r_util/r_annotated_code.h>
#include <r_core.h>

R_API RAnnotatedCode* r2ghidra_decompile_annotated_code(RCore *core, ut64 addr);
Copy link
Member

@thestr4ng3r thestr4ng3r Jun 6, 2020

Choose a reason for hiding this comment

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

Suggested change
R_API RAnnotatedCode* r2ghidra_decompile_annotated_code(RCore *core, ut64 addr);
R_API RAnnotatedCode *r2ghidra_decompile_annotated_code(RCore *core, ut64 addr);

Copy link
Member Author

@NirmalManoj NirmalManoj Jun 6, 2020

Choose a reason for hiding this comment

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

Changed

@NirmalManoj NirmalManoj requested a review from thestr4ng3r Jun 6, 2020
@thestr4ng3r thestr4ng3r merged commit 9994a2e into rizinorg:annotated-code Jun 7, 2020
1 check passed
@ITAYC0HEN
Copy link
Member

ITAYC0HEN commented Jun 7, 2020

Woo-hoo! merged :)
Nirmal asked to "Merge all these three PRs together to ensure smooth functioning of all decompilers."

so we should quickly move on to the others, right?
:)

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

4 participants