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

Patched with changes required after migrating RAnnotatedCode to radare2 #16

Merged
merged 1 commit into from May 30, 2020

Conversation

NirmalManoj
Copy link
Contributor

This PR is from the radare2 team. We have migrated RAnnotatedCode used in retdec-r2plugin to radare2. In the future, RAnnotatedCode will work as the standard decompiler interface for all decompiler plugins in Cutter. We have a series of work going on for improving the decompiler widget. This PR contains necessary changes required to make retdec-r2plugin work with updated radare2.

@s3rvac s3rvac requested a review from xkubov May 29, 2020 06:33
@NirmalManoj
Copy link
Contributor Author

It seems it checks out an old version of radare2 in the automated tests. That's the reason why tests failed.

@xkubov
Copy link
Contributor

xkubov commented May 29, 2020

Hi, thank you for the PR. As you have pointed out, the reason why tests failed is that this plugin is tested on an older Radare2 release (4.3.1) which is missing the interface for RAnnotatedCode. So these changes won't work on older releases that are available from package managers of different Linux distributions.

Don't you think that we should maintain some kind of compatibility with older versions? I can think of a few solutions: we can fetch the latest radare2 sources in CMake as external project - but the r_annotated_code would need to be linked statically. Another solution would be to conditionally include r_util/r_annotated_code.h.

Do you think it would make sense to do it? Or we should change the requirements of the radare2 version to the newest one?

@xkubov
Copy link
Contributor

xkubov commented May 29, 2020

I will merge this PR into branch win-support where I am preparing changes for the next release. I will test the options I mentioned above. And perhaps provide support for previous releases.

@thestr4ng3r
Copy link
Contributor

Conditional include could work temporarily, however it will break at some point too if the API in r2 changes.
What we do in r2ghidra is to always develop the master against r2 master and also run the tests against it. When r2 creates a release, we also tag r2ghidra's latest commit, so it is transparent which r2ghidra version is guaranteed to work with a certain r2 version, see rizinorg/rz-ghidra#92
On the other hand, this model also means previous r2 versions will not receive updates of the plugin.

@xkubov xkubov merged commit 569c7be into radareorg:master May 30, 2020
@xkubov
Copy link
Contributor

xkubov commented May 31, 2020

I agree, this solution sounds fine. I have merged this PR into master now with other changes and we'll tag commits with radare2 version supported so that installer could implement the logic of choosing appropriate commit.

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

3 participants