[PATCH v3 00/27] misc: Replace sprintf

[PATCH v3 00/27] misc: Replace sprintf

[Richard Henderson]

Hi Phil,

This incorporates my comments against
    20240411104340.6617-1-philmd@linaro.org
and
    20240411101550.99392-1-philmd@linaro.org

Especially cleanups to hexdup and disas.


r~


Philippe Mathieu-Daudé (13):
  hw/mips/malta: Add re-usable rng_seed_hex_new() method
  system/qtest: Replace sprintf by qemu_hexdump_line
  hw/scsi/scsi-disk: Use qemu_hexdump_line to avoid sprintf
  hw/ide/atapi: Use qemu_hexdump_line to avoid sprintf
  hw/dma/pl330: Use qemu_hexdump_line to avoid sprintf
  backends/tpm: Use qemu_hexdump_line to avoid sprintf
  disas/m68k: Replace sprintf() by snprintf()
  disas/microblaze: Replace sprintf() by snprintf()
  linux-user/flatload: Replace sprintf() by snprintf()
  hw/misc/imx: Replace sprintf() by snprintf()
  hw/net/rocker: Replace sprintf() by snprintf()
  hw/riscv/virt: Replace sprintf by g_strdup_printf
  target/arm: Replace sprintf() by snprintf()

Richard Henderson (14):
  util/hexdump: Remove b parameter from qemu_hexdump_line
  util/hexdump: Remove ascii parameter from qemu_hexdump_line
  util/hexdump: Use a GString for qemu_hexdump_line
  util/hexdump: Add unit_len and block_len to qemu_hexdump_line
  util/hexdump: Inline g_string_append_printf "%02x"
  disas/microblaze: Split out print_immval_addr
  target/microblaze: Re-indent print_insn_microblaze
  disas/microblaze: Merge op->name output into each fprintf
  disas/microblaze: Print registers directly with PRIreg
  disas/microblaze: Print immediates directly with PRIimm
  disas/microblaze: Print registers directly with PRIrfsl
  disas/microblaze: Split get_field_special
  disas/riscv: Use GString in format_inst
  target/i386/kvm: Improve KVM_EXIT_NOTIFY warnings

 include/qemu/cutils.h   |  20 +-
 backends/tpm/tpm_util.c |  24 +-
 disas/m68k.c            |   2 +-
 disas/microblaze.c      | 544 +++++++++++++++++-----------------------
 disas/riscv.c           | 209 +++++++--------
 hw/dma/pl330.c          |  23 +-
 hw/ide/atapi.c          |  12 +-
 hw/mips/malta.c         |  25 +-
 hw/misc/imx25_ccm.c     |   2 +-
 hw/misc/imx31_ccm.c     |   2 +-
 hw/misc/imx6_ccm.c      |   4 +-
 hw/misc/imx6_src.c      |   2 +-
 hw/misc/imx6ul_ccm.c    |   4 +-
 hw/misc/imx7_src.c      |   2 +-
 hw/net/imx_fec.c        |   2 +-
 hw/net/rocker/rocker.c  |  24 +-
 hw/riscv/virt.c         |  17 +-
 hw/scsi/scsi-disk.c     |  13 +-
 hw/ssi/imx_spi.c        |   2 +-
 hw/virtio/vhost-vdpa.c  |  14 +-
 linux-user/flatload.c   |   2 +-
 system/qtest.c          |  12 +-
 target/arm/cpu64.c      |   4 +-
 target/i386/kvm/kvm.c   |  12 +-
 util/hexdump.c          |  98 +++++---
 hw/virtio/trace-events  |   2 +-
 26 files changed, 494 insertions(+), 583 deletions(-)

-- 
2.34.1

[PATCH v3 01/27] util/hexdump: Remove b parameter from qemu_hexdump_line

[Richard Henderson]

Require that the caller output the offset and increment bufptr.
Use QEMU_HEXDUMP_LINE_BYTES in vhost_vdpa_dump_config instead
of raw integer.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/qemu/cutils.h  |  2 +-
 hw/virtio/vhost-vdpa.c |  4 ++--
 util/hexdump.c         | 13 ++++++-------
 hw/virtio/trace-events |  2 +-
 4 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index 92c927a6a3..3415f5d249 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -257,7 +257,7 @@ int parse_debug_env(const char *name, int max, int initial);
  */
 #define QEMU_HEXDUMP_LINE_BYTES 16 /* Number of bytes to dump */
 #define QEMU_HEXDUMP_LINE_LEN 75   /* Number of characters in line */
-void qemu_hexdump_line(char *line, unsigned int b, const void *bufptr,
+void qemu_hexdump_line(char *line, const void *bufptr,
                        unsigned int len, bool ascii);
 
 /*
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index e827b9175f..b4afa142f6 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -946,8 +946,8 @@ static void vhost_vdpa_dump_config(struct vhost_dev *dev, const uint8_t *config,
 
     for (b = 0; b < config_len; b += 16) {
         len = config_len - b;
-        qemu_hexdump_line(line, b, config, len, false);
-        trace_vhost_vdpa_dump_config(dev, line);
+        qemu_hexdump_line(line, config + b, len, false);
+        trace_vhost_vdpa_dump_config(dev, b, line);
     }
 }
 
diff --git a/util/hexdump.c b/util/hexdump.c
index 9921114b3c..7324e7b126 100644
--- a/util/hexdump.c
+++ b/util/hexdump.c
@@ -16,7 +16,7 @@
 #include "qemu/osdep.h"
 #include "qemu/cutils.h"
 
-void qemu_hexdump_line(char *line, unsigned int b, const void *bufptr,
+void qemu_hexdump_line(char *line, const void *bufptr,
                        unsigned int len, bool ascii)
 {
     const char *buf = bufptr;
@@ -26,13 +26,12 @@ void qemu_hexdump_line(char *line, unsigned int b, const void *bufptr,
         len = QEMU_HEXDUMP_LINE_BYTES;
     }
 
-    line += snprintf(line, 6, "%04x:", b);
     for (i = 0; i < QEMU_HEXDUMP_LINE_BYTES; i++) {
-        if ((i % 4) == 0) {
+        if (i != 0 && (i % 4) == 0) {
             *line++ = ' ';
         }
         if (i < len) {
-            line += sprintf(line, " %02x", (unsigned char)buf[b + i]);
+            line += sprintf(line, " %02x", (unsigned char)buf[i]);
         } else {
             line += sprintf(line, "   ");
         }
@@ -40,7 +39,7 @@ void qemu_hexdump_line(char *line, unsigned int b, const void *bufptr,
     if (ascii) {
         *line++ = ' ';
         for (i = 0; i < len; i++) {
-            c = buf[b + i];
+            c = buf[i];
             if (c < ' ' || c > '~') {
                 c = '.';
             }
@@ -58,8 +57,8 @@ void qemu_hexdump(FILE *fp, const char *prefix,
 
     for (b = 0; b < size; b += QEMU_HEXDUMP_LINE_BYTES) {
         len = size - b;
-        qemu_hexdump_line(line, b, bufptr, len, true);
-        fprintf(fp, "%s: %s\n", prefix, line);
+        qemu_hexdump_line(line, bufptr + b, len, true);
+        fprintf(fp, "%s: %04x: %s\n", prefix, b, line);
     }
 
 }
diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 96632fd026..d01bc85037 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -50,7 +50,7 @@ vhost_vdpa_get_device_id(void *dev, uint32_t device_id) "dev: %p device_id %"PRI
 vhost_vdpa_reset_device(void *dev) "dev: %p"
 vhost_vdpa_get_vq_index(void *dev, int idx, int vq_idx) "dev: %p idx: %d vq idx: %d"
 vhost_vdpa_set_vring_enable_one(void *dev, unsigned i, int enable, int r) "dev: %p, idx: %u, enable: %u, r: %d"
-vhost_vdpa_dump_config(void *dev, const char *line) "dev: %p %s"
+vhost_vdpa_dump_config(void *dev, unsigned ofs, const char *line) "dev: %p %04x: %s"
 vhost_vdpa_set_config(void *dev, uint32_t offset, uint32_t size, uint32_t flags) "dev: %p offset: %"PRIu32" size: %"PRIu32" flags: 0x%"PRIx32
 vhost_vdpa_get_config(void *dev, void *config, uint32_t config_len) "dev: %p config: %p config_len: %"PRIu32
 vhost_vdpa_suspend(void *dev) "dev: %p"
-- 
2.34.1

Re: [PATCH v3 01/27] util/hexdump: Remove b parameter from qemu_hexdump_line

[Philippe Mathieu-Daudé]

On 12/4/24 09:33, Richard Henderson wrote:
> Require that the caller output the offset and increment bufptr.
> Use QEMU_HEXDUMP_LINE_BYTES in vhost_vdpa_dump_config instead
> of raw integer.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   include/qemu/cutils.h  |  2 +-
>   hw/virtio/vhost-vdpa.c |  4 ++--
>   util/hexdump.c         | 13 ++++++-------
>   hw/virtio/trace-events |  2 +-
>   4 files changed, 10 insertions(+), 11 deletions(-)


> @@ -58,8 +57,8 @@ void qemu_hexdump(FILE *fp, const char *prefix,
>   
>       for (b = 0; b < size; b += QEMU_HEXDUMP_LINE_BYTES) {
>           len = size - b;
> -        qemu_hexdump_line(line, b, bufptr, len, true);
> -        fprintf(fp, "%s: %s\n", prefix, line);
> +        qemu_hexdump_line(line, bufptr + b, len, true);
> +        fprintf(fp, "%s: %04x: %s\n", prefix, b, line);

Clever :)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v3 01/27] util/hexdump: Remove b parameter from qemu_hexdump_line

[Philippe Mathieu-Daudé]

On 12/4/24 09:33, Richard Henderson wrote:
> Require that the caller output the offset and increment bufptr.
> Use QEMU_HEXDUMP_LINE_BYTES in vhost_vdpa_dump_config instead
> of raw integer.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   include/qemu/cutils.h  |  2 +-
>   hw/virtio/vhost-vdpa.c |  4 ++--
>   util/hexdump.c         | 13 ++++++-------
>   hw/virtio/trace-events |  2 +-
>   4 files changed, 10 insertions(+), 11 deletions(-)


> diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> index 96632fd026..d01bc85037 100644
> --- a/hw/virtio/trace-events
> +++ b/hw/virtio/trace-events
> @@ -50,7 +50,7 @@ vhost_vdpa_get_device_id(void *dev, uint32_t device_id) "dev: %p device_id %"PRI
>   vhost_vdpa_reset_device(void *dev) "dev: %p"
>   vhost_vdpa_get_vq_index(void *dev, int idx, int vq_idx) "dev: %p idx: %d vq idx: %d"
>   vhost_vdpa_set_vring_enable_one(void *dev, unsigned i, int enable, int r) "dev: %p, idx: %u, enable: %u, r: %d"
> -vhost_vdpa_dump_config(void *dev, const char *line) "dev: %p %s"
> +vhost_vdpa_dump_config(void *dev, unsigned ofs, const char *line) "dev: %p %04x: %s"

Queued squashing "0x" due to:

     ERROR: Hex numbers must be prefixed with '0x'
     #108: FILE: hw/virtio/trace-events:53:
     +vhost_vdpa_dump_config(void *dev, unsigned ofs, const char *line) 
"dev: %p %04x: %s"

     total: 1 errors, 0 warnings, 67 lines checked

[PATCH v3 02/27] util/hexdump: Remove ascii parameter from qemu_hexdump_line

[Richard Henderson]

Split out asciidump_line as a separate function, local to hexdump.c,
for use by qemu_hexdump.  Use "%-*s" to generate the alignment
between the hex and the ascii, rather than explicit spaces.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/qemu/cutils.h  |  3 +--
 hw/virtio/vhost-vdpa.c |  2 +-
 util/hexdump.c         | 54 ++++++++++++++++++++++++------------------
 3 files changed, 33 insertions(+), 26 deletions(-)

diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index 3415f5d249..d0c5386e6c 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -257,8 +257,7 @@ int parse_debug_env(const char *name, int max, int initial);
  */
 #define QEMU_HEXDUMP_LINE_BYTES 16 /* Number of bytes to dump */
 #define QEMU_HEXDUMP_LINE_LEN 75   /* Number of characters in line */
-void qemu_hexdump_line(char *line, const void *bufptr,
-                       unsigned int len, bool ascii);
+void qemu_hexdump_line(char *line, const void *bufptr, size_t len);
 
 /*
  * Hexdump a buffer to a file. An optional string prefix is added to every line
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index b4afa142f6..32bad5ce68 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -946,7 +946,7 @@ static void vhost_vdpa_dump_config(struct vhost_dev *dev, const uint8_t *config,
 
     for (b = 0; b < config_len; b += 16) {
         len = config_len - b;
-        qemu_hexdump_line(line, config + b, len, false);
+        qemu_hexdump_line(line, config + b, len);
         trace_vhost_vdpa_dump_config(dev, b, line);
     }
 }
diff --git a/util/hexdump.c b/util/hexdump.c
index 7324e7b126..dbc536fe84 100644
--- a/util/hexdump.c
+++ b/util/hexdump.c
@@ -16,8 +16,7 @@
 #include "qemu/osdep.h"
 #include "qemu/cutils.h"
 
-void qemu_hexdump_line(char *line, const void *bufptr,
-                       unsigned int len, bool ascii)
+void qemu_hexdump_line(char *line, const void *bufptr, size_t len)
 {
     const char *buf = bufptr;
     int i, c;
@@ -26,39 +25,48 @@ void qemu_hexdump_line(char *line, const void *bufptr,
         len = QEMU_HEXDUMP_LINE_BYTES;
     }
 
-    for (i = 0; i < QEMU_HEXDUMP_LINE_BYTES; i++) {
+    for (i = 0; i < len; i++) {
         if (i != 0 && (i % 4) == 0) {
             *line++ = ' ';
         }
-        if (i < len) {
-            line += sprintf(line, " %02x", (unsigned char)buf[i]);
-        } else {
-            line += sprintf(line, "   ");
-        }
-    }
-    if (ascii) {
-        *line++ = ' ';
-        for (i = 0; i < len; i++) {
-            c = buf[i];
-            if (c < ' ' || c > '~') {
-                c = '.';
-            }
-            *line++ = c;
-        }
+        line += sprintf(line, " %02x", (unsigned char)buf[i]);
     }
     *line = '\0';
 }
 
+static void asciidump_line(char *line, const void *bufptr, size_t len)
+{
+    const char *buf = bufptr;
+
+    for (size_t i = 0; i < len; i++) {
+        char c = buf[i];
+
+        if (c < ' ' || c > '~') {
+            c = '.';
+        }
+        *line++ = c;
+    }
+    *line = '\0';
+}
+
+#define QEMU_HEXDUMP_LINE_WIDTH \
+    (QEMU_HEXDUMP_LINE_BYTES * 2 + QEMU_HEXDUMP_LINE_BYTES / 4)
+
 void qemu_hexdump(FILE *fp, const char *prefix,
                   const void *bufptr, size_t size)
 {
-    unsigned int b, len;
     char line[QEMU_HEXDUMP_LINE_LEN];
+    char ascii[QEMU_HEXDUMP_LINE_BYTES + 1];
+    size_t b, len;
 
-    for (b = 0; b < size; b += QEMU_HEXDUMP_LINE_BYTES) {
-        len = size - b;
-        qemu_hexdump_line(line, bufptr + b, len, true);
-        fprintf(fp, "%s: %04x: %s\n", prefix, b, line);
+    for (b = 0; b < size; b += len) {
+        len = MIN(size - b, QEMU_HEXDUMP_LINE_BYTES);
+
+        qemu_hexdump_line(line, bufptr + b, len);
+        asciidump_line(ascii, bufptr + b, len);
+
+        fprintf(fp, "%s: %04x: %-*s %s\n",
+                prefix, b, QEMU_HEXDUMP_LINE_WIDTH, line, ascii);
     }
 
 }
-- 
2.34.1

Re: [PATCH v3 02/27] util/hexdump: Remove ascii parameter from qemu_hexdump_line

[Philippe Mathieu-Daudé]

On 12/4/24 09:33, Richard Henderson wrote:
> Split out asciidump_line as a separate function, local to hexdump.c,
> for use by qemu_hexdump.  Use "%-*s" to generate the alignment
> between the hex and the ascii, rather than explicit spaces.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   include/qemu/cutils.h  |  3 +--
>   hw/virtio/vhost-vdpa.c |  2 +-
>   util/hexdump.c         | 54 ++++++++++++++++++++++++------------------
>   3 files changed, 33 insertions(+), 26 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v3 02/27] util/hexdump: Remove ascii parameter from qemu_hexdump_line

[Philippe Mathieu-Daudé]

On 12/4/24 09:33, Richard Henderson wrote:
> Split out asciidump_line as a separate function, local to hexdump.c,
> for use by qemu_hexdump.  Use "%-*s" to generate the alignment
> between the hex and the ascii, rather than explicit spaces.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   include/qemu/cutils.h  |  3 +--
>   hw/virtio/vhost-vdpa.c |  2 +-
>   util/hexdump.c         | 54 ++++++++++++++++++++++++------------------
>   3 files changed, 33 insertions(+), 26 deletions(-)
> 
> diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
> index 3415f5d249..d0c5386e6c 100644
> --- a/include/qemu/cutils.h
> +++ b/include/qemu/cutils.h
> @@ -257,8 +257,7 @@ int parse_debug_env(const char *name, int max, int initial);
>    */
>   #define QEMU_HEXDUMP_LINE_BYTES 16 /* Number of bytes to dump */
>   #define QEMU_HEXDUMP_LINE_LEN 75   /* Number of characters in line */
> -void qemu_hexdump_line(char *line, const void *bufptr,
> -                       unsigned int len, bool ascii);
> +void qemu_hexdump_line(char *line, const void *bufptr, size_t len);
>   
>   /*
>    * Hexdump a buffer to a file. An optional string prefix is added to every line
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index b4afa142f6..32bad5ce68 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -946,7 +946,7 @@ static void vhost_vdpa_dump_config(struct vhost_dev *dev, const uint8_t *config,
>   
>       for (b = 0; b < config_len; b += 16) {
>           len = config_len - b;
> -        qemu_hexdump_line(line, config + b, len, false);
> +        qemu_hexdump_line(line, config + b, len);
>           trace_vhost_vdpa_dump_config(dev, b, line);
>       }
>   }
> diff --git a/util/hexdump.c b/util/hexdump.c
> index 7324e7b126..dbc536fe84 100644
> --- a/util/hexdump.c
> +++ b/util/hexdump.c
> @@ -16,8 +16,7 @@
>   #include "qemu/osdep.h"
>   #include "qemu/cutils.h"
>   
> -void qemu_hexdump_line(char *line, const void *bufptr,
> -                       unsigned int len, bool ascii)
> +void qemu_hexdump_line(char *line, const void *bufptr, size_t len)
>   {
>       const char *buf = bufptr;
>       int i, c;

'c' ...

> @@ -26,39 +25,48 @@ void qemu_hexdump_line(char *line, const void *bufptr,
>           len = QEMU_HEXDUMP_LINE_BYTES;
>       }
>   
> -    for (i = 0; i < QEMU_HEXDUMP_LINE_BYTES; i++) {
> +    for (i = 0; i < len; i++) {
>           if (i != 0 && (i % 4) == 0) {
>               *line++ = ' ';
>           }
> -        if (i < len) {
> -            line += sprintf(line, " %02x", (unsigned char)buf[i]);
> -        } else {
> -            line += sprintf(line, "   ");
> -        }
> -    }
> -    if (ascii) {
> -        *line++ = ' ';
> -        for (i = 0; i < len; i++) {
> -            c = buf[i];
> -            if (c < ' ' || c > '~') {
> -                c = '.';
> -            }
> -            *line++ = c;

... is no more used;

> -        }
> +        line += sprintf(line, " %02x", (unsigned char)buf[i]);
>       }
>       *line = '\0';
>   }
>   
> +static void asciidump_line(char *line, const void *bufptr, size_t len)
> +{
> +    const char *buf = bufptr;
> +
> +    for (size_t i = 0; i < len; i++) {
> +        char c = buf[i];
> +
> +        if (c < ' ' || c > '~') {
> +            c = '.';
> +        }
> +        *line++ = c;
> +    }
> +    *line = '\0';
> +}
> +
> +#define QEMU_HEXDUMP_LINE_WIDTH \
> +    (QEMU_HEXDUMP_LINE_BYTES * 2 + QEMU_HEXDUMP_LINE_BYTES / 4)
> +
>   void qemu_hexdump(FILE *fp, const char *prefix,
>                     const void *bufptr, size_t size)
>   {
> -    unsigned int b, len;
>       char line[QEMU_HEXDUMP_LINE_LEN];
> +    char ascii[QEMU_HEXDUMP_LINE_BYTES + 1];
> +    size_t b, len;
>   
> -    for (b = 0; b < size; b += QEMU_HEXDUMP_LINE_BYTES) {
> -        len = size - b;
> -        qemu_hexdump_line(line, bufptr + b, len, true);
> -        fprintf(fp, "%s: %04x: %s\n", prefix, b, line);
> +    for (b = 0; b < size; b += len) {
> +        len = MIN(size - b, QEMU_HEXDUMP_LINE_BYTES);
> +
> +        qemu_hexdump_line(line, bufptr + b, len);
> +        asciidump_line(ascii, bufptr + b, len);
> +
> +        fprintf(fp, "%s: %04x: %-*s %s\n",

'b' is size_t.

> +                prefix, b, QEMU_HEXDUMP_LINE_WIDTH, line, ascii);
>       }
>   
>   }

Queued squashing:

-- >8 --
diff --git a/util/hexdump.c b/util/hexdump.c
index dbc536fe84..0f943e31e5 100644
--- a/util/hexdump.c
+++ b/util/hexdump.c
@@ -19,7 +19,7 @@
  void qemu_hexdump_line(char *line, const void *bufptr, size_t len)
  {
      const char *buf = bufptr;
-    int i, c;
+    int i;

      if (len > QEMU_HEXDUMP_LINE_BYTES) {
          len = QEMU_HEXDUMP_LINE_BYTES;
@@ -65,7 +65,7 @@ void qemu_hexdump(FILE *fp, const char *prefix,
          qemu_hexdump_line(line, bufptr + b, len);
          asciidump_line(ascii, bufptr + b, len);

-        fprintf(fp, "%s: %04x: %-*s %s\n",
+        fprintf(fp, "%s: %04zx: %-*s %s\n",
                  prefix, b, QEMU_HEXDUMP_LINE_WIDTH, line, ascii);
      }

---

[PATCH v3 03/27] util/hexdump: Use a GString for qemu_hexdump_line

[Richard Henderson]

Allocate a new, or append to an existing GString instead of
using a fixed sized buffer.  Require the caller to determine
the length of the line -- do not bound len here.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/qemu/cutils.h  | 15 ++++++++++-----
 hw/virtio/vhost-vdpa.c | 14 ++++++++------
 util/hexdump.c         | 29 +++++++++++++++++------------
 3 files changed, 35 insertions(+), 23 deletions(-)

diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index d0c5386e6c..7311fb36ca 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -252,12 +252,17 @@ static inline const char *yes_no(bool b)
  */
 int parse_debug_env(const char *name, int max, int initial);
 
-/*
- * Hexdump a line of a byte buffer into a hexadecimal/ASCII buffer
+/**
+ * qemu_hexdump_line:
+ * @str: GString into which to append
+ * @buf: buffer to dump
+ * @len: number of bytes to dump
+ *
+ * Append @len bytes of @buf as hexadecimal into @str.
+ * If @str is NULL, allocate a new string and return it;
+ * otherwise return @str.
  */
-#define QEMU_HEXDUMP_LINE_BYTES 16 /* Number of bytes to dump */
-#define QEMU_HEXDUMP_LINE_LEN 75   /* Number of characters in line */
-void qemu_hexdump_line(char *line, const void *bufptr, size_t len);
+GString *qemu_hexdump_line(GString *str, const void *buf, size_t len);
 
 /*
  * Hexdump a buffer to a file. An optional string prefix is added to every line
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 32bad5ce68..ab047d8ee0 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -941,13 +941,15 @@ static int vhost_vdpa_set_config_call(struct vhost_dev *dev,
 static void vhost_vdpa_dump_config(struct vhost_dev *dev, const uint8_t *config,
                                    uint32_t config_len)
 {
-    int b, len;
-    char line[QEMU_HEXDUMP_LINE_LEN];
+    g_autoptr(GString) str = g_string_sized_new(4 * 16);
+    size_t b, len;
 
-    for (b = 0; b < config_len; b += 16) {
-        len = config_len - b;
-        qemu_hexdump_line(line, config + b, len);
-        trace_vhost_vdpa_dump_config(dev, b, line);
+    for (b = 0; b < config_len; b += len) {
+        len = MIN(config_len - b, 16);
+
+        g_string_truncate(str, 0);
+        qemu_hexdump_line(str, config + b, len);
+        trace_vhost_vdpa_dump_config(dev, b, str->str);
     }
 }
 
diff --git a/util/hexdump.c b/util/hexdump.c
index dbc536fe84..521e346bc6 100644
--- a/util/hexdump.c
+++ b/util/hexdump.c
@@ -16,22 +16,25 @@
 #include "qemu/osdep.h"
 #include "qemu/cutils.h"
 
-void qemu_hexdump_line(char *line, const void *bufptr, size_t len)
+GString *qemu_hexdump_line(GString *str, const void *vbuf, size_t len)
 {
-    const char *buf = bufptr;
-    int i, c;
+    const uint8_t *buf = vbuf;
+    size_t i;
 
-    if (len > QEMU_HEXDUMP_LINE_BYTES) {
-        len = QEMU_HEXDUMP_LINE_BYTES;
+    if (str == NULL) {
+        /* Estimate the length of the output to avoid reallocs. */
+        i = len * 3 + len / 4;
+        str = g_string_sized_new(i + 1);
     }
 
     for (i = 0; i < len; i++) {
         if (i != 0 && (i % 4) == 0) {
-            *line++ = ' ';
+            g_string_append_c(str, ' ');
         }
-        line += sprintf(line, " %02x", (unsigned char)buf[i]);
+        g_string_append_printf(str, " %02x", buf[i]);
     }
-    *line = '\0';
+
+    return str;
 }
 
 static void asciidump_line(char *line, const void *bufptr, size_t len)
@@ -49,24 +52,26 @@ static void asciidump_line(char *line, const void *bufptr, size_t len)
     *line = '\0';
 }
 
+#define QEMU_HEXDUMP_LINE_BYTES 16
 #define QEMU_HEXDUMP_LINE_WIDTH \
     (QEMU_HEXDUMP_LINE_BYTES * 2 + QEMU_HEXDUMP_LINE_BYTES / 4)
 
 void qemu_hexdump(FILE *fp, const char *prefix,
                   const void *bufptr, size_t size)
 {
-    char line[QEMU_HEXDUMP_LINE_LEN];
+    g_autoptr(GString) str = g_string_sized_new(QEMU_HEXDUMP_LINE_WIDTH + 1);
     char ascii[QEMU_HEXDUMP_LINE_BYTES + 1];
     size_t b, len;
 
     for (b = 0; b < size; b += len) {
         len = MIN(size - b, QEMU_HEXDUMP_LINE_BYTES);
 
-        qemu_hexdump_line(line, bufptr + b, len);
+        g_string_truncate(str, 0);
+        qemu_hexdump_line(str, bufptr + b, len);
         asciidump_line(ascii, bufptr + b, len);
 
-        fprintf(fp, "%s: %04x: %-*s %s\n",
-                prefix, b, QEMU_HEXDUMP_LINE_WIDTH, line, ascii);
+        fprintf(fp, "%s: %04zx: %-*s %s\n",
+                prefix, b, QEMU_HEXDUMP_LINE_WIDTH, str->str, ascii);
     }
 
 }
-- 
2.34.1

Re: [PATCH v3 03/27] util/hexdump: Use a GString for qemu_hexdump_line

[Philippe Mathieu-Daudé]

On 12/4/24 09:33, Richard Henderson wrote:
> Allocate a new, or append to an existing GString instead of
> using a fixed sized buffer.  Require the caller to determine
> the length of the line -- do not bound len here.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   include/qemu/cutils.h  | 15 ++++++++++-----
>   hw/virtio/vhost-vdpa.c | 14 ++++++++------
>   util/hexdump.c         | 29 +++++++++++++++++------------
>   3 files changed, 35 insertions(+), 23 deletions(-)
> 
> diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
> index d0c5386e6c..7311fb36ca 100644
> --- a/include/qemu/cutils.h
> +++ b/include/qemu/cutils.h
> @@ -252,12 +252,17 @@ static inline const char *yes_no(bool b)
>    */
>   int parse_debug_env(const char *name, int max, int initial);
>   
> -/*
> - * Hexdump a line of a byte buffer into a hexadecimal/ASCII buffer
> +/**
> + * qemu_hexdump_line:
> + * @str: GString into which to append
> + * @buf: buffer to dump
> + * @len: number of bytes to dump
> + *
> + * Append @len bytes of @buf as hexadecimal into @str.
> + * If @str is NULL, allocate a new string and return it;
> + * otherwise return @str.
>    */
> -#define QEMU_HEXDUMP_LINE_BYTES 16 /* Number of bytes to dump */
> -#define QEMU_HEXDUMP_LINE_LEN 75   /* Number of characters in line */
> -void qemu_hexdump_line(char *line, const void *bufptr, size_t len);
> +GString *qemu_hexdump_line(GString *str, const void *buf, size_t len);


> diff --git a/util/hexdump.c b/util/hexdump.c
> index dbc536fe84..521e346bc6 100644
> --- a/util/hexdump.c
> +++ b/util/hexdump.c
> @@ -16,22 +16,25 @@
>   #include "qemu/osdep.h"
>   #include "qemu/cutils.h"
>   
> -void qemu_hexdump_line(char *line, const void *bufptr, size_t len)
> +GString *qemu_hexdump_line(GString *str, const void *vbuf, size_t len)
>   {
> -    const char *buf = bufptr;
> -    int i, c;
> +    const uint8_t *buf = vbuf;
> +    size_t i;
>   
> -    if (len > QEMU_HEXDUMP_LINE_BYTES) {
> -        len = QEMU_HEXDUMP_LINE_BYTES;
> +    if (str == NULL) {
> +        /* Estimate the length of the output to avoid reallocs. */
> +        i = len * 3 + len / 4;
> +        str = g_string_sized_new(i + 1);
>       }

[*]
          else {
            g_string_truncate(str, 0);
          }

>   
>       for (i = 0; i < len; i++) {
>           if (i != 0 && (i % 4) == 0) {
> -            *line++ = ' ';
> +            g_string_append_c(str, ' ');
>           }
> -        line += sprintf(line, " %02x", (unsigned char)buf[i]);
> +        g_string_append_printf(str, " %02x", buf[i]);
>       }
> -    *line = '\0';
> +
> +    return str;
>   }
>   
>   static void asciidump_line(char *line, const void *bufptr, size_t len)
> @@ -49,24 +52,26 @@ static void asciidump_line(char *line, const void *bufptr, size_t len)
>       *line = '\0';
>   }
>   
> +#define QEMU_HEXDUMP_LINE_BYTES 16
>   #define QEMU_HEXDUMP_LINE_WIDTH \
>       (QEMU_HEXDUMP_LINE_BYTES * 2 + QEMU_HEXDUMP_LINE_BYTES / 4)
>   
>   void qemu_hexdump(FILE *fp, const char *prefix,
>                     const void *bufptr, size_t size)
>   {
> -    char line[QEMU_HEXDUMP_LINE_LEN];
> +    g_autoptr(GString) str = g_string_sized_new(QEMU_HEXDUMP_LINE_WIDTH + 1);
>       char ascii[QEMU_HEXDUMP_LINE_BYTES + 1];
>       size_t b, len;
>   
>       for (b = 0; b < size; b += len) {
>           len = MIN(size - b, QEMU_HEXDUMP_LINE_BYTES);
>   
> -        qemu_hexdump_line(line, bufptr + b, len);
> +        g_string_truncate(str, 0);

Shouldn't we truncate in [*] ?

> +        qemu_hexdump_line(str, bufptr + b, len);
>           asciidump_line(ascii, bufptr + b, len);
>   
> -        fprintf(fp, "%s: %04x: %-*s %s\n",
> -                prefix, b, QEMU_HEXDUMP_LINE_WIDTH, line, ascii);
> +        fprintf(fp, "%s: %04zx: %-*s %s\n",
> +                prefix, b, QEMU_HEXDUMP_LINE_WIDTH, str->str, ascii);
>       }
>   
>   }

Re: [PATCH v3 03/27] util/hexdump: Use a GString for qemu_hexdump_line

[Richard Henderson]

On 4/12/24 10:41, Philippe Mathieu-Daudé wrote:
>> -void qemu_hexdump_line(char *line, const void *bufptr, size_t len)
>> +GString *qemu_hexdump_line(GString *str, const void *vbuf, size_t len)
>>   {
>> -    const char *buf = bufptr;
>> -    int i, c;
>> +    const uint8_t *buf = vbuf;
>> +    size_t i;
>> -    if (len > QEMU_HEXDUMP_LINE_BYTES) {
>> -        len = QEMU_HEXDUMP_LINE_BYTES;
>> +    if (str == NULL) {
>> +        /* Estimate the length of the output to avoid reallocs. */
>> +        i = len * 3 + len / 4;
>> +        str = g_string_sized_new(i + 1);
>>       }
> 
> [*]
>           else {
>             g_string_truncate(str, 0);
>           }
> 
...
>> @@ -49,24 +52,26 @@ static void asciidump_line(char *line, const void *bufptr, size_t len)
>>       *line = '\0';
>>   }
>> +#define QEMU_HEXDUMP_LINE_BYTES 16
>>   #define QEMU_HEXDUMP_LINE_WIDTH \
>>       (QEMU_HEXDUMP_LINE_BYTES * 2 + QEMU_HEXDUMP_LINE_BYTES / 4)
>>   void qemu_hexdump(FILE *fp, const char *prefix,
>>                     const void *bufptr, size_t size)
>>   {
>> -    char line[QEMU_HEXDUMP_LINE_LEN];
>> +    g_autoptr(GString) str = g_string_sized_new(QEMU_HEXDUMP_LINE_WIDTH + 1);
>>       char ascii[QEMU_HEXDUMP_LINE_BYTES + 1];
>>       size_t b, len;
>>       for (b = 0; b < size; b += len) {
>>           len = MIN(size - b, QEMU_HEXDUMP_LINE_BYTES);
>> -        qemu_hexdump_line(line, bufptr + b, len);
>> +        g_string_truncate(str, 0);
> 
> Shouldn't we truncate in [*] ?

The usage in tpm puts several lines together in one string,
adding \n in between, for output in one go.


r~

Re: [PATCH v3 03/27] util/hexdump: Use a GString for qemu_hexdump_line

[Philippe Mathieu-Daudé]

On 12/4/24 20:59, Richard Henderson wrote:
> On 4/12/24 10:41, Philippe Mathieu-Daudé wrote:
>>> -void qemu_hexdump_line(char *line, const void *bufptr, size_t len)
>>> +GString *qemu_hexdump_line(GString *str, const void *vbuf, size_t len)
>>>   {
>>> -    const char *buf = bufptr;
>>> -    int i, c;
>>> +    const uint8_t *buf = vbuf;
>>> +    size_t i;
>>> -    if (len > QEMU_HEXDUMP_LINE_BYTES) {
>>> -        len = QEMU_HEXDUMP_LINE_BYTES;
>>> +    if (str == NULL) {
>>> +        /* Estimate the length of the output to avoid reallocs. */
>>> +        i = len * 3 + len / 4;
>>> +        str = g_string_sized_new(i + 1);
>>>       }
>>
>> [*]
>>           else {
>>             g_string_truncate(str, 0);
>>           }
>>
> ...
>>> @@ -49,24 +52,26 @@ static void asciidump_line(char *line, const void 
>>> *bufptr, size_t len)
>>>       *line = '\0';
>>>   }
>>> +#define QEMU_HEXDUMP_LINE_BYTES 16
>>>   #define QEMU_HEXDUMP_LINE_WIDTH \
>>>       (QEMU_HEXDUMP_LINE_BYTES * 2 + QEMU_HEXDUMP_LINE_BYTES / 4)
>>>   void qemu_hexdump(FILE *fp, const char *prefix,
>>>                     const void *bufptr, size_t size)
>>>   {
>>> -    char line[QEMU_HEXDUMP_LINE_LEN];
>>> +    g_autoptr(GString) str = 
>>> g_string_sized_new(QEMU_HEXDUMP_LINE_WIDTH + 1);
>>>       char ascii[QEMU_HEXDUMP_LINE_BYTES + 1];
>>>       size_t b, len;
>>>       for (b = 0; b < size; b += len) {
>>>           len = MIN(size - b, QEMU_HEXDUMP_LINE_BYTES);
>>> -        qemu_hexdump_line(line, bufptr + b, len);
>>> +        g_string_truncate(str, 0);
>>
>> Shouldn't we truncate in [*] ?
> 
> The usage in tpm puts several lines together in one string,
> adding \n in between, for output in one go.

I see the trace_tpm_util_show_buffer() call. However this
isn't a recommended use of the tracing API (Cc'ing Stefan).
It breaks the "log" backend output, and is sub-optimal for
all other backends.

IMHO the TPM buffer should be traced by multiple calls of
(offset, hexbuf) instead.

Regards,

Phil.

Re: [PATCH v3 03/27] util/hexdump: Use a GString for qemu_hexdump_line

[Stefan Hajnoczi]

On Sat, 13 Apr 2024 at 05:46, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 12/4/24 20:59, Richard Henderson wrote:
> > On 4/12/24 10:41, Philippe Mathieu-Daudé wrote:
> >>> -void qemu_hexdump_line(char *line, const void *bufptr, size_t len)
> >>> +GString *qemu_hexdump_line(GString *str, const void *vbuf, size_t len)
> >>>   {
> >>> -    const char *buf = bufptr;
> >>> -    int i, c;
> >>> +    const uint8_t *buf = vbuf;
> >>> +    size_t i;
> >>> -    if (len > QEMU_HEXDUMP_LINE_BYTES) {
> >>> -        len = QEMU_HEXDUMP_LINE_BYTES;
> >>> +    if (str == NULL) {
> >>> +        /* Estimate the length of the output to avoid reallocs. */
> >>> +        i = len * 3 + len / 4;
> >>> +        str = g_string_sized_new(i + 1);
> >>>       }
> >>
> >> [*]
> >>           else {
> >>             g_string_truncate(str, 0);
> >>           }
> >>
> > ...
> >>> @@ -49,24 +52,26 @@ static void asciidump_line(char *line, const void
> >>> *bufptr, size_t len)
> >>>       *line = '\0';
> >>>   }
> >>> +#define QEMU_HEXDUMP_LINE_BYTES 16
> >>>   #define QEMU_HEXDUMP_LINE_WIDTH \
> >>>       (QEMU_HEXDUMP_LINE_BYTES * 2 + QEMU_HEXDUMP_LINE_BYTES / 4)
> >>>   void qemu_hexdump(FILE *fp, const char *prefix,
> >>>                     const void *bufptr, size_t size)
> >>>   {
> >>> -    char line[QEMU_HEXDUMP_LINE_LEN];
> >>> +    g_autoptr(GString) str =
> >>> g_string_sized_new(QEMU_HEXDUMP_LINE_WIDTH + 1);
> >>>       char ascii[QEMU_HEXDUMP_LINE_BYTES + 1];
> >>>       size_t b, len;
> >>>       for (b = 0; b < size; b += len) {
> >>>           len = MIN(size - b, QEMU_HEXDUMP_LINE_BYTES);
> >>> -        qemu_hexdump_line(line, bufptr + b, len);
> >>> +        g_string_truncate(str, 0);
> >>
> >> Shouldn't we truncate in [*] ?
> >
> > The usage in tpm puts several lines together in one string,
> > adding \n in between, for output in one go.
>
> I see the trace_tpm_util_show_buffer() call. However this
> isn't a recommended use of the tracing API (Cc'ing Stefan).
> It breaks the "log" backend output, and is sub-optimal for
> all other backends.
>
> IMHO the TPM buffer should be traced by multiple calls of
> (offset, hexbuf) instead.

I think so too.

Stefan

[PATCH v3 04/27] util/hexdump: Add unit_len and block_len to qemu_hexdump_line

[Richard Henderson]

Generalize the current 1 byte unit and 4 byte blocking
within the output.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/qemu/cutils.h  |  6 +++++-
 hw/virtio/vhost-vdpa.c |  2 +-
 util/hexdump.c         | 30 +++++++++++++++++++++---------
 3 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index 7311fb36ca..e28adedab9 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -257,12 +257,16 @@ int parse_debug_env(const char *name, int max, int initial);
  * @str: GString into which to append
  * @buf: buffer to dump
  * @len: number of bytes to dump
+ * @unit_len: add a space between every @unit_len bytes
+ * @block_len: add an extra space between every @block_len bytes
  *
  * Append @len bytes of @buf as hexadecimal into @str.
+ * Add spaces between every @unit_len and @block_len bytes.
  * If @str is NULL, allocate a new string and return it;
  * otherwise return @str.
  */
-GString *qemu_hexdump_line(GString *str, const void *buf, size_t len);
+GString *qemu_hexdump_line(GString *str, const void *buf, size_t len,
+                           size_t unit_len, size_t block_len);
 
 /*
  * Hexdump a buffer to a file. An optional string prefix is added to every line
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index ab047d8ee0..911742e291 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -948,7 +948,7 @@ static void vhost_vdpa_dump_config(struct vhost_dev *dev, const uint8_t *config,
         len = MIN(config_len - b, 16);
 
         g_string_truncate(str, 0);
-        qemu_hexdump_line(str, config + b, len);
+        qemu_hexdump_line(str, config + b, len, 1, 4);
         trace_vhost_vdpa_dump_config(dev, b, str->str);
     }
 }
diff --git a/util/hexdump.c b/util/hexdump.c
index 521e346bc6..b29326b7f2 100644
--- a/util/hexdump.c
+++ b/util/hexdump.c
@@ -1,5 +1,5 @@
 /*
- * Helper to hexdump a buffer
+* Helper to hexdump a buffer
  *
  * Copyright (c) 2013 Red Hat, Inc.
  * Copyright (c) 2013 Gerd Hoffmann <kraxel@redhat.com>
@@ -16,22 +16,34 @@
 #include "qemu/osdep.h"
 #include "qemu/cutils.h"
 
-GString *qemu_hexdump_line(GString *str, const void *vbuf, size_t len)
+GString *qemu_hexdump_line(GString *str, const void *vbuf, size_t len,
+                           size_t unit_len, size_t block_len)
 {
     const uint8_t *buf = vbuf;
-    size_t i;
+    size_t u, b;
 
     if (str == NULL) {
         /* Estimate the length of the output to avoid reallocs. */
-        i = len * 3 + len / 4;
-        str = g_string_sized_new(i + 1);
+        size_t est = len * 2;
+        if (unit_len) {
+            est += len / unit_len;
+        }
+        if (block_len) {
+            est += len / block_len;
+        }
+        str = g_string_sized_new(est + 1);
     }
 
-    for (i = 0; i < len; i++) {
-        if (i != 0 && (i % 4) == 0) {
+    for (u = 0, b = 0; len; u++, b++, len--, buf++) {
+        if (unit_len && u == unit_len) {
             g_string_append_c(str, ' ');
+            u = 0;
         }
-        g_string_append_printf(str, " %02x", buf[i]);
+        if (block_len && b == block_len) {
+            g_string_append_c(str, ' ');
+            b = 0;
+        }
+        g_string_append_printf(str, "%02x", *buf);
     }
 
     return str;
@@ -67,7 +79,7 @@ void qemu_hexdump(FILE *fp, const char *prefix,
         len = MIN(size - b, QEMU_HEXDUMP_LINE_BYTES);
 
         g_string_truncate(str, 0);
-        qemu_hexdump_line(str, bufptr + b, len);
+        qemu_hexdump_line(str, bufptr + b, len, 1, 4);
         asciidump_line(ascii, bufptr + b, len);
 
         fprintf(fp, "%s: %04zx: %-*s %s\n",
-- 
2.34.1

Re: [PATCH v3 04/27] util/hexdump: Add unit_len and block_len to qemu_hexdump_line

[Philippe Mathieu-Daudé]

On 12/4/24 09:33, Richard Henderson wrote:
> Generalize the current 1 byte unit and 4 byte blocking
> within the output.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   include/qemu/cutils.h  |  6 +++++-
>   hw/virtio/vhost-vdpa.c |  2 +-
>   util/hexdump.c         | 30 +++++++++++++++++++++---------
>   3 files changed, 27 insertions(+), 11 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v3 05/27] util/hexdump: Inline g_string_append_printf "%02x"

[Richard Henderson]

Trivial arithmetic can be used for emitting the nibbles,
rather than full-blown printf formatting.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 util/hexdump.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/util/hexdump.c b/util/hexdump.c
index b29326b7f2..ae0d4992dc 100644
--- a/util/hexdump.c
+++ b/util/hexdump.c
@@ -16,6 +16,11 @@
 #include "qemu/osdep.h"
 #include "qemu/cutils.h"
 
+static inline char hexdump_nibble(unsigned x)
+{
+    return (x < 10 ? '0' : 'a' - 10) + x;
+}
+
 GString *qemu_hexdump_line(GString *str, const void *vbuf, size_t len,
                            size_t unit_len, size_t block_len)
 {
@@ -35,6 +40,8 @@ GString *qemu_hexdump_line(GString *str, const void *vbuf, size_t len,
     }
 
     for (u = 0, b = 0; len; u++, b++, len--, buf++) {
+        uint8_t c;
+
         if (unit_len && u == unit_len) {
             g_string_append_c(str, ' ');
             u = 0;
@@ -43,7 +50,10 @@ GString *qemu_hexdump_line(GString *str, const void *vbuf, size_t len,
             g_string_append_c(str, ' ');
             b = 0;
         }
-        g_string_append_printf(str, "%02x", *buf);
+
+        c = *buf;
+        g_string_append_c(str, hexdump_nibble(c / 16));
+        g_string_append_c(str, hexdump_nibble(c % 16));
     }
 
     return str;
-- 
2.34.1

Re: [PATCH v3 05/27] util/hexdump: Inline g_string_append_printf "%02x"

[Philippe Mathieu-Daudé]

On 12/4/24 09:33, Richard Henderson wrote:
> Trivial arithmetic can be used for emitting the nibbles,
> rather than full-blown printf formatting.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   util/hexdump.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v3 06/27] hw/mips/malta: Add re-usable rng_seed_hex_new() method

[Richard Henderson]

From: Philippe Mathieu-Daudé <philmd@linaro.org>

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1.

Extract common code from reinitialize_rng_seed and load_kernel
to rng_seed_hex_new.  Using qemu_hexdump_line both fixes the
deprecation warning and simplifies the code base.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
[rth: Use qemu_hexdump_line.]
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/mips/malta.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index af74008c82..664a2ae0a9 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -26,6 +26,7 @@
 #include "qemu/units.h"
 #include "qemu/bitops.h"
 #include "qemu/datadir.h"
+#include "qemu/cutils.h"
 #include "qemu/guest-random.h"
 #include "hw/clock.h"
 #include "hw/southbridge/piix.h"
@@ -850,15 +851,18 @@ static void G_GNUC_PRINTF(3, 4) prom_set(uint32_t *prom_buf, int index,
     va_end(ap);
 }
 
-static void reinitialize_rng_seed(void *opaque)
+static GString *rng_seed_hex_new(void)
 {
-    char *rng_seed_hex = opaque;
     uint8_t rng_seed[32];
 
     qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed));
-    for (size_t i = 0; i < sizeof(rng_seed); ++i) {
-        sprintf(rng_seed_hex + i * 2, "%02x", rng_seed[i]);
-    }
+    return qemu_hexdump_line(NULL, rng_seed, sizeof(rng_seed), 0, 0);
+}
+
+static void reinitialize_rng_seed(void *opaque)
+{
+    g_autoptr(GString) hex = rng_seed_hex_new();
+    memcpy(opaque, hex->str, hex->len);
 }
 
 /* Kernel */
@@ -870,8 +874,6 @@ static uint64_t load_kernel(void)
     uint32_t *prom_buf;
     long prom_size;
     int prom_index = 0;
-    uint8_t rng_seed[32];
-    char rng_seed_hex[sizeof(rng_seed) * 2 + 1];
     size_t rng_seed_prom_offset;
 
     kernel_size = load_elf(loaderparams.kernel_filename, NULL,
@@ -946,14 +948,13 @@ static uint64_t load_kernel(void)
     prom_set(prom_buf, prom_index++, "modetty0");
     prom_set(prom_buf, prom_index++, "38400n8r");
 
-    qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed));
-    for (size_t i = 0; i < sizeof(rng_seed); ++i) {
-        sprintf(rng_seed_hex + i * 2, "%02x", rng_seed[i]);
-    }
     prom_set(prom_buf, prom_index++, "rngseed");
     rng_seed_prom_offset = prom_index * ENVP_ENTRY_SIZE +
                            sizeof(uint32_t) * ENVP_NB_ENTRIES;
-    prom_set(prom_buf, prom_index++, "%s", rng_seed_hex);
+    {
+        g_autoptr(GString) hex = rng_seed_hex_new();
+        prom_set(prom_buf, prom_index++, "%s", hex->str);
+    }
 
     prom_set(prom_buf, prom_index++, NULL);
 
-- 
2.34.1

Re: [PATCH v3 06/27] hw/mips/malta: Add re-usable rng_seed_hex_new() method

[Pierrick Bouvier]

On 4/12/24 00:33, Richard Henderson wrote:
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1.
> 
> Extract common code from reinitialize_rng_seed and load_kernel
> to rng_seed_hex_new.  Using qemu_hexdump_line both fixes the
> deprecation warning and simplifies the code base.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> [rth: Use qemu_hexdump_line.]
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   hw/mips/malta.c | 25 +++++++++++++------------
>   1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/mips/malta.c b/hw/mips/malta.c
> index af74008c82..664a2ae0a9 100644
> --- a/hw/mips/malta.c
> +++ b/hw/mips/malta.c
> @@ -26,6 +26,7 @@
>   #include "qemu/units.h"
>   #include "qemu/bitops.h"
>   #include "qemu/datadir.h"
> +#include "qemu/cutils.h"
>   #include "qemu/guest-random.h"
>   #include "hw/clock.h"
>   #include "hw/southbridge/piix.h"
> @@ -850,15 +851,18 @@ static void G_GNUC_PRINTF(3, 4) prom_set(uint32_t *prom_buf, int index,
>       va_end(ap);
>   }
>   
> -static void reinitialize_rng_seed(void *opaque)
> +static GString *rng_seed_hex_new(void)
>   {
> -    char *rng_seed_hex = opaque;
>       uint8_t rng_seed[32];
>   
>       qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed));
> -    for (size_t i = 0; i < sizeof(rng_seed); ++i) {
> -        sprintf(rng_seed_hex + i * 2, "%02x", rng_seed[i]);
> -    }
> +    return qemu_hexdump_line(NULL, rng_seed, sizeof(rng_seed), 0, 0);
> +}
> +
> +static void reinitialize_rng_seed(void *opaque)
> +{
> +    g_autoptr(GString) hex = rng_seed_hex_new();
> +    memcpy(opaque, hex->str, hex->len);
>   }
>   
>   /* Kernel */
> @@ -870,8 +874,6 @@ static uint64_t load_kernel(void)
>       uint32_t *prom_buf;
>       long prom_size;
>       int prom_index = 0;
> -    uint8_t rng_seed[32];
> -    char rng_seed_hex[sizeof(rng_seed) * 2 + 1];
>       size_t rng_seed_prom_offset;
>   
>       kernel_size = load_elf(loaderparams.kernel_filename, NULL,
> @@ -946,14 +948,13 @@ static uint64_t load_kernel(void)
>       prom_set(prom_buf, prom_index++, "modetty0");
>       prom_set(prom_buf, prom_index++, "38400n8r");
>   
> -    qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed));
> -    for (size_t i = 0; i < sizeof(rng_seed); ++i) {
> -        sprintf(rng_seed_hex + i * 2, "%02x", rng_seed[i]);
> -    }
>       prom_set(prom_buf, prom_index++, "rngseed");
>       rng_seed_prom_offset = prom_index * ENVP_ENTRY_SIZE +
>                              sizeof(uint32_t) * ENVP_NB_ENTRIES;
> -    prom_set(prom_buf, prom_index++, "%s", rng_seed_hex);
> +    {
> +        g_autoptr(GString) hex = rng_seed_hex_new();
> +        prom_set(prom_buf, prom_index++, "%s", hex->str);
> +    }
>   
>       prom_set(prom_buf, prom_index++, NULL);
>   

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

[PATCH v3 07/27] system/qtest: Replace sprintf by qemu_hexdump_line

[Richard Henderson]

From: Philippe Mathieu-Daudé <philmd@linaro.org>

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1.
Using qemu_hexdump_line both fixes the deprecation warning and
simplifies the code base.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>`
[rth: Use qemu_hexdump_line]
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 system/qtest.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/system/qtest.c b/system/qtest.c
index 6da58b3874..507a358f3b 100644
--- a/system/qtest.c
+++ b/system/qtest.c
@@ -601,9 +601,9 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
         qtest_send_prefix(chr);
         qtest_sendf(chr, "OK 0x%016" PRIx64 "\n", value);
     } else if (strcmp(words[0], "read") == 0) {
-        uint64_t addr, len, i;
+        g_autoptr(GString) enc = NULL;
+        uint64_t addr, len;
         uint8_t *data;
-        char *enc;
         int ret;
 
         g_assert(words[1] && words[2]);
@@ -618,16 +618,12 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
         address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
                            len);
 
-        enc = g_malloc(2 * len + 1);
-        for (i = 0; i < len; i++) {
-            sprintf(&enc[i * 2], "%02x", data[i]);
-        }
+        enc = qemu_hexdump_line(NULL, data, len, 0, 0);
 
         qtest_send_prefix(chr);
-        qtest_sendf(chr, "OK 0x%s\n", enc);
+        qtest_sendf(chr, "OK 0x%s\n", enc->str);
 
         g_free(data);
-        g_free(enc);
     } else if (strcmp(words[0], "b64read") == 0) {
         uint64_t addr, len;
         uint8_t *data;
-- 
2.34.1

Re: [PATCH v3 07/27] system/qtest: Replace sprintf by qemu_hexdump_line

[Philippe Mathieu-Daudé]

On 12/4/24 09:33, Richard Henderson wrote:
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1.
> Using qemu_hexdump_line both fixes the deprecation warning and
> simplifies the code base.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>`
> [rth: Use qemu_hexdump_line]
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   system/qtest.c | 12 ++++--------
>   1 file changed, 4 insertions(+), 8 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v3 08/27] hw/scsi/scsi-disk: Use qemu_hexdump_line to avoid sprintf

[Richard Henderson]

From: Philippe Mathieu-Daudé <philmd@linaro.org>

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1.
Using qemu_hexdump_line both fixes the deprecation warning and
simplifies the code base.

Note that this drops the "0x" prefix to every byte, which should
be of no consequence to tracing.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/scsi/scsi-disk.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 4bd7af9d0c..f386a2f01c 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2648,19 +2648,12 @@ static const SCSIReqOps *const scsi_disk_reqops_dispatch[256] = {
 
 static void scsi_disk_new_request_dump(uint32_t lun, uint32_t tag, uint8_t *buf)
 {
-    int i;
     int len = scsi_cdb_length(buf);
-    char *line_buffer, *p;
+    g_autoptr(GString) str = NULL;
 
     assert(len > 0 && len <= 16);
-    line_buffer = g_malloc(len * 5 + 1);
-
-    for (i = 0, p = line_buffer; i < len; i++) {
-        p += sprintf(p, " 0x%02x", buf[i]);
-    }
-    trace_scsi_disk_new_request(lun, tag, line_buffer);
-
-    g_free(line_buffer);
+    str = qemu_hexdump_line(NULL, buf, len, 1, 0);
+    trace_scsi_disk_new_request(lun, tag, str->str);
 }
 
 static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lun,
-- 
2.34.1

Re: [PATCH v3 08/27] hw/scsi/scsi-disk: Use qemu_hexdump_line to avoid sprintf

[Philippe Mathieu-Daudé]

On 12/4/24 09:33, Richard Henderson wrote:
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1.
> Using qemu_hexdump_line both fixes the deprecation warning and
> simplifies the code base.
> 
> Note that this drops the "0x" prefix to every byte, which should
> be of no consequence to tracing.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   hw/scsi/scsi-disk.c | 13 +++----------
>   1 file changed, 3 insertions(+), 10 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v3 09/27] hw/ide/atapi: Use qemu_hexdump_line to avoid sprintf

[Richard Henderson]

From: Philippe Mathieu-Daudé <philmd@linaro.org>

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1.
Using qemu_hexdump_line both fixes the deprecation warning and
simplifies the code base.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/ide/atapi.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 73ec373184..fcb6cca157 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -24,6 +24,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/cutils.h"
 #include "hw/scsi/scsi.h"
 #include "sysemu/block-backend.h"
 #include "scsi/constants.h"
@@ -1309,14 +1310,9 @@ void ide_atapi_cmd(IDEState *s)
     trace_ide_atapi_cmd(s, s->io_buffer[0]);
 
     if (trace_event_get_state_backends(TRACE_IDE_ATAPI_CMD_PACKET)) {
-        /* Each pretty-printed byte needs two bytes and a space; */
-        char *ppacket = g_malloc(ATAPI_PACKET_SIZE * 3 + 1);
-        int i;
-        for (i = 0; i < ATAPI_PACKET_SIZE; i++) {
-            sprintf(ppacket + (i * 3), "%02x ", buf[i]);
-        }
-        trace_ide_atapi_cmd_packet(s, s->lcyl | (s->hcyl << 8), ppacket);
-        g_free(ppacket);
+        g_autoptr(GString) str =
+            qemu_hexdump_line(NULL, buf, ATAPI_PACKET_SIZE, 1, 0);
+        trace_ide_atapi_cmd_packet(s, s->lcyl | (s->hcyl << 8), str->str);
     }
 
     /*
-- 
2.34.1

Re: [PATCH v3 09/27] hw/ide/atapi: Use qemu_hexdump_line to avoid sprintf

[Pierrick Bouvier]

On 4/12/24 00:33, Richard Henderson wrote:
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1.
> Using qemu_hexdump_line both fixes the deprecation warning and
> simplifies the code base.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   hw/ide/atapi.c | 12 ++++--------
>   1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> index 73ec373184..fcb6cca157 100644
> --- a/hw/ide/atapi.c
> +++ b/hw/ide/atapi.c
> @@ -24,6 +24,7 @@
>    */
>   
>   #include "qemu/osdep.h"
> +#include "qemu/cutils.h"
>   #include "hw/scsi/scsi.h"
>   #include "sysemu/block-backend.h"
>   #include "scsi/constants.h"
> @@ -1309,14 +1310,9 @@ void ide_atapi_cmd(IDEState *s)
>       trace_ide_atapi_cmd(s, s->io_buffer[0]);
>   
>       if (trace_event_get_state_backends(TRACE_IDE_ATAPI_CMD_PACKET)) {
> -        /* Each pretty-printed byte needs two bytes and a space; */
> -        char *ppacket = g_malloc(ATAPI_PACKET_SIZE * 3 + 1);
> -        int i;
> -        for (i = 0; i < ATAPI_PACKET_SIZE; i++) {
> -            sprintf(ppacket + (i * 3), "%02x ", buf[i]);
> -        }
> -        trace_ide_atapi_cmd_packet(s, s->lcyl | (s->hcyl << 8), ppacket);
> -        g_free(ppacket);
> +        g_autoptr(GString) str =
> +            qemu_hexdump_line(NULL, buf, ATAPI_PACKET_SIZE, 1, 0);
> +        trace_ide_atapi_cmd_packet(s, s->lcyl | (s->hcyl << 8), str->str);
>       }
>   
>       /*

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

[PATCH v3 10/27] hw/dma/pl330: Use qemu_hexdump_line to avoid sprintf

[Richard Henderson]

From: Philippe Mathieu-Daudé <philmd@linaro.org>

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1.
Using qemu_hexdump_line both fixes the deprecation warning and
simplifies the code base.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/dma/pl330.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/hw/dma/pl330.c b/hw/dma/pl330.c
index 70a502d245..5f89295af3 100644
--- a/hw/dma/pl330.c
+++ b/hw/dma/pl330.c
@@ -15,6 +15,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/cutils.h"
 #include "hw/irq.h"
 #include "hw/qdev-properties.h"
 #include "hw/sysbus.h"
@@ -317,22 +318,14 @@ typedef struct PL330InsnDesc {
 
 static void pl330_hexdump(uint8_t *buf, size_t size)
 {
-    unsigned int b, i, len;
-    char tmpbuf[80];
+    g_autoptr(GString) str = g_string_sized_new(64);
+    size_t b, len;
 
-    for (b = 0; b < size; b += 16) {
-        len = size - b;
-        if (len > 16) {
-            len = 16;
-        }
-        tmpbuf[0] = '\0';
-        for (i = 0; i < len; i++) {
-            if ((i % 4) == 0) {
-                strcat(tmpbuf, " ");
-            }
-            sprintf(tmpbuf + strlen(tmpbuf), " %02x", buf[b + i]);
-        }
-        trace_pl330_hexdump(b, tmpbuf);
+    for (b = 0; b < size; b += len) {
+        len = MIN(16, size - b);
+        g_string_truncate(str, 0);
+        qemu_hexdump_line(str, buf + b, len, 1, 4);
+        trace_pl330_hexdump(b, str->str);
     }
 }
 
-- 
2.34.1

Re: [PATCH v3 10/27] hw/dma/pl330: Use qemu_hexdump_line to avoid sprintf

[Pierrick Bouvier]

On 4/12/24 00:33, Richard Henderson wrote:
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1.
> Using qemu_hexdump_line both fixes the deprecation warning and
> simplifies the code base.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   hw/dma/pl330.c | 23 ++++++++---------------
>   1 file changed, 8 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/dma/pl330.c b/hw/dma/pl330.c
> index 70a502d245..5f89295af3 100644
> --- a/hw/dma/pl330.c
> +++ b/hw/dma/pl330.c
> @@ -15,6 +15,7 @@
>    */
>   
>   #include "qemu/osdep.h"
> +#include "qemu/cutils.h"
>   #include "hw/irq.h"
>   #include "hw/qdev-properties.h"
>   #include "hw/sysbus.h"
> @@ -317,22 +318,14 @@ typedef struct PL330InsnDesc {
>   
>   static void pl330_hexdump(uint8_t *buf, size_t size)
>   {
> -    unsigned int b, i, len;
> -    char tmpbuf[80];
> +    g_autoptr(GString) str = g_string_sized_new(64);
> +    size_t b, len;
>   
> -    for (b = 0; b < size; b += 16) {
> -        len = size - b;
> -        if (len > 16) {
> -            len = 16;
> -        }
> -        tmpbuf[0] = '\0';
> -        for (i = 0; i < len; i++) {
> -            if ((i % 4) == 0) {
> -                strcat(tmpbuf, " ");
> -            }
> -            sprintf(tmpbuf + strlen(tmpbuf), " %02x", buf[b + i]);
> -        }
> -        trace_pl330_hexdump(b, tmpbuf);
> +    for (b = 0; b < size; b += len) {
> +        len = MIN(16, size - b);
> +        g_string_truncate(str, 0);
> +        qemu_hexdump_line(str, buf + b, len, 1, 4);
> +        trace_pl330_hexdump(b, str->str);
>       }
>   }
>   

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

[PATCH v3 11/27] backends/tpm: Use qemu_hexdump_line to avoid sprintf

[Richard Henderson]

From: Philippe Mathieu-Daudé <philmd@linaro.org>

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1.
Using qemu_hexdump_line both fixes the deprecation warning and
simplifies the code base.

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
[rth: Keep the linebreaks every 16 bytes]
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 backends/tpm/tpm_util.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/backends/tpm/tpm_util.c b/backends/tpm/tpm_util.c
index 1856589c3b..bf3378fbbe 100644
--- a/backends/tpm/tpm_util.c
+++ b/backends/tpm/tpm_util.c
@@ -21,6 +21,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu/error-report.h"
+#include "qemu/cutils.h"
 #include "qapi/error.h"
 #include "qapi/visitor.h"
 #include "tpm_int.h"
@@ -336,27 +337,22 @@ void tpm_sized_buffer_reset(TPMSizedBuffer *tsb)
 void tpm_util_show_buffer(const unsigned char *buffer,
                           size_t buffer_size, const char *string)
 {
-    size_t len, i;
-    char *line_buffer, *p;
+    g_autoptr(GString) str = NULL;
+    size_t len, i, l;
 
     if (!trace_event_get_state_backends(TRACE_TPM_UTIL_SHOW_BUFFER)) {
         return;
     }
     len = MIN(tpm_cmd_get_size(buffer), buffer_size);
 
-    /*
-     * allocate enough room for 3 chars per buffer entry plus a
-     * newline after every 16 chars and a final null terminator.
-     */
-    line_buffer = g_malloc(len * 3 + (len / 16) + 1);
-
-    for (i = 0, p = line_buffer; i < len; i++) {
-        if (i && !(i % 16)) {
-            p += sprintf(p, "\n");
+    for (i = 0; i < len; i += l) {
+        if (str) {
+            g_string_append_c(str, '\n');
         }
-        p += sprintf(p, "%.2X ", buffer[i]);
+        l = MIN(len, 16);
+        str = qemu_hexdump_line(str, buffer, l, 1, 0);
     }
-    trace_tpm_util_show_buffer(string, len, line_buffer);
 
-    g_free(line_buffer);
+    g_string_ascii_up(str);
+    trace_tpm_util_show_buffer(string, len, str->str);
 }
-- 
2.34.1

Re: [PATCH v3 11/27] backends/tpm: Use qemu_hexdump_line to avoid sprintf

[Philippe Mathieu-Daudé]

On 12/4/24 09:33, Richard Henderson wrote:
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1.
> Using qemu_hexdump_line both fixes the deprecation warning and
> simplifies the code base.
> 
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> [rth: Keep the linebreaks every 16 bytes]
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   backends/tpm/tpm_util.c | 24 ++++++++++--------------
>   1 file changed, 10 insertions(+), 14 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v3 12/27] disas/m68k: Replace sprintf() by snprintf()

[Richard Henderson]

From: Philippe Mathieu-Daudé <philmd@linaro.org>

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience. Use snprintf() instead.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Message-Id: <20240411104340.6617-2-philmd@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 disas/m68k.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/disas/m68k.c b/disas/m68k.c
index 1f16e295ab..800b4145ac 100644
--- a/disas/m68k.c
+++ b/disas/m68k.c
@@ -1000,7 +1000,7 @@ print_indexed (int basereg,
 
   /* Generate the text for the index register.
      Where this will be output is not yet determined.  */
-  sprintf (buf, "%s:%c%s",
+  snprintf(buf, sizeof(buf), "%s:%c%s",
 	   reg_names[(word >> 12) & 0xf],
 	   (word & 0x800) ? 'l' : 'w',
 	   scales[(word >> 9) & 3]);
-- 
2.34.1

[PATCH v3 13/27] disas/microblaze: Replace sprintf() by snprintf()

[Richard Henderson]

From: Philippe Mathieu-Daudé <philmd@linaro.org>

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience. Use snprintf() instead.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
Message-Id: <20240411104340.6617-3-philmd@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 disas/microblaze.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/disas/microblaze.c b/disas/microblaze.c
index 0b89b9c4fa..49a4c0fd40 100644
--- a/disas/microblaze.c
+++ b/disas/microblaze.c
@@ -600,7 +600,8 @@ static char *
 get_field (long instr, long mask, unsigned short low)
 {
   char tmpstr[25];
-  sprintf(tmpstr, "%s%d", register_prefix, (int)((instr & mask) >> low));
+  snprintf(tmpstr, sizeof(tmpstr), "%s%d", register_prefix,
+           (int)((instr & mask) >> low));
   return(strdup(tmpstr));
 }
 
@@ -608,7 +609,8 @@ static char *
 get_field_imm (long instr)
 {
   char tmpstr[25];
-  sprintf(tmpstr, "%d", (short)((instr & IMM_MASK) >> IMM_LOW));
+  snprintf(tmpstr, sizeof(tmpstr), "%d",
+           (short)((instr & IMM_MASK) >> IMM_LOW));
   return(strdup(tmpstr));
 }
 
@@ -616,7 +618,8 @@ static char *
 get_field_imm5 (long instr)
 {
   char tmpstr[25];
-  sprintf(tmpstr, "%d", (short)((instr & IMM5_MASK) >> IMM_LOW));
+  snprintf(tmpstr, sizeof(tmpstr), "%d",
+           (short)((instr & IMM5_MASK) >> IMM_LOW));
   return(strdup(tmpstr));
 }
 
@@ -624,7 +627,8 @@ static char *
 get_field_rfsl (long instr)
 {
   char tmpstr[25];
-  sprintf(tmpstr, "%s%d", fsl_register_prefix, (short)((instr & RFSL_MASK) >> IMM_LOW));
+  snprintf(tmpstr, sizeof(tmpstr), "%s%d", fsl_register_prefix,
+           (short)((instr & RFSL_MASK) >> IMM_LOW));
   return(strdup(tmpstr));
 }
 
@@ -632,7 +636,8 @@ static char *
 get_field_imm15 (long instr)
 {
   char tmpstr[25];
-  sprintf(tmpstr, "%d", (short)((instr & IMM15_MASK) >> IMM_LOW));
+  snprintf(tmpstr, sizeof(tmpstr), "%d",
+           (short)((instr & IMM15_MASK) >> IMM_LOW));
   return(strdup(tmpstr));
 }
 
@@ -641,7 +646,8 @@ static char *
 get_field_unsigned_imm (long instr)
 {
   char tmpstr[25];
-  sprintf(tmpstr, "%d", (int)((instr & IMM_MASK) >> IMM_LOW));
+  snprintf(tmpstr, sizeof(tmpstr), "%d",
+           (int)((instr & IMM_MASK) >> IMM_LOW));
   return(strdup(tmpstr));
 }
 #endif
@@ -653,7 +659,8 @@ get_field_unsigned_imm (long instr)
   {
   char tmpstr[25];
   
-  sprintf(tmpstr, "%s%s", register_prefix, (((instr & IMM_MASK) >> IMM_LOW) & REG_MSR_MASK) == 0 ? "pc" : "msr");
+  snprintf(tmpstr, sizeof(tmpstr), "%s%s", register_prefix,
+          (((instr & IMM_MASK) >> IMM_LOW) & REG_MSR_MASK) == 0 ? "pc" : "msr");
   
   return(strdup(tmpstr));
   }
@@ -709,7 +716,7 @@ get_field_special(long instr, const struct op_code_struct *op)
    default :
      {
        if ( ((((instr & IMM_MASK) >> IMM_LOW) ^ op->immval_mask) & 0xE000) == REG_PVR_MASK) {
-	  sprintf(tmpstr, "%s%u", pvr_register_prefix,
+          snprintf(tmpstr, sizeof(tmpstr), "%s%u", pvr_register_prefix,
                  (unsigned short)(((instr & IMM_MASK) >> IMM_LOW) ^
                                   op->immval_mask) ^ REG_PVR_MASK);
 	 return(strdup(tmpstr));
@@ -720,7 +727,7 @@ get_field_special(long instr, const struct op_code_struct *op)
      break;
    }
    
-   sprintf(tmpstr, "%s%s", register_prefix, spr);
+   snprintf(tmpstr, sizeof(tmpstr), "%s%s", register_prefix, spr);
    return(strdup(tmpstr));
 }
 
-- 
2.34.1

[PATCH v3 14/27] disas/microblaze: Split out print_immval_addr

[Richard Henderson]

Unify the code blocks that try to print a symbolic address.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 disas/microblaze.c | 89 +++++++++++++++-------------------------------
 1 file changed, 29 insertions(+), 60 deletions(-)

diff --git a/disas/microblaze.c b/disas/microblaze.c
index 49a4c0fd40..3473c94164 100644
--- a/disas/microblaze.c
+++ b/disas/microblaze.c
@@ -767,6 +767,24 @@ read_insn_microblaze (bfd_vma memaddr,
   return inst;
 }
 
+static void print_immval_addr(struct disassemble_info *info, bool immfound,
+                              int immval, unsigned inst, int addend)
+{
+    if (info->print_address_func && info->symbol_at_address_func) {
+        if (immfound) {
+            immval |= get_int_field_imm(inst) & 0x0000ffff;
+        } else {
+            immval = (int16_t)get_int_field_imm(inst);
+        }
+        immval += addend;
+        if (immval != 0 && info->symbol_at_address_func(immval, info)) {
+            info->fprintf_func(info->stream, "\t// ");
+            info->print_address_func (immval, info);
+        } else if (addend) {
+            info->fprintf_func(info->stream, "\t// %x", immval);
+        }
+    }
+}
 
 int 
 print_insn_microblaze (bfd_vma memaddr, struct disassemble_info * info)
@@ -821,18 +839,8 @@ print_insn_microblaze (bfd_vma memaddr, struct disassemble_info * info)
      break;
         case INST_TYPE_RD_R1_IMM:
 	  fprintf_func(stream, "\t%s, %s, %s", get_field_rd(inst), get_field_r1(inst), get_field_imm(inst));
-	  if (info->print_address_func && get_int_field_r1(inst) == 0 && info->symbol_at_address_func) {
-	    if (immfound)
-	      immval |= (get_int_field_imm(inst) & 0x0000ffff);
-	    else {
-	      immval = get_int_field_imm(inst);
-	      if (immval & 0x8000)
-		immval |= 0xFFFF0000;
-	    }
-	    if (immval > 0 && info->symbol_at_address_func(immval, info)) {
-	      fprintf_func (stream, "\t// ");
-	      info->print_address_func (immval, info);
-	    }
+	  if (get_int_field_r1(inst) == 0) {
+              print_immval_addr(info, immfound, immval, inst, 0);
 	  }
 	  break;
 	case INST_TYPE_RD_R1_IMM5:
@@ -860,61 +868,22 @@ print_insn_microblaze (bfd_vma memaddr, struct disassemble_info * info)
 	  fprintf_func(stream, "\t%s, %s", get_field_r1(inst), get_field_imm(inst));
 	  /* The non-pc relative instructions are returns, which shouldn't 
 	     have a label printed */
-	  if (info->print_address_func && op->inst_offset_type == INST_PC_OFFSET && info->symbol_at_address_func) {
-	    if (immfound)
-	      immval |= (get_int_field_imm(inst) & 0x0000ffff);
-	    else {
-	      immval = get_int_field_imm(inst);
-	      if (immval & 0x8000)
-		immval |= 0xFFFF0000;
-	    }
-	    immval += memaddr;
-	    if (immval > 0 && info->symbol_at_address_func(immval, info)) {
-	      fprintf_func (stream, "\t// ");
-	      info->print_address_func (immval, info);
-	    } else {
-	      fprintf_func (stream, "\t\t// ");
-	      fprintf_func (stream, "%x", immval);
-	    }
+	  if (op->inst_offset_type == INST_PC_OFFSET) {
+              print_immval_addr(info, immfound, immval, inst, memaddr);
 	  }
 	  break;
         case INST_TYPE_RD_IMM:
 	  fprintf_func(stream, "\t%s, %s", get_field_rd(inst), get_field_imm(inst));
-	  if (info->print_address_func && info->symbol_at_address_func) {
-	    if (immfound)
-	      immval |= (get_int_field_imm(inst) & 0x0000ffff);
-	    else {
-	      immval = get_int_field_imm(inst);
-	      if (immval & 0x8000)
-		immval |= 0xFFFF0000;
-	    }
-	    if (op->inst_offset_type == INST_PC_OFFSET)
-	      immval += (int) memaddr;
-	    if (info->symbol_at_address_func(immval, info)) {
-	      fprintf_func (stream, "\t// ");
-	      info->print_address_func (immval, info);
-	    } 
-	  }
+          print_immval_addr(info, immfound, immval, inst,
+                            op->inst_offset_type == INST_PC_OFFSET
+                            ? memaddr : 0);
 	  break;
         case INST_TYPE_IMM:
 	  fprintf_func(stream, "\t%s", get_field_imm(inst));
-	  if (info->print_address_func && info->symbol_at_address_func && op->instr != imm) {
-	    if (immfound)
-	      immval |= (get_int_field_imm(inst) & 0x0000ffff);
-	    else {
-	      immval = get_int_field_imm(inst);
-	      if (immval & 0x8000)
-		immval |= 0xFFFF0000;
-	    }
-	    if (op->inst_offset_type == INST_PC_OFFSET)
-	      immval += (int) memaddr;
-	    if (immval > 0 && info->symbol_at_address_func(immval, info)) {
-	      fprintf_func (stream, "\t// ");
-	      info->print_address_func (immval, info);
-	    } else if (op->inst_offset_type == INST_PC_OFFSET) {
-	      fprintf_func (stream, "\t\t// ");
-	      fprintf_func (stream, "%x", immval);
-	    }
+	  if (op->instr != imm) {
+              print_immval_addr(info, immfound, immval, inst,
+                                op->inst_offset_type == INST_PC_OFFSET
+                                ? memaddr : 0);
 	  }
 	  break;
         case INST_TYPE_RD_R2:
-- 
2.34.1

Re: [PATCH v3 14/27] disas/microblaze: Split out print_immval_addr

[Pierrick Bouvier]

On 4/12/24 00:33, Richard Henderson wrote:
> Unify the code blocks that try to print a symbolic address.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   disas/microblaze.c | 89 +++++++++++++++-------------------------------
>   1 file changed, 29 insertions(+), 60 deletions(-)
> 
> diff --git a/disas/microblaze.c b/disas/microblaze.c
> index 49a4c0fd40..3473c94164 100644
> --- a/disas/microblaze.c
> +++ b/disas/microblaze.c
> @@ -767,6 +767,24 @@ read_insn_microblaze (bfd_vma memaddr,
>     return inst;
>   }
>   
> +static void print_immval_addr(struct disassemble_info *info, bool immfound,
> +                              int immval, unsigned inst, int addend)
> +{
> +    if (info->print_address_func && info->symbol_at_address_func) {
> +        if (immfound) {
> +            immval |= get_int_field_imm(inst) & 0x0000ffff;
> +        } else {
> +            immval = (int16_t)get_int_field_imm(inst);
> +        }
> +        immval += addend;
> +        if (immval != 0 && info->symbol_at_address_func(immval, info)) {
> +            info->fprintf_func(info->stream, "\t// ");
> +            info->print_address_func (immval, info);
> +        } else if (addend) {
> +            info->fprintf_func(info->stream, "\t// %x", immval);
> +        }
> +    }
> +}
>   
>   int
>   print_insn_microblaze (bfd_vma memaddr, struct disassemble_info * info)
> @@ -821,18 +839,8 @@ print_insn_microblaze (bfd_vma memaddr, struct disassemble_info * info)
>        break;
>           case INST_TYPE_RD_R1_IMM:
>   	  fprintf_func(stream, "\t%s, %s, %s", get_field_rd(inst), get_field_r1(inst), get_field_imm(inst));
> -	  if (info->print_address_func && get_int_field_r1(inst) == 0 && info->symbol_at_address_func) {
> -	    if (immfound)
> -	      immval |= (get_int_field_imm(inst) & 0x0000ffff);
> -	    else {
> -	      immval = get_int_field_imm(inst);
> -	      if (immval & 0x8000)
> -		immval |= 0xFFFF0000;
> -	    }
> -	    if (immval > 0 && info->symbol_at_address_func(immval, info)) {
> -	      fprintf_func (stream, "\t// ");
> -	      info->print_address_func (immval, info);
> -	    }
> +	  if (get_int_field_r1(inst) == 0) {
> +              print_immval_addr(info, immfound, immval, inst, 0);
>   	  }
>   	  break;
>   	case INST_TYPE_RD_R1_IMM5:
> @@ -860,61 +868,22 @@ print_insn_microblaze (bfd_vma memaddr, struct disassemble_info * info)
>   	  fprintf_func(stream, "\t%s, %s", get_field_r1(inst), get_field_imm(inst));
>   	  /* The non-pc relative instructions are returns, which shouldn't
>   	     have a label printed */
> -	  if (info->print_address_func && op->inst_offset_type == INST_PC_OFFSET && info->symbol_at_address_func) {
> -	    if (immfound)
> -	      immval |= (get_int_field_imm(inst) & 0x0000ffff);
> -	    else {
> -	      immval = get_int_field_imm(inst);
> -	      if (immval & 0x8000)
> -		immval |= 0xFFFF0000;
> -	    }
> -	    immval += memaddr;
> -	    if (immval > 0 && info->symbol_at_address_func(immval, info)) {
> -	      fprintf_func (stream, "\t// ");
> -	      info->print_address_func (immval, info);
> -	    } else {
> -	      fprintf_func (stream, "\t\t// ");
> -	      fprintf_func (stream, "%x", immval);
> -	    }
> +	  if (op->inst_offset_type == INST_PC_OFFSET) {
> +              print_immval_addr(info, immfound, immval, inst, memaddr);
>   	  }
>   	  break;
>           case INST_TYPE_RD_IMM:
>   	  fprintf_func(stream, "\t%s, %s", get_field_rd(inst), get_field_imm(inst));
> -	  if (info->print_address_func && info->symbol_at_address_func) {
> -	    if (immfound)
> -	      immval |= (get_int_field_imm(inst) & 0x0000ffff);
> -	    else {
> -	      immval = get_int_field_imm(inst);
> -	      if (immval & 0x8000)
> -		immval |= 0xFFFF0000;
> -	    }
> -	    if (op->inst_offset_type == INST_PC_OFFSET)
> -	      immval += (int) memaddr;
> -	    if (info->symbol_at_address_func(immval, info)) {
> -	      fprintf_func (stream, "\t// ");
> -	      info->print_address_func (immval, info);
> -	    }
> -	  }
> +          print_immval_addr(info, immfound, immval, inst,
> +                            op->inst_offset_type == INST_PC_OFFSET
> +                            ? memaddr : 0);
>   	  break;
>           case INST_TYPE_IMM:
>   	  fprintf_func(stream, "\t%s", get_field_imm(inst));
> -	  if (info->print_address_func && info->symbol_at_address_func && op->instr != imm) {
> -	    if (immfound)
> -	      immval |= (get_int_field_imm(inst) & 0x0000ffff);
> -	    else {
> -	      immval = get_int_field_imm(inst);
> -	      if (immval & 0x8000)
> -		immval |= 0xFFFF0000;
> -	    }
> -	    if (op->inst_offset_type == INST_PC_OFFSET)
> -	      immval += (int) memaddr;
> -	    if (immval > 0 && info->symbol_at_address_func(immval, info)) {
> -	      fprintf_func (stream, "\t// ");
> -	      info->print_address_func (immval, info);
> -	    } else if (op->inst_offset_type == INST_PC_OFFSET) {
> -	      fprintf_func (stream, "\t\t// ");
> -	      fprintf_func (stream, "%x", immval);
> -	    }
> +	  if (op->instr != imm) {
> +              print_immval_addr(info, immfound, immval, inst,
> +                                op->inst_offset_type == INST_PC_OFFSET
> +                                ? memaddr : 0);
>   	  }
>   	  break;
>           case INST_TYPE_RD_R2:

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

[PATCH v3 15/27] target/microblaze: Re-indent print_insn_microblaze

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 disas/microblaze.c | 263 ++++++++++++++++++++++++---------------------
 1 file changed, 141 insertions(+), 122 deletions(-)

diff --git a/disas/microblaze.c b/disas/microblaze.c
index 3473c94164..c729c76585 100644
--- a/disas/microblaze.c
+++ b/disas/microblaze.c
@@ -787,134 +787,153 @@ static void print_immval_addr(struct disassemble_info *info, bool immfound,
 }
 
 int 
-print_insn_microblaze (bfd_vma memaddr, struct disassemble_info * info)
+print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
 {
-  fprintf_function    fprintf_func = info->fprintf_func;
-  void *              stream = info->stream;
-  unsigned long       inst, prev_inst;
-  const struct op_code_struct *op, *pop;
-  int                 immval = 0;
-  bfd_boolean         immfound = FALSE;
-  static bfd_vma prev_insn_addr = -1; /*init the prev insn addr */
-  static int     prev_insn_vma = -1;  /*init the prev insn vma */
-  int            curr_insn_vma = info->buffer_vma;
+    fprintf_function fprintf_func = info->fprintf_func;
+    void *stream = info->stream;
+    unsigned long inst, prev_inst;
+    const struct op_code_struct *op, *pop;
+    int immval = 0;
+    bool immfound = false;
+    static bfd_vma prev_insn_addr = -1; /*init the prev insn addr */
+    static int prev_insn_vma = -1;  /*init the prev insn vma */
+    int curr_insn_vma = info->buffer_vma;
 
-  info->bytes_per_chunk = 4;
+    info->bytes_per_chunk = 4;
 
-  inst = read_insn_microblaze (memaddr, info, &op);
-  if (inst == 0) {
-    return -1;
-  }
+    inst = read_insn_microblaze (memaddr, info, &op);
+    if (inst == 0) {
+        return -1;
+    }
   
-  if (prev_insn_vma == curr_insn_vma) {
-  if (memaddr-(info->bytes_per_chunk) == prev_insn_addr) {
-    prev_inst = read_insn_microblaze (prev_insn_addr, info, &pop);
-    if (prev_inst == 0)
-      return -1;
-    if (pop->instr == imm) {
-      immval = (get_int_field_imm(prev_inst) << 16) & 0xffff0000;
-      immfound = TRUE;
+    if (prev_insn_vma == curr_insn_vma) {
+        if (memaddr - info->bytes_per_chunk == prev_insn_addr) {
+            prev_inst = read_insn_microblaze (prev_insn_addr, info, &pop);
+            if (prev_inst == 0)
+                return -1;
+            if (pop->instr == imm) {
+                immval = (get_int_field_imm(prev_inst) << 16) & 0xffff0000;
+                immfound = TRUE;
+            }
+            else {
+                immval = 0;
+                immfound = FALSE;
+            }
+        }
     }
-    else {
-      immval = 0;
-      immfound = FALSE;
-    }
-  }
-  }
-  /* make curr insn as prev insn */
-  prev_insn_addr = memaddr;
-  prev_insn_vma = curr_insn_vma;
+    /* make curr insn as prev insn */
+    prev_insn_addr = memaddr;
+    prev_insn_vma = curr_insn_vma;
 
-  if (op->name == 0) {
-    fprintf_func (stream, ".short 0x%04lx", inst);
-  }
-  else
-    {
-      fprintf_func (stream, "%s", op->name);
+    if (op->name == 0) {
+        fprintf_func (stream, ".short 0x%04lx", inst);
+        return 4;
+    }
+
+    fprintf_func (stream, "%s", op->name);
       
-      switch (op->inst_type)
-	{
-  case INST_TYPE_RD_R1_R2:
-     fprintf_func(stream, "\t%s, %s, %s", get_field_rd(inst), get_field_r1(inst), get_field_r2(inst));
-     break;
-        case INST_TYPE_RD_R1_IMM:
-	  fprintf_func(stream, "\t%s, %s, %s", get_field_rd(inst), get_field_r1(inst), get_field_imm(inst));
-	  if (get_int_field_r1(inst) == 0) {
-              print_immval_addr(info, immfound, immval, inst, 0);
-	  }
-	  break;
-	case INST_TYPE_RD_R1_IMM5:
-	  fprintf_func(stream, "\t%s, %s, %s", get_field_rd(inst), get_field_r1(inst), get_field_imm5(inst));
-	  break;
-	case INST_TYPE_RD_RFSL:
-	  fprintf_func(stream, "\t%s, %s", get_field_rd(inst), get_field_rfsl(inst));
-	  break;
-	case INST_TYPE_R1_RFSL:
-	  fprintf_func(stream, "\t%s, %s", get_field_r1(inst), get_field_rfsl(inst));
-	  break;
-	case INST_TYPE_RD_SPECIAL:
-	  fprintf_func(stream, "\t%s, %s", get_field_rd(inst), get_field_special(inst, op));
-	  break;
-	case INST_TYPE_SPECIAL_R1:
-	  fprintf_func(stream, "\t%s, %s", get_field_special(inst, op), get_field_r1(inst));
-	  break;
-	case INST_TYPE_RD_R1:
-	  fprintf_func(stream, "\t%s, %s", get_field_rd(inst), get_field_r1(inst));
-	  break;
-	case INST_TYPE_R1_R2:
-	  fprintf_func(stream, "\t%s, %s", get_field_r1(inst), get_field_r2(inst));
-	  break;
-	case INST_TYPE_R1_IMM:
-	  fprintf_func(stream, "\t%s, %s", get_field_r1(inst), get_field_imm(inst));
-	  /* The non-pc relative instructions are returns, which shouldn't 
-	     have a label printed */
-	  if (op->inst_offset_type == INST_PC_OFFSET) {
-              print_immval_addr(info, immfound, immval, inst, memaddr);
-	  }
-	  break;
-        case INST_TYPE_RD_IMM:
-	  fprintf_func(stream, "\t%s, %s", get_field_rd(inst), get_field_imm(inst));
-          print_immval_addr(info, immfound, immval, inst,
-                            op->inst_offset_type == INST_PC_OFFSET
-                            ? memaddr : 0);
-	  break;
-        case INST_TYPE_IMM:
-	  fprintf_func(stream, "\t%s", get_field_imm(inst));
-	  if (op->instr != imm) {
-              print_immval_addr(info, immfound, immval, inst,
-                                op->inst_offset_type == INST_PC_OFFSET
-                                ? memaddr : 0);
-	  }
-	  break;
-        case INST_TYPE_RD_R2:
-	  fprintf_func(stream, "\t%s, %s", get_field_rd(inst), get_field_r2(inst));
-	  break;
-  case INST_TYPE_R2:
-     fprintf_func(stream, "\t%s", get_field_r2(inst));
-     break;
-  case INST_TYPE_R1:
-     fprintf_func(stream, "\t%s", get_field_r1(inst));
-     break;
-  case INST_TYPE_RD_R1_SPECIAL:
-     fprintf_func(stream, "\t%s, %s", get_field_rd(inst), get_field_r2(inst));
-     break;
-  case INST_TYPE_RD_IMM15:
-     fprintf_func(stream, "\t%s, %s", get_field_rd(inst), get_field_imm15(inst));
-     break;
-     /* For tuqula instruction */
-  case INST_TYPE_RD:
-     fprintf_func(stream, "\t%s", get_field_rd(inst));
-     break;
-  case INST_TYPE_RFSL:
-     fprintf_func(stream, "\t%s", get_field_rfsl(inst));
-     break;
-  default:
-	  /* if the disassembler lags the instruction set */
-	  fprintf_func (stream, "\tundecoded operands, inst is 0x%04lx", inst);
-	  break;
-	}
+    switch (op->inst_type) {
+    case INST_TYPE_RD_R1_R2:
+        fprintf_func(stream, "\t%s, %s, %s",
+                     get_field_rd(inst), get_field_r1(inst),
+                     get_field_r2(inst));
+        break;
+    case INST_TYPE_RD_R1_IMM:
+        fprintf_func(stream, "\t%s, %s, %s",
+                     get_field_rd(inst), get_field_r1(inst),
+                     get_field_imm(inst));
+        if (get_int_field_r1(inst) == 0) {
+            print_immval_addr(info, immfound, immval, inst, 0);
+        }
+        break;
+    case INST_TYPE_RD_R1_IMM5:
+        fprintf_func(stream, "\t%s, %s, %s",
+                     get_field_rd(inst), get_field_r1(inst),
+                     get_field_imm5(inst));
+        break;
+    case INST_TYPE_RD_RFSL:
+        fprintf_func(stream, "\t%s, %s",
+                     get_field_rd(inst), get_field_rfsl(inst));
+        break;
+    case INST_TYPE_R1_RFSL:
+        fprintf_func(stream, "\t%s, %s",
+                     get_field_r1(inst), get_field_rfsl(inst));
+        break;
+    case INST_TYPE_RD_SPECIAL:
+        fprintf_func(stream, "\t%s, %s",
+                     get_field_rd(inst), get_field_special(inst, op));
+        break;
+    case INST_TYPE_SPECIAL_R1:
+        fprintf_func(stream, "\t%s, %s",
+                     get_field_special(inst, op), get_field_r1(inst));
+        break;
+    case INST_TYPE_RD_R1:
+        fprintf_func(stream, "\t%s, %s",
+                     get_field_rd(inst), get_field_r1(inst));
+        break;
+    case INST_TYPE_R1_R2:
+        fprintf_func(stream, "\t%s, %s",
+                     get_field_r1(inst), get_field_r2(inst));
+        break;
+    case INST_TYPE_R1_IMM:
+        fprintf_func(stream, "\t%s, %s",
+                     get_field_r1(inst), get_field_imm(inst));
+        /*
+         * The non-pc relative instructions are returns,
+         * which shouldn't have a label printed.
+         */
+        if (op->inst_offset_type == INST_PC_OFFSET) {
+            print_immval_addr(info, immfound, immval, inst, memaddr);
+        }
+        break;
+    case INST_TYPE_RD_IMM:
+        fprintf_func(stream, "\t%s, %s",
+                     get_field_rd(inst), get_field_imm(inst));
+        print_immval_addr(info, immfound, immval, inst,
+                          op->inst_offset_type == INST_PC_OFFSET
+                          ? memaddr : 0);
+        break;
+    case INST_TYPE_IMM:
+        fprintf_func(stream, "\t%s", get_field_imm(inst));
+        if (op->instr != imm) {
+            print_immval_addr(info, immfound, immval, inst,
+                              op->inst_offset_type == INST_PC_OFFSET
+                              ? memaddr : 0);
+        }
+        break;
+    case INST_TYPE_RD_R2:
+        fprintf_func(stream, "\t%s, %s",
+                     get_field_rd(inst), get_field_r2(inst));
+        break;
+    case INST_TYPE_R2:
+        fprintf_func(stream, "\t%s",
+                     get_field_r2(inst));
+        break;
+    case INST_TYPE_R1:
+        fprintf_func(stream, "\t%s",
+                     get_field_r1(inst));
+        break;
+    case INST_TYPE_RD_R1_SPECIAL:
+        fprintf_func(stream, "\t%s, %s",
+                     get_field_rd(inst), get_field_r2(inst));
+        break;
+    case INST_TYPE_RD_IMM15:
+        fprintf_func(stream, "\t%s, %s",
+                     get_field_rd(inst), get_field_imm15(inst));
+        break;
+        /* For tuqula instruction */
+    case INST_TYPE_RD:
+        fprintf_func(stream, "\t%s",
+                     get_field_rd(inst));
+        break;
+    case INST_TYPE_RFSL:
+        fprintf_func(stream, "\t%s",
+                     get_field_rfsl(inst));
+        break;
+    default:
+        /* if the disassembler lags the instruction set */
+        fprintf_func(stream, "\tundecoded operands, inst is 0x%04lx", inst);
+        break;
     }
-  
-  /* Say how many bytes we consumed? */
-  return 4;
+    return 4;
 }
-- 
2.34.1

Re: [PATCH v3 15/27] target/microblaze: Re-indent print_insn_microblaze

[Pierrick Bouvier]

On 4/12/24 00:33, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   disas/microblaze.c | 263 ++++++++++++++++++++++++---------------------
>   1 file changed, 141 insertions(+), 122 deletions(-)
> 
> diff --git a/disas/microblaze.c b/disas/microblaze.c
> index 3473c94164..c729c76585 100644
> --- a/disas/microblaze.c
> +++ b/disas/microblaze.c
> @@ -787,134 +787,153 @@ static void print_immval_addr(struct disassemble_info *info, bool immfound,
>   }
>   
>   int
> -print_insn_microblaze (bfd_vma memaddr, struct disassemble_info * info)
> +print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
>   {
> -  fprintf_function    fprintf_func = info->fprintf_func;
> -  void *              stream = info->stream;
> -  unsigned long       inst, prev_inst;
> -  const struct op_code_struct *op, *pop;
> -  int                 immval = 0;
> -  bfd_boolean         immfound = FALSE;
> -  static bfd_vma prev_insn_addr = -1; /*init the prev insn addr */
> -  static int     prev_insn_vma = -1;  /*init the prev insn vma */
> -  int            curr_insn_vma = info->buffer_vma;
> +    fprintf_function fprintf_func = info->fprintf_func;
> +    void *stream = info->stream;
> +    unsigned long inst, prev_inst;
> +    const struct op_code_struct *op, *pop;
> +    int immval = 0;
> +    bool immfound = false;
> +    static bfd_vma prev_insn_addr = -1; /*init the prev insn addr */
> +    static int prev_insn_vma = -1;  /*init the prev insn vma */
> +    int curr_insn_vma = info->buffer_vma;
>   
> -  info->bytes_per_chunk = 4;
> +    info->bytes_per_chunk = 4;
>   
> -  inst = read_insn_microblaze (memaddr, info, &op);
> -  if (inst == 0) {
> -    return -1;
> -  }
> +    inst = read_insn_microblaze (memaddr, info, &op);
> +    if (inst == 0) {
> +        return -1;
> +    }
>     
> -  if (prev_insn_vma == curr_insn_vma) {
> -  if (memaddr-(info->bytes_per_chunk) == prev_insn_addr) {
> -    prev_inst = read_insn_microblaze (prev_insn_addr, info, &pop);
> -    if (prev_inst == 0)
> -      return -1;
> -    if (pop->instr == imm) {
> -      immval = (get_int_field_imm(prev_inst) << 16) & 0xffff0000;
> -      immfound = TRUE;
> +    if (prev_insn_vma == curr_insn_vma) {
> +        if (memaddr - info->bytes_per_chunk == prev_insn_addr) {
> +            prev_inst = read_insn_microblaze (prev_insn_addr, info, &pop);
> +            if (prev_inst == 0)
> +                return -1;
> +            if (pop->instr == imm) {
> +                immval = (get_int_field_imm(prev_inst) << 16) & 0xffff0000;
> +                immfound = TRUE;
> +            }
> +            else {
> +                immval = 0;
> +                immfound = FALSE;
> +            }
> +        }
>       }
> -    else {
> -      immval = 0;
> -      immfound = FALSE;
> -    }
> -  }
> -  }
> -  /* make curr insn as prev insn */
> -  prev_insn_addr = memaddr;
> -  prev_insn_vma = curr_insn_vma;
> +    /* make curr insn as prev insn */
> +    prev_insn_addr = memaddr;
> +    prev_insn_vma = curr_insn_vma;
>   
> -  if (op->name == 0) {
> -    fprintf_func (stream, ".short 0x%04lx", inst);
> -  }
> -  else
> -    {
> -      fprintf_func (stream, "%s", op->name);
> +    if (op->name == 0) {
> +        fprintf_func (stream, ".short 0x%04lx", inst);
> +        return 4;
> +    }
> +
> +    fprintf_func (stream, "%s", op->name);
>         
> -      switch (op->inst_type)
> -	{
> -  case INST_TYPE_RD_R1_R2:
> -     fprintf_func(stream, "\t%s, %s, %s", get_field_rd(inst), get_field_r1(inst), get_field_r2(inst));
> -     break;
> -        case INST_TYPE_RD_R1_IMM:
> -	  fprintf_func(stream, "\t%s, %s, %s", get_field_rd(inst), get_field_r1(inst), get_field_imm(inst));
> -	  if (get_int_field_r1(inst) == 0) {
> -              print_immval_addr(info, immfound, immval, inst, 0);
> -	  }
> -	  break;
> -	case INST_TYPE_RD_R1_IMM5:
> -	  fprintf_func(stream, "\t%s, %s, %s", get_field_rd(inst), get_field_r1(inst), get_field_imm5(inst));
> -	  break;
> -	case INST_TYPE_RD_RFSL:
> -	  fprintf_func(stream, "\t%s, %s", get_field_rd(inst), get_field_rfsl(inst));
> -	  break;
> -	case INST_TYPE_R1_RFSL:
> -	  fprintf_func(stream, "\t%s, %s", get_field_r1(inst), get_field_rfsl(inst));
> -	  break;
> -	case INST_TYPE_RD_SPECIAL:
> -	  fprintf_func(stream, "\t%s, %s", get_field_rd(inst), get_field_special(inst, op));
> -	  break;
> -	case INST_TYPE_SPECIAL_R1:
> -	  fprintf_func(stream, "\t%s, %s", get_field_special(inst, op), get_field_r1(inst));
> -	  break;
> -	case INST_TYPE_RD_R1:
> -	  fprintf_func(stream, "\t%s, %s", get_field_rd(inst), get_field_r1(inst));
> -	  break;
> -	case INST_TYPE_R1_R2:
> -	  fprintf_func(stream, "\t%s, %s", get_field_r1(inst), get_field_r2(inst));
> -	  break;
> -	case INST_TYPE_R1_IMM:
> -	  fprintf_func(stream, "\t%s, %s", get_field_r1(inst), get_field_imm(inst));
> -	  /* The non-pc relative instructions are returns, which shouldn't
> -	     have a label printed */
> -	  if (op->inst_offset_type == INST_PC_OFFSET) {
> -              print_immval_addr(info, immfound, immval, inst, memaddr);
> -	  }
> -	  break;
> -        case INST_TYPE_RD_IMM:
> -	  fprintf_func(stream, "\t%s, %s", get_field_rd(inst), get_field_imm(inst));
> -          print_immval_addr(info, immfound, immval, inst,
> -                            op->inst_offset_type == INST_PC_OFFSET
> -                            ? memaddr : 0);
> -	  break;
> -        case INST_TYPE_IMM:
> -	  fprintf_func(stream, "\t%s", get_field_imm(inst));
> -	  if (op->instr != imm) {
> -              print_immval_addr(info, immfound, immval, inst,
> -                                op->inst_offset_type == INST_PC_OFFSET
> -                                ? memaddr : 0);
> -	  }
> -	  break;
> -        case INST_TYPE_RD_R2:
> -	  fprintf_func(stream, "\t%s, %s", get_field_rd(inst), get_field_r2(inst));
> -	  break;
> -  case INST_TYPE_R2:
> -     fprintf_func(stream, "\t%s", get_field_r2(inst));
> -     break;
> -  case INST_TYPE_R1:
> -     fprintf_func(stream, "\t%s", get_field_r1(inst));
> -     break;
> -  case INST_TYPE_RD_R1_SPECIAL:
> -     fprintf_func(stream, "\t%s, %s", get_field_rd(inst), get_field_r2(inst));
> -     break;
> -  case INST_TYPE_RD_IMM15:
> -     fprintf_func(stream, "\t%s, %s", get_field_rd(inst), get_field_imm15(inst));
> -     break;
> -     /* For tuqula instruction */
> -  case INST_TYPE_RD:
> -     fprintf_func(stream, "\t%s", get_field_rd(inst));
> -     break;
> -  case INST_TYPE_RFSL:
> -     fprintf_func(stream, "\t%s", get_field_rfsl(inst));
> -     break;
> -  default:
> -	  /* if the disassembler lags the instruction set */
> -	  fprintf_func (stream, "\tundecoded operands, inst is 0x%04lx", inst);
> -	  break;
> -	}
> +    switch (op->inst_type) {
> +    case INST_TYPE_RD_R1_R2:
> +        fprintf_func(stream, "\t%s, %s, %s",
> +                     get_field_rd(inst), get_field_r1(inst),
> +                     get_field_r2(inst));
> +        break;
> +    case INST_TYPE_RD_R1_IMM:
> +        fprintf_func(stream, "\t%s, %s, %s",
> +                     get_field_rd(inst), get_field_r1(inst),
> +                     get_field_imm(inst));
> +        if (get_int_field_r1(inst) == 0) {
> +            print_immval_addr(info, immfound, immval, inst, 0);
> +        }
> +        break;
> +    case INST_TYPE_RD_R1_IMM5:
> +        fprintf_func(stream, "\t%s, %s, %s",
> +                     get_field_rd(inst), get_field_r1(inst),
> +                     get_field_imm5(inst));
> +        break;
> +    case INST_TYPE_RD_RFSL:
> +        fprintf_func(stream, "\t%s, %s",
> +                     get_field_rd(inst), get_field_rfsl(inst));
> +        break;
> +    case INST_TYPE_R1_RFSL:
> +        fprintf_func(stream, "\t%s, %s",
> +                     get_field_r1(inst), get_field_rfsl(inst));
> +        break;
> +    case INST_TYPE_RD_SPECIAL:
> +        fprintf_func(stream, "\t%s, %s",
> +                     get_field_rd(inst), get_field_special(inst, op));
> +        break;
> +    case INST_TYPE_SPECIAL_R1:
> +        fprintf_func(stream, "\t%s, %s",
> +                     get_field_special(inst, op), get_field_r1(inst));
> +        break;
> +    case INST_TYPE_RD_R1:
> +        fprintf_func(stream, "\t%s, %s",
> +                     get_field_rd(inst), get_field_r1(inst));
> +        break;
> +    case INST_TYPE_R1_R2:
> +        fprintf_func(stream, "\t%s, %s",
> +                     get_field_r1(inst), get_field_r2(inst));
> +        break;
> +    case INST_TYPE_R1_IMM:
> +        fprintf_func(stream, "\t%s, %s",
> +                     get_field_r1(inst), get_field_imm(inst));
> +        /*
> +         * The non-pc relative instructions are returns,
> +         * which shouldn't have a label printed.
> +         */
> +        if (op->inst_offset_type == INST_PC_OFFSET) {
> +            print_immval_addr(info, immfound, immval, inst, memaddr);
> +        }
> +        break;
> +    case INST_TYPE_RD_IMM:
> +        fprintf_func(stream, "\t%s, %s",
> +                     get_field_rd(inst), get_field_imm(inst));
> +        print_immval_addr(info, immfound, immval, inst,
> +                          op->inst_offset_type == INST_PC_OFFSET
> +                          ? memaddr : 0);
> +        break;
> +    case INST_TYPE_IMM:
> +        fprintf_func(stream, "\t%s", get_field_imm(inst));
> +        if (op->instr != imm) {
> +            print_immval_addr(info, immfound, immval, inst,
> +                              op->inst_offset_type == INST_PC_OFFSET
> +                              ? memaddr : 0);
> +        }
> +        break;
> +    case INST_TYPE_RD_R2:
> +        fprintf_func(stream, "\t%s, %s",
> +                     get_field_rd(inst), get_field_r2(inst));
> +        break;
> +    case INST_TYPE_R2:
> +        fprintf_func(stream, "\t%s",
> +                     get_field_r2(inst));
> +        break;
> +    case INST_TYPE_R1:
> +        fprintf_func(stream, "\t%s",
> +                     get_field_r1(inst));
> +        break;
> +    case INST_TYPE_RD_R1_SPECIAL:
> +        fprintf_func(stream, "\t%s, %s",
> +                     get_field_rd(inst), get_field_r2(inst));
> +        break;
> +    case INST_TYPE_RD_IMM15:
> +        fprintf_func(stream, "\t%s, %s",
> +                     get_field_rd(inst), get_field_imm15(inst));
> +        break;
> +        /* For tuqula instruction */
> +    case INST_TYPE_RD:
> +        fprintf_func(stream, "\t%s",
> +                     get_field_rd(inst));
> +        break;
> +    case INST_TYPE_RFSL:
> +        fprintf_func(stream, "\t%s",
> +                     get_field_rfsl(inst));
> +        break;
> +    default:
> +        /* if the disassembler lags the instruction set */
> +        fprintf_func(stream, "\tundecoded operands, inst is 0x%04lx", inst);
> +        break;
>       }
> -
> -  /* Say how many bytes we consumed? */
> -  return 4;
> +    return 4;
>   }

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

[PATCH v3 16/27] disas/microblaze: Merge op->name output into each fprintf

[Richard Henderson]

In the common case, issue one single fprintf.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 disas/microblaze.c | 80 +++++++++++++++++++++++-----------------------
 1 file changed, 40 insertions(+), 40 deletions(-)

diff --git a/disas/microblaze.c b/disas/microblaze.c
index c729c76585..a537ac65dd 100644
--- a/disas/microblaze.c
+++ b/disas/microblaze.c
@@ -830,54 +830,52 @@ print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
         return 4;
     }
 
-    fprintf_func (stream, "%s", op->name);
-      
     switch (op->inst_type) {
     case INST_TYPE_RD_R1_R2:
-        fprintf_func(stream, "\t%s, %s, %s",
-                     get_field_rd(inst), get_field_r1(inst),
+        fprintf_func(stream, "%s\t%s, %s, %s",
+                     op->name, get_field_rd(inst), get_field_r1(inst),
                      get_field_r2(inst));
         break;
     case INST_TYPE_RD_R1_IMM:
-        fprintf_func(stream, "\t%s, %s, %s",
-                     get_field_rd(inst), get_field_r1(inst),
+        fprintf_func(stream, "%s\t%s, %s, %s",
+                     op->name, get_field_rd(inst), get_field_r1(inst),
                      get_field_imm(inst));
         if (get_int_field_r1(inst) == 0) {
             print_immval_addr(info, immfound, immval, inst, 0);
         }
         break;
     case INST_TYPE_RD_R1_IMM5:
-        fprintf_func(stream, "\t%s, %s, %s",
-                     get_field_rd(inst), get_field_r1(inst),
+        fprintf_func(stream, "%s\t%s, %s, %s",
+                     op->name, get_field_rd(inst), get_field_r1(inst),
                      get_field_imm5(inst));
         break;
     case INST_TYPE_RD_RFSL:
-        fprintf_func(stream, "\t%s, %s",
-                     get_field_rd(inst), get_field_rfsl(inst));
+        fprintf_func(stream, "%s\t%s, %s",
+                     op->name, get_field_rd(inst), get_field_rfsl(inst));
         break;
     case INST_TYPE_R1_RFSL:
-        fprintf_func(stream, "\t%s, %s",
-                     get_field_r1(inst), get_field_rfsl(inst));
+        fprintf_func(stream, "%s\t%s, %s",
+                     op->name, get_field_r1(inst), get_field_rfsl(inst));
         break;
     case INST_TYPE_RD_SPECIAL:
-        fprintf_func(stream, "\t%s, %s",
-                     get_field_rd(inst), get_field_special(inst, op));
+        fprintf_func(stream, "%s\t%s, %s",
+                     op->name, get_field_rd(inst), get_field_special(inst, op));
         break;
     case INST_TYPE_SPECIAL_R1:
-        fprintf_func(stream, "\t%s, %s",
-                     get_field_special(inst, op), get_field_r1(inst));
+        fprintf_func(stream, "%s\t%s, %s",
+                     op->name, get_field_special(inst, op), get_field_r1(inst));
         break;
     case INST_TYPE_RD_R1:
-        fprintf_func(stream, "\t%s, %s",
-                     get_field_rd(inst), get_field_r1(inst));
+        fprintf_func(stream, "%s\t%s, %s",
+                     op->name, get_field_rd(inst), get_field_r1(inst));
         break;
     case INST_TYPE_R1_R2:
-        fprintf_func(stream, "\t%s, %s",
-                     get_field_r1(inst), get_field_r2(inst));
+        fprintf_func(stream, "%s\t%s, %s",
+                     op->name, get_field_r1(inst), get_field_r2(inst));
         break;
     case INST_TYPE_R1_IMM:
-        fprintf_func(stream, "\t%s, %s",
-                     get_field_r1(inst), get_field_imm(inst));
+        fprintf_func(stream, "%s\t%s, %s",
+                     op->name, get_field_r1(inst), get_field_imm(inst));
         /*
          * The non-pc relative instructions are returns,
          * which shouldn't have a label printed.
@@ -887,14 +885,15 @@ print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
         }
         break;
     case INST_TYPE_RD_IMM:
-        fprintf_func(stream, "\t%s, %s",
-                     get_field_rd(inst), get_field_imm(inst));
+        fprintf_func(stream, "%s\t%s, %s",
+                     op->name, get_field_rd(inst), get_field_imm(inst));
         print_immval_addr(info, immfound, immval, inst,
                           op->inst_offset_type == INST_PC_OFFSET
                           ? memaddr : 0);
         break;
     case INST_TYPE_IMM:
-        fprintf_func(stream, "\t%s", get_field_imm(inst));
+        fprintf_func(stream, "%s\t%s",
+                     op->name, get_field_imm(inst));
         if (op->instr != imm) {
             print_immval_addr(info, immfound, immval, inst,
                               op->inst_offset_type == INST_PC_OFFSET
@@ -902,37 +901,38 @@ print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
         }
         break;
     case INST_TYPE_RD_R2:
-        fprintf_func(stream, "\t%s, %s",
-                     get_field_rd(inst), get_field_r2(inst));
+        fprintf_func(stream, "%s\t%s, %s",
+                     op->name, get_field_rd(inst), get_field_r2(inst));
         break;
     case INST_TYPE_R2:
-        fprintf_func(stream, "\t%s",
-                     get_field_r2(inst));
+        fprintf_func(stream, "%s\t%s",
+                     op->name, get_field_r2(inst));
         break;
     case INST_TYPE_R1:
-        fprintf_func(stream, "\t%s",
-                     get_field_r1(inst));
+        fprintf_func(stream, "%s\t%s",
+                     op->name, get_field_r1(inst));
         break;
     case INST_TYPE_RD_R1_SPECIAL:
-        fprintf_func(stream, "\t%s, %s",
-                     get_field_rd(inst), get_field_r2(inst));
+        fprintf_func(stream, "%s\t%s, %s",
+                     op->name, get_field_rd(inst), get_field_r2(inst));
         break;
     case INST_TYPE_RD_IMM15:
-        fprintf_func(stream, "\t%s, %s",
-                     get_field_rd(inst), get_field_imm15(inst));
+        fprintf_func(stream, "%s\t%s, %s",
+                     op->name, get_field_rd(inst), get_field_imm15(inst));
         break;
         /* For tuqula instruction */
     case INST_TYPE_RD:
-        fprintf_func(stream, "\t%s",
-                     get_field_rd(inst));
+        fprintf_func(stream, "%s\t%s",
+                     op->name, get_field_rd(inst));
         break;
     case INST_TYPE_RFSL:
-        fprintf_func(stream, "\t%s",
-                     get_field_rfsl(inst));
+        fprintf_func(stream, "%s\t%s",
+                     op->name, get_field_rfsl(inst));
         break;
     default:
         /* if the disassembler lags the instruction set */
-        fprintf_func(stream, "\tundecoded operands, inst is 0x%04lx", inst);
+        fprintf_func(stream, "%s\tundecoded operands, inst is 0x%04lx",
+                     op->name, inst);
         break;
     }
     return 4;
-- 
2.34.1

Re: [PATCH v3 16/27] disas/microblaze: Merge op->name output into each fprintf

[Pierrick Bouvier]

On 4/12/24 00:33, Richard Henderson wrote:
> In the common case, issue one single fprintf.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   disas/microblaze.c | 80 +++++++++++++++++++++++-----------------------
>   1 file changed, 40 insertions(+), 40 deletions(-)
> 
> diff --git a/disas/microblaze.c b/disas/microblaze.c
> index c729c76585..a537ac65dd 100644
> --- a/disas/microblaze.c
> +++ b/disas/microblaze.c
> @@ -830,54 +830,52 @@ print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
>           return 4;
>       }
>   
> -    fprintf_func (stream, "%s", op->name);
> -
>       switch (op->inst_type) {
>       case INST_TYPE_RD_R1_R2:
> -        fprintf_func(stream, "\t%s, %s, %s",
> -                     get_field_rd(inst), get_field_r1(inst),
> +        fprintf_func(stream, "%s\t%s, %s, %s",
> +                     op->name, get_field_rd(inst), get_field_r1(inst),
>                        get_field_r2(inst));
>           break;
>       case INST_TYPE_RD_R1_IMM:
> -        fprintf_func(stream, "\t%s, %s, %s",
> -                     get_field_rd(inst), get_field_r1(inst),
> +        fprintf_func(stream, "%s\t%s, %s, %s",
> +                     op->name, get_field_rd(inst), get_field_r1(inst),
>                        get_field_imm(inst));
>           if (get_int_field_r1(inst) == 0) {
>               print_immval_addr(info, immfound, immval, inst, 0);
>           }
>           break;
>       case INST_TYPE_RD_R1_IMM5:
> -        fprintf_func(stream, "\t%s, %s, %s",
> -                     get_field_rd(inst), get_field_r1(inst),
> +        fprintf_func(stream, "%s\t%s, %s, %s",
> +                     op->name, get_field_rd(inst), get_field_r1(inst),
>                        get_field_imm5(inst));
>           break;
>       case INST_TYPE_RD_RFSL:
> -        fprintf_func(stream, "\t%s, %s",
> -                     get_field_rd(inst), get_field_rfsl(inst));
> +        fprintf_func(stream, "%s\t%s, %s",
> +                     op->name, get_field_rd(inst), get_field_rfsl(inst));
>           break;
>       case INST_TYPE_R1_RFSL:
> -        fprintf_func(stream, "\t%s, %s",
> -                     get_field_r1(inst), get_field_rfsl(inst));
> +        fprintf_func(stream, "%s\t%s, %s",
> +                     op->name, get_field_r1(inst), get_field_rfsl(inst));
>           break;
>       case INST_TYPE_RD_SPECIAL:
> -        fprintf_func(stream, "\t%s, %s",
> -                     get_field_rd(inst), get_field_special(inst, op));
> +        fprintf_func(stream, "%s\t%s, %s",
> +                     op->name, get_field_rd(inst), get_field_special(inst, op));
>           break;
>       case INST_TYPE_SPECIAL_R1:
> -        fprintf_func(stream, "\t%s, %s",
> -                     get_field_special(inst, op), get_field_r1(inst));
> +        fprintf_func(stream, "%s\t%s, %s",
> +                     op->name, get_field_special(inst, op), get_field_r1(inst));
>           break;
>       case INST_TYPE_RD_R1:
> -        fprintf_func(stream, "\t%s, %s",
> -                     get_field_rd(inst), get_field_r1(inst));
> +        fprintf_func(stream, "%s\t%s, %s",
> +                     op->name, get_field_rd(inst), get_field_r1(inst));
>           break;
>       case INST_TYPE_R1_R2:
> -        fprintf_func(stream, "\t%s, %s",
> -                     get_field_r1(inst), get_field_r2(inst));
> +        fprintf_func(stream, "%s\t%s, %s",
> +                     op->name, get_field_r1(inst), get_field_r2(inst));
>           break;
>       case INST_TYPE_R1_IMM:
> -        fprintf_func(stream, "\t%s, %s",
> -                     get_field_r1(inst), get_field_imm(inst));
> +        fprintf_func(stream, "%s\t%s, %s",
> +                     op->name, get_field_r1(inst), get_field_imm(inst));
>           /*
>            * The non-pc relative instructions are returns,
>            * which shouldn't have a label printed.
> @@ -887,14 +885,15 @@ print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
>           }
>           break;
>       case INST_TYPE_RD_IMM:
> -        fprintf_func(stream, "\t%s, %s",
> -                     get_field_rd(inst), get_field_imm(inst));
> +        fprintf_func(stream, "%s\t%s, %s",
> +                     op->name, get_field_rd(inst), get_field_imm(inst));
>           print_immval_addr(info, immfound, immval, inst,
>                             op->inst_offset_type == INST_PC_OFFSET
>                             ? memaddr : 0);
>           break;
>       case INST_TYPE_IMM:
> -        fprintf_func(stream, "\t%s", get_field_imm(inst));
> +        fprintf_func(stream, "%s\t%s",
> +                     op->name, get_field_imm(inst));
>           if (op->instr != imm) {
>               print_immval_addr(info, immfound, immval, inst,
>                                 op->inst_offset_type == INST_PC_OFFSET
> @@ -902,37 +901,38 @@ print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
>           }
>           break;
>       case INST_TYPE_RD_R2:
> -        fprintf_func(stream, "\t%s, %s",
> -                     get_field_rd(inst), get_field_r2(inst));
> +        fprintf_func(stream, "%s\t%s, %s",
> +                     op->name, get_field_rd(inst), get_field_r2(inst));
>           break;
>       case INST_TYPE_R2:
> -        fprintf_func(stream, "\t%s",
> -                     get_field_r2(inst));
> +        fprintf_func(stream, "%s\t%s",
> +                     op->name, get_field_r2(inst));
>           break;
>       case INST_TYPE_R1:
> -        fprintf_func(stream, "\t%s",
> -                     get_field_r1(inst));
> +        fprintf_func(stream, "%s\t%s",
> +                     op->name, get_field_r1(inst));
>           break;
>       case INST_TYPE_RD_R1_SPECIAL:
> -        fprintf_func(stream, "\t%s, %s",
> -                     get_field_rd(inst), get_field_r2(inst));
> +        fprintf_func(stream, "%s\t%s, %s",
> +                     op->name, get_field_rd(inst), get_field_r2(inst));
>           break;
>       case INST_TYPE_RD_IMM15:
> -        fprintf_func(stream, "\t%s, %s",
> -                     get_field_rd(inst), get_field_imm15(inst));
> +        fprintf_func(stream, "%s\t%s, %s",
> +                     op->name, get_field_rd(inst), get_field_imm15(inst));
>           break;
>           /* For tuqula instruction */
>       case INST_TYPE_RD:
> -        fprintf_func(stream, "\t%s",
> -                     get_field_rd(inst));
> +        fprintf_func(stream, "%s\t%s",
> +                     op->name, get_field_rd(inst));
>           break;
>       case INST_TYPE_RFSL:
> -        fprintf_func(stream, "\t%s",
> -                     get_field_rfsl(inst));
> +        fprintf_func(stream, "%s\t%s",
> +                     op->name, get_field_rfsl(inst));
>           break;
>       default:
>           /* if the disassembler lags the instruction set */
> -        fprintf_func(stream, "\tundecoded operands, inst is 0x%04lx", inst);
> +        fprintf_func(stream, "%s\tundecoded operands, inst is 0x%04lx",
> +                     op->name, inst);
>           break;
>       }
>       return 4;

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

[PATCH v3 17/27] disas/microblaze: Print registers directly with PRIreg

[Richard Henderson]

Use a printf format instead of sprintf into a buffer.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 disas/microblaze.c | 54 ++++++++++++++++++++--------------------------
 1 file changed, 23 insertions(+), 31 deletions(-)

diff --git a/disas/microblaze.c b/disas/microblaze.c
index a537ac65dd..c12968f3b9 100644
--- a/disas/microblaze.c
+++ b/disas/microblaze.c
@@ -563,7 +563,7 @@ static const struct op_code_struct {
 };
 
 /* prefix for register names */
-static const char register_prefix[] = "r";
+#define register_prefix "r"
 static const char fsl_register_prefix[] = "rfsl";
 static const char pvr_register_prefix[] = "rpvr";
 
@@ -579,15 +579,16 @@ static const char pvr_register_prefix[] = "rpvr";
 
 #include "disas/dis-asm.h"
 
-#define get_field_rd(instr) get_field(instr, RD_MASK, RD_LOW)
-#define get_field_r1(instr) get_field(instr, RA_MASK, RA_LOW)
-#define get_field_r2(instr) get_field(instr, RB_MASK, RB_LOW)
+#define PRIreg    register_prefix "%ld"
+
+#define get_field_rd(instr)      ((instr & RD_MASK) >> RD_LOW)
+#define get_field_r1(instr)      ((instr & RA_MASK) >> RA_LOW)
+#define get_field_r2(instr)      ((instr & RB_MASK) >> RB_LOW)
 #define get_int_field_imm(instr) ((instr & IMM_MASK) >> IMM_LOW)
 #define get_int_field_r1(instr) ((instr & RA_MASK) >> RA_LOW)
 
 /* Local function prototypes. */
 
-static char * get_field (long instr, long mask, unsigned short low);
 static char * get_field_imm (long instr);
 static char * get_field_imm5 (long instr);
 static char * get_field_rfsl (long instr);
@@ -596,15 +597,6 @@ static char * get_field_imm15 (long instr);
 static char * get_field_unsigned_imm (long instr);
 #endif
 
-static char *
-get_field (long instr, long mask, unsigned short low)
-{
-  char tmpstr[25];
-  snprintf(tmpstr, sizeof(tmpstr), "%s%d", register_prefix,
-           (int)((instr & mask) >> low));
-  return(strdup(tmpstr));
-}
-
 static char *
 get_field_imm (long instr)
 {
@@ -832,12 +824,12 @@ print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
 
     switch (op->inst_type) {
     case INST_TYPE_RD_R1_R2:
-        fprintf_func(stream, "%s\t%s, %s, %s",
+        fprintf_func(stream, "%s\t" PRIreg ", " PRIreg ", " PRIreg,
                      op->name, get_field_rd(inst), get_field_r1(inst),
                      get_field_r2(inst));
         break;
     case INST_TYPE_RD_R1_IMM:
-        fprintf_func(stream, "%s\t%s, %s, %s",
+        fprintf_func(stream, "%s\t" PRIreg ", " PRIreg ", %s",
                      op->name, get_field_rd(inst), get_field_r1(inst),
                      get_field_imm(inst));
         if (get_int_field_r1(inst) == 0) {
@@ -845,36 +837,36 @@ print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
         }
         break;
     case INST_TYPE_RD_R1_IMM5:
-        fprintf_func(stream, "%s\t%s, %s, %s",
+        fprintf_func(stream, "%s\t" PRIreg ", " PRIreg ", %s",
                      op->name, get_field_rd(inst), get_field_r1(inst),
                      get_field_imm5(inst));
         break;
     case INST_TYPE_RD_RFSL:
-        fprintf_func(stream, "%s\t%s, %s",
+        fprintf_func(stream, "%s\t" PRIreg ", %s",
                      op->name, get_field_rd(inst), get_field_rfsl(inst));
         break;
     case INST_TYPE_R1_RFSL:
-        fprintf_func(stream, "%s\t%s, %s",
+        fprintf_func(stream, "%s\t" PRIreg ", %s",
                      op->name, get_field_r1(inst), get_field_rfsl(inst));
         break;
     case INST_TYPE_RD_SPECIAL:
-        fprintf_func(stream, "%s\t%s, %s",
+        fprintf_func(stream, "%s\t" PRIreg ", %s",
                      op->name, get_field_rd(inst), get_field_special(inst, op));
         break;
     case INST_TYPE_SPECIAL_R1:
-        fprintf_func(stream, "%s\t%s, %s",
+        fprintf_func(stream, "%s\t%s, " PRIreg,
                      op->name, get_field_special(inst, op), get_field_r1(inst));
         break;
     case INST_TYPE_RD_R1:
-        fprintf_func(stream, "%s\t%s, %s",
+        fprintf_func(stream, "%s\t" PRIreg ", " PRIreg,
                      op->name, get_field_rd(inst), get_field_r1(inst));
         break;
     case INST_TYPE_R1_R2:
-        fprintf_func(stream, "%s\t%s, %s",
+        fprintf_func(stream, "%s\t" PRIreg ", " PRIreg,
                      op->name, get_field_r1(inst), get_field_r2(inst));
         break;
     case INST_TYPE_R1_IMM:
-        fprintf_func(stream, "%s\t%s, %s",
+        fprintf_func(stream, "%s\t" PRIreg ", %s",
                      op->name, get_field_r1(inst), get_field_imm(inst));
         /*
          * The non-pc relative instructions are returns,
@@ -885,7 +877,7 @@ print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
         }
         break;
     case INST_TYPE_RD_IMM:
-        fprintf_func(stream, "%s\t%s, %s",
+        fprintf_func(stream, "%s\t" PRIreg ", %s",
                      op->name, get_field_rd(inst), get_field_imm(inst));
         print_immval_addr(info, immfound, immval, inst,
                           op->inst_offset_type == INST_PC_OFFSET
@@ -901,28 +893,28 @@ print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
         }
         break;
     case INST_TYPE_RD_R2:
-        fprintf_func(stream, "%s\t%s, %s",
+        fprintf_func(stream, "%s\t" PRIreg ", " PRIreg,
                      op->name, get_field_rd(inst), get_field_r2(inst));
         break;
     case INST_TYPE_R2:
-        fprintf_func(stream, "%s\t%s",
+        fprintf_func(stream, "%s\t" PRIreg,
                      op->name, get_field_r2(inst));
         break;
     case INST_TYPE_R1:
-        fprintf_func(stream, "%s\t%s",
+        fprintf_func(stream, "%s\t" PRIreg,
                      op->name, get_field_r1(inst));
         break;
     case INST_TYPE_RD_R1_SPECIAL:
-        fprintf_func(stream, "%s\t%s, %s",
+        fprintf_func(stream, "%s\t" PRIreg ", " PRIreg,
                      op->name, get_field_rd(inst), get_field_r2(inst));
         break;
     case INST_TYPE_RD_IMM15:
-        fprintf_func(stream, "%s\t%s, %s",
+        fprintf_func(stream, "%s\t" PRIreg ", %s",
                      op->name, get_field_rd(inst), get_field_imm15(inst));
         break;
         /* For tuqula instruction */
     case INST_TYPE_RD:
-        fprintf_func(stream, "%s\t%s",
+        fprintf_func(stream, "%s\t" PRIreg,
                      op->name, get_field_rd(inst));
         break;
     case INST_TYPE_RFSL:
-- 
2.34.1

Re: [PATCH v3 17/27] disas/microblaze: Print registers directly with PRIreg

[Pierrick Bouvier]

On 4/12/24 00:33, Richard Henderson wrote:
> Use a printf format instead of sprintf into a buffer.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   disas/microblaze.c | 54 ++++++++++++++++++++--------------------------
>   1 file changed, 23 insertions(+), 31 deletions(-)
> 
> diff --git a/disas/microblaze.c b/disas/microblaze.c
> index a537ac65dd..c12968f3b9 100644
> --- a/disas/microblaze.c
> +++ b/disas/microblaze.c
> @@ -563,7 +563,7 @@ static const struct op_code_struct {
>   };
>   
>   /* prefix for register names */
> -static const char register_prefix[] = "r";
> +#define register_prefix "r"
>   static const char fsl_register_prefix[] = "rfsl";
>   static const char pvr_register_prefix[] = "rpvr";
>   
> @@ -579,15 +579,16 @@ static const char pvr_register_prefix[] = "rpvr";
>   
>   #include "disas/dis-asm.h"
>   
> -#define get_field_rd(instr) get_field(instr, RD_MASK, RD_LOW)
> -#define get_field_r1(instr) get_field(instr, RA_MASK, RA_LOW)
> -#define get_field_r2(instr) get_field(instr, RB_MASK, RB_LOW)
> +#define PRIreg    register_prefix "%ld"
> +
> +#define get_field_rd(instr)      ((instr & RD_MASK) >> RD_LOW)
> +#define get_field_r1(instr)      ((instr & RA_MASK) >> RA_LOW)
> +#define get_field_r2(instr)      ((instr & RB_MASK) >> RB_LOW)
>   #define get_int_field_imm(instr) ((instr & IMM_MASK) >> IMM_LOW)
>   #define get_int_field_r1(instr) ((instr & RA_MASK) >> RA_LOW)
>   
>   /* Local function prototypes. */
>   
> -static char * get_field (long instr, long mask, unsigned short low);
>   static char * get_field_imm (long instr);
>   static char * get_field_imm5 (long instr);
>   static char * get_field_rfsl (long instr);
> @@ -596,15 +597,6 @@ static char * get_field_imm15 (long instr);
>   static char * get_field_unsigned_imm (long instr);
>   #endif
>   
> -static char *
> -get_field (long instr, long mask, unsigned short low)
> -{
> -  char tmpstr[25];
> -  snprintf(tmpstr, sizeof(tmpstr), "%s%d", register_prefix,
> -           (int)((instr & mask) >> low));
> -  return(strdup(tmpstr));
> -}
> -
>   static char *
>   get_field_imm (long instr)
>   {
> @@ -832,12 +824,12 @@ print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
>   
>       switch (op->inst_type) {
>       case INST_TYPE_RD_R1_R2:
> -        fprintf_func(stream, "%s\t%s, %s, %s",
> +        fprintf_func(stream, "%s\t" PRIreg ", " PRIreg ", " PRIreg,
>                        op->name, get_field_rd(inst), get_field_r1(inst),
>                        get_field_r2(inst));
>           break;
>       case INST_TYPE_RD_R1_IMM:
> -        fprintf_func(stream, "%s\t%s, %s, %s",
> +        fprintf_func(stream, "%s\t" PRIreg ", " PRIreg ", %s",
>                        op->name, get_field_rd(inst), get_field_r1(inst),
>                        get_field_imm(inst));
>           if (get_int_field_r1(inst) == 0) {
> @@ -845,36 +837,36 @@ print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
>           }
>           break;
>       case INST_TYPE_RD_R1_IMM5:
> -        fprintf_func(stream, "%s\t%s, %s, %s",
> +        fprintf_func(stream, "%s\t" PRIreg ", " PRIreg ", %s",
>                        op->name, get_field_rd(inst), get_field_r1(inst),
>                        get_field_imm5(inst));
>           break;
>       case INST_TYPE_RD_RFSL:
> -        fprintf_func(stream, "%s\t%s, %s",
> +        fprintf_func(stream, "%s\t" PRIreg ", %s",
>                        op->name, get_field_rd(inst), get_field_rfsl(inst));
>           break;
>       case INST_TYPE_R1_RFSL:
> -        fprintf_func(stream, "%s\t%s, %s",
> +        fprintf_func(stream, "%s\t" PRIreg ", %s",
>                        op->name, get_field_r1(inst), get_field_rfsl(inst));
>           break;
>       case INST_TYPE_RD_SPECIAL:
> -        fprintf_func(stream, "%s\t%s, %s",
> +        fprintf_func(stream, "%s\t" PRIreg ", %s",
>                        op->name, get_field_rd(inst), get_field_special(inst, op));
>           break;
>       case INST_TYPE_SPECIAL_R1:
> -        fprintf_func(stream, "%s\t%s, %s",
> +        fprintf_func(stream, "%s\t%s, " PRIreg,
>                        op->name, get_field_special(inst, op), get_field_r1(inst));
>           break;
>       case INST_TYPE_RD_R1:
> -        fprintf_func(stream, "%s\t%s, %s",
> +        fprintf_func(stream, "%s\t" PRIreg ", " PRIreg,
>                        op->name, get_field_rd(inst), get_field_r1(inst));
>           break;
>       case INST_TYPE_R1_R2:
> -        fprintf_func(stream, "%s\t%s, %s",
> +        fprintf_func(stream, "%s\t" PRIreg ", " PRIreg,
>                        op->name, get_field_r1(inst), get_field_r2(inst));
>           break;
>       case INST_TYPE_R1_IMM:
> -        fprintf_func(stream, "%s\t%s, %s",
> +        fprintf_func(stream, "%s\t" PRIreg ", %s",
>                        op->name, get_field_r1(inst), get_field_imm(inst));
>           /*
>            * The non-pc relative instructions are returns,
> @@ -885,7 +877,7 @@ print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
>           }
>           break;
>       case INST_TYPE_RD_IMM:
> -        fprintf_func(stream, "%s\t%s, %s",
> +        fprintf_func(stream, "%s\t" PRIreg ", %s",
>                        op->name, get_field_rd(inst), get_field_imm(inst));
>           print_immval_addr(info, immfound, immval, inst,
>                             op->inst_offset_type == INST_PC_OFFSET
> @@ -901,28 +893,28 @@ print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
>           }
>           break;
>       case INST_TYPE_RD_R2:
> -        fprintf_func(stream, "%s\t%s, %s",
> +        fprintf_func(stream, "%s\t" PRIreg ", " PRIreg,
>                        op->name, get_field_rd(inst), get_field_r2(inst));
>           break;
>       case INST_TYPE_R2:
> -        fprintf_func(stream, "%s\t%s",
> +        fprintf_func(stream, "%s\t" PRIreg,
>                        op->name, get_field_r2(inst));
>           break;
>       case INST_TYPE_R1:
> -        fprintf_func(stream, "%s\t%s",
> +        fprintf_func(stream, "%s\t" PRIreg,
>                        op->name, get_field_r1(inst));
>           break;
>       case INST_TYPE_RD_R1_SPECIAL:
> -        fprintf_func(stream, "%s\t%s, %s",
> +        fprintf_func(stream, "%s\t" PRIreg ", " PRIreg,
>                        op->name, get_field_rd(inst), get_field_r2(inst));
>           break;
>       case INST_TYPE_RD_IMM15:
> -        fprintf_func(stream, "%s\t%s, %s",
> +        fprintf_func(stream, "%s\t" PRIreg ", %s",
>                        op->name, get_field_rd(inst), get_field_imm15(inst));
>           break;
>           /* For tuqula instruction */
>       case INST_TYPE_RD:
> -        fprintf_func(stream, "%s\t%s",
> +        fprintf_func(stream, "%s\t" PRIreg,
>                        op->name, get_field_rd(inst));
>           break;
>       case INST_TYPE_RFSL:

I like PRIreg definition :)

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

[PATCH v3 18/27] disas/microblaze: Print immediates directly with PRIimm

[Richard Henderson]

Use a printf format instead of sprintf into a buffer.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 disas/microblaze.c | 61 +++++++++-------------------------------------
 1 file changed, 11 insertions(+), 50 deletions(-)

diff --git a/disas/microblaze.c b/disas/microblaze.c
index c12968f3b9..390f98c0a3 100644
--- a/disas/microblaze.c
+++ b/disas/microblaze.c
@@ -580,40 +580,21 @@ static const char pvr_register_prefix[] = "rpvr";
 #include "disas/dis-asm.h"
 
 #define PRIreg    register_prefix "%ld"
+#define PRIimm    "%d"
 
 #define get_field_rd(instr)      ((instr & RD_MASK) >> RD_LOW)
 #define get_field_r1(instr)      ((instr & RA_MASK) >> RA_LOW)
 #define get_field_r2(instr)      ((instr & RB_MASK) >> RB_LOW)
+#define get_field_imm(instr)     ((int16_t)instr)
+#define get_field_imm5(instr)    ((int)instr & IMM5_MASK)
+#define get_field_imm15(instr)   ((int)instr & IMM15_MASK)
+
 #define get_int_field_imm(instr) ((instr & IMM_MASK) >> IMM_LOW)
 #define get_int_field_r1(instr) ((instr & RA_MASK) >> RA_LOW)
 
 /* Local function prototypes. */
 
-static char * get_field_imm (long instr);
-static char * get_field_imm5 (long instr);
 static char * get_field_rfsl (long instr);
-static char * get_field_imm15 (long instr);
-#if 0
-static char * get_field_unsigned_imm (long instr);
-#endif
-
-static char *
-get_field_imm (long instr)
-{
-  char tmpstr[25];
-  snprintf(tmpstr, sizeof(tmpstr), "%d",
-           (short)((instr & IMM_MASK) >> IMM_LOW));
-  return(strdup(tmpstr));
-}
-
-static char *
-get_field_imm5 (long instr)
-{
-  char tmpstr[25];
-  snprintf(tmpstr, sizeof(tmpstr), "%d",
-           (short)((instr & IMM5_MASK) >> IMM_LOW));
-  return(strdup(tmpstr));
-}
 
 static char *
 get_field_rfsl (long instr)
@@ -624,26 +605,6 @@ get_field_rfsl (long instr)
   return(strdup(tmpstr));
 }
 
-static char *
-get_field_imm15 (long instr)
-{
-  char tmpstr[25];
-  snprintf(tmpstr, sizeof(tmpstr), "%d",
-           (short)((instr & IMM15_MASK) >> IMM_LOW));
-  return(strdup(tmpstr));
-}
-
-#if 0
-static char *
-get_field_unsigned_imm (long instr)
-{
-  char tmpstr[25];
-  snprintf(tmpstr, sizeof(tmpstr), "%d",
-           (int)((instr & IMM_MASK) >> IMM_LOW));
-  return(strdup(tmpstr));
-}
-#endif
-
 /*
   char *
   get_field_special (instr) 
@@ -829,7 +790,7 @@ print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
                      get_field_r2(inst));
         break;
     case INST_TYPE_RD_R1_IMM:
-        fprintf_func(stream, "%s\t" PRIreg ", " PRIreg ", %s",
+        fprintf_func(stream, "%s\t" PRIreg ", " PRIreg ", " PRIimm,
                      op->name, get_field_rd(inst), get_field_r1(inst),
                      get_field_imm(inst));
         if (get_int_field_r1(inst) == 0) {
@@ -837,7 +798,7 @@ print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
         }
         break;
     case INST_TYPE_RD_R1_IMM5:
-        fprintf_func(stream, "%s\t" PRIreg ", " PRIreg ", %s",
+        fprintf_func(stream, "%s\t" PRIreg ", " PRIreg ", " PRIimm,
                      op->name, get_field_rd(inst), get_field_r1(inst),
                      get_field_imm5(inst));
         break;
@@ -866,7 +827,7 @@ print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
                      op->name, get_field_r1(inst), get_field_r2(inst));
         break;
     case INST_TYPE_R1_IMM:
-        fprintf_func(stream, "%s\t" PRIreg ", %s",
+        fprintf_func(stream, "%s\t" PRIreg ", " PRIimm,
                      op->name, get_field_r1(inst), get_field_imm(inst));
         /*
          * The non-pc relative instructions are returns,
@@ -877,14 +838,14 @@ print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
         }
         break;
     case INST_TYPE_RD_IMM:
-        fprintf_func(stream, "%s\t" PRIreg ", %s",
+        fprintf_func(stream, "%s\t" PRIreg ", " PRIimm,
                      op->name, get_field_rd(inst), get_field_imm(inst));
         print_immval_addr(info, immfound, immval, inst,
                           op->inst_offset_type == INST_PC_OFFSET
                           ? memaddr : 0);
         break;
     case INST_TYPE_IMM:
-        fprintf_func(stream, "%s\t%s",
+        fprintf_func(stream, "%s\t" PRIimm,
                      op->name, get_field_imm(inst));
         if (op->instr != imm) {
             print_immval_addr(info, immfound, immval, inst,
@@ -909,7 +870,7 @@ print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
                      op->name, get_field_rd(inst), get_field_r2(inst));
         break;
     case INST_TYPE_RD_IMM15:
-        fprintf_func(stream, "%s\t" PRIreg ", %s",
+        fprintf_func(stream, "%s\t" PRIreg ", " PRIimm,
                      op->name, get_field_rd(inst), get_field_imm15(inst));
         break;
         /* For tuqula instruction */
-- 
2.34.1

Re: [PATCH v3 18/27] disas/microblaze: Print immediates directly with PRIimm

[Pierrick Bouvier]

On 4/12/24 00:33, Richard Henderson wrote:
> Use a printf format instead of sprintf into a buffer.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   disas/microblaze.c | 61 +++++++++-------------------------------------
>   1 file changed, 11 insertions(+), 50 deletions(-)
> 
> diff --git a/disas/microblaze.c b/disas/microblaze.c
> index c12968f3b9..390f98c0a3 100644
> --- a/disas/microblaze.c
> +++ b/disas/microblaze.c
> @@ -580,40 +580,21 @@ static const char pvr_register_prefix[] = "rpvr";
>   #include "disas/dis-asm.h"
>   
>   #define PRIreg    register_prefix "%ld"
> +#define PRIimm    "%d"
>   
>   #define get_field_rd(instr)      ((instr & RD_MASK) >> RD_LOW)
>   #define get_field_r1(instr)      ((instr & RA_MASK) >> RA_LOW)
>   #define get_field_r2(instr)      ((instr & RB_MASK) >> RB_LOW)
> +#define get_field_imm(instr)     ((int16_t)instr)
> +#define get_field_imm5(instr)    ((int)instr & IMM5_MASK)
> +#define get_field_imm15(instr)   ((int)instr & IMM15_MASK)
> +
>   #define get_int_field_imm(instr) ((instr & IMM_MASK) >> IMM_LOW)
>   #define get_int_field_r1(instr) ((instr & RA_MASK) >> RA_LOW)
>   
>   /* Local function prototypes. */
>   
> -static char * get_field_imm (long instr);
> -static char * get_field_imm5 (long instr);
>   static char * get_field_rfsl (long instr);
> -static char * get_field_imm15 (long instr);
> -#if 0
> -static char * get_field_unsigned_imm (long instr);
> -#endif
> -
> -static char *
> -get_field_imm (long instr)
> -{
> -  char tmpstr[25];
> -  snprintf(tmpstr, sizeof(tmpstr), "%d",
> -           (short)((instr & IMM_MASK) >> IMM_LOW));
> -  return(strdup(tmpstr));
> -}
> -
> -static char *
> -get_field_imm5 (long instr)
> -{
> -  char tmpstr[25];
> -  snprintf(tmpstr, sizeof(tmpstr), "%d",
> -           (short)((instr & IMM5_MASK) >> IMM_LOW));
> -  return(strdup(tmpstr));
> -}
>   
>   static char *
>   get_field_rfsl (long instr)
> @@ -624,26 +605,6 @@ get_field_rfsl (long instr)
>     return(strdup(tmpstr));
>   }
>   
> -static char *
> -get_field_imm15 (long instr)
> -{
> -  char tmpstr[25];
> -  snprintf(tmpstr, sizeof(tmpstr), "%d",
> -           (short)((instr & IMM15_MASK) >> IMM_LOW));
> -  return(strdup(tmpstr));
> -}
> -
> -#if 0
> -static char *
> -get_field_unsigned_imm (long instr)
> -{
> -  char tmpstr[25];
> -  snprintf(tmpstr, sizeof(tmpstr), "%d",
> -           (int)((instr & IMM_MASK) >> IMM_LOW));
> -  return(strdup(tmpstr));
> -}
> -#endif
> -
>   /*
>     char *
>     get_field_special (instr)
> @@ -829,7 +790,7 @@ print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
>                        get_field_r2(inst));
>           break;
>       case INST_TYPE_RD_R1_IMM:
> -        fprintf_func(stream, "%s\t" PRIreg ", " PRIreg ", %s",
> +        fprintf_func(stream, "%s\t" PRIreg ", " PRIreg ", " PRIimm,
>                        op->name, get_field_rd(inst), get_field_r1(inst),
>                        get_field_imm(inst));
>           if (get_int_field_r1(inst) == 0) {
> @@ -837,7 +798,7 @@ print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
>           }
>           break;
>       case INST_TYPE_RD_R1_IMM5:
> -        fprintf_func(stream, "%s\t" PRIreg ", " PRIreg ", %s",
> +        fprintf_func(stream, "%s\t" PRIreg ", " PRIreg ", " PRIimm,
>                        op->name, get_field_rd(inst), get_field_r1(inst),
>                        get_field_imm5(inst));
>           break;
> @@ -866,7 +827,7 @@ print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
>                        op->name, get_field_r1(inst), get_field_r2(inst));
>           break;
>       case INST_TYPE_R1_IMM:
> -        fprintf_func(stream, "%s\t" PRIreg ", %s",
> +        fprintf_func(stream, "%s\t" PRIreg ", " PRIimm,
>                        op->name, get_field_r1(inst), get_field_imm(inst));
>           /*
>            * The non-pc relative instructions are returns,
> @@ -877,14 +838,14 @@ print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
>           }
>           break;
>       case INST_TYPE_RD_IMM:
> -        fprintf_func(stream, "%s\t" PRIreg ", %s",
> +        fprintf_func(stream, "%s\t" PRIreg ", " PRIimm,
>                        op->name, get_field_rd(inst), get_field_imm(inst));
>           print_immval_addr(info, immfound, immval, inst,
>                             op->inst_offset_type == INST_PC_OFFSET
>                             ? memaddr : 0);
>           break;
>       case INST_TYPE_IMM:
> -        fprintf_func(stream, "%s\t%s",
> +        fprintf_func(stream, "%s\t" PRIimm,
>                        op->name, get_field_imm(inst));
>           if (op->instr != imm) {
>               print_immval_addr(info, immfound, immval, inst,
> @@ -909,7 +870,7 @@ print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
>                        op->name, get_field_rd(inst), get_field_r2(inst));
>           break;
>       case INST_TYPE_RD_IMM15:
> -        fprintf_func(stream, "%s\t" PRIreg ", %s",
> +        fprintf_func(stream, "%s\t" PRIreg ", " PRIimm,
>                        op->name, get_field_rd(inst), get_field_imm15(inst));
>           break;
>           /* For tuqula instruction */

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

[PATCH v3 19/27] disas/microblaze: Print registers directly with PRIrfsl

[Richard Henderson]

Use a printf format instead of sprintf into a buffer.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 disas/microblaze.c | 22 +++++-----------------
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/disas/microblaze.c b/disas/microblaze.c
index 390f98c0a3..24febfdea9 100644
--- a/disas/microblaze.c
+++ b/disas/microblaze.c
@@ -564,7 +564,6 @@ static const struct op_code_struct {
 
 /* prefix for register names */
 #define register_prefix "r"
-static const char fsl_register_prefix[] = "rfsl";
 static const char pvr_register_prefix[] = "rpvr";
 
 
@@ -580,11 +579,13 @@ static const char pvr_register_prefix[] = "rpvr";
 #include "disas/dis-asm.h"
 
 #define PRIreg    register_prefix "%ld"
+#define PRIrfsl   register_prefix "fsl%ld"
 #define PRIimm    "%d"
 
 #define get_field_rd(instr)      ((instr & RD_MASK) >> RD_LOW)
 #define get_field_r1(instr)      ((instr & RA_MASK) >> RA_LOW)
 #define get_field_r2(instr)      ((instr & RB_MASK) >> RB_LOW)
+#define get_field_rfsl(instr)    (instr & RFSL_MASK)
 #define get_field_imm(instr)     ((int16_t)instr)
 #define get_field_imm5(instr)    ((int)instr & IMM5_MASK)
 #define get_field_imm15(instr)   ((int)instr & IMM15_MASK)
@@ -592,19 +593,6 @@ static const char pvr_register_prefix[] = "rpvr";
 #define get_int_field_imm(instr) ((instr & IMM_MASK) >> IMM_LOW)
 #define get_int_field_r1(instr) ((instr & RA_MASK) >> RA_LOW)
 
-/* Local function prototypes. */
-
-static char * get_field_rfsl (long instr);
-
-static char *
-get_field_rfsl (long instr)
-{
-  char tmpstr[25];
-  snprintf(tmpstr, sizeof(tmpstr), "%s%d", fsl_register_prefix,
-           (short)((instr & RFSL_MASK) >> IMM_LOW));
-  return(strdup(tmpstr));
-}
-
 /*
   char *
   get_field_special (instr) 
@@ -803,11 +791,11 @@ print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
                      get_field_imm5(inst));
         break;
     case INST_TYPE_RD_RFSL:
-        fprintf_func(stream, "%s\t" PRIreg ", %s",
+        fprintf_func(stream, "%s\t" PRIreg ", " PRIrfsl,
                      op->name, get_field_rd(inst), get_field_rfsl(inst));
         break;
     case INST_TYPE_R1_RFSL:
-        fprintf_func(stream, "%s\t" PRIreg ", %s",
+        fprintf_func(stream, "%s\t" PRIreg ", " PRIrfsl,
                      op->name, get_field_r1(inst), get_field_rfsl(inst));
         break;
     case INST_TYPE_RD_SPECIAL:
@@ -879,7 +867,7 @@ print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
                      op->name, get_field_rd(inst));
         break;
     case INST_TYPE_RFSL:
-        fprintf_func(stream, "%s\t%s",
+        fprintf_func(stream, "%s\t" PRIrfsl,
                      op->name, get_field_rfsl(inst));
         break;
     default:
-- 
2.34.1

Re: [PATCH v3 19/27] disas/microblaze: Print registers directly with PRIrfsl

[Pierrick Bouvier]

On 4/12/24 00:33, Richard Henderson wrote:
> Use a printf format instead of sprintf into a buffer.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   disas/microblaze.c | 22 +++++-----------------
>   1 file changed, 5 insertions(+), 17 deletions(-)
> 
> diff --git a/disas/microblaze.c b/disas/microblaze.c
> index 390f98c0a3..24febfdea9 100644
> --- a/disas/microblaze.c
> +++ b/disas/microblaze.c
> @@ -564,7 +564,6 @@ static const struct op_code_struct {
>   
>   /* prefix for register names */
>   #define register_prefix "r"
> -static const char fsl_register_prefix[] = "rfsl";
>   static const char pvr_register_prefix[] = "rpvr";
>   
>   
> @@ -580,11 +579,13 @@ static const char pvr_register_prefix[] = "rpvr";
>   #include "disas/dis-asm.h"
>   
>   #define PRIreg    register_prefix "%ld"
> +#define PRIrfsl   register_prefix "fsl%ld"
>   #define PRIimm    "%d"
>   
>   #define get_field_rd(instr)      ((instr & RD_MASK) >> RD_LOW)
>   #define get_field_r1(instr)      ((instr & RA_MASK) >> RA_LOW)
>   #define get_field_r2(instr)      ((instr & RB_MASK) >> RB_LOW)
> +#define get_field_rfsl(instr)    (instr & RFSL_MASK)
>   #define get_field_imm(instr)     ((int16_t)instr)
>   #define get_field_imm5(instr)    ((int)instr & IMM5_MASK)
>   #define get_field_imm15(instr)   ((int)instr & IMM15_MASK)
> @@ -592,19 +593,6 @@ static const char pvr_register_prefix[] = "rpvr";
>   #define get_int_field_imm(instr) ((instr & IMM_MASK) >> IMM_LOW)
>   #define get_int_field_r1(instr) ((instr & RA_MASK) >> RA_LOW)
>   
> -/* Local function prototypes. */
> -
> -static char * get_field_rfsl (long instr);
> -
> -static char *
> -get_field_rfsl (long instr)
> -{
> -  char tmpstr[25];
> -  snprintf(tmpstr, sizeof(tmpstr), "%s%d", fsl_register_prefix,
> -           (short)((instr & RFSL_MASK) >> IMM_LOW));
> -  return(strdup(tmpstr));
> -}
> -
>   /*
>     char *
>     get_field_special (instr)
> @@ -803,11 +791,11 @@ print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
>                        get_field_imm5(inst));
>           break;
>       case INST_TYPE_RD_RFSL:
> -        fprintf_func(stream, "%s\t" PRIreg ", %s",
> +        fprintf_func(stream, "%s\t" PRIreg ", " PRIrfsl,
>                        op->name, get_field_rd(inst), get_field_rfsl(inst));
>           break;
>       case INST_TYPE_R1_RFSL:
> -        fprintf_func(stream, "%s\t" PRIreg ", %s",
> +        fprintf_func(stream, "%s\t" PRIreg ", " PRIrfsl,
>                        op->name, get_field_r1(inst), get_field_rfsl(inst));
>           break;
>       case INST_TYPE_RD_SPECIAL:
> @@ -879,7 +867,7 @@ print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
>                        op->name, get_field_rd(inst));
>           break;
>       case INST_TYPE_RFSL:
> -        fprintf_func(stream, "%s\t%s",
> +        fprintf_func(stream, "%s\t" PRIrfsl,
>                        op->name, get_field_rfsl(inst));
>           break;
>       default:

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH v3 19/27] disas/microblaze: Print registers directly with PRIrfsl

[Pierrick Bouvier]

On 4/12/24 00:33, Richard Henderson wrote:
> Use a printf format instead of sprintf into a buffer.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   disas/microblaze.c | 22 +++++-----------------
>   1 file changed, 5 insertions(+), 17 deletions(-)
> 
> diff --git a/disas/microblaze.c b/disas/microblaze.c
> index 390f98c0a3..24febfdea9 100644
> --- a/disas/microblaze.c
> +++ b/disas/microblaze.c
> @@ -564,7 +564,6 @@ static const struct op_code_struct {
>   
>   /* prefix for register names */
>   #define register_prefix "r"
> -static const char fsl_register_prefix[] = "rfsl";
>   static const char pvr_register_prefix[] = "rpvr";
>   
>   
> @@ -580,11 +579,13 @@ static const char pvr_register_prefix[] = "rpvr";
>   #include "disas/dis-asm.h"
>   
>   #define PRIreg    register_prefix "%ld"
> +#define PRIrfsl   register_prefix "fsl%ld"
>   #define PRIimm    "%d"
>   
>   #define get_field_rd(instr)      ((instr & RD_MASK) >> RD_LOW)
>   #define get_field_r1(instr)      ((instr & RA_MASK) >> RA_LOW)
>   #define get_field_r2(instr)      ((instr & RB_MASK) >> RB_LOW)
> +#define get_field_rfsl(instr)    (instr & RFSL_MASK)
>   #define get_field_imm(instr)     ((int16_t)instr)
>   #define get_field_imm5(instr)    ((int)instr & IMM5_MASK)
>   #define get_field_imm15(instr)   ((int)instr & IMM15_MASK)
> @@ -592,19 +593,6 @@ static const char pvr_register_prefix[] = "rpvr";
>   #define get_int_field_imm(instr) ((instr & IMM_MASK) >> IMM_LOW)
>   #define get_int_field_r1(instr) ((instr & RA_MASK) >> RA_LOW)
>   
> -/* Local function prototypes. */
> -
> -static char * get_field_rfsl (long instr);
> -
> -static char *
> -get_field_rfsl (long instr)
> -{
> -  char tmpstr[25];
> -  snprintf(tmpstr, sizeof(tmpstr), "%s%d", fsl_register_prefix,
> -           (short)((instr & RFSL_MASK) >> IMM_LOW));
> -  return(strdup(tmpstr));
> -}
> -
>   /*
>     char *
>     get_field_special (instr)
> @@ -803,11 +791,11 @@ print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
>                        get_field_imm5(inst));
>           break;
>       case INST_TYPE_RD_RFSL:
> -        fprintf_func(stream, "%s\t" PRIreg ", %s",
> +        fprintf_func(stream, "%s\t" PRIreg ", " PRIrfsl,
>                        op->name, get_field_rd(inst), get_field_rfsl(inst));
>           break;
>       case INST_TYPE_R1_RFSL:
> -        fprintf_func(stream, "%s\t" PRIreg ", %s",
> +        fprintf_func(stream, "%s\t" PRIreg ", " PRIrfsl,
>                        op->name, get_field_r1(inst), get_field_rfsl(inst));
>           break;
>       case INST_TYPE_RD_SPECIAL:
> @@ -879,7 +867,7 @@ print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
>                        op->name, get_field_rd(inst));
>           break;
>       case INST_TYPE_RFSL:
> -        fprintf_func(stream, "%s\t%s",
> +        fprintf_func(stream, "%s\t" PRIrfsl,
>                        op->name, get_field_rfsl(inst));
>           break;
>       default:

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

[PATCH v3 20/27] disas/microblaze: Split get_field_special

[Richard Henderson]

Extract the raw special index and a function to lookup a name.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 disas/microblaze.c | 142 +++++++++++++++++++--------------------------
 1 file changed, 61 insertions(+), 81 deletions(-)

diff --git a/disas/microblaze.c b/disas/microblaze.c
index 24febfdea9..197327fae4 100644
--- a/disas/microblaze.c
+++ b/disas/microblaze.c
@@ -564,8 +564,6 @@ static const struct op_code_struct {
 
 /* prefix for register names */
 #define register_prefix "r"
-static const char pvr_register_prefix[] = "rpvr";
-
 
 /* #defines for valid immediate range */
 #define MIN_IMM  ((int) 0x80000000)
@@ -580,6 +578,7 @@ static const char pvr_register_prefix[] = "rpvr";
 
 #define PRIreg    register_prefix "%ld"
 #define PRIrfsl   register_prefix "fsl%ld"
+#define PRIpvr    register_prefix "pvr%d"
 #define PRIimm    "%d"
 
 #define get_field_rd(instr)      ((instr & RD_MASK) >> RD_LOW)
@@ -593,83 +592,48 @@ static const char pvr_register_prefix[] = "rpvr";
 #define get_int_field_imm(instr) ((instr & IMM_MASK) >> IMM_LOW)
 #define get_int_field_r1(instr) ((instr & RA_MASK) >> RA_LOW)
 
-/*
-  char *
-  get_field_special (instr) 
-  long instr;
-  {
-  char tmpstr[25];
-  
-  snprintf(tmpstr, sizeof(tmpstr), "%s%s", register_prefix,
-          (((instr & IMM_MASK) >> IMM_LOW) & REG_MSR_MASK) == 0 ? "pc" : "msr");
-  
-  return(strdup(tmpstr));
-  }
-*/
-
-static char *
-get_field_special(long instr, const struct op_code_struct *op)
+static int get_field_special(long instr, const struct op_code_struct *op)
 {
-   char tmpstr[25];
-   char spr[6];
+    return ((instr & IMM_MASK) >> IMM_LOW) ^ op->immval_mask;
+}
 
-   switch ( (((instr & IMM_MASK) >> IMM_LOW) ^ op->immval_mask) ) {
-
-   case REG_MSR_MASK :
-      strcpy(spr, "msr");
-      break;
-   case REG_PC_MASK :
-      strcpy(spr, "pc");
-      break;
-   case REG_EAR_MASK :
-      strcpy(spr, "ear");
-      break;
-   case REG_ESR_MASK :
-      strcpy(spr, "esr");
-      break;
-   case REG_FSR_MASK :
-      strcpy(spr, "fsr");
-      break;
-   case REG_BTR_MASK :
-      strcpy(spr, "btr");
-      break;      
-   case REG_EDR_MASK :
-      strcpy(spr, "edr");
-      break;
-   case REG_PID_MASK :
-      strcpy(spr, "pid");
-      break;
-   case REG_ZPR_MASK :
-      strcpy(spr, "zpr");
-      break;
-   case REG_TLBX_MASK :
-      strcpy(spr, "tlbx");
-      break;
-   case REG_TLBLO_MASK :
-      strcpy(spr, "tlblo");
-      break;
-   case REG_TLBHI_MASK :
-      strcpy(spr, "tlbhi");
-      break;
-   case REG_TLBSX_MASK :
-      strcpy(spr, "tlbsx");
-      break;
-   default :
-     {
-       if ( ((((instr & IMM_MASK) >> IMM_LOW) ^ op->immval_mask) & 0xE000) == REG_PVR_MASK) {
-          snprintf(tmpstr, sizeof(tmpstr), "%s%u", pvr_register_prefix,
-                 (unsigned short)(((instr & IMM_MASK) >> IMM_LOW) ^
-                                  op->immval_mask) ^ REG_PVR_MASK);
-	 return(strdup(tmpstr));
-       } else {
-	 strcpy(spr, "pc");
-       }
-     }
-     break;
-   }
-   
-   snprintf(tmpstr, sizeof(tmpstr), "%s%s", register_prefix, spr);
-   return(strdup(tmpstr));
+/* Returns NULL for PVR registers, which should be rendered differently. */
+static const char *get_special_name(int special)
+{
+    switch (special) {
+    case REG_MSR_MASK:
+        return register_prefix "msr";
+    case REG_PC_MASK:
+        return register_prefix "pc";
+    case REG_EAR_MASK:
+        return register_prefix "ear";
+    case REG_ESR_MASK:
+        return register_prefix "esr";
+    case REG_FSR_MASK:
+        return register_prefix "fsr";
+    case REG_BTR_MASK:
+        return register_prefix "btr";
+    case REG_EDR_MASK:
+        return register_prefix "edr";
+    case REG_PID_MASK:
+        return register_prefix "pid";
+    case REG_ZPR_MASK:
+        return register_prefix "zpr";
+    case REG_TLBX_MASK:
+        return register_prefix "tlbx";
+    case REG_TLBLO_MASK:
+        return register_prefix "tlblo";
+    case REG_TLBHI_MASK:
+        return register_prefix "tlbhi";
+    case REG_TLBSX_MASK:
+        return register_prefix "tlbsx";
+    default:
+        if ((special & 0xE000) == REG_PVR_MASK) {
+            /* pvr register */
+            return NULL;
+        }
+        return register_prefix "pc";
+    }
 }
 
 static unsigned long
@@ -739,6 +703,8 @@ print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
     static bfd_vma prev_insn_addr = -1; /*init the prev insn addr */
     static int prev_insn_vma = -1;  /*init the prev insn vma */
     int curr_insn_vma = info->buffer_vma;
+    int special;
+    const char *special_name;
 
     info->bytes_per_chunk = 4;
 
@@ -799,12 +765,26 @@ print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
                      op->name, get_field_r1(inst), get_field_rfsl(inst));
         break;
     case INST_TYPE_RD_SPECIAL:
-        fprintf_func(stream, "%s\t" PRIreg ", %s",
-                     op->name, get_field_rd(inst), get_field_special(inst, op));
+        special = get_field_special(inst, op);
+        special_name = get_special_name(special);
+        if (special_name) {
+            fprintf_func(stream, "%s\t" PRIreg ", %s",
+                         op->name, get_field_rd(inst), special_name);
+        } else {
+            fprintf_func(stream, "%s\t" PRIreg ", " PRIpvr,
+                         op->name, get_field_rd(inst), special ^ REG_PVR_MASK);
+        }
         break;
     case INST_TYPE_SPECIAL_R1:
-        fprintf_func(stream, "%s\t%s, " PRIreg,
-                     op->name, get_field_special(inst, op), get_field_r1(inst));
+        special = get_field_special(inst, op);
+        special_name = get_special_name(special);
+        if (special_name) {
+            fprintf_func(stream, "%s\t%s, " PRIreg,
+                         op->name, special_name, get_field_r1(inst));
+        } else {
+            fprintf_func(stream, "%s\t" PRIpvr ", " PRIreg,
+                         op->name, special ^ REG_PVR_MASK, get_field_r1(inst));
+        }
         break;
     case INST_TYPE_RD_R1:
         fprintf_func(stream, "%s\t" PRIreg ", " PRIreg,
-- 
2.34.1

Re: [PATCH v3 20/27] disas/microblaze: Split get_field_special

[Pierrick Bouvier]

On 4/12/24 00:33, Richard Henderson wrote:
> Extract the raw special index and a function to lookup a name.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   disas/microblaze.c | 142 +++++++++++++++++++--------------------------
>   1 file changed, 61 insertions(+), 81 deletions(-)
> 
> diff --git a/disas/microblaze.c b/disas/microblaze.c
> index 24febfdea9..197327fae4 100644
> --- a/disas/microblaze.c
> +++ b/disas/microblaze.c
> @@ -564,8 +564,6 @@ static const struct op_code_struct {
>   
>   /* prefix for register names */
>   #define register_prefix "r"
> -static const char pvr_register_prefix[] = "rpvr";
> -
>   
>   /* #defines for valid immediate range */
>   #define MIN_IMM  ((int) 0x80000000)
> @@ -580,6 +578,7 @@ static const char pvr_register_prefix[] = "rpvr";
>   
>   #define PRIreg    register_prefix "%ld"
>   #define PRIrfsl   register_prefix "fsl%ld"
> +#define PRIpvr    register_prefix "pvr%d"
>   #define PRIimm    "%d"
>   
>   #define get_field_rd(instr)      ((instr & RD_MASK) >> RD_LOW)
> @@ -593,83 +592,48 @@ static const char pvr_register_prefix[] = "rpvr";
>   #define get_int_field_imm(instr) ((instr & IMM_MASK) >> IMM_LOW)
>   #define get_int_field_r1(instr) ((instr & RA_MASK) >> RA_LOW)
>   
> -/*
> -  char *
> -  get_field_special (instr)
> -  long instr;
> -  {
> -  char tmpstr[25];
> -
> -  snprintf(tmpstr, sizeof(tmpstr), "%s%s", register_prefix,
> -          (((instr & IMM_MASK) >> IMM_LOW) & REG_MSR_MASK) == 0 ? "pc" : "msr");
> -
> -  return(strdup(tmpstr));
> -  }
> -*/
> -
> -static char *
> -get_field_special(long instr, const struct op_code_struct *op)
> +static int get_field_special(long instr, const struct op_code_struct *op)
>   {
> -   char tmpstr[25];
> -   char spr[6];
> +    return ((instr & IMM_MASK) >> IMM_LOW) ^ op->immval_mask;
> +}
>   
> -   switch ( (((instr & IMM_MASK) >> IMM_LOW) ^ op->immval_mask) ) {
> -
> -   case REG_MSR_MASK :
> -      strcpy(spr, "msr");
> -      break;
> -   case REG_PC_MASK :
> -      strcpy(spr, "pc");
> -      break;
> -   case REG_EAR_MASK :
> -      strcpy(spr, "ear");
> -      break;
> -   case REG_ESR_MASK :
> -      strcpy(spr, "esr");
> -      break;
> -   case REG_FSR_MASK :
> -      strcpy(spr, "fsr");
> -      break;
> -   case REG_BTR_MASK :
> -      strcpy(spr, "btr");
> -      break;
> -   case REG_EDR_MASK :
> -      strcpy(spr, "edr");
> -      break;
> -   case REG_PID_MASK :
> -      strcpy(spr, "pid");
> -      break;
> -   case REG_ZPR_MASK :
> -      strcpy(spr, "zpr");
> -      break;
> -   case REG_TLBX_MASK :
> -      strcpy(spr, "tlbx");
> -      break;
> -   case REG_TLBLO_MASK :
> -      strcpy(spr, "tlblo");
> -      break;
> -   case REG_TLBHI_MASK :
> -      strcpy(spr, "tlbhi");
> -      break;
> -   case REG_TLBSX_MASK :
> -      strcpy(spr, "tlbsx");
> -      break;
> -   default :
> -     {
> -       if ( ((((instr & IMM_MASK) >> IMM_LOW) ^ op->immval_mask) & 0xE000) == REG_PVR_MASK) {
> -          snprintf(tmpstr, sizeof(tmpstr), "%s%u", pvr_register_prefix,
> -                 (unsigned short)(((instr & IMM_MASK) >> IMM_LOW) ^
> -                                  op->immval_mask) ^ REG_PVR_MASK);
> -	 return(strdup(tmpstr));
> -       } else {
> -	 strcpy(spr, "pc");
> -       }
> -     }
> -     break;
> -   }
> -
> -   snprintf(tmpstr, sizeof(tmpstr), "%s%s", register_prefix, spr);
> -   return(strdup(tmpstr));
> +/* Returns NULL for PVR registers, which should be rendered differently. */
> +static const char *get_special_name(int special)
> +{
> +    switch (special) {
> +    case REG_MSR_MASK:
> +        return register_prefix "msr";
> +    case REG_PC_MASK:
> +        return register_prefix "pc";
> +    case REG_EAR_MASK:
> +        return register_prefix "ear";
> +    case REG_ESR_MASK:
> +        return register_prefix "esr";
> +    case REG_FSR_MASK:
> +        return register_prefix "fsr";
> +    case REG_BTR_MASK:
> +        return register_prefix "btr";
> +    case REG_EDR_MASK:
> +        return register_prefix "edr";
> +    case REG_PID_MASK:
> +        return register_prefix "pid";
> +    case REG_ZPR_MASK:
> +        return register_prefix "zpr";
> +    case REG_TLBX_MASK:
> +        return register_prefix "tlbx";
> +    case REG_TLBLO_MASK:
> +        return register_prefix "tlblo";
> +    case REG_TLBHI_MASK:
> +        return register_prefix "tlbhi";
> +    case REG_TLBSX_MASK:
> +        return register_prefix "tlbsx";
> +    default:
> +        if ((special & 0xE000) == REG_PVR_MASK) {
> +            /* pvr register */
> +            return NULL;
> +        }
> +        return register_prefix "pc";
> +    }
>   }
>   
>   static unsigned long
> @@ -739,6 +703,8 @@ print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
>       static bfd_vma prev_insn_addr = -1; /*init the prev insn addr */
>       static int prev_insn_vma = -1;  /*init the prev insn vma */
>       int curr_insn_vma = info->buffer_vma;
> +    int special;
> +    const char *special_name;
>   
>       info->bytes_per_chunk = 4;
>   
> @@ -799,12 +765,26 @@ print_insn_microblaze(bfd_vma memaddr, struct disassemble_info *info)
>                        op->name, get_field_r1(inst), get_field_rfsl(inst));
>           break;
>       case INST_TYPE_RD_SPECIAL:
> -        fprintf_func(stream, "%s\t" PRIreg ", %s",
> -                     op->name, get_field_rd(inst), get_field_special(inst, op));
> +        special = get_field_special(inst, op);
> +        special_name = get_special_name(special);
> +        if (special_name) {
> +            fprintf_func(stream, "%s\t" PRIreg ", %s",
> +                         op->name, get_field_rd(inst), special_name);
> +        } else {
> +            fprintf_func(stream, "%s\t" PRIreg ", " PRIpvr,
> +                         op->name, get_field_rd(inst), special ^ REG_PVR_MASK);
> +        }
>           break;
>       case INST_TYPE_SPECIAL_R1:
> -        fprintf_func(stream, "%s\t%s, " PRIreg,
> -                     op->name, get_field_special(inst, op), get_field_r1(inst));
> +        special = get_field_special(inst, op);
> +        special_name = get_special_name(special);
> +        if (special_name) {
> +            fprintf_func(stream, "%s\t%s, " PRIreg,
> +                         op->name, special_name, get_field_r1(inst));
> +        } else {
> +            fprintf_func(stream, "%s\t" PRIpvr ", " PRIreg,
> +                         op->name, special ^ REG_PVR_MASK, get_field_r1(inst));
> +        }
>           break;
>       case INST_TYPE_RD_R1:
>           fprintf_func(stream, "%s\t" PRIreg ", " PRIreg,

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

[PATCH v3 21/27] disas/riscv: Use GString in format_inst

[Richard Henderson]

Allocate and fill a GString instead of snprintf and
appending to a fixed sized buffer.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 disas/riscv.c | 209 ++++++++++++++++++++++----------------------------
 1 file changed, 92 insertions(+), 117 deletions(-)

diff --git a/disas/riscv.c b/disas/riscv.c
index e236c8b5b7..f60f9bda43 100644
--- a/disas/riscv.c
+++ b/disas/riscv.c
@@ -4757,272 +4757,249 @@ static size_t inst_length(rv_inst inst)
 
 /* format instruction */
 
-static void append(char *s1, const char *s2, size_t n)
-{
-    size_t l1 = strlen(s1);
-    if (n - l1 - 1 > 0) {
-        strncat(s1, s2, n - l1);
-    }
-}
-
-static void format_inst(char *buf, size_t buflen, size_t tab, rv_decode *dec)
+static GString *format_inst(size_t tab, rv_decode *dec)
 {
     const rv_opcode_data *opcode_data = dec->opcode_data;
-    char tmp[64];
+    GString *buf = g_string_sized_new(64);
     const char *fmt;
 
     fmt = opcode_data[dec->op].format;
     while (*fmt) {
         switch (*fmt) {
         case 'O':
-            append(buf, opcode_data[dec->op].name, buflen);
+            g_string_append(buf, opcode_data[dec->op].name);
             break;
         case '(':
-            append(buf, "(", buflen);
-            break;
         case ',':
-            append(buf, ",", buflen);
-            break;
         case ')':
-            append(buf, ")", buflen);
-            break;
         case '-':
-            append(buf, "-", buflen);
+            g_string_append_c(buf, *fmt);
             break;
         case 'b':
-            snprintf(tmp, sizeof(tmp), "%d", dec->bs);
-            append(buf, tmp, buflen);
+            g_string_append_printf(buf, "%d", dec->bs);
             break;
         case 'n':
-            snprintf(tmp, sizeof(tmp), "%d", dec->rnum);
-            append(buf, tmp, buflen);
+            g_string_append_printf(buf, "%d", dec->rnum);
             break;
         case '0':
-            append(buf, rv_ireg_name_sym[dec->rd], buflen);
+            g_string_append(buf, rv_ireg_name_sym[dec->rd]);
             break;
         case '1':
-            append(buf, rv_ireg_name_sym[dec->rs1], buflen);
+            g_string_append(buf, rv_ireg_name_sym[dec->rs1]);
             break;
         case '2':
-            append(buf, rv_ireg_name_sym[dec->rs2], buflen);
+            g_string_append(buf, rv_ireg_name_sym[dec->rs2]);
             break;
         case '3':
-            append(buf, dec->cfg->ext_zfinx ? rv_ireg_name_sym[dec->rd] :
-                                              rv_freg_name_sym[dec->rd],
-                   buflen);
+            if (dec->cfg->ext_zfinx) {
+                g_string_append(buf, rv_ireg_name_sym[dec->rd]);
+            } else {
+                g_string_append(buf, rv_freg_name_sym[dec->rd]);
+            }
             break;
         case '4':
-            append(buf, dec->cfg->ext_zfinx ? rv_ireg_name_sym[dec->rs1] :
-                                              rv_freg_name_sym[dec->rs1],
-                   buflen);
+            if (dec->cfg->ext_zfinx) {
+                g_string_append(buf, rv_ireg_name_sym[dec->rs1]);
+            } else {
+                g_string_append(buf, rv_freg_name_sym[dec->rs1]);
+            }
             break;
         case '5':
-            append(buf, dec->cfg->ext_zfinx ? rv_ireg_name_sym[dec->rs2] :
-                                              rv_freg_name_sym[dec->rs2],
-                   buflen);
+            if (dec->cfg->ext_zfinx) {
+                g_string_append(buf, rv_ireg_name_sym[dec->rs2]);
+            } else {
+                g_string_append(buf, rv_freg_name_sym[dec->rs2]);
+            }
             break;
         case '6':
-            append(buf, dec->cfg->ext_zfinx ? rv_ireg_name_sym[dec->rs3] :
-                                              rv_freg_name_sym[dec->rs3],
-                   buflen);
+            if (dec->cfg->ext_zfinx) {
+                g_string_append(buf, rv_ireg_name_sym[dec->rs3]);
+            } else {
+                g_string_append(buf, rv_freg_name_sym[dec->rs3]);
+            }
             break;
         case '7':
-            snprintf(tmp, sizeof(tmp), "%d", dec->rs1);
-            append(buf, tmp, buflen);
+            g_string_append_printf(buf, "%d", dec->rs1);
             break;
         case 'i':
-            snprintf(tmp, sizeof(tmp), "%d", dec->imm);
-            append(buf, tmp, buflen);
+            g_string_append_printf(buf, "%d", dec->imm);
             break;
         case 'u':
-            snprintf(tmp, sizeof(tmp), "%u", ((uint32_t)dec->imm & 0b111111));
-            append(buf, tmp, buflen);
+            g_string_append_printf(buf, "%u", ((uint32_t)dec->imm & 0b111111));
             break;
         case 'j':
-            snprintf(tmp, sizeof(tmp), "%d", dec->imm1);
-            append(buf, tmp, buflen);
+            g_string_append_printf(buf, "%d", dec->imm1);
             break;
         case 'o':
-            snprintf(tmp, sizeof(tmp), "%d", dec->imm);
-            append(buf, tmp, buflen);
-            while (strlen(buf) < tab * 2) {
-                append(buf, " ", buflen);
+            g_string_append_printf(buf, "%d", dec->imm);
+            while (buf->len < tab * 2) {
+                g_string_append_c(buf, ' ');
             }
-            snprintf(tmp, sizeof(tmp), "# 0x%" PRIx64,
-                dec->pc + dec->imm);
-            append(buf, tmp, buflen);
+            g_string_append_printf(buf, "# 0x%" PRIx64, dec->pc + dec->imm);
             break;
         case 'U':
             fmt++;
-            snprintf(tmp, sizeof(tmp), "%d", dec->imm >> 12);
-            append(buf, tmp, buflen);
+            g_string_append_printf(buf, "%d", dec->imm >> 12);
             if (*fmt == 'o') {
-                while (strlen(buf) < tab * 2) {
-                    append(buf, " ", buflen);
+                while (buf->len < tab * 2) {
+                    g_string_append_c(buf, ' ');
                 }
-                snprintf(tmp, sizeof(tmp), "# 0x%" PRIx64,
-                    dec->pc + dec->imm);
-                append(buf, tmp, buflen);
+                g_string_append_printf(buf, "# 0x%" PRIx64, dec->pc + dec->imm);
             }
             break;
         case 'c': {
             const char *name = csr_name(dec->imm & 0xfff);
             if (name) {
-                append(buf, name, buflen);
+                g_string_append(buf, name);
             } else {
-                snprintf(tmp, sizeof(tmp), "0x%03x", dec->imm & 0xfff);
-                append(buf, tmp, buflen);
+                g_string_append_printf(buf, "0x%03x", dec->imm & 0xfff);
             }
             break;
         }
         case 'r':
             switch (dec->rm) {
             case rv_rm_rne:
-                append(buf, "rne", buflen);
+                g_string_append(buf, "rne");
                 break;
             case rv_rm_rtz:
-                append(buf, "rtz", buflen);
+                g_string_append(buf, "rtz");
                 break;
             case rv_rm_rdn:
-                append(buf, "rdn", buflen);
+                g_string_append(buf, "rdn");
                 break;
             case rv_rm_rup:
-                append(buf, "rup", buflen);
+                g_string_append(buf, "rup");
                 break;
             case rv_rm_rmm:
-                append(buf, "rmm", buflen);
+                g_string_append(buf, "rmm");
                 break;
             case rv_rm_dyn:
-                append(buf, "dyn", buflen);
+                g_string_append(buf, "dyn");
                 break;
             default:
-                append(buf, "inv", buflen);
+                g_string_append(buf, "inv");
                 break;
             }
             break;
         case 'p':
             if (dec->pred & rv_fence_i) {
-                append(buf, "i", buflen);
+                g_string_append_c(buf, 'i');
             }
             if (dec->pred & rv_fence_o) {
-                append(buf, "o", buflen);
+                g_string_append_c(buf, 'o');
             }
             if (dec->pred & rv_fence_r) {
-                append(buf, "r", buflen);
+                g_string_append_c(buf, 'r');
             }
             if (dec->pred & rv_fence_w) {
-                append(buf, "w", buflen);
+                g_string_append_c(buf, 'w');
             }
             break;
         case 's':
             if (dec->succ & rv_fence_i) {
-                append(buf, "i", buflen);
+                g_string_append_c(buf, 'i');
             }
             if (dec->succ & rv_fence_o) {
-                append(buf, "o", buflen);
+                g_string_append_c(buf, 'o');
             }
             if (dec->succ & rv_fence_r) {
-                append(buf, "r", buflen);
+                g_string_append_c(buf, 'r');
             }
             if (dec->succ & rv_fence_w) {
-                append(buf, "w", buflen);
+                g_string_append_c(buf, 'w');
             }
             break;
         case '\t':
-            while (strlen(buf) < tab) {
-                append(buf, " ", buflen);
+            while (buf->len < tab) {
+                g_string_append_c(buf, ' ');
             }
             break;
         case 'A':
             if (dec->aq) {
-                append(buf, ".aq", buflen);
+                g_string_append(buf, ".aq");
             }
             break;
         case 'R':
             if (dec->rl) {
-                append(buf, ".rl", buflen);
+                g_string_append(buf, ".rl");
             }
             break;
         case 'l':
-            append(buf, ",v0", buflen);
+            g_string_append(buf, ",v0");
             break;
         case 'm':
             if (dec->vm == 0) {
-                append(buf, ",v0.t", buflen);
+                g_string_append(buf, ",v0.t");
             }
             break;
         case 'D':
-            append(buf, rv_vreg_name_sym[dec->rd], buflen);
+            g_string_append(buf, rv_vreg_name_sym[dec->rd]);
             break;
         case 'E':
-            append(buf, rv_vreg_name_sym[dec->rs1], buflen);
+            g_string_append(buf, rv_vreg_name_sym[dec->rs1]);
             break;
         case 'F':
-            append(buf, rv_vreg_name_sym[dec->rs2], buflen);
+            g_string_append(buf, rv_vreg_name_sym[dec->rs2]);
             break;
         case 'G':
-            append(buf, rv_vreg_name_sym[dec->rs3], buflen);
+            g_string_append(buf, rv_vreg_name_sym[dec->rs3]);
             break;
         case 'v': {
-            char nbuf[32] = {0};
             const int sew = 1 << (((dec->vzimm >> 3) & 0b111) + 3);
-            sprintf(nbuf, "%d", sew);
             const int lmul = dec->vzimm & 0b11;
             const int flmul = (dec->vzimm >> 2) & 1;
             const char *vta = (dec->vzimm >> 6) & 1 ? "ta" : "tu";
             const char *vma = (dec->vzimm >> 7) & 1 ? "ma" : "mu";
-            append(buf, "e", buflen);
-            append(buf, nbuf, buflen);
-            append(buf, ",m", buflen);
+
+            g_string_append_printf(buf, "e%d,m", sew);
             if (flmul) {
                 switch (lmul) {
                 case 3:
-                    sprintf(nbuf, "f2");
+                    g_string_append(buf, "f2");
                     break;
                 case 2:
-                    sprintf(nbuf, "f4");
+                    g_string_append(buf, "f4");
                     break;
                 case 1:
-                    sprintf(nbuf, "f8");
-                break;
+                    g_string_append(buf, "f8");
+                    break;
                 }
-                append(buf, nbuf, buflen);
             } else {
-                sprintf(nbuf, "%d", 1 << lmul);
-                append(buf, nbuf, buflen);
+                g_string_append_printf(buf, "%d", 1 << lmul);
             }
-            append(buf, ",", buflen);
-            append(buf, vta, buflen);
-            append(buf, ",", buflen);
-            append(buf, vma, buflen);
+            g_string_append_c(buf, ',');
+            g_string_append(buf, vta);
+            g_string_append_c(buf, ',');
+            g_string_append(buf, vma);
             break;
         }
         case 'x': {
             switch (dec->rlist) {
             case 4:
-                snprintf(tmp, sizeof(tmp), "{ra}");
+                g_string_append(buf, "{ra}");
                 break;
             case 5:
-                snprintf(tmp, sizeof(tmp), "{ra, s0}");
+                g_string_append(buf, "{ra, s0}");
                 break;
             case 15:
-                snprintf(tmp, sizeof(tmp), "{ra, s0-s11}");
+                g_string_append(buf, "{ra, s0-s11}");
                 break;
             default:
-                snprintf(tmp, sizeof(tmp), "{ra, s0-s%d}", dec->rlist - 5);
+                g_string_append_printf(buf, "{ra, s0-s%d}", dec->rlist - 5);
                 break;
             }
-            append(buf, tmp, buflen);
             break;
         }
         case 'h':
-            append(buf, rv_fli_name_const[dec->imm], buflen);
+            g_string_append(buf, rv_fli_name_const[dec->imm]);
             break;
         default:
             break;
         }
         fmt++;
     }
+
+    return buf;
 }
 
 /* lift instruction to pseudo-instruction */
@@ -5108,9 +5085,8 @@ static void decode_inst_decompress(rv_decode *dec, rv_isa isa)
 
 /* disassemble instruction */
 
-static void
-disasm_inst(char *buf, size_t buflen, rv_isa isa, uint64_t pc, rv_inst inst,
-            RISCVCPUConfig *cfg)
+static GString *disasm_inst(rv_isa isa, uint64_t pc, rv_inst inst,
+                            RISCVCPUConfig *cfg)
 {
     rv_decode dec = { 0 };
     dec.pc = pc;
@@ -5157,7 +5133,7 @@ disasm_inst(char *buf, size_t buflen, rv_isa isa, uint64_t pc, rv_inst inst,
     decode_inst_operands(&dec, isa);
     decode_inst_decompress(&dec, isa);
     decode_inst_lift_pseudo(&dec);
-    format_inst(buf, buflen, 24, &dec);
+    return format_inst(24, &dec);
 }
 
 #define INST_FMT_2 "%04" PRIx64 "              "
@@ -5168,7 +5144,6 @@ disasm_inst(char *buf, size_t buflen, rv_isa isa, uint64_t pc, rv_inst inst,
 static int
 print_insn_riscv(bfd_vma memaddr, struct disassemble_info *info, rv_isa isa)
 {
-    char buf[128] = { 0 };
     bfd_byte packet[2];
     rv_inst inst = 0;
     size_t len = 2;
@@ -5209,9 +5184,9 @@ print_insn_riscv(bfd_vma memaddr, struct disassemble_info *info, rv_isa isa)
         }
     }
 
-    disasm_inst(buf, sizeof(buf), isa, memaddr, inst,
-                (RISCVCPUConfig *)info->target_info);
-    (*info->fprintf_func)(info->stream, "%s", buf);
+    g_autoptr(GString) str =
+        disasm_inst(isa, memaddr, inst, (RISCVCPUConfig *)info->target_info);
+    (*info->fprintf_func)(info->stream, "%s", str->str);
 
     return len;
 }
-- 
2.34.1

Re: [PATCH v3 21/27] disas/riscv: Use GString in format_inst

[Pierrick Bouvier]

On 4/12/24 00:33, Richard Henderson wrote:
> Allocate and fill a GString instead of snprintf and
> appending to a fixed sized buffer.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   disas/riscv.c | 209 ++++++++++++++++++++++----------------------------
>   1 file changed, 92 insertions(+), 117 deletions(-)
> 
> diff --git a/disas/riscv.c b/disas/riscv.c
> index e236c8b5b7..f60f9bda43 100644
> --- a/disas/riscv.c
> +++ b/disas/riscv.c
> @@ -4757,272 +4757,249 @@ static size_t inst_length(rv_inst inst)
>   
>   /* format instruction */
>   
> -static void append(char *s1, const char *s2, size_t n)
> -{
> -    size_t l1 = strlen(s1);
> -    if (n - l1 - 1 > 0) {
> -        strncat(s1, s2, n - l1);
> -    }
> -}
> -
> -static void format_inst(char *buf, size_t buflen, size_t tab, rv_decode *dec)
> +static GString *format_inst(size_t tab, rv_decode *dec)
>   {
>       const rv_opcode_data *opcode_data = dec->opcode_data;
> -    char tmp[64];
> +    GString *buf = g_string_sized_new(64);
>       const char *fmt;
>   
>       fmt = opcode_data[dec->op].format;
>       while (*fmt) {
>           switch (*fmt) {
>           case 'O':
> -            append(buf, opcode_data[dec->op].name, buflen);
> +            g_string_append(buf, opcode_data[dec->op].name);
>               break;
>           case '(':
> -            append(buf, "(", buflen);
> -            break;
>           case ',':
> -            append(buf, ",", buflen);
> -            break;
>           case ')':
> -            append(buf, ")", buflen);
> -            break;
>           case '-':
> -            append(buf, "-", buflen);
> +            g_string_append_c(buf, *fmt);
>               break;
>           case 'b':
> -            snprintf(tmp, sizeof(tmp), "%d", dec->bs);
> -            append(buf, tmp, buflen);
> +            g_string_append_printf(buf, "%d", dec->bs);
>               break;
>           case 'n':
> -            snprintf(tmp, sizeof(tmp), "%d", dec->rnum);
> -            append(buf, tmp, buflen);
> +            g_string_append_printf(buf, "%d", dec->rnum);
>               break;
>           case '0':
> -            append(buf, rv_ireg_name_sym[dec->rd], buflen);
> +            g_string_append(buf, rv_ireg_name_sym[dec->rd]);
>               break;
>           case '1':
> -            append(buf, rv_ireg_name_sym[dec->rs1], buflen);
> +            g_string_append(buf, rv_ireg_name_sym[dec->rs1]);
>               break;
>           case '2':
> -            append(buf, rv_ireg_name_sym[dec->rs2], buflen);
> +            g_string_append(buf, rv_ireg_name_sym[dec->rs2]);
>               break;
>           case '3':
> -            append(buf, dec->cfg->ext_zfinx ? rv_ireg_name_sym[dec->rd] :
> -                                              rv_freg_name_sym[dec->rd],
> -                   buflen);
> +            if (dec->cfg->ext_zfinx) {
> +                g_string_append(buf, rv_ireg_name_sym[dec->rd]);
> +            } else {
> +                g_string_append(buf, rv_freg_name_sym[dec->rd]);
> +            }
>               break;
>           case '4':
> -            append(buf, dec->cfg->ext_zfinx ? rv_ireg_name_sym[dec->rs1] :
> -                                              rv_freg_name_sym[dec->rs1],
> -                   buflen);
> +            if (dec->cfg->ext_zfinx) {
> +                g_string_append(buf, rv_ireg_name_sym[dec->rs1]);
> +            } else {
> +                g_string_append(buf, rv_freg_name_sym[dec->rs1]);
> +            }
>               break;
>           case '5':
> -            append(buf, dec->cfg->ext_zfinx ? rv_ireg_name_sym[dec->rs2] :
> -                                              rv_freg_name_sym[dec->rs2],
> -                   buflen);
> +            if (dec->cfg->ext_zfinx) {
> +                g_string_append(buf, rv_ireg_name_sym[dec->rs2]);
> +            } else {
> +                g_string_append(buf, rv_freg_name_sym[dec->rs2]);
> +            }
>               break;
>           case '6':
> -            append(buf, dec->cfg->ext_zfinx ? rv_ireg_name_sym[dec->rs3] :
> -                                              rv_freg_name_sym[dec->rs3],
> -                   buflen);
> +            if (dec->cfg->ext_zfinx) {
> +                g_string_append(buf, rv_ireg_name_sym[dec->rs3]);
> +            } else {
> +                g_string_append(buf, rv_freg_name_sym[dec->rs3]);
> +            }
>               break;
>           case '7':
> -            snprintf(tmp, sizeof(tmp), "%d", dec->rs1);
> -            append(buf, tmp, buflen);
> +            g_string_append_printf(buf, "%d", dec->rs1);
>               break;
>           case 'i':
> -            snprintf(tmp, sizeof(tmp), "%d", dec->imm);
> -            append(buf, tmp, buflen);
> +            g_string_append_printf(buf, "%d", dec->imm);
>               break;
>           case 'u':
> -            snprintf(tmp, sizeof(tmp), "%u", ((uint32_t)dec->imm & 0b111111));
> -            append(buf, tmp, buflen);
> +            g_string_append_printf(buf, "%u", ((uint32_t)dec->imm & 0b111111));
>               break;
>           case 'j':
> -            snprintf(tmp, sizeof(tmp), "%d", dec->imm1);
> -            append(buf, tmp, buflen);
> +            g_string_append_printf(buf, "%d", dec->imm1);
>               break;
>           case 'o':
> -            snprintf(tmp, sizeof(tmp), "%d", dec->imm);
> -            append(buf, tmp, buflen);
> -            while (strlen(buf) < tab * 2) {
> -                append(buf, " ", buflen);
> +            g_string_append_printf(buf, "%d", dec->imm);
> +            while (buf->len < tab * 2) {
> +                g_string_append_c(buf, ' ');
>               }
> -            snprintf(tmp, sizeof(tmp), "# 0x%" PRIx64,
> -                dec->pc + dec->imm);
> -            append(buf, tmp, buflen);
> +            g_string_append_printf(buf, "# 0x%" PRIx64, dec->pc + dec->imm);
>               break;
>           case 'U':
>               fmt++;
> -            snprintf(tmp, sizeof(tmp), "%d", dec->imm >> 12);
> -            append(buf, tmp, buflen);
> +            g_string_append_printf(buf, "%d", dec->imm >> 12);
>               if (*fmt == 'o') {
> -                while (strlen(buf) < tab * 2) {
> -                    append(buf, " ", buflen);
> +                while (buf->len < tab * 2) {
> +                    g_string_append_c(buf, ' ');
>                   }
> -                snprintf(tmp, sizeof(tmp), "# 0x%" PRIx64,
> -                    dec->pc + dec->imm);
> -                append(buf, tmp, buflen);
> +                g_string_append_printf(buf, "# 0x%" PRIx64, dec->pc + dec->imm);
>               }
>               break;
>           case 'c': {
>               const char *name = csr_name(dec->imm & 0xfff);
>               if (name) {
> -                append(buf, name, buflen);
> +                g_string_append(buf, name);
>               } else {
> -                snprintf(tmp, sizeof(tmp), "0x%03x", dec->imm & 0xfff);
> -                append(buf, tmp, buflen);
> +                g_string_append_printf(buf, "0x%03x", dec->imm & 0xfff);
>               }
>               break;
>           }
>           case 'r':
>               switch (dec->rm) {
>               case rv_rm_rne:
> -                append(buf, "rne", buflen);
> +                g_string_append(buf, "rne");
>                   break;
>               case rv_rm_rtz:
> -                append(buf, "rtz", buflen);
> +                g_string_append(buf, "rtz");
>                   break;
>               case rv_rm_rdn:
> -                append(buf, "rdn", buflen);
> +                g_string_append(buf, "rdn");
>                   break;
>               case rv_rm_rup:
> -                append(buf, "rup", buflen);
> +                g_string_append(buf, "rup");
>                   break;
>               case rv_rm_rmm:
> -                append(buf, "rmm", buflen);
> +                g_string_append(buf, "rmm");
>                   break;
>               case rv_rm_dyn:
> -                append(buf, "dyn", buflen);
> +                g_string_append(buf, "dyn");
>                   break;
>               default:
> -                append(buf, "inv", buflen);
> +                g_string_append(buf, "inv");
>                   break;
>               }
>               break;
>           case 'p':
>               if (dec->pred & rv_fence_i) {
> -                append(buf, "i", buflen);
> +                g_string_append_c(buf, 'i');
>               }
>               if (dec->pred & rv_fence_o) {
> -                append(buf, "o", buflen);
> +                g_string_append_c(buf, 'o');
>               }
>               if (dec->pred & rv_fence_r) {
> -                append(buf, "r", buflen);
> +                g_string_append_c(buf, 'r');
>               }
>               if (dec->pred & rv_fence_w) {
> -                append(buf, "w", buflen);
> +                g_string_append_c(buf, 'w');
>               }
>               break;
>           case 's':
>               if (dec->succ & rv_fence_i) {
> -                append(buf, "i", buflen);
> +                g_string_append_c(buf, 'i');
>               }
>               if (dec->succ & rv_fence_o) {
> -                append(buf, "o", buflen);
> +                g_string_append_c(buf, 'o');
>               }
>               if (dec->succ & rv_fence_r) {
> -                append(buf, "r", buflen);
> +                g_string_append_c(buf, 'r');
>               }
>               if (dec->succ & rv_fence_w) {
> -                append(buf, "w", buflen);
> +                g_string_append_c(buf, 'w');
>               }
>               break;
>           case '\t':
> -            while (strlen(buf) < tab) {
> -                append(buf, " ", buflen);
> +            while (buf->len < tab) {
> +                g_string_append_c(buf, ' ');
>               }
>               break;
>           case 'A':
>               if (dec->aq) {
> -                append(buf, ".aq", buflen);
> +                g_string_append(buf, ".aq");
>               }
>               break;
>           case 'R':
>               if (dec->rl) {
> -                append(buf, ".rl", buflen);
> +                g_string_append(buf, ".rl");
>               }
>               break;
>           case 'l':
> -            append(buf, ",v0", buflen);
> +            g_string_append(buf, ",v0");
>               break;
>           case 'm':
>               if (dec->vm == 0) {
> -                append(buf, ",v0.t", buflen);
> +                g_string_append(buf, ",v0.t");
>               }
>               break;
>           case 'D':
> -            append(buf, rv_vreg_name_sym[dec->rd], buflen);
> +            g_string_append(buf, rv_vreg_name_sym[dec->rd]);
>               break;
>           case 'E':
> -            append(buf, rv_vreg_name_sym[dec->rs1], buflen);
> +            g_string_append(buf, rv_vreg_name_sym[dec->rs1]);
>               break;
>           case 'F':
> -            append(buf, rv_vreg_name_sym[dec->rs2], buflen);
> +            g_string_append(buf, rv_vreg_name_sym[dec->rs2]);
>               break;
>           case 'G':
> -            append(buf, rv_vreg_name_sym[dec->rs3], buflen);
> +            g_string_append(buf, rv_vreg_name_sym[dec->rs3]);
>               break;
>           case 'v': {
> -            char nbuf[32] = {0};
>               const int sew = 1 << (((dec->vzimm >> 3) & 0b111) + 3);
> -            sprintf(nbuf, "%d", sew);
>               const int lmul = dec->vzimm & 0b11;
>               const int flmul = (dec->vzimm >> 2) & 1;
>               const char *vta = (dec->vzimm >> 6) & 1 ? "ta" : "tu";
>               const char *vma = (dec->vzimm >> 7) & 1 ? "ma" : "mu";
> -            append(buf, "e", buflen);
> -            append(buf, nbuf, buflen);
> -            append(buf, ",m", buflen);
> +
> +            g_string_append_printf(buf, "e%d,m", sew);
>               if (flmul) {
>                   switch (lmul) {
>                   case 3:
> -                    sprintf(nbuf, "f2");
> +                    g_string_append(buf, "f2");
>                       break;
>                   case 2:
> -                    sprintf(nbuf, "f4");
> +                    g_string_append(buf, "f4");
>                       break;
>                   case 1:
> -                    sprintf(nbuf, "f8");
> -                break;
> +                    g_string_append(buf, "f8");
> +                    break;
>                   }
> -                append(buf, nbuf, buflen);
>               } else {
> -                sprintf(nbuf, "%d", 1 << lmul);
> -                append(buf, nbuf, buflen);
> +                g_string_append_printf(buf, "%d", 1 << lmul);
>               }
> -            append(buf, ",", buflen);
> -            append(buf, vta, buflen);
> -            append(buf, ",", buflen);
> -            append(buf, vma, buflen);
> +            g_string_append_c(buf, ',');
> +            g_string_append(buf, vta);
> +            g_string_append_c(buf, ',');
> +            g_string_append(buf, vma);
>               break;
>           }
>           case 'x': {
>               switch (dec->rlist) {
>               case 4:
> -                snprintf(tmp, sizeof(tmp), "{ra}");
> +                g_string_append(buf, "{ra}");
>                   break;
>               case 5:
> -                snprintf(tmp, sizeof(tmp), "{ra, s0}");
> +                g_string_append(buf, "{ra, s0}");
>                   break;
>               case 15:
> -                snprintf(tmp, sizeof(tmp), "{ra, s0-s11}");
> +                g_string_append(buf, "{ra, s0-s11}");
>                   break;
>               default:
> -                snprintf(tmp, sizeof(tmp), "{ra, s0-s%d}", dec->rlist - 5);
> +                g_string_append_printf(buf, "{ra, s0-s%d}", dec->rlist - 5);
>                   break;
>               }
> -            append(buf, tmp, buflen);
>               break;
>           }
>           case 'h':
> -            append(buf, rv_fli_name_const[dec->imm], buflen);
> +            g_string_append(buf, rv_fli_name_const[dec->imm]);
>               break;
>           default:
>               break;
>           }
>           fmt++;
>       }
> +
> +    return buf;
>   }
>   
>   /* lift instruction to pseudo-instruction */
> @@ -5108,9 +5085,8 @@ static void decode_inst_decompress(rv_decode *dec, rv_isa isa)
>   
>   /* disassemble instruction */
>   
> -static void
> -disasm_inst(char *buf, size_t buflen, rv_isa isa, uint64_t pc, rv_inst inst,
> -            RISCVCPUConfig *cfg)
> +static GString *disasm_inst(rv_isa isa, uint64_t pc, rv_inst inst,
> +                            RISCVCPUConfig *cfg)
>   {
>       rv_decode dec = { 0 };
>       dec.pc = pc;
> @@ -5157,7 +5133,7 @@ disasm_inst(char *buf, size_t buflen, rv_isa isa, uint64_t pc, rv_inst inst,
>       decode_inst_operands(&dec, isa);
>       decode_inst_decompress(&dec, isa);
>       decode_inst_lift_pseudo(&dec);
> -    format_inst(buf, buflen, 24, &dec);
> +    return format_inst(24, &dec);
>   }
>   
>   #define INST_FMT_2 "%04" PRIx64 "              "
> @@ -5168,7 +5144,6 @@ disasm_inst(char *buf, size_t buflen, rv_isa isa, uint64_t pc, rv_inst inst,
>   static int
>   print_insn_riscv(bfd_vma memaddr, struct disassemble_info *info, rv_isa isa)
>   {
> -    char buf[128] = { 0 };
>       bfd_byte packet[2];
>       rv_inst inst = 0;
>       size_t len = 2;
> @@ -5209,9 +5184,9 @@ print_insn_riscv(bfd_vma memaddr, struct disassemble_info *info, rv_isa isa)
>           }
>       }
>   
> -    disasm_inst(buf, sizeof(buf), isa, memaddr, inst,
> -                (RISCVCPUConfig *)info->target_info);
> -    (*info->fprintf_func)(info->stream, "%s", buf);
> +    g_autoptr(GString) str =
> +        disasm_inst(isa, memaddr, inst, (RISCVCPUConfig *)info->target_info);
> +    (*info->fprintf_func)(info->stream, "%s", str->str);
>   
>       return len;
>   }

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH v3 21/27] disas/riscv: Use GString in format_inst

[Alistair Francis]

On Fri, Apr 12, 2024 at 5:37 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Allocate and fill a GString instead of snprintf and
> appending to a fixed sized buffer.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

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

Alistair

> ---
>  disas/riscv.c | 209 ++++++++++++++++++++++----------------------------
>  1 file changed, 92 insertions(+), 117 deletions(-)
>
> diff --git a/disas/riscv.c b/disas/riscv.c
> index e236c8b5b7..f60f9bda43 100644
> --- a/disas/riscv.c
> +++ b/disas/riscv.c
> @@ -4757,272 +4757,249 @@ static size_t inst_length(rv_inst inst)
>
>  /* format instruction */
>
> -static void append(char *s1, const char *s2, size_t n)
> -{
> -    size_t l1 = strlen(s1);
> -    if (n - l1 - 1 > 0) {
> -        strncat(s1, s2, n - l1);
> -    }
> -}
> -
> -static void format_inst(char *buf, size_t buflen, size_t tab, rv_decode *dec)
> +static GString *format_inst(size_t tab, rv_decode *dec)
>  {
>      const rv_opcode_data *opcode_data = dec->opcode_data;
> -    char tmp[64];
> +    GString *buf = g_string_sized_new(64);
>      const char *fmt;
>
>      fmt = opcode_data[dec->op].format;
>      while (*fmt) {
>          switch (*fmt) {
>          case 'O':
> -            append(buf, opcode_data[dec->op].name, buflen);
> +            g_string_append(buf, opcode_data[dec->op].name);
>              break;
>          case '(':
> -            append(buf, "(", buflen);
> -            break;
>          case ',':
> -            append(buf, ",", buflen);
> -            break;
>          case ')':
> -            append(buf, ")", buflen);
> -            break;
>          case '-':
> -            append(buf, "-", buflen);
> +            g_string_append_c(buf, *fmt);
>              break;
>          case 'b':
> -            snprintf(tmp, sizeof(tmp), "%d", dec->bs);
> -            append(buf, tmp, buflen);
> +            g_string_append_printf(buf, "%d", dec->bs);
>              break;
>          case 'n':
> -            snprintf(tmp, sizeof(tmp), "%d", dec->rnum);
> -            append(buf, tmp, buflen);
> +            g_string_append_printf(buf, "%d", dec->rnum);
>              break;
>          case '0':
> -            append(buf, rv_ireg_name_sym[dec->rd], buflen);
> +            g_string_append(buf, rv_ireg_name_sym[dec->rd]);
>              break;
>          case '1':
> -            append(buf, rv_ireg_name_sym[dec->rs1], buflen);
> +            g_string_append(buf, rv_ireg_name_sym[dec->rs1]);
>              break;
>          case '2':
> -            append(buf, rv_ireg_name_sym[dec->rs2], buflen);
> +            g_string_append(buf, rv_ireg_name_sym[dec->rs2]);
>              break;
>          case '3':
> -            append(buf, dec->cfg->ext_zfinx ? rv_ireg_name_sym[dec->rd] :
> -                                              rv_freg_name_sym[dec->rd],
> -                   buflen);
> +            if (dec->cfg->ext_zfinx) {
> +                g_string_append(buf, rv_ireg_name_sym[dec->rd]);
> +            } else {
> +                g_string_append(buf, rv_freg_name_sym[dec->rd]);
> +            }
>              break;
>          case '4':
> -            append(buf, dec->cfg->ext_zfinx ? rv_ireg_name_sym[dec->rs1] :
> -                                              rv_freg_name_sym[dec->rs1],
> -                   buflen);
> +            if (dec->cfg->ext_zfinx) {
> +                g_string_append(buf, rv_ireg_name_sym[dec->rs1]);
> +            } else {
> +                g_string_append(buf, rv_freg_name_sym[dec->rs1]);
> +            }
>              break;
>          case '5':
> -            append(buf, dec->cfg->ext_zfinx ? rv_ireg_name_sym[dec->rs2] :
> -                                              rv_freg_name_sym[dec->rs2],
> -                   buflen);
> +            if (dec->cfg->ext_zfinx) {
> +                g_string_append(buf, rv_ireg_name_sym[dec->rs2]);
> +            } else {
> +                g_string_append(buf, rv_freg_name_sym[dec->rs2]);
> +            }
>              break;
>          case '6':
> -            append(buf, dec->cfg->ext_zfinx ? rv_ireg_name_sym[dec->rs3] :
> -                                              rv_freg_name_sym[dec->rs3],
> -                   buflen);
> +            if (dec->cfg->ext_zfinx) {
> +                g_string_append(buf, rv_ireg_name_sym[dec->rs3]);
> +            } else {
> +                g_string_append(buf, rv_freg_name_sym[dec->rs3]);
> +            }
>              break;
>          case '7':
> -            snprintf(tmp, sizeof(tmp), "%d", dec->rs1);
> -            append(buf, tmp, buflen);
> +            g_string_append_printf(buf, "%d", dec->rs1);
>              break;
>          case 'i':
> -            snprintf(tmp, sizeof(tmp), "%d", dec->imm);
> -            append(buf, tmp, buflen);
> +            g_string_append_printf(buf, "%d", dec->imm);
>              break;
>          case 'u':
> -            snprintf(tmp, sizeof(tmp), "%u", ((uint32_t)dec->imm & 0b111111));
> -            append(buf, tmp, buflen);
> +            g_string_append_printf(buf, "%u", ((uint32_t)dec->imm & 0b111111));
>              break;
>          case 'j':
> -            snprintf(tmp, sizeof(tmp), "%d", dec->imm1);
> -            append(buf, tmp, buflen);
> +            g_string_append_printf(buf, "%d", dec->imm1);
>              break;
>          case 'o':
> -            snprintf(tmp, sizeof(tmp), "%d", dec->imm);
> -            append(buf, tmp, buflen);
> -            while (strlen(buf) < tab * 2) {
> -                append(buf, " ", buflen);
> +            g_string_append_printf(buf, "%d", dec->imm);
> +            while (buf->len < tab * 2) {
> +                g_string_append_c(buf, ' ');
>              }
> -            snprintf(tmp, sizeof(tmp), "# 0x%" PRIx64,
> -                dec->pc + dec->imm);
> -            append(buf, tmp, buflen);
> +            g_string_append_printf(buf, "# 0x%" PRIx64, dec->pc + dec->imm);
>              break;
>          case 'U':
>              fmt++;
> -            snprintf(tmp, sizeof(tmp), "%d", dec->imm >> 12);
> -            append(buf, tmp, buflen);
> +            g_string_append_printf(buf, "%d", dec->imm >> 12);
>              if (*fmt == 'o') {
> -                while (strlen(buf) < tab * 2) {
> -                    append(buf, " ", buflen);
> +                while (buf->len < tab * 2) {
> +                    g_string_append_c(buf, ' ');
>                  }
> -                snprintf(tmp, sizeof(tmp), "# 0x%" PRIx64,
> -                    dec->pc + dec->imm);
> -                append(buf, tmp, buflen);
> +                g_string_append_printf(buf, "# 0x%" PRIx64, dec->pc + dec->imm);
>              }
>              break;
>          case 'c': {
>              const char *name = csr_name(dec->imm & 0xfff);
>              if (name) {
> -                append(buf, name, buflen);
> +                g_string_append(buf, name);
>              } else {
> -                snprintf(tmp, sizeof(tmp), "0x%03x", dec->imm & 0xfff);
> -                append(buf, tmp, buflen);
> +                g_string_append_printf(buf, "0x%03x", dec->imm & 0xfff);
>              }
>              break;
>          }
>          case 'r':
>              switch (dec->rm) {
>              case rv_rm_rne:
> -                append(buf, "rne", buflen);
> +                g_string_append(buf, "rne");
>                  break;
>              case rv_rm_rtz:
> -                append(buf, "rtz", buflen);
> +                g_string_append(buf, "rtz");
>                  break;
>              case rv_rm_rdn:
> -                append(buf, "rdn", buflen);
> +                g_string_append(buf, "rdn");
>                  break;
>              case rv_rm_rup:
> -                append(buf, "rup", buflen);
> +                g_string_append(buf, "rup");
>                  break;
>              case rv_rm_rmm:
> -                append(buf, "rmm", buflen);
> +                g_string_append(buf, "rmm");
>                  break;
>              case rv_rm_dyn:
> -                append(buf, "dyn", buflen);
> +                g_string_append(buf, "dyn");
>                  break;
>              default:
> -                append(buf, "inv", buflen);
> +                g_string_append(buf, "inv");
>                  break;
>              }
>              break;
>          case 'p':
>              if (dec->pred & rv_fence_i) {
> -                append(buf, "i", buflen);
> +                g_string_append_c(buf, 'i');
>              }
>              if (dec->pred & rv_fence_o) {
> -                append(buf, "o", buflen);
> +                g_string_append_c(buf, 'o');
>              }
>              if (dec->pred & rv_fence_r) {
> -                append(buf, "r", buflen);
> +                g_string_append_c(buf, 'r');
>              }
>              if (dec->pred & rv_fence_w) {
> -                append(buf, "w", buflen);
> +                g_string_append_c(buf, 'w');
>              }
>              break;
>          case 's':
>              if (dec->succ & rv_fence_i) {
> -                append(buf, "i", buflen);
> +                g_string_append_c(buf, 'i');
>              }
>              if (dec->succ & rv_fence_o) {
> -                append(buf, "o", buflen);
> +                g_string_append_c(buf, 'o');
>              }
>              if (dec->succ & rv_fence_r) {
> -                append(buf, "r", buflen);
> +                g_string_append_c(buf, 'r');
>              }
>              if (dec->succ & rv_fence_w) {
> -                append(buf, "w", buflen);
> +                g_string_append_c(buf, 'w');
>              }
>              break;
>          case '\t':
> -            while (strlen(buf) < tab) {
> -                append(buf, " ", buflen);
> +            while (buf->len < tab) {
> +                g_string_append_c(buf, ' ');
>              }
>              break;
>          case 'A':
>              if (dec->aq) {
> -                append(buf, ".aq", buflen);
> +                g_string_append(buf, ".aq");
>              }
>              break;
>          case 'R':
>              if (dec->rl) {
> -                append(buf, ".rl", buflen);
> +                g_string_append(buf, ".rl");
>              }
>              break;
>          case 'l':
> -            append(buf, ",v0", buflen);
> +            g_string_append(buf, ",v0");
>              break;
>          case 'm':
>              if (dec->vm == 0) {
> -                append(buf, ",v0.t", buflen);
> +                g_string_append(buf, ",v0.t");
>              }
>              break;
>          case 'D':
> -            append(buf, rv_vreg_name_sym[dec->rd], buflen);
> +            g_string_append(buf, rv_vreg_name_sym[dec->rd]);
>              break;
>          case 'E':
> -            append(buf, rv_vreg_name_sym[dec->rs1], buflen);
> +            g_string_append(buf, rv_vreg_name_sym[dec->rs1]);
>              break;
>          case 'F':
> -            append(buf, rv_vreg_name_sym[dec->rs2], buflen);
> +            g_string_append(buf, rv_vreg_name_sym[dec->rs2]);
>              break;
>          case 'G':
> -            append(buf, rv_vreg_name_sym[dec->rs3], buflen);
> +            g_string_append(buf, rv_vreg_name_sym[dec->rs3]);
>              break;
>          case 'v': {
> -            char nbuf[32] = {0};
>              const int sew = 1 << (((dec->vzimm >> 3) & 0b111) + 3);
> -            sprintf(nbuf, "%d", sew);
>              const int lmul = dec->vzimm & 0b11;
>              const int flmul = (dec->vzimm >> 2) & 1;
>              const char *vta = (dec->vzimm >> 6) & 1 ? "ta" : "tu";
>              const char *vma = (dec->vzimm >> 7) & 1 ? "ma" : "mu";
> -            append(buf, "e", buflen);
> -            append(buf, nbuf, buflen);
> -            append(buf, ",m", buflen);
> +
> +            g_string_append_printf(buf, "e%d,m", sew);
>              if (flmul) {
>                  switch (lmul) {
>                  case 3:
> -                    sprintf(nbuf, "f2");
> +                    g_string_append(buf, "f2");
>                      break;
>                  case 2:
> -                    sprintf(nbuf, "f4");
> +                    g_string_append(buf, "f4");
>                      break;
>                  case 1:
> -                    sprintf(nbuf, "f8");
> -                break;
> +                    g_string_append(buf, "f8");
> +                    break;
>                  }
> -                append(buf, nbuf, buflen);
>              } else {
> -                sprintf(nbuf, "%d", 1 << lmul);
> -                append(buf, nbuf, buflen);
> +                g_string_append_printf(buf, "%d", 1 << lmul);
>              }
> -            append(buf, ",", buflen);
> -            append(buf, vta, buflen);
> -            append(buf, ",", buflen);
> -            append(buf, vma, buflen);
> +            g_string_append_c(buf, ',');
> +            g_string_append(buf, vta);
> +            g_string_append_c(buf, ',');
> +            g_string_append(buf, vma);
>              break;
>          }
>          case 'x': {
>              switch (dec->rlist) {
>              case 4:
> -                snprintf(tmp, sizeof(tmp), "{ra}");
> +                g_string_append(buf, "{ra}");
>                  break;
>              case 5:
> -                snprintf(tmp, sizeof(tmp), "{ra, s0}");
> +                g_string_append(buf, "{ra, s0}");
>                  break;
>              case 15:
> -                snprintf(tmp, sizeof(tmp), "{ra, s0-s11}");
> +                g_string_append(buf, "{ra, s0-s11}");
>                  break;
>              default:
> -                snprintf(tmp, sizeof(tmp), "{ra, s0-s%d}", dec->rlist - 5);
> +                g_string_append_printf(buf, "{ra, s0-s%d}", dec->rlist - 5);
>                  break;
>              }
> -            append(buf, tmp, buflen);
>              break;
>          }
>          case 'h':
> -            append(buf, rv_fli_name_const[dec->imm], buflen);
> +            g_string_append(buf, rv_fli_name_const[dec->imm]);
>              break;
>          default:
>              break;
>          }
>          fmt++;
>      }
> +
> +    return buf;
>  }
>
>  /* lift instruction to pseudo-instruction */
> @@ -5108,9 +5085,8 @@ static void decode_inst_decompress(rv_decode *dec, rv_isa isa)
>
>  /* disassemble instruction */
>
> -static void
> -disasm_inst(char *buf, size_t buflen, rv_isa isa, uint64_t pc, rv_inst inst,
> -            RISCVCPUConfig *cfg)
> +static GString *disasm_inst(rv_isa isa, uint64_t pc, rv_inst inst,
> +                            RISCVCPUConfig *cfg)
>  {
>      rv_decode dec = { 0 };
>      dec.pc = pc;
> @@ -5157,7 +5133,7 @@ disasm_inst(char *buf, size_t buflen, rv_isa isa, uint64_t pc, rv_inst inst,
>      decode_inst_operands(&dec, isa);
>      decode_inst_decompress(&dec, isa);
>      decode_inst_lift_pseudo(&dec);
> -    format_inst(buf, buflen, 24, &dec);
> +    return format_inst(24, &dec);
>  }
>
>  #define INST_FMT_2 "%04" PRIx64 "              "
> @@ -5168,7 +5144,6 @@ disasm_inst(char *buf, size_t buflen, rv_isa isa, uint64_t pc, rv_inst inst,
>  static int
>  print_insn_riscv(bfd_vma memaddr, struct disassemble_info *info, rv_isa isa)
>  {
> -    char buf[128] = { 0 };
>      bfd_byte packet[2];
>      rv_inst inst = 0;
>      size_t len = 2;
> @@ -5209,9 +5184,9 @@ print_insn_riscv(bfd_vma memaddr, struct disassemble_info *info, rv_isa isa)
>          }
>      }
>
> -    disasm_inst(buf, sizeof(buf), isa, memaddr, inst,
> -                (RISCVCPUConfig *)info->target_info);
> -    (*info->fprintf_func)(info->stream, "%s", buf);
> +    g_autoptr(GString) str =
> +        disasm_inst(isa, memaddr, inst, (RISCVCPUConfig *)info->target_info);
> +    (*info->fprintf_func)(info->stream, "%s", str->str);
>
>      return len;
>  }
> --
> 2.34.1
>
>

[PATCH v3 22/27] linux-user/flatload: Replace sprintf() by snprintf()

[Richard Henderson]

From: Philippe Mathieu-Daudé <philmd@linaro.org>

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience. Use snprintf() instead.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-Id: <20240411104340.6617-5-philmd@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/flatload.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/flatload.c b/linux-user/flatload.c
index 5b62aa0a2b..1bb2f400bc 100644
--- a/linux-user/flatload.c
+++ b/linux-user/flatload.c
@@ -692,7 +692,7 @@ static int load_flat_shared_library(int id, struct lib_info *libs)
 	char buf[16];
 
 	/* Create the file name */
-	sprintf(buf, "/lib/lib%d.so", id);
+        snprintf(buf, sizeof(buf), "/lib/lib%d.so", id);
 
 	/* Open the file up */
 	bprm.filename = buf;
-- 
2.34.1

Re: [PATCH v3 22/27] linux-user/flatload: Replace sprintf() by snprintf()

[Peter Maydell]

On Fri, 12 Apr 2024 at 08:36, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
>
> sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
> resulting in painful developper experience. Use snprintf() instead.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Message-Id: <20240411104340.6617-5-philmd@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Rather than this, I think we should use my patch which
drops this dead code entirely:

https://patchew.org/QEMU/20240411115313.680433-1-peter.maydell@linaro.org/

-- PMM

Re: [PATCH v3 22/27] linux-user/flatload: Replace sprintf() by snprintf()

[Pierrick Bouvier]

On 4/12/24 00:33, Richard Henderson wrote:
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
> resulting in painful developper experience. Use snprintf() instead.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Message-Id: <20240411104340.6617-5-philmd@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   linux-user/flatload.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/linux-user/flatload.c b/linux-user/flatload.c
> index 5b62aa0a2b..1bb2f400bc 100644
> --- a/linux-user/flatload.c
> +++ b/linux-user/flatload.c
> @@ -692,7 +692,7 @@ static int load_flat_shared_library(int id, struct lib_info *libs)
>   	char buf[16];
>   
>   	/* Create the file name */
> -	sprintf(buf, "/lib/lib%d.so", id);
> +        snprintf(buf, sizeof(buf), "/lib/lib%d.so", id);
>   
>   	/* Open the file up */
>   	bprm.filename = buf;

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

[PATCH v3 23/27] hw/misc/imx: Replace sprintf() by snprintf()

[Richard Henderson]

From: Philippe Mathieu-Daudé <philmd@linaro.org>

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience. Use snprintf() instead.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Message-Id: <20240411104340.6617-6-philmd@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/misc/imx25_ccm.c  | 2 +-
 hw/misc/imx31_ccm.c  | 2 +-
 hw/misc/imx6_ccm.c   | 4 ++--
 hw/misc/imx6_src.c   | 2 +-
 hw/misc/imx6ul_ccm.c | 4 ++--
 hw/misc/imx7_src.c   | 2 +-
 hw/net/imx_fec.c     | 2 +-
 hw/ssi/imx_spi.c     | 2 +-
 8 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/hw/misc/imx25_ccm.c b/hw/misc/imx25_ccm.c
index d888966014..faa726a86a 100644
--- a/hw/misc/imx25_ccm.c
+++ b/hw/misc/imx25_ccm.c
@@ -91,7 +91,7 @@ static const char *imx25_ccm_reg_name(uint32_t reg)
     case IMX25_CCM_LPIMR1_REG:
         return "lpimr1";
     default:
-        sprintf(unknown, "[%u ?]", reg);
+        snprintf(unknown, sizeof(unknown), "[%u ?]", reg);
         return unknown;
     }
 }
diff --git a/hw/misc/imx31_ccm.c b/hw/misc/imx31_ccm.c
index a9059bb1f7..125d4fceeb 100644
--- a/hw/misc/imx31_ccm.c
+++ b/hw/misc/imx31_ccm.c
@@ -89,7 +89,7 @@ static const char *imx31_ccm_reg_name(uint32_t reg)
     case IMX31_CCM_PDR2_REG:
         return "PDR2";
     default:
-        sprintf(unknown, "[%u ?]", reg);
+        snprintf(unknown, sizeof(unknown), "[%u ?]", reg);
         return unknown;
     }
 }
diff --git a/hw/misc/imx6_ccm.c b/hw/misc/imx6_ccm.c
index 56489d8b57..b1def7f05b 100644
--- a/hw/misc/imx6_ccm.c
+++ b/hw/misc/imx6_ccm.c
@@ -85,7 +85,7 @@ static const char *imx6_ccm_reg_name(uint32_t reg)
     case CCM_CMEOR:
         return "CMEOR";
     default:
-        sprintf(unknown, "%u ?", reg);
+        snprintf(unknown, sizeof(unknown), "%u ?", reg);
         return unknown;
     }
 }
@@ -224,7 +224,7 @@ static const char *imx6_analog_reg_name(uint32_t reg)
     case USB_ANALOG_DIGPROG:
         return "USB_ANALOG_DIGPROG";
     default:
-        sprintf(unknown, "%u ?", reg);
+        snprintf(unknown, sizeof(unknown), "%u ?", reg);
         return unknown;
     }
 }
diff --git a/hw/misc/imx6_src.c b/hw/misc/imx6_src.c
index 0c6003559f..3766bdf561 100644
--- a/hw/misc/imx6_src.c
+++ b/hw/misc/imx6_src.c
@@ -68,7 +68,7 @@ static const char *imx6_src_reg_name(uint32_t reg)
     case SRC_GPR10:
         return "SRC_GPR10";
     default:
-        sprintf(unknown, "%u ?", reg);
+        snprintf(unknown, sizeof(unknown), "%u ?", reg);
         return unknown;
     }
 }
diff --git a/hw/misc/imx6ul_ccm.c b/hw/misc/imx6ul_ccm.c
index bbc0be9921..0ac49ea34b 100644
--- a/hw/misc/imx6ul_ccm.c
+++ b/hw/misc/imx6ul_ccm.c
@@ -143,7 +143,7 @@ static const char *imx6ul_ccm_reg_name(uint32_t reg)
     case CCM_CMEOR:
         return "CMEOR";
     default:
-        sprintf(unknown, "%u ?", reg);
+        snprintf(unknown, sizeof(unknown), "%u ?", reg);
         return unknown;
     }
 }
@@ -274,7 +274,7 @@ static const char *imx6ul_analog_reg_name(uint32_t reg)
     case USB_ANALOG_DIGPROG:
         return "USB_ANALOG_DIGPROG";
     default:
-        sprintf(unknown, "%u ?", reg);
+        snprintf(unknown, sizeof(unknown), "%u ?", reg);
         return unknown;
     }
 }
diff --git a/hw/misc/imx7_src.c b/hw/misc/imx7_src.c
index b3725ff6e7..d19f0450d4 100644
--- a/hw/misc/imx7_src.c
+++ b/hw/misc/imx7_src.c
@@ -75,7 +75,7 @@ static const char *imx7_src_reg_name(uint32_t reg)
     case SRC_GPR10:
         return "SRC_GPR10";
     default:
-        sprintf(unknown, "%u ?", reg);
+        snprintf(unknown, sizeof(unknown), "%u ?", reg);
         return unknown;
     }
 }
diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index cee84af7ba..8c91d20d44 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -41,7 +41,7 @@
 static const char *imx_default_reg_name(IMXFECState *s, uint32_t index)
 {
     static char tmp[20];
-    sprintf(tmp, "index %d", index);
+    snprintf(tmp, sizeof(tmp), "index %d", index);
     return tmp;
 }
 
diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
index d8a7583ff3..12d897d306 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -53,7 +53,7 @@ static const char *imx_spi_reg_name(uint32_t reg)
     case ECSPI_MSGDATA:
         return  "ECSPI_MSGDATA";
     default:
-        sprintf(unknown, "%u ?", reg);
+        snprintf(unknown, sizeof(unknown), "%u ?", reg);
         return unknown;
     }
 }
-- 
2.34.1

[PATCH v3 24/27] hw/net/rocker: Replace sprintf() by snprintf()

[Richard Henderson]

From: Philippe Mathieu-Daudé <philmd@linaro.org>

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience. Use snprintf() instead.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-Id: <20240411104340.6617-7-philmd@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/net/rocker/rocker.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
index 7ea8eb6ba5..03ac7a7ae9 100644
--- a/hw/net/rocker/rocker.c
+++ b/hw/net/rocker/rocker.c
@@ -915,40 +915,40 @@ static const char *rocker_reg_name(void *opaque, hwaddr addr)
 
         switch (index) {
         case 0:
-            sprintf(ring_name, "cmd");
+            snprintf(ring_name, sizeof(ring_name), "cmd");
             break;
         case 1:
-            sprintf(ring_name, "event");
+            snprintf(ring_name, sizeof(ring_name), "event");
             break;
         default:
-            sprintf(ring_name, "%s-%d", index % 2 ? "rx" : "tx",
-                    (index - 2) / 2);
+            snprintf(ring_name, sizeof(ring_name), "%s-%d",
+                     index % 2 ? "rx" : "tx", (index - 2) / 2);
         }
 
         switch (offset) {
         case ROCKER_DMA_DESC_ADDR_OFFSET:
-            sprintf(buf, "Ring[%s] ADDR", ring_name);
+            snprintf(buf, sizeofbuf), "Ring[%s] ADDR", ring_name);
             return buf;
         case ROCKER_DMA_DESC_ADDR_OFFSET+4:
-            sprintf(buf, "Ring[%s] ADDR+4", ring_name);
+            snprintf(buf, sizeofbuf), "Ring[%s] ADDR+4", ring_name);
             return buf;
         case ROCKER_DMA_DESC_SIZE_OFFSET:
-            sprintf(buf, "Ring[%s] SIZE", ring_name);
+            snprintf(buf, sizeofbuf), "Ring[%s] SIZE", ring_name);
             return buf;
         case ROCKER_DMA_DESC_HEAD_OFFSET:
-            sprintf(buf, "Ring[%s] HEAD", ring_name);
+            snprintf(buf, sizeofbuf), "Ring[%s] HEAD", ring_name);
             return buf;
         case ROCKER_DMA_DESC_TAIL_OFFSET:
-            sprintf(buf, "Ring[%s] TAIL", ring_name);
+            snprintf(buf, sizeofbuf), "Ring[%s] TAIL", ring_name);
             return buf;
         case ROCKER_DMA_DESC_CTRL_OFFSET:
-            sprintf(buf, "Ring[%s] CTRL", ring_name);
+            snprintf(buf, sizeofbuf), "Ring[%s] CTRL", ring_name);
             return buf;
         case ROCKER_DMA_DESC_CREDITS_OFFSET:
-            sprintf(buf, "Ring[%s] CREDITS", ring_name);
+            snprintf(buf, sizeofbuf), "Ring[%s] CREDITS", ring_name);
             return buf;
         default:
-            sprintf(buf, "Ring[%s] ???", ring_name);
+            snprintf(buf, sizeofbuf), "Ring[%s] ???", ring_name);
             return buf;
         }
     } else {
-- 
2.34.1

Re: [PATCH v3 24/27] hw/net/rocker: Replace sprintf() by snprintf()

[Philippe Mathieu-Daudé]

On 12/4/24 09:33, Richard Henderson wrote:
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
> resulting in painful developper experience. Use snprintf() instead.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Message-Id: <20240411104340.6617-7-philmd@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   hw/net/rocker/rocker.c | 24 ++++++++++++------------
>   1 file changed, 12 insertions(+), 12 deletions(-)


>           switch (offset) {
>           case ROCKER_DMA_DESC_ADDR_OFFSET:
> -            sprintf(buf, "Ring[%s] ADDR", ring_name);
> +            snprintf(buf, sizeofbuf), "Ring[%s] ADDR", ring_name);

Ideally we should convert the DEBUG_FOO guards to trace events,
to avoid to maintain dead code.

Re: [PATCH v3 24/27] hw/net/rocker: Replace sprintf() by snprintf()

[Richard Henderson]

On 4/12/24 03:58, Philippe Mathieu-Daudé wrote:
> On 12/4/24 09:33, Richard Henderson wrote:
>> From: Philippe Mathieu-Daudé <philmd@linaro.org>
>>
>> sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
>> resulting in painful developper experience. Use snprintf() instead.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Message-Id: <20240411104340.6617-7-philmd@linaro.org>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   hw/net/rocker/rocker.c | 24 ++++++++++++------------
>>   1 file changed, 12 insertions(+), 12 deletions(-)
> 
> 
>>           switch (offset) {
>>           case ROCKER_DMA_DESC_ADDR_OFFSET:
>> -            sprintf(buf, "Ring[%s] ADDR", ring_name);
>> +            snprintf(buf, sizeofbuf), "Ring[%s] ADDR", ring_name);
> 
> Ideally we should convert the DEBUG_FOO guards to trace events,
> to avoid to maintain dead code.

Grr, I knew there was another of these that needed fixing up.


r~

[PATCH v3 25/27] hw/riscv/virt: Replace sprintf by g_strdup_printf

[Richard Henderson]

From: Philippe Mathieu-Daudé <philmd@linaro.org>

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1.
Use g_strdup_printf instead.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-Id: <20240411104340.6617-8-philmd@linaro.org>
[rth: Use g_strdup_printf]
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/riscv/virt.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index d171e74f7b..4fdb660525 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1617,10 +1617,8 @@ static void virt_machine_instance_init(Object *obj)
 static char *virt_get_aia_guests(Object *obj, Error **errp)
 {
     RISCVVirtState *s = RISCV_VIRT_MACHINE(obj);
-    char val[32];
 
-    sprintf(val, "%d", s->aia_guests);
-    return g_strdup(val);
+    return g_strdup_printf("%d", s->aia_guests);
 }
 
 static void virt_set_aia_guests(Object *obj, const char *val, Error **errp)
@@ -1741,7 +1739,6 @@ static void virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,
 
 static void virt_machine_class_init(ObjectClass *oc, void *data)
 {
-    char str[128];
     MachineClass *mc = MACHINE_CLASS(oc);
     HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
 
@@ -1767,7 +1764,6 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
     machine_class_allow_dynamic_sysbus_dev(mc, TYPE_TPM_TIS_SYSBUS);
 #endif
 
-
     object_class_property_add_bool(oc, "aclint", virt_get_aclint,
                                    virt_set_aclint);
     object_class_property_set_description(oc, "aclint",
@@ -1785,9 +1781,14 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
     object_class_property_add_str(oc, "aia-guests",
                                   virt_get_aia_guests,
                                   virt_set_aia_guests);
-    sprintf(str, "Set number of guest MMIO pages for AIA IMSIC. Valid value "
-                 "should be between 0 and %d.", VIRT_IRQCHIP_MAX_GUESTS);
-    object_class_property_set_description(oc, "aia-guests", str);
+    {
+        g_autofree char *str =
+            g_strdup_printf("Set number of guest MMIO pages for AIA IMSIC. "
+                            "Valid value should be between 0 and %d.",
+                            VIRT_IRQCHIP_MAX_GUESTS);
+        object_class_property_set_description(oc, "aia-guests", str);
+    }
+
     object_class_property_add(oc, "acpi", "OnOffAuto",
                               virt_get_acpi, virt_set_acpi,
                               NULL, NULL);
-- 
2.34.1

Re: [PATCH v3 25/27] hw/riscv/virt: Replace sprintf by g_strdup_printf

[Pierrick Bouvier]

On 4/12/24 00:33, Richard Henderson wrote:
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1.
> Use g_strdup_printf instead.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Message-Id: <20240411104340.6617-8-philmd@linaro.org>
> [rth: Use g_strdup_printf]
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   hw/riscv/virt.c | 17 +++++++++--------
>   1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index d171e74f7b..4fdb660525 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -1617,10 +1617,8 @@ static void virt_machine_instance_init(Object *obj)
>   static char *virt_get_aia_guests(Object *obj, Error **errp)
>   {
>       RISCVVirtState *s = RISCV_VIRT_MACHINE(obj);
> -    char val[32];
>   
> -    sprintf(val, "%d", s->aia_guests);
> -    return g_strdup(val);
> +    return g_strdup_printf("%d", s->aia_guests);
>   }
>   
>   static void virt_set_aia_guests(Object *obj, const char *val, Error **errp)
> @@ -1741,7 +1739,6 @@ static void virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>   
>   static void virt_machine_class_init(ObjectClass *oc, void *data)
>   {
> -    char str[128];
>       MachineClass *mc = MACHINE_CLASS(oc);
>       HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
>   
> @@ -1767,7 +1764,6 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>       machine_class_allow_dynamic_sysbus_dev(mc, TYPE_TPM_TIS_SYSBUS);
>   #endif
>   
> -
>       object_class_property_add_bool(oc, "aclint", virt_get_aclint,
>                                      virt_set_aclint);
>       object_class_property_set_description(oc, "aclint",
> @@ -1785,9 +1781,14 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>       object_class_property_add_str(oc, "aia-guests",
>                                     virt_get_aia_guests,
>                                     virt_set_aia_guests);
> -    sprintf(str, "Set number of guest MMIO pages for AIA IMSIC. Valid value "
> -                 "should be between 0 and %d.", VIRT_IRQCHIP_MAX_GUESTS);
> -    object_class_property_set_description(oc, "aia-guests", str);
> +    {
> +        g_autofree char *str =
> +            g_strdup_printf("Set number of guest MMIO pages for AIA IMSIC. "
> +                            "Valid value should be between 0 and %d.",
> +                            VIRT_IRQCHIP_MAX_GUESTS);
> +        object_class_property_set_description(oc, "aia-guests", str);
> +    }
> +
>       object_class_property_add(oc, "acpi", "OnOffAuto",
>                                 virt_get_acpi, virt_set_acpi,
>                                 NULL, NULL);

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH v3 25/27] hw/riscv/virt: Replace sprintf by g_strdup_printf

[Alistair Francis]

On Fri, Apr 12, 2024 at 5:36 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
>
> sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1.
> Use g_strdup_printf instead.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Message-Id: <20240411104340.6617-8-philmd@linaro.org>
> [rth: Use g_strdup_printf]
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

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

Alistair

> ---
>  hw/riscv/virt.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index d171e74f7b..4fdb660525 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -1617,10 +1617,8 @@ static void virt_machine_instance_init(Object *obj)
>  static char *virt_get_aia_guests(Object *obj, Error **errp)
>  {
>      RISCVVirtState *s = RISCV_VIRT_MACHINE(obj);
> -    char val[32];
>
> -    sprintf(val, "%d", s->aia_guests);
> -    return g_strdup(val);
> +    return g_strdup_printf("%d", s->aia_guests);
>  }
>
>  static void virt_set_aia_guests(Object *obj, const char *val, Error **errp)
> @@ -1741,7 +1739,6 @@ static void virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>
>  static void virt_machine_class_init(ObjectClass *oc, void *data)
>  {
> -    char str[128];
>      MachineClass *mc = MACHINE_CLASS(oc);
>      HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
>
> @@ -1767,7 +1764,6 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>      machine_class_allow_dynamic_sysbus_dev(mc, TYPE_TPM_TIS_SYSBUS);
>  #endif
>
> -
>      object_class_property_add_bool(oc, "aclint", virt_get_aclint,
>                                     virt_set_aclint);
>      object_class_property_set_description(oc, "aclint",
> @@ -1785,9 +1781,14 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>      object_class_property_add_str(oc, "aia-guests",
>                                    virt_get_aia_guests,
>                                    virt_set_aia_guests);
> -    sprintf(str, "Set number of guest MMIO pages for AIA IMSIC. Valid value "
> -                 "should be between 0 and %d.", VIRT_IRQCHIP_MAX_GUESTS);
> -    object_class_property_set_description(oc, "aia-guests", str);
> +    {
> +        g_autofree char *str =
> +            g_strdup_printf("Set number of guest MMIO pages for AIA IMSIC. "
> +                            "Valid value should be between 0 and %d.",
> +                            VIRT_IRQCHIP_MAX_GUESTS);
> +        object_class_property_set_description(oc, "aia-guests", str);
> +    }
> +
>      object_class_property_add(oc, "acpi", "OnOffAuto",
>                                virt_get_acpi, virt_set_acpi,
>                                NULL, NULL);
> --
> 2.34.1
>
>

[PATCH v3 26/27] target/arm: Replace sprintf() by snprintf()

[Richard Henderson]

From: Philippe Mathieu-Daudé <philmd@linaro.org>

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience. Use snprintf() instead.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Message-Id: <20240411104340.6617-9-philmd@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/cpu64.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 985b1efe16..f0f4fe6714 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -433,7 +433,7 @@ void aarch64_add_sve_properties(Object *obj)
 
     for (vq = 1; vq <= ARM_MAX_VQ; ++vq) {
         char name[8];
-        sprintf(name, "sve%d", vq * 128);
+        snprintf(name, sizeof(name), "sve%d", vq * 128);
         object_property_add(obj, name, "bool", cpu_arm_get_vq,
                             cpu_arm_set_vq, NULL, &cpu->sve_vq);
     }
@@ -458,7 +458,7 @@ void aarch64_add_sme_properties(Object *obj)
 
     for (vq = 1; vq <= ARM_MAX_VQ; vq <<= 1) {
         char name[8];
-        sprintf(name, "sme%d", vq * 128);
+        snprintf(name, sizeof(name), "sme%d", vq * 128);
         object_property_add(obj, name, "bool", cpu_arm_get_vq,
                             cpu_arm_set_vq, NULL, &cpu->sme_vq);
     }
-- 
2.34.1

[PATCH v3 27/27] target/i386/kvm: Improve KVM_EXIT_NOTIFY warnings

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/i386/kvm/kvm.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index e68cbe9293..1fc809b2f9 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -5275,7 +5275,6 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
     uint64_t code;
     int ret;
     bool ctx_invalid;
-    char str[256];
     KVMState *state;
 
     switch (run->exit_reason) {
@@ -5335,15 +5334,16 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
     case KVM_EXIT_NOTIFY:
         ctx_invalid = !!(run->notify.flags & KVM_NOTIFY_CONTEXT_INVALID);
         state = KVM_STATE(current_accel());
-        sprintf(str, "Encounter a notify exit with %svalid context in"
-                     " guest. There can be possible misbehaves in guest."
-                     " Please have a look.", ctx_invalid ? "in" : "");
         if (ctx_invalid ||
             state->notify_vmexit == NOTIFY_VMEXIT_OPTION_INTERNAL_ERROR) {
-            warn_report("KVM internal error: %s", str);
+            warn_report("KVM internal error: Encountered a notify exit "
+                        "with invalid context in guest.");
             ret = -1;
         } else {
-            warn_report_once("KVM: %s", str);
+            warn_report_once("KVM: Encounterd a notify exit with valid "
+                             "context in guest. "
+                             "The guest could be misbehaving. "
+                             "Please have a look.");
             ret = 0;
         }
         break;
-- 
2.34.1

Re: [PATCH v3 27/27] target/i386/kvm: Improve KVM_EXIT_NOTIFY warnings

[Philippe Mathieu-Daudé]

On 12/4/24 09:33, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/i386/kvm/kvm.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v3 27/27] target/i386/kvm: Improve KVM_EXIT_NOTIFY warnings

[Peter Maydell]

On Fri, 12 Apr 2024 at 08:37, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/i386/kvm/kvm.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index e68cbe9293..1fc809b2f9 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -5275,7 +5275,6 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
>      uint64_t code;
>      int ret;
>      bool ctx_invalid;
> -    char str[256];
>      KVMState *state;
>
>      switch (run->exit_reason) {
> @@ -5335,15 +5334,16 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
>      case KVM_EXIT_NOTIFY:
>          ctx_invalid = !!(run->notify.flags & KVM_NOTIFY_CONTEXT_INVALID);
>          state = KVM_STATE(current_accel());
> -        sprintf(str, "Encounter a notify exit with %svalid context in"
> -                     " guest. There can be possible misbehaves in guest."
> -                     " Please have a look.", ctx_invalid ? "in" : "");
>          if (ctx_invalid ||
>              state->notify_vmexit == NOTIFY_VMEXIT_OPTION_INTERNAL_ERROR) {
> -            warn_report("KVM internal error: %s", str);
> +            warn_report("KVM internal error: Encountered a notify exit "
> +                        "with invalid context in guest.");
>              ret = -1;
>          } else {
> -            warn_report_once("KVM: %s", str);
> +            warn_report_once("KVM: Encounterd a notify exit with valid "

"Encountered"

> +                             "context in guest. "
> +                             "The guest could be misbehaving. "
> +                             "Please have a look.");

"Please have a look" is a bit more chatty than we usually are
in warning messages...

thanks
-- PMM

Re: [PATCH v3 27/27] target/i386/kvm: Improve KVM_EXIT_NOTIFY warnings

[Zhao Liu]

On Fri, Apr 12, 2024 at 12:33:46AM -0700, Richard Henderson wrote:
> Date: Fri, 12 Apr 2024 00:33:46 -0700
> From: Richard Henderson <richard.henderson@linaro.org>
> Subject: [PATCH v3 27/27] target/i386/kvm: Improve KVM_EXIT_NOTIFY warnings
> X-Mailer: git-send-email 2.34.1
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/i386/kvm/kvm.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>