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 from decompileAt() #17

Closed

Conversation

NirmalManoj
Copy link
Contributor

@NirmalManoj NirmalManoj commented Jun 18, 2020

This PR is from the Cutter team. We have a series of work going on for improving the decompiler widget as mentioned in PR #16 . Now we have changed the decompiler widget to base it directly on RAnnotatedCode removing the custom AnnotatedCode struct that was present in Cutter. The changes have been merged to the master branch in Cutter. This PR contains necessary changes required to make retdec-r2plugin work with the updated Cutter. It seems to be working as well as it was before. But I am not entirely sure if I missed anything important.

@s3rvac s3rvac requested a review from xkubov Jun 18, 2020
@NirmalManoj
Copy link
Contributor Author

NirmalManoj commented Jun 18, 2020

From what I understand, tests are failing because it compiles with an older version of Cutter.

src/r2retdec.h Outdated

RAnnotatedCode* r2retdec_decompile_annotated_code(RCore *core, ut64 addr);

#endif //R2RETDEC_H
Copy link
Collaborator

@xkubov xkubov Jun 19, 2020

Choose a reason for hiding this comment

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

Can you move this file to the include/r2plugin?

With this update it should be required to add core_retdec here in cutter-plugin/CMakeLists.txt.

Copy link
Contributor Author

@NirmalManoj NirmalManoj Jun 20, 2020

Choose a reason for hiding this comment

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

Hi, I'm thinking of making a new PR from a new branch made from the current master of retdec-r2plugin. I tried rebasing it, but couldn't succeed with minimal effort. I think to make a PR from the new branch will be less time-consuming. Are you okay with this? Also, can you please tell me what exactly I should add in cutter-plugin/CMakeLists.txt.

@@ -7,6 +7,7 @@
*/

#include "R2RetDec.h"
#include "../src/r2retdec.h"
Copy link
Collaborator

@xkubov xkubov Jun 19, 2020

Choose a reason for hiding this comment

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

See the comment below regarding the position of r2retdec.h.

@xkubov
Copy link
Collaborator

xkubov commented Jun 19, 2020

Hi, thank you for the PR! Can you also provide an update to GIT TAG of cutter version in deps/cutter/CMakeLists.txt?

I'll see into why the tests failed - currently in the master is Cutter plugin not built.

I have some notes that I have provided in the review, otherwise, changes look good to me.

@NirmalManoj
Copy link
Contributor Author

NirmalManoj commented Jun 19, 2020

@xkubov Hi, as of now, 1.10.3 (current GIT TAG) is the latest release of Cutter. New API will be used by the next release.

I will make all the other changes that you requested.

@xkubov
Copy link
Collaborator

xkubov commented Jun 19, 2020

Thanks. Regarding the GIT_TAG please change it to master - There was a discussion because of a similar issue with radare2 version support in #16 and I think we would like to have r2retdec master to be compatible with both radare2 master and Cutter master. That's why I think that for these changes to work GIT_TAG should state master. Otherwise the old cutter version will be fetched and build of cutter plugin will fail.

@NirmalManoj
Copy link
Contributor Author

NirmalManoj commented Jun 21, 2020

Hi @xkubov , yesterday I had mentioned that I failed to successfully rebase this branch. Later I made a new branch and introduced all changes into it. Please have a look here. Unfortunately, it's still failing to work in both r2 and Cutter. This is the error message I get when running pdz
[0x004005bd]> pdz
retdec-r2plugin: decompilation was not successful: exit code: 256.

It took me quite a while to realize that this problem was not caused by changes I introduced. The same problem happens with the current master of retdec-r2plugin. Investigating further, I went back to the following commit in r2.

1cb18df8b4dfd2ec98ca6697b5efb9381e1d2f40 (HEAD)
Author: Khairul Azhar Kasmiran kazarmy@gmail.com
Date: Sat May 30 01:31:22 2020 +0800

Last commit in the retdec-r2plugin master was this

commit 1ba2247
Date: Sat May 30 18:11:44 2020 +0200
Release v0.1.2

It still fails. So I am wondering if it's a problem at my end. Can you please have a look at this?

@xkubov
Copy link
Collaborator

xkubov commented Jun 21, 2020

Hi @NirmalManoj, I am going to take a look at this, thanks for the report. I'll also take a look at the new branch you have prepared and I think that you can just force push new commits to the branch you have opened this PR with. That should not affect this conversation and I'll be able to review the new code from here.

@NirmalManoj NirmalManoj force-pushed the emit-rannotatedcode-to-cutter branch from 1372dc6 to e6c46c7 Compare Jun 21, 2020
@NirmalManoj
Copy link
Contributor Author

NirmalManoj commented Jun 21, 2020

I think that you can just force push new commits to the branch you have opened this PR with.

Yes, I have force pushed changes to this branch now.

@@ -7,7 +7,7 @@
*/

#include "R2RetDec.h"

#include "../include/r2plugin/r2retdec.h"
Copy link
Collaborator

@xkubov xkubov Jun 21, 2020

Choose a reason for hiding this comment

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

I think that this should be only

#include "r2plugin/r2retdec.h"

For this to work add the following to the cutter-plugin/CMakeLists.txt to this list

 target_link_libraries(r2retdec_cutter core_retdec)

This is needed anyways as we want the core_retdec.cpp to be linked to the cutter plugin too.

Also to build your branch with -DBUILD_CUTTER_PLUGIN=on I had to change the GIT_TAG to master so I think that this should be included in this PR too (in deps/cutter/CMakeLists.txt).

Copy link
Contributor Author

@NirmalManoj NirmalManoj Jun 21, 2020

Choose a reason for hiding this comment

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

Made these changes.

@xkubov
Copy link
Collaborator

xkubov commented Jun 21, 2020

So, I was able to build and test your changes locally with a clean build of retdec-r2plugin with following cmake arguments...

cmake .. -DCMAKE_INSTALL_PREFIX=~/.local -DCMAKE_EXPORT_COMPILE_COMMANDS=on -DBUILD_BUNDLED_RETDEC=on -DBUILD_CUTTER_PLUGIN=on

... and latest radare2 master installed in the system.

Can you attach logs of RetDec that can be obtained by doing following steps?

  1. Create and export path to the directory that will contain logs:
    mkdir tmp
    export DEC_SAVE_DIR=tmp

  2. Run pdz in r2. After decompilation there should be following files available: tmp/rd_{err,out}.log.

@NirmalManoj
Copy link
Contributor Author

NirmalManoj commented Jun 21, 2020

@xkubov Thanks. I was using the following cmake arguments: cmake -DBUILD_CUTTER_PLUGIN=ON -DCUTTER_SOURCE_DIR=PATH_TO_CUTTER -DCMAKE_INSTALL_PREFIX=~/.local ... The problem still persists. I'm attaching the logs that you asked for log.zip.

Can you confirm if retdec-r2plugin is working as expected for you?

@xkubov
Copy link
Collaborator

xkubov commented Jun 21, 2020

@NirmalManoj I had a similar problem when I used the latest RetDec with r2retdec master. Did you build RetDec manually (separately from this plugin)?

Right now the last supported RetDec version is that which is included with this plugin. The reason why the latest RetDec does not work with this plugin is that in the current master RetDec decompiler was rewritten to the library and there is not a retdec-decompiler.py script provided, only retdec-decompiler executable (And old retdec-decompiler.py script does not seem to work with latest changes).

I am currently finishing the integration of these changes into the plugin (retdec-lib branch) and merge them to master soon. But I would like those changes to be rebased onto this PR so thanks for the updates. Now it looks good. I'll test that manually and meanwhile fix the integration tests that seem not to work properly. I was able to run r2plugin with changes in this PR before your latest update so I'll let you know.

@NirmalManoj
Copy link
Contributor Author

NirmalManoj commented Jun 21, 2020

Did you build RetDec manually (separately from this plugin)?

No. I built RetDec with plugin using the cmake command I mentioned.

xkubov
xkubov approved these changes Jun 21, 2020
@xkubov
Copy link
Collaborator

xkubov commented Aug 18, 2020

This is now merged into master, commit 5fbe470.

@xkubov xkubov closed this Aug 18, 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