[PATCH 1/4] disas: introduce no_raw_bytes

Alex Bennée posted 4 patches 8 months, 3 weeks ago
Maintainers: Richard Henderson <richard.henderson@linaro.org>, Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <Alistair.Francis@wdc.com>, Song Gao <gaosong@loongson.cn>
[PATCH 1/4] disas: introduce no_raw_bytes
Posted by Alex Bennée 8 months, 3 weeks ago
For plugins we don't expect the raw bytes in the disassembly. We
already deal with this by hand crafting our capstone call but for
other diassemblers we need a flag.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/disas/dis-asm.h | 7 +++++++
 disas/disas.c           | 1 +
 2 files changed, 8 insertions(+)

diff --git a/include/disas/dis-asm.h b/include/disas/dis-asm.h
index 2324f6b1a46..5c32e7a310c 100644
--- a/include/disas/dis-asm.h
+++ b/include/disas/dis-asm.h
@@ -396,6 +396,13 @@ typedef struct disassemble_info {
   /* Command line options specific to the target disassembler.  */
   char * disassembler_options;
 
+  /*
+   * When true instruct the disassembler to not preface opcodes with
+   * raw bytes. This is mainly for the benefit of the plugin
+   * interface.
+   */
+  bool no_raw_bytes;
+
   /* Field intended to be used by targets in any way they deem suitable.  */
   void *target_info;
 
diff --git a/disas/disas.c b/disas/disas.c
index 0d2d06c2ecc..feb5bc4b665 100644
--- a/disas/disas.c
+++ b/disas/disas.c
@@ -273,6 +273,7 @@ char *plugin_disas(CPUState *cpu, uint64_t addr, size_t size)
     s.info.buffer_vma = addr;
     s.info.buffer_length = size;
     s.info.print_address_func = plugin_print_address;
+    s.info.no_raw_bytes = true;
 
     if (s.info.cap_arch >= 0 && cap_disas_plugin(&s.info, addr, size)) {
         ; /* done */
-- 
2.39.2


Re: [PATCH 1/4] disas: introduce no_raw_bytes
Posted by Helge Deller 8 months, 3 weeks ago
On 3/4/24 20:13, Alex Bennée wrote:
> For plugins we don't expect the raw bytes in the disassembly. We
> already deal with this by hand crafting our capstone call but for
> other diassemblers we need a flag.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   include/disas/dis-asm.h | 7 +++++++
>   disas/disas.c           | 1 +
>   2 files changed, 8 insertions(+)
>
> diff --git a/include/disas/dis-asm.h b/include/disas/dis-asm.h
> index 2324f6b1a46..5c32e7a310c 100644
> --- a/include/disas/dis-asm.h
> +++ b/include/disas/dis-asm.h
> @@ -396,6 +396,13 @@ typedef struct disassemble_info {
>     /* Command line options specific to the target disassembler.  */
>     char * disassembler_options;
>
> +  /*
> +   * When true instruct the disassembler to not preface opcodes with
> +   * raw bytes. This is mainly for the benefit of the plugin
> +   * interface.
> +   */
> +  bool no_raw_bytes;

Patch in general and idea is OK, but I don't like
the "no_raw_bytes" naming very much.
In patch #2 you use "if (!info->no_raw_bytes) {.."
which is double-negation.

"hide_raw_bytes" is better but still double negation.

Maybe something like "show_opcodes" which defaults to "false"
when used with plugins is better?

Helge

> +
>     /* Field intended to be used by targets in any way they deem suitable.  */
>     void *target_info;
>
> diff --git a/disas/disas.c b/disas/disas.c
> index 0d2d06c2ecc..feb5bc4b665 100644
> --- a/disas/disas.c
> +++ b/disas/disas.c
> @@ -273,6 +273,7 @@ char *plugin_disas(CPUState *cpu, uint64_t addr, size_t size)
>       s.info.buffer_vma = addr;
>       s.info.buffer_length = size;
>       s.info.print_address_func = plugin_print_address;
> +    s.info.no_raw_bytes = true;
>
>       if (s.info.cap_arch >= 0 && cap_disas_plugin(&s.info, addr, size)) {
>           ; /* done */
Re: [PATCH 1/4] disas: introduce no_raw_bytes
Posted by Richard Henderson 8 months, 3 weeks ago
On 3/4/24 09:26, Helge Deller wrote:
> On 3/4/24 20:13, Alex Bennée wrote:
>> For plugins we don't expect the raw bytes in the disassembly. We
>> already deal with this by hand crafting our capstone call but for
>> other diassemblers we need a flag.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>   include/disas/dis-asm.h | 7 +++++++
>>   disas/disas.c           | 1 +
>>   2 files changed, 8 insertions(+)
>>
>> diff --git a/include/disas/dis-asm.h b/include/disas/dis-asm.h
>> index 2324f6b1a46..5c32e7a310c 100644
>> --- a/include/disas/dis-asm.h
>> +++ b/include/disas/dis-asm.h
>> @@ -396,6 +396,13 @@ typedef struct disassemble_info {
>>     /* Command line options specific to the target disassembler.  */
>>     char * disassembler_options;
>>
>> +  /*
>> +   * When true instruct the disassembler to not preface opcodes with
>> +   * raw bytes. This is mainly for the benefit of the plugin
>> +   * interface.
>> +   */
>> +  bool no_raw_bytes;
> 
> Patch in general and idea is OK, but I don't like
> the "no_raw_bytes" naming very much.
> In patch #2 you use "if (!info->no_raw_bytes) {.."
> which is double-negation.
> 
> "hide_raw_bytes" is better but still double negation.
> 
> Maybe something like "show_opcodes" which defaults to "false"
> when used with plugins is better?

Agreed.


r~