Add /<type>/ annotations everwhere #2986

wingdeans · 2022-08-28T16:11:23Z

Addresses #2981

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 documented or updated the documentation of every function and struct this PR changes. If not so I've explained why.
I've added tests that prove my fix is effective or that my feature works (if possible)
I've updated the rizin book with the relevant information (if needed)

Detailed description

This commit adds /*<type>*/ annotations to almost all RzList, RzVector, RzPVector, and RzGraph occurrences in function declarations. The missing annotations were detected using a libclang python script and the types were then manually added.

There are a few non-comment changes which shouldn't affect any functionality. Let me know if I should revert them.

removed unused intern_table arguments in pyc_dis.c, pyc_dis.h, asm_pyc.c
removed unused classes argument from place_nodes in agraph.c
removed unused recurse and recurse_bb functions in canalysis.c
removed unused vars field from RzPrint struct
removed unused RzAnalysisType* structs from rz_analysis.h
removed unused list field from RzEgg struct
fixed bug in bp_plugin.c where duplication checking logic iterates over the wrong list
removed unused q_regs field from RzDebug struct
removed unused backtrace field from RzDebugPlugin struct
removed unused classes_list field from RzBinNXOObj struct
removed unused methods_list and classes_list fields from RzBinZimgObj struct

TODO

extend checks to struct members

Test plan

The annotations are currently only for developer ergonomics and are not verified in any way. rz-bindgen utilizes them, but only from a limited subset of the headers, so there is no way to verify their correctness.

wingdeans · 2022-08-30T00:09:33Z

Not sure what the RzDebugPlugin fields tids and kill_list are supposed to return, but they actually have usages (returning null) unlike the other things I removed.

Do these function pointers have a specified return type, or should I or remove them (or annotate with void*)?

XVilka · 2022-08-31T00:07:59Z

@wingdeans annotate with void * for now.

wingdeans · 2022-09-05T16:44:22Z

Only errors are due to inconsistencies in rz-hexagon (PR here)

Missing type comment at <rizin/librz/asm/arch/hexagon/hexagon.h:151:10> (token is not a comment)
Mismatched function annotation for hex_const_ext_free at <rizin/librz/asm/arch/hexagon/hexagon_arch.h:69:13> : None (was RZ_NULLABLE at <rizin/librz/asm/arch/hexagon/hexagon_arch.c:226:13>
Missing type comment at <rizin/librz/asm/arch/hexagon/hexagon_arch.c:787:59> (token is not a comment)

XVilka · 2022-09-07T13:35:34Z

@wingdeans what is the status of this PR? Could you please update and rebase it on top of the latest dev?

wingdeans · 2022-09-07T14:35:18Z

Only errors are due to inconsistencies in rz-hexagon (PR here)

Missing type comment at <rizin/librz/asm/arch/hexagon/hexagon.h:151:10> (token is not a comment)
Mismatched function annotation for hex_const_ext_free at <rizin/librz/asm/arch/hexagon/hexagon_arch.h:69:13> : None (was RZ_NULLABLE at <rizin/librz/asm/arch/hexagon/hexagon_arch.c:226:13>
Missing type comment at <rizin/librz/asm/arch/hexagon/hexagon_arch.c:787:59> (token is not a comment)

The linter is ready, it just needs rizinorg/rz-hexagon#78 in rz-hexagon to address the emitted warnings.

wingdeans · 2022-09-07T14:37:56Z

* removed unused `intern_table` arguments in `pyc_dis.c`, `pyc_dis.h`, `asm_pyc.c`
* removed unused `classes` argument from `place_nodes` in `agraph.c`
* removed unused `recurse` and `recurse_bb` functions in `canalysis.c`
* removed unused `vars` field from `RzPrint` struct
* removed unused `RzAnalysisType*` structs from `rz_analysis.h`
* removed unused `list` field from `RzEgg` struct
* fixed bug in `bp_plugin.c` where duplication checking logic iterates over the wrong list
* removed unused `q_regs` field from `RzDebug` struct
* removed unused `backtrace` field from `RzDebugPlugin` struct
* removed unused `classes_list` field from `RzBinNXOObj` struct
* removed unused `methods_list` and `classes_list` fields from `RzBinZimgObj` struct

There are a lot of non-comment changes, for generic types I couldn't infer the specializations for since they weren't being used.

wargio · 2022-09-07T14:38:54Z

RzBinNXOObj

why you are exporting plugin headers? the only headers that should be exported are those in the rzlib/include folder

XVilka · 2022-09-07T14:39:24Z

@wingdeans yes, step by step. Having the linter opens the potential to improve the inference and detection.

wingdeans · 2022-09-07T14:46:58Z

RzBinNXOObj

why you are exporting plugin headers? the only headers that should be exported are those in the rzlib/include folder

The goal is to add type annotations everywhere and make sure that they are consistent between definition and declaration, with the linter checking this. I can take them out if it's too much but I think it's useful for development (and maybe adding actual typechecking in the future).

wargio

Nice cleanup

XVilka · 2022-09-11T02:23:40Z

@wingdeans LGTM, but please rebase and fix the conflicts.

Adds /*<type>*/ comments and a linter check from rz-bindgen to enforce their existence and consistency Also includes the following fixes made when adding the annotations: * removed unused intern_table arguments in pyc_dis.c, pyc_dis.h, asm_pyc.c * removed unused classes argument from place_nodes in agraph.c * removed unused recurse and recurse_bb functions in canalysis.c * removed unused vars field from RzPrint struct * removed unused RzAnalysisType* structs from rz_analysis.h * removed unused list field from RzEgg struct * fixed bug in bp_plugin.c where duplication checking logic iterates over the wrong list * removed unused q_regs field from RzDebug struct * removed unused backtrace field from RzDebugPlugin struct * removed unused classes_list field from RzBinNXOObj struct * removed unused methods_list and classes_list fields from RzBinZimgObj struct

XVilka

Lets merge this and fix the hexagon issue separately.

XVilka · 2022-09-11T05:05:41Z

I merged it as is then and created a separate issue to fix those warnings: #3020
Please address it as soon as possible. Thank you!

github-actions bot added API ARM DEX dietline DWARF ELF GDB java Mach-O MIPS PDB PE PPC rizin rz-diff rz-hash rz-test RzAnalysis RzAsm-Disassembler RzBin RzBreakpoint RzCons RzCore RzDebug RzDiff RzIO RzParse RzReg RzSearch RzSyscall labels Aug 28, 2022

wingdeans force-pushed the header-types branch from decc642 to 11ee5d3 Compare August 29, 2022 23:36

wingdeans mentioned this pull request Aug 30, 2022

Add /*<type>*/ comments and fix annotation inconsistencies rizinorg/rz-hexagon#78

Merged

wingdeans force-pushed the header-types branch from 11ee5d3 to b14c88b Compare August 30, 2022 00:23

wingdeans force-pushed the header-types branch from b14c88b to 385a78c Compare August 31, 2022 12:53

XVilka added this to the 0.5.0 milestone Sep 4, 2022

wingdeans force-pushed the header-types branch from 385a78c to 35af7d5 Compare September 5, 2022 14:59

wingdeans force-pushed the header-types branch from 35af7d5 to 20ac7f9 Compare September 7, 2022 13:46

wingdeans marked this pull request as ready for review September 7, 2022 14:31

wingdeans requested review from yossizap, ret2libc, thestr4ng3r, wargio and kazarmy as code owners September 7, 2022 14:31

wargio approved these changes Sep 8, 2022

View reviewed changes

wargio requested a review from XVilka September 8, 2022 20:10

wingdeans force-pushed the header-types branch from 20ac7f9 to 237f5a8 Compare September 11, 2022 03:46

XVilka approved these changes Sep 11, 2022

View reviewed changes

XVilka merged commit 59b38e6 into rizinorg:dev Sep 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add /<type>/ annotations everwhere #2986

Add /<type>/ annotations everwhere #2986

wingdeans commented Aug 28, 2022 •

edited

wingdeans commented Aug 30, 2022 •

edited

XVilka commented Aug 31, 2022

wingdeans commented Sep 5, 2022 •

edited

XVilka commented Sep 7, 2022

wingdeans commented Sep 7, 2022

wingdeans commented Sep 7, 2022

wargio commented Sep 7, 2022 •

edited

XVilka commented Sep 7, 2022

wingdeans commented Sep 7, 2022

wargio left a comment

XVilka commented Sep 11, 2022

XVilka left a comment

XVilka commented Sep 11, 2022

Add /*<type>*/ annotations everwhere #2986

Add /*<type>*/ annotations everwhere #2986

Conversation

wingdeans commented Aug 28, 2022 • edited

wingdeans commented Aug 30, 2022 • edited

XVilka commented Aug 31, 2022

wingdeans commented Sep 5, 2022 • edited

XVilka commented Sep 7, 2022

wingdeans commented Sep 7, 2022

wingdeans commented Sep 7, 2022

wargio commented Sep 7, 2022 • edited

XVilka commented Sep 7, 2022

wingdeans commented Sep 7, 2022

wargio left a comment

Choose a reason for hiding this comment

XVilka commented Sep 11, 2022

XVilka left a comment

Choose a reason for hiding this comment

XVilka commented Sep 11, 2022

Add /<type>/ annotations everwhere #2986

Add /<type>/ annotations everwhere #2986

wingdeans commented Aug 28, 2022 •

edited

wingdeans commented Aug 30, 2022 •

edited

wingdeans commented Sep 5, 2022 •

edited

wargio commented Sep 7, 2022 •

edited