JSON output is inconsistent between `p` subcommands about radare2 HOT 4 OPEN

Can you make a pr doing the changes to make both commands return the same structure? I agree on the change but will be easier for you to do it because you know whats different. Ill review it and see if that change affects any plugin or tool later and update the tests

thanks!

from radare2.

This issue is either going to be a hack or a bit of a rewrite. I worked on it a bit yesterday.

The current behavior is that 'pdj' command calls r_core_print_disasm_json, and the 'pduoj' command calls r_core_print_disasm. The two functions handle json very differently. r_core_print_disasm_json fills out the pj structure fully using keywords for each part of the assembly instruction, r_core_print_disasm just grabs the console line and puts it in pj as a 'text' element.

I was changing disasm.c and cmd_print.inc.c so that all of the 'j' subcommands call r_core_print_disasm_json instead of r_core_print_disasm and removed the json arg to r_core_print_disasm.

This seemed to be working until I found that r_core_print_disasm_json doesn't use the ds_ functionality... it's calling r_asm_op_ functions directly rather than the ds_ wrapper functions. ds_disassemble has a much more advanced way to handle disassembled instruction edge cases that r_core_print_disasm_json doesn't have and it seems silly to duplicate it all.

So I see 2 paths forward:

1. Update r_core_print_disasm_json so that it uses the ds_ functions.
2. Go back to passing in json into r_core_print_disasm and add the full pj_ functionality to build the full json structure that you have in r_core_print_disasm_json

Make sense?

Do you have a preference for a path forward? I think option 2 is a little more elegant imho. Along with a separate function for adding a new instruction to pj to make it a bit cleaner.

Man, this project is huge. Kudos to you for keeping it up solo for so long. Could use some tech debt cleanup though.

from radare2.

One restriction is that we can't change public structure or function signatures, so having this in mind, both approaches look good, as one solves the problem quickier, so it may work for you sooner, but a proper rewrite/refactoring/cleanup is always welcome, everthing in r2land is prompt to be changed for the good.

So my vote goes for the 2nd approach.

If you need to break APIs you can use the R2_USE_NEW_ABI ifdef, and this code will be swapped before r2-6.0.0 is out (during the 5.9.9 development time)

from radare2.

Ping

from radare2.