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

Shifting of RAnnotatedCode to radare2 #16939

Merged
merged 24 commits into from May 27, 2020

Conversation

NirmalManoj
Copy link
Contributor

@NirmalManoj NirmalManoj commented May 22, 2020

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 added tests that prove my fix is effective or that my feature works (if possible)
  • I've updated the documentation and the radare2 book with the relevant information (if needed)

Detailed description
As part of improving the decompiler and how it represents data, the custom struct AnnotatedCode in Cutter will be replaced by RAnnotatedCode. RAnnotatedCode is in r2ghidra-dec as of now. This PR shifts it to radare2. Necessary changes are made to r2ghidra-dec as well, and it will be a part of a corresponding PR in r2ghidra-dec, as these changes should go live together. Corresponding PR in r2ghidra-dec is this #107

Unit tests for RAnnotatedCode are also made and uploaded as part of this PR.

Test plan

Closing issues

...

@NirmalManoj NirmalManoj requested a review from thestr4ng3r May 22, 2020
@NirmalManoj NirmalManoj marked this pull request as ready for review May 22, 2020
@NirmalManoj NirmalManoj requested a review from ITAYC0HEN May 22, 2020
@NirmalManoj NirmalManoj changed the title R2 annotation shift Shifting of RAnnotatedCode to radare2 May 22, 2020
test/unit/test_annotated_code.c Outdated Show resolved Hide resolved
libr/include/r_util/r_annotated_code.h Outdated Show resolved Hide resolved
libr/include/r_util/r_annotated_code.h Outdated Show resolved Hide resolved
/*
Takes RAnnotatedCode structure and prints it in JSON format.
*/
R_API void r_annotated_code_print_json(RAnnotatedCode *code);
Copy link
Contributor

@ret2libc ret2libc May 22, 2020

Choose a reason for hiding this comment

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

this should not be here, because this file is for r_util and the code is defined in core. This should be somewhere in in libr/include, maybe even in its own files or just directly in libr/include/r_core.h but not here.

Copy link
Contributor

@ret2libc ret2libc May 22, 2020

Choose a reason for hiding this comment

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

same for the other functions that are defined in core_annotated_code.c

Copy link
Contributor Author

@NirmalManoj NirmalManoj May 23, 2020

Choose a reason for hiding this comment

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

@thestr4ng3r told me to do it this way. Maybe he overlooked this. I think I should wait for his opinion on this.

Copy link
Contributor

@thestr4ng3r thestr4ng3r May 23, 2020

Choose a reason for hiding this comment

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

@ret2libc is right. I would prefix these functions with r_core_annotated_code_... and put their declarations right here:

Copy link
Contributor Author

@NirmalManoj NirmalManoj May 23, 2020

Choose a reason for hiding this comment

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

@thestr4ng3r These printing functions need RAnnotatedCode that is is r_annotated_code.h

R_API void r_core_annotated_code_print_json(RAnnotatedCode *code);
R_API void r_core_annotated_code_print(RAnnotatedCode *code, RVector *line_offsets);
R_API void r_core_annotated_code_print_comment_cmds(RAnnotatedCode *code);

How can we handle this issue?

Copy link
Contributor Author

@NirmalManoj NirmalManoj May 23, 2020

Choose a reason for hiding this comment

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

@thestr4ng3r @ret2libc as I said earlier, the printing function depends on r_annotated_code.h and r_annotated_code.h depends on r_core.h, so we can't move this without changing any of these dependencies. But looking at the code, I don't understand why we need #include <r_core.h> in r_annotated_code.h . Can we not have that in r_annotated_code.h ?

Copy link
Contributor Author

@NirmalManoj NirmalManoj May 23, 2020

Choose a reason for hiding this comment

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

I am committing with this change now.

libr/core/core_annotated_code.c Outdated Show resolved Hide resolved
@github-actions github-actions bot added API New API requests, changes, removal r2r Regression tests labels May 22, 2020
Copy link
Contributor

@ITAYC0HEN ITAYC0HEN left a comment

Good work. Will go over it soon,
but as we discussed - use standard functiondocumentation style

https://www.kernel.org/doc/html/v4.16/doc-guide/kernel-doc.html#how-to-format-kernel-doc-comments

libr/core/core_annotated_code.c Outdated Show resolved Hide resolved
libr/include/r_util/r_annotated_code.h Outdated Show resolved Hide resolved
/*
Takes RAnnotatedCode structure and prints it in JSON format.
*/
R_API void r_annotated_code_print_json(RAnnotatedCode *code);
Copy link
Contributor

@thestr4ng3r thestr4ng3r May 23, 2020

Choose a reason for hiding this comment

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

@ret2libc is right. I would prefix these functions with r_core_annotated_code_... and put their declarations right here:

test/unit/test_annotated_code.c Outdated Show resolved Hide resolved
test/unit/test_annotated_code.c Outdated Show resolved Hide resolved
test/unit/test_annotated_code.c Outdated Show resolved Hide resolved
test/unit/test_annotated_code.c Outdated Show resolved Hide resolved
test/unit/test_annotated_code.c Outdated Show resolved Hide resolved
test/unit/test_annotated_code.c Outdated Show resolved Hide resolved
test/unit/test_annotated_code.c Outdated Show resolved Hide resolved
libr/include/r_util/r_annotated_code.h Outdated Show resolved Hide resolved
test/unit/test_annotated_code.c Outdated Show resolved Hide resolved
test/unit/test_annotated_code.c Outdated Show resolved Hide resolved
test/unit/test_annotated_code.c Outdated Show resolved Hide resolved
test/unit/test_annotated_code.c Outdated Show resolved Hide resolved
test/unit/test_annotated_code.c Outdated Show resolved Hide resolved
test/unit/test_annotated_code.c Outdated Show resolved Hide resolved
test/unit/test_annotated_code.c Outdated Show resolved Hide resolved
@thestr4ng3r
Copy link
Contributor

thestr4ng3r commented May 23, 2020

Sorry for the review comments getting a bit messy, some of them were written before you re-pushed. I marked the now outdated ones as resolved so you can ignore them.

@NirmalManoj
Copy link
Contributor Author

NirmalManoj commented May 23, 2020

Good work. Will go over it soon,
but as we discussed - use standard functiondocumentation style

https://www.kernel.org/doc/html/v4.16/doc-guide/kernel-doc.html#how-to-format-kernel-doc-comments

I have made better documentation following the format in this link in the new commit. Please check.

libr/core/Makefile Outdated Show resolved Hide resolved
libr/include/r_util/r_annotated_code.h Outdated Show resolved Hide resolved
libr/include/r_util/r_annotated_code.h Show resolved Hide resolved
libr/include/r_util/r_annotated_code.h Outdated Show resolved Hide resolved
libr/include/r_util/r_annotated_code.h Outdated Show resolved Hide resolved
libr/include/r_util/r_annotated_code.h Show resolved Hide resolved
@XVilka
Copy link
Contributor

XVilka commented May 25, 2020

Please rebase on top of the latest master, since the BSD builds fail with the following error since radareorg/radare2-testbins#21 was merged.

[XX] db/cmd/cmd_print prc text color
R2_NOPLUGINS=1 radare2 -escr.utf8=0 -escr.color=0 -escr.interactive=0 -N -Qc '. bins/other/palette.r2
.. bins/src/prc_256.py
e scr.color=3
prc 256
' -
-- stdout
--- .a	Sat May 23 20:12:06 2020
+++ .b	Sat May 23 20:12:06 2020
@@ -1,16 +1,16 @@
-0x00000000 000102030405060708090a0b0c0d0e0f
-0x00000010 101112131415161718191a1b1c1d1e1f
-0x00000020 202122232425262728292a2b2c2d2e2f
-0x00000030 303132333435363738393a3b3c3d3e3f
-0x00000040 404142434445464748494a4b4c4d4e4f
-0x00000050 505152535455565758595a5b5c5d5e5f
-0x00000060 606162636465666768696a6b6c6d6e6f
-0x00000070 707172737475767778797a7b7c7d7e7f
-0x00000080 808182838485868788898a8b8c8d8e8f
-0x00000090 909192939495969798999a9b9c9d9e9f
-0x000000a0 a0a1a2a3a4a5a6a7a8a9aaabacadaeaf
-0x000000b0 b0b1b2b3b4b5b6b7b8b9babbbcbdbebf
-0x000000c0 c0c1c2c3c4c5c6c7c8c9cacbcccdcecf
-0x000000d0 d0d1d2d3d4d5d6d7d8d9dadbdcdddedf
-0x000000e0 e0e1e2e3e4e5e6e7e8e9eaebecedeeef
-0x000000f0 f0f1f2f3f4f5f6f7f8f9fafbfcfdfeff
+0x00000000                                 
+0x00000010                                 
+0x00000020                                 
+0x00000030                                 
+0x00000040                                 
+0x00000050                                 
+0x00000060                                 
+0x00000070                                 
+0x00000080                                 
+0x00000090                                 
+0x000000a0                                 
+0x000000b0                                 
+0x000000c0                                 
+0x000000d0                                 
+0x000000e0                                 
+0x000000f0                                 


-- stderr
Cannot find script 'bins/other/palette.r2'
python: can't open file 'bins/src/prc_256.py': [Errno 2] No such file or directory
Cannot open ttyname(0) (null)

@XVilka XVilka self-requested a review May 25, 2020
XVilka
XVilka approved these changes May 25, 2020
@XVilka XVilka requested review from thestr4ng3r and ret2libc May 25, 2020
Copy link
Contributor

@ITAYC0HEN ITAYC0HEN left a comment

Looks very good! The documentation is very impressive - keep on this great work

Left some comments for grammar and typos stuff

libr/include/r_core.h Outdated Show resolved Hide resolved
libr/include/r_util/r_annotated_code.h Outdated Show resolved Hide resolved
libr/include/r_util/r_annotated_code.h Outdated Show resolved Hide resolved
libr/include/r_core.h Outdated Show resolved Hide resolved
libr/include/r_core.h Outdated Show resolved Hide resolved
libr/include/r_util/r_annotated_code.h Outdated Show resolved Hide resolved
libr/include/r_util/r_annotated_code.h Outdated Show resolved Hide resolved
test/unit/test_annotated_code.c Outdated Show resolved Hide resolved
test/unit/test_annotated_code.c Show resolved Hide resolved
test/unit/test_annotated_code.c Show resolved Hide resolved
test/unit/test_annotated_code.c Show resolved Hide resolved
test/unit/test_annotated_code.c Show resolved Hide resolved
test/unit/test_annotated_code.c Show resolved Hide resolved
@thestr4ng3r
Copy link
Contributor

thestr4ng3r commented May 25, 2020

Here is a tip for finding these leaks: If you run valgrind --leak-check=full ./test_annotated_code, it will show you exactly where something is leaked.
The idea is that the unit tests should be 100% leak-free, so leaks in the actual code can be easily detected, i.e. unit tests should succeed the above command with something like this:

==59525== LEAK SUMMARY:
==59525==    definitely lost: 0 bytes in 0 blocks
==59525==    indirectly lost: 0 bytes in 0 blocks

@thestr4ng3r
Copy link
Contributor

thestr4ng3r commented May 25, 2020

Btw I forgot to mention that you also have to register the unit test file here:
https://github.com/radareorg/radare2/blob/master/test/unit/meson.build#L12
Just add annotated_code in the list (ordered alphabetically).

@NirmalManoj NirmalManoj requested a review from thestr4ng3r May 25, 2020
@XVilka XVilka added this to the 4.5.0 - Organized Chaos milestone May 26, 2020
@NirmalManoj
Copy link
Contributor Author

NirmalManoj commented May 26, 2020

@thestr4ng3r Thanks for the insightful review comments, I learnt basics of valgrind from this.

@NirmalManoj
Copy link
Contributor Author

NirmalManoj commented May 26, 2020

@thestr4ng3r I think this PR can be merged as you said yesterday. Or are you waiting for the approval of @ret2libc?

@XVilka XVilka merged commit 305cc00 into radareorg:master May 27, 2020
12 checks passed
Emi1305 pushed a commit to Emi1305/radare2 that referenced this pull request Jul 12, 2020
* Added comments for functions in RAnnotatedCode
* Modified code to follow coding style
* Added more documentation and changed the name of core_annotated_code.c
* Fixed memory leaks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API New API requests, changes, removal r2r Regression tests
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants