[PATCH 0/4 for 9.0] Clean-up disassembler handling for plugins

[PATCH 0/4 for 9.0] Clean-up disassembler handling for plugins

[Alex Bennée]

I was overly optimistic with my last RFC that HPPA was the only
affected architecture by this issue. I've introduced a new flag in
disassemble_info and fixed up those arches that include raw bytes by
default.

It would be nice to get this in for 9.0

Alex.

Alex Bennée (4):
  disas: introduce no_raw_bytes
  disas/hppa: honour no_raw_bytes
  target/loongarch: honour no_raw_bytes when disassembling
  target/riscv: honour no_raw_bytes when disassembling

 include/disas/dis-asm.h  |  7 +++++++
 disas/disas.c            |  1 +
 disas/hppa.c             |  8 +++++---
 disas/riscv.c            | 28 +++++++++++++++-------------
 target/loongarch/disas.c | 13 +++++++++----
 5 files changed, 37 insertions(+), 20 deletions(-)

-- 
2.39.2

[PATCH 1/4] disas: introduce no_raw_bytes

[Alex Bennée]

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

[Helge Deller]

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

[Richard Henderson]

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~

[PATCH 2/4] disas/hppa: honour no_raw_bytes

[Alex Bennée]

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 disas/hppa.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/disas/hppa.c b/disas/hppa.c
index 22dce9b41bb..17e4f1ccdc6 100644
--- a/disas/hppa.c
+++ b/disas/hppa.c
@@ -1972,9 +1972,11 @@ print_insn_hppa (bfd_vma memaddr, disassemble_info *info)
 
   insn = bfd_getb32 (buffer);
 
-  info->fprintf_func(info->stream, " %02x %02x %02x %02x   ",
-                (insn >> 24) & 0xff, (insn >> 16) & 0xff,
-                (insn >>  8) & 0xff, insn & 0xff);
+  if (!info->no_raw_bytes) {
+      info->fprintf_func(info->stream, " %02x %02x %02x %02x   ",
+                         (insn >> 24) & 0xff, (insn >> 16) & 0xff,
+                         (insn >>  8) & 0xff, insn & 0xff);
+  }
 
   for (i = 0; i < NUMOPCODES; ++i)
     {
-- 
2.39.2

[PATCH 3/4] target/loongarch: honour no_raw_bytes when disassembling

[Alex Bennée]

This makes the output suitable when used for plugins.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/loongarch/disas.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/target/loongarch/disas.c b/target/loongarch/disas.c
index 2040f3e44db..b9118878d97 100644
--- a/target/loongarch/disas.c
+++ b/target/loongarch/disas.c
@@ -120,10 +120,15 @@ static const char *get_csr_name(unsigned num)
            csr_names[num] : "Undefined CSR";
 }
 
-#define output(C, INSN, FMT, ...)                                   \
-{                                                                   \
-    (C)->info->fprintf_func((C)->info->stream, "%08x   %-9s\t" FMT, \
-                            (C)->insn, INSN, ##__VA_ARGS__);        \
+#define output(C, INSN, FMT, ...)                                      \
+ {                                                                     \
+    if ((C)->info->no_raw_bytes) {                                     \
+        (C)->info->fprintf_func((C)->info->stream, "%-9s\t" FMT,       \
+                            INSN, ##__VA_ARGS__);                      \
+    } else {                                                           \
+        (C)->info->fprintf_func((C)->info->stream, "%08x   %-9s\t" FMT,\
+                            (C)->insn, INSN, ##__VA_ARGS__);           \
+    }                                                                  \
 }
 
 #include "decode-insns.c.inc"
-- 
2.39.2

[PATCH 4/4] target/riscv: honour no_raw_bytes when disassembling

[Alex Bennée]

This makes the output suitable when used for plugins.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 disas/riscv.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/disas/riscv.c b/disas/riscv.c
index 8a546d5ea53..86028efea85 100644
--- a/disas/riscv.c
+++ b/disas/riscv.c
@@ -5192,19 +5192,21 @@ print_insn_riscv(bfd_vma memaddr, struct disassemble_info *info, rv_isa isa)
         }
     }
 
-    switch (len) {
-    case 2:
-        (*info->fprintf_func)(info->stream, INST_FMT_2, inst);
-        break;
-    case 4:
-        (*info->fprintf_func)(info->stream, INST_FMT_4, inst);
-        break;
-    case 6:
-        (*info->fprintf_func)(info->stream, INST_FMT_6, inst);
-        break;
-    default:
-        (*info->fprintf_func)(info->stream, INST_FMT_8, inst);
-        break;
+    if (!info->no_raw_bytes) {
+        switch (len) {
+        case 2:
+            (*info->fprintf_func)(info->stream, INST_FMT_2, inst);
+            break;
+        case 4:
+            (*info->fprintf_func)(info->stream, INST_FMT_4, inst);
+            break;
+        case 6:
+            (*info->fprintf_func)(info->stream, INST_FMT_6, inst);
+            break;
+        default:
+            (*info->fprintf_func)(info->stream, INST_FMT_8, inst);
+            break;
+        }
     }
 
     disasm_inst(buf, sizeof(buf), isa, memaddr, inst,
-- 
2.39.2

Re: [PATCH 4/4] target/riscv: honour no_raw_bytes when disassembling

[Alistair Francis]

On Tue, Mar 5, 2024 at 5:15 AM Alex Bennée <alex.bennee@linaro.org> wrote:
>
> This makes the output suitable when used for plugins.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Acked-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  disas/riscv.c | 28 +++++++++++++++-------------
>  1 file changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/disas/riscv.c b/disas/riscv.c
> index 8a546d5ea53..86028efea85 100644
> --- a/disas/riscv.c
> +++ b/disas/riscv.c
> @@ -5192,19 +5192,21 @@ print_insn_riscv(bfd_vma memaddr, struct disassemble_info *info, rv_isa isa)
>          }
>      }
>
> -    switch (len) {
> -    case 2:
> -        (*info->fprintf_func)(info->stream, INST_FMT_2, inst);
> -        break;
> -    case 4:
> -        (*info->fprintf_func)(info->stream, INST_FMT_4, inst);
> -        break;
> -    case 6:
> -        (*info->fprintf_func)(info->stream, INST_FMT_6, inst);
> -        break;
> -    default:
> -        (*info->fprintf_func)(info->stream, INST_FMT_8, inst);
> -        break;
> +    if (!info->no_raw_bytes) {
> +        switch (len) {
> +        case 2:
> +            (*info->fprintf_func)(info->stream, INST_FMT_2, inst);
> +            break;
> +        case 4:
> +            (*info->fprintf_func)(info->stream, INST_FMT_4, inst);
> +            break;
> +        case 6:
> +            (*info->fprintf_func)(info->stream, INST_FMT_6, inst);
> +            break;
> +        default:
> +            (*info->fprintf_func)(info->stream, INST_FMT_8, inst);
> +            break;
> +        }
>      }
>
>      disasm_inst(buf, sizeof(buf), isa, memaddr, inst,
> --
> 2.39.2
>
>