[RFC PATCH 0/2] Make the core disassembler functions target-independent

[RFC PATCH 0/2] Make the core disassembler functions target-independent

[Thomas Huth]

Move disas.c into the target-independent source set, so that we
only have to compile this code once instead multiple times (one
time for each target).

Marked as RFC since we have to replace the target_ulongs here
with hwaddr, and the TARGET_FMT_lx with HWADDR_FMT_plx, which is
a little bit ugly ... what's your opinion?

Thomas Huth (2):
  disas: Move softmmu specific code to separate file
  disas: Move disas.c into the target-independent source set

 meson.build            |  4 +-
 disas/disas-internal.h | 20 +++++++++
 include/disas/disas.h  | 21 +++------
 include/hw/elf_ops.h   |  2 +-
 bsd-user/elfload.c     |  2 +-
 disas.c                | 98 ++++++++----------------------------------
 disas/disas-mon.c      | 65 ++++++++++++++++++++++++++++
 linux-user/elfload.c   |  2 +-
 disas/meson.build      |  2 +
 9 files changed, 116 insertions(+), 100 deletions(-)
 create mode 100644 disas/disas-internal.h
 create mode 100644 disas/disas-mon.c

-- 
2.31.1

[RFC PATCH 1/2] disas: Move softmmu specific code to separate file

[Thomas Huth]

We'd like to move disas.c into the common code source set, where
CONFIG_USER_ONLY is not available anymore. So we have to move
the related code into a separate file instead.

While doing the movement, change the type of the "pc" parameter of
the monitor_disas() function to "hwaddr" instead of "target_ulong",
so we can add the file to the generic softmmu_ss source set instead
of the specific_ss source set.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 disas/disas-internal.h | 20 +++++++++++
 include/disas/disas.h  |  6 ++--
 disas.c                | 76 ++++--------------------------------------
 disas/disas-mon.c      | 65 ++++++++++++++++++++++++++++++++++++
 disas/meson.build      |  2 ++
 5 files changed, 96 insertions(+), 73 deletions(-)
 create mode 100644 disas/disas-internal.h
 create mode 100644 disas/disas-mon.c

diff --git a/disas/disas-internal.h b/disas/disas-internal.h
new file mode 100644
index 0000000000..354c01307b
--- /dev/null
+++ b/disas/disas-internal.h
@@ -0,0 +1,20 @@
+/*
+ * Definitions used internally in the disassembly code
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef DISAS_INTERNAL_H
+#define DISAS_INTERNAL_H
+
+#include "disas/dis-asm.h"
+
+typedef struct CPUDebug {
+    struct disassemble_info info;
+    CPUState *cpu;
+} CPUDebug;
+
+void disas_initialize_debug_target(CPUDebug *s, CPUState *cpu);
+int disas_gstring_printf(FILE *stream, const char *fmt, ...) G_GNUC_PRINTF(2, 3);
+
+#endif
diff --git a/include/disas/disas.h b/include/disas/disas.h
index d363e95ede..6c04428620 100644
--- a/include/disas/disas.h
+++ b/include/disas/disas.h
@@ -11,15 +11,15 @@ void disas(FILE *out, const void *code, unsigned long size);
 void target_disas(FILE *out, CPUState *cpu, target_ulong code,
                   target_ulong size);
 
-void monitor_disas(Monitor *mon, CPUState *cpu,
-                   target_ulong pc, int nb_insn, int is_physical);
-
 char *plugin_disas(CPUState *cpu, uint64_t addr, size_t size);
 
 /* Look up symbol for debugging purpose.  Returns "" if unknown. */
 const char *lookup_symbol(target_ulong orig_addr);
 #endif
 
+void monitor_disas(Monitor *mon, CPUState *cpu,
+                   hwaddr pc, int nb_insn, int is_physical);
+
 struct syminfo;
 struct elf32_sym;
 struct elf64_sym;
diff --git a/disas.c b/disas.c
index b087c12c47..4f4a07d611 100644
--- a/disas.c
+++ b/disas.c
@@ -1,17 +1,12 @@
 /* General "disassemble this chunk" code.  Used for debugging. */
 #include "qemu/osdep.h"
-#include "disas/dis-asm.h"
+#include "disas/disas-internal.h"
 #include "elf.h"
 #include "qemu/qemu-print.h"
 
 #include "disas/disas.h"
 #include "disas/capstone.h"
 
-typedef struct CPUDebug {
-    struct disassemble_info info;
-    CPUState *cpu;
-} CPUDebug;
-
 /* Filled in by elfload.c.  Simplistic, but will do for now. */
 struct syminfo *syminfos = NULL;
 
@@ -119,7 +114,7 @@ static void initialize_debug(CPUDebug *s)
     s->info.symbol_at_address_func = symbol_at_address;
 }
 
-static void initialize_debug_target(CPUDebug *s, CPUState *cpu)
+void disas_initialize_debug_target(CPUDebug *s, CPUState *cpu)
 {
     initialize_debug(s);
 
@@ -211,7 +206,7 @@ void target_disas(FILE *out, CPUState *cpu, target_ulong code,
     int count;
     CPUDebug s;
 
-    initialize_debug_target(&s, cpu);
+    disas_initialize_debug_target(&s, cpu);
     s.info.fprintf_func = fprintf;
     s.info.stream = out;
     s.info.buffer_vma = code;
@@ -241,8 +236,7 @@ void target_disas(FILE *out, CPUState *cpu, target_ulong code,
     }
 }
 
-static int G_GNUC_PRINTF(2, 3)
-gstring_printf(FILE *stream, const char *fmt, ...)
+int disas_gstring_printf(FILE *stream, const char *fmt, ...)
 {
     /* We abuse the FILE parameter to pass a GString. */
     GString *s = (GString *)stream;
@@ -272,8 +266,8 @@ char *plugin_disas(CPUState *cpu, uint64_t addr, size_t size)
     CPUDebug s;
     GString *ds = g_string_new(NULL);
 
-    initialize_debug_target(&s, cpu);
-    s.info.fprintf_func = gstring_printf;
+    disas_initialize_debug_target(&s, cpu);
+    s.info.fprintf_func = disas_gstring_printf;
     s.info.stream = (FILE *)ds;  /* abuse this slot */
     s.info.buffer_vma = addr;
     s.info.buffer_length = size;
@@ -338,61 +332,3 @@ const char *lookup_symbol(target_ulong orig_addr)
 
     return symbol;
 }
-
-#if !defined(CONFIG_USER_ONLY)
-
-#include "monitor/monitor.h"
-
-static int
-physical_read_memory(bfd_vma memaddr, bfd_byte *myaddr, int length,
-                     struct disassemble_info *info)
-{
-    CPUDebug *s = container_of(info, CPUDebug, info);
-    MemTxResult res;
-
-    res = address_space_read(s->cpu->as, memaddr, MEMTXATTRS_UNSPECIFIED,
-                             myaddr, length);
-    return res == MEMTX_OK ? 0 : EIO;
-}
-
-/* Disassembler for the monitor.  */
-void monitor_disas(Monitor *mon, CPUState *cpu,
-                   target_ulong pc, int nb_insn, int is_physical)
-{
-    int count, i;
-    CPUDebug s;
-    g_autoptr(GString) ds = g_string_new("");
-
-    initialize_debug_target(&s, cpu);
-    s.info.fprintf_func = gstring_printf;
-    s.info.stream = (FILE *)ds;  /* abuse this slot */
-
-    if (is_physical) {
-        s.info.read_memory_func = physical_read_memory;
-    }
-    s.info.buffer_vma = pc;
-
-    if (s.info.cap_arch >= 0 && cap_disas_monitor(&s.info, pc, nb_insn)) {
-        monitor_puts(mon, ds->str);
-        return;
-    }
-
-    if (!s.info.print_insn) {
-        monitor_printf(mon, "0x" TARGET_FMT_lx
-                       ": Asm output not supported on this arch\n", pc);
-        return;
-    }
-
-    for (i = 0; i < nb_insn; i++) {
-        g_string_append_printf(ds, "0x" TARGET_FMT_lx ":  ", pc);
-        count = s.info.print_insn(pc, &s.info);
-        g_string_append_c(ds, '\n');
-        if (count < 0) {
-            break;
-        }
-        pc += count;
-    }
-
-    monitor_puts(mon, ds->str);
-}
-#endif
diff --git a/disas/disas-mon.c b/disas/disas-mon.c
new file mode 100644
index 0000000000..31b7413c40
--- /dev/null
+++ b/disas/disas-mon.c
@@ -0,0 +1,65 @@
+/*
+ * Functions related to disassembly from the monitor
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "disas-internal.h"
+#include "disas/disas.h"
+#include "exec/memory.h"
+#include "hw/core/cpu.h"
+#include "monitor/monitor.h"
+
+static int
+physical_read_memory(bfd_vma memaddr, bfd_byte *myaddr, int length,
+                     struct disassemble_info *info)
+{
+    CPUDebug *s = container_of(info, CPUDebug, info);
+    MemTxResult res;
+
+    res = address_space_read(s->cpu->as, memaddr, MEMTXATTRS_UNSPECIFIED,
+                             myaddr, length);
+    return res == MEMTX_OK ? 0 : EIO;
+}
+
+/* Disassembler for the monitor.  */
+void monitor_disas(Monitor *mon, CPUState *cpu,
+                   hwaddr pc, int nb_insn, int is_physical)
+{
+    int count, i;
+    CPUDebug s;
+    g_autoptr(GString) ds = g_string_new("");
+
+    disas_initialize_debug_target(&s, cpu);
+    s.info.fprintf_func = disas_gstring_printf;
+    s.info.stream = (FILE *)ds;  /* abuse this slot */
+
+    if (is_physical) {
+        s.info.read_memory_func = physical_read_memory;
+    }
+    s.info.buffer_vma = pc;
+
+    if (s.info.cap_arch >= 0 && cap_disas_monitor(&s.info, pc, nb_insn)) {
+        monitor_puts(mon, ds->str);
+        return;
+    }
+
+    if (!s.info.print_insn) {
+        monitor_printf(mon, "0x" HWADDR_FMT_plx
+                       ": Asm output not supported on this arch\n", pc);
+        return;
+    }
+
+    for (i = 0; i < nb_insn; i++) {
+        g_string_append_printf(ds, "0x" HWADDR_FMT_plx ":  ", pc);
+        count = s.info.print_insn(pc, &s.info);
+        g_string_append_c(ds, '\n');
+        if (count < 0) {
+            break;
+        }
+        pc += count;
+    }
+
+    monitor_puts(mon, ds->str);
+}
diff --git a/disas/meson.build b/disas/meson.build
index c865bdd882..ca6dd81ecf 100644
--- a/disas/meson.build
+++ b/disas/meson.build
@@ -11,3 +11,5 @@ common_ss.add(when: 'CONFIG_SH4_DIS', if_true: files('sh4.c'))
 common_ss.add(when: 'CONFIG_SPARC_DIS', if_true: files('sparc.c'))
 common_ss.add(when: 'CONFIG_XTENSA_DIS', if_true: files('xtensa.c'))
 common_ss.add(when: capstone, if_true: files('capstone.c'))
+
+softmmu_ss.add(files('disas-mon.c'))
-- 
2.31.1

[RFC PATCH 2/2] disas: Move disas.c into the target-independent source set

[Thomas Huth]

By using hwaddr instead of target_ulong and by tweaking some other
spots, we can turn this code into target-independent code for
compiling it only once and not multiple times during the build
process.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 meson.build           |  4 ++--
 include/disas/disas.h | 15 +++------------
 include/hw/elf_ops.h  |  2 +-
 bsd-user/elfload.c    |  2 +-
 disas.c               | 22 ++++++++++++----------
 linux-user/elfload.c  |  2 +-
 6 files changed, 20 insertions(+), 27 deletions(-)

diff --git a/meson.build b/meson.build
index 229eb585f7..73ceed037f 100644
--- a/meson.build
+++ b/meson.build
@@ -3152,8 +3152,8 @@ specific_ss.add(files('cpu.c'))
 
 subdir('softmmu')
 
-common_ss.add(capstone)
-specific_ss.add(files('disas.c'), capstone)
+common_ss.add(files('disas.c'), capstone)
+specific_ss.add(capstone)
 
 # Work around a gcc bug/misfeature wherein constant propagation looks
 # through an alias:
diff --git a/include/disas/disas.h b/include/disas/disas.h
index 6c04428620..5132ebf982 100644
--- a/include/disas/disas.h
+++ b/include/disas/disas.h
@@ -3,19 +3,14 @@
 
 #include "exec/hwaddr.h"
 
-#ifdef NEED_CPU_H
-#include "cpu.h"
-
 /* Disassemble this for me please... (debugging). */
 void disas(FILE *out, const void *code, unsigned long size);
-void target_disas(FILE *out, CPUState *cpu, target_ulong code,
-                  target_ulong size);
+void target_disas(FILE *out, CPUState *cpu, hwaddr code, long size);
 
 char *plugin_disas(CPUState *cpu, uint64_t addr, size_t size);
 
 /* Look up symbol for debugging purpose.  Returns "" if unknown. */
-const char *lookup_symbol(target_ulong orig_addr);
-#endif
+const char *lookup_symbol(hwaddr orig_addr);
 
 void monitor_disas(Monitor *mon, CPUState *cpu,
                    hwaddr pc, int nb_insn, int is_physical);
@@ -24,11 +19,7 @@ struct syminfo;
 struct elf32_sym;
 struct elf64_sym;
 
-#if defined(CONFIG_USER_ONLY)
-typedef const char *(*lookup_symbol_t)(struct syminfo *s, target_ulong orig_addr);
-#else
-typedef const char *(*lookup_symbol_t)(struct syminfo *s, hwaddr orig_addr);
-#endif
+typedef const char *(*lookup_symbol_t)(struct syminfo *s, uint64_t orig_addr);
 
 struct syminfo {
     lookup_symbol_t lookup_symbol;
diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
index dffb0e73d2..5e2af4d504 100644
--- a/include/hw/elf_ops.h
+++ b/include/hw/elf_ops.h
@@ -81,7 +81,7 @@ static int glue(symfind, SZ)(const void *s0, const void *s1)
 }
 
 static const char *glue(lookup_symbol, SZ)(struct syminfo *s,
-                                           hwaddr orig_addr)
+                                           uint64_t orig_addr)
 {
     struct elf_sym *syms = glue(s->disas_symtab.elf, SZ);
     struct elf_sym *sym;
diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c
index fbcdc94b96..7c784518ed 100644
--- a/bsd-user/elfload.c
+++ b/bsd-user/elfload.c
@@ -363,7 +363,7 @@ static int symfind(const void *s0, const void *s1)
     return result;
 }
 
-static const char *lookup_symbolxx(struct syminfo *s, target_ulong orig_addr)
+static const char *lookup_symbolxx(struct syminfo *s, uint64_t orig_addr)
 {
 #if ELF_CLASS == ELFCLASS32
     struct elf_sym *syms = s->disas_symtab.elf32;
diff --git a/disas.c b/disas.c
index 4f4a07d611..31edbd2e70 100644
--- a/disas.c
+++ b/disas.c
@@ -3,6 +3,9 @@
 #include "disas/disas-internal.h"
 #include "elf.h"
 #include "qemu/qemu-print.h"
+#include "exec/cpu-common.h"
+#include "exec/memory.h"
+#include "hw/core/cpu.h"
 
 #include "disas/disas.h"
 #include "disas/capstone.h"
@@ -121,11 +124,11 @@ void disas_initialize_debug_target(CPUDebug *s, CPUState *cpu)
     s->cpu = cpu;
     s->info.read_memory_func = target_read_memory;
     s->info.print_address_func = print_address;
-#if TARGET_BIG_ENDIAN
-    s->info.endian = BFD_ENDIAN_BIG;
-#else
-    s->info.endian = BFD_ENDIAN_LITTLE;
-#endif
+    if (target_words_bigendian()) {
+        s->info.endian = BFD_ENDIAN_BIG;
+    } else {
+        s->info.endian =  BFD_ENDIAN_LITTLE;
+    }
 
     CPUClass *cc = CPU_GET_CLASS(cpu);
     if (cc->disas_set_info) {
@@ -199,10 +202,9 @@ static void initialize_debug_host(CPUDebug *s)
 }
 
 /* Disassemble this for me please... (debugging).  */
-void target_disas(FILE *out, CPUState *cpu, target_ulong code,
-                  target_ulong size)
+void target_disas(FILE *out, CPUState *cpu, hwaddr code, long size)
 {
-    target_ulong pc;
+    hwaddr pc;
     int count;
     CPUDebug s;
 
@@ -221,7 +223,7 @@ void target_disas(FILE *out, CPUState *cpu, target_ulong code,
     }
 
     for (pc = code; size > 0; pc += count, size -= count) {
-	fprintf(out, "0x" TARGET_FMT_lx ":  ", pc);
+        fprintf(out, "0x" HWADDR_FMT_plx ":  ", pc);
 	count = s.info.print_insn(pc, &s.info);
 	fprintf(out, "\n");
 	if (count < 0)
@@ -318,7 +320,7 @@ void disas(FILE *out, const void *code, unsigned long size)
 }
 
 /* Look up symbol for debugging purpose.  Returns "" if unknown. */
-const char *lookup_symbol(target_ulong orig_addr)
+const char *lookup_symbol(hwaddr orig_addr)
 {
     const char *symbol = "";
     struct syminfo *s;
diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 703f7434a0..48c9e910a8 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -3338,7 +3338,7 @@ static int symfind(const void *s0, const void *s1)
     return result;
 }
 
-static const char *lookup_symbolxx(struct syminfo *s, target_ulong orig_addr)
+static const char *lookup_symbolxx(struct syminfo *s, uint64_t orig_addr)
 {
 #if ELF_CLASS == ELFCLASS32
     struct elf_sym *syms = s->disas_symtab.elf32;
-- 
2.31.1

Re: [RFC PATCH 0/2] Make the core disassembler functions target-independent

[Richard Henderson]

On 5/8/23 14:37, Thomas Huth wrote:
> Move disas.c into the target-independent source set, so that we
> only have to compile this code once instead multiple times (one
> time for each target).
> 
> Marked as RFC since we have to replace the target_ulongs here
> with hwaddr, and the TARGET_FMT_lx with HWADDR_FMT_plx, which is
> a little bit ugly ... what's your opinion?
> 
> Thomas Huth (2):
>    disas: Move softmmu specific code to separate file
>    disas: Move disas.c into the target-independent source set

Patches 79-83 from

https://patchew.org/QEMU/20230503072331.1747057-1-richard.henderson@linaro.org/

do the same thing, using uint64_t instead of hwaddr (it's not).


r~

Re: [RFC PATCH 0/2] Make the core disassembler functions target-independent

[Thomas Huth]

On 08/05/2023 16.04, Richard Henderson wrote:
> On 5/8/23 14:37, Thomas Huth wrote:
>> Move disas.c into the target-independent source set, so that we
>> only have to compile this code once instead multiple times (one
>> time for each target).
>>
>> Marked as RFC since we have to replace the target_ulongs here
>> with hwaddr, and the TARGET_FMT_lx with HWADDR_FMT_plx, which is
>> a little bit ugly ... what's your opinion?
>>
>> Thomas Huth (2):
>>    disas: Move softmmu specific code to separate file
>>    disas: Move disas.c into the target-independent source set
> 
> Patches 79-83 from
> 
> https://patchew.org/QEMU/20230503072331.1747057-1-richard.henderson@linaro.org/
> 
> do the same thing, using uint64_t instead of hwaddr (it's not).

Ah, great, so I can scratch this from my list :-)

But FYI, you missed to update bsd-user/elfload.c in the patch that updates 
linux-user/elfload.c.

  Thomas

Re: [RFC PATCH 0/2] Make the core disassembler functions target-independent

[Richard Henderson]

On 5/8/23 15:04, Richard Henderson wrote:
> On 5/8/23 14:37, Thomas Huth wrote:
>> Move disas.c into the target-independent source set, so that we
>> only have to compile this code once instead multiple times (one
>> time for each target).
>>
>> Marked as RFC since we have to replace the target_ulongs here
>> with hwaddr, and the TARGET_FMT_lx with HWADDR_FMT_plx, which is
>> a little bit ugly ... what's your opinion?
>>
>> Thomas Huth (2):
>>    disas: Move softmmu specific code to separate file
>>    disas: Move disas.c into the target-independent source set
> 
> Patches 79-83 from
> 
> https://patchew.org/QEMU/20230503072331.1747057-1-richard.henderson@linaro.org/
> 
> do the same thing, using uint64_t instead of hwaddr (it's not).

I've just realized that only the choice of hwaddr/uint64_t overlap; I failed to take this 
all of the way to building disas once.

I will integrate our two sets.


r~