[PATCH v2] Fix %#0 misuses

Dr. David Alan Gilbert (git) posted 1 patch 2 years, 3 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20220202183417.116478-1-dgilbert@redhat.com
Maintainers: Alistair Francis <alistair.francis@wdc.com>, Richard Henderson <richard.henderson@linaro.org>, Samuel Thibault <samuel.thibault@ens-lyon.org>, Artyom Tarasenko <atar4qemu@gmail.com>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, Stafford Horne <shorne@gmail.com>, David Gibson <david@gibson.dropbear.id.au>, Peter Maydell <peter.maydell@linaro.org>, Paolo Bonzini <pbonzini@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>
chardev/baum.c          | 2 +-
disas/alpha.c           | 2 +-
disas/sparc.c           | 2 +-
hw/arm/omap1.c          | 2 +-
hw/timer/a9gtimer.c     | 4 ++--
include/hw/arm/omap.h   | 5 +++--
softmmu/device_tree.c   | 2 +-
target/openrisc/disas.c | 2 +-
8 files changed, 11 insertions(+), 10 deletions(-)
[PATCH v2] Fix %#0 misuses
Posted by Dr. David Alan Gilbert (git) 2 years, 3 months ago
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Joe Perches pointed out on lkml [1] that the format special %# (which
adds 0x) is included in the character count, i.e.:

  printf("0: %#08x\n0: %#08x\n", 0xabcdef01,1);
gives:
0: 0xabcdef01
0: 0x000001

rather than padding to the expected 8 data characters.

Replace all the '%#08' cases by '0x%08' and
also handle a '%#02'; there are some other cases mostly
in testing and a few that look like the authors have
thought about the size, that I've ignored for now.

(Note I've not managed to test most of these)

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

[1] https://lore.kernel.org/lkml/9499203f1e993872b384aabdec59ac223a8ab931.camel@perches.com/

Fixup %#
---
 chardev/baum.c          | 2 +-
 disas/alpha.c           | 2 +-
 disas/sparc.c           | 2 +-
 hw/arm/omap1.c          | 2 +-
 hw/timer/a9gtimer.c     | 4 ++--
 include/hw/arm/omap.h   | 5 +++--
 softmmu/device_tree.c   | 2 +-
 target/openrisc/disas.c | 2 +-
 8 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/chardev/baum.c b/chardev/baum.c
index 79d618e350..9a842c2bbb 100644
--- a/chardev/baum.c
+++ b/chardev/baum.c
@@ -366,7 +366,7 @@ static int baum_eat_packet(BaumChardev *baum, const uint8_t *buf, int len)
         if (!len--) \
             return 0; \
         if (*cur++ != ESC) { \
-            DPRINTF("Broken packet %#2x, tossing\n", req); \
+            DPRINTF("Broken packet 0x%02x, tossing\n", req); \
             if (timer_pending(baum->cellCount_timer)) {    \
                 timer_del(baum->cellCount_timer);     \
                 baum_cellCount_timer_cb(baum);             \
diff --git a/disas/alpha.c b/disas/alpha.c
index 3db90fa665..f218bf023a 100644
--- a/disas/alpha.c
+++ b/disas/alpha.c
@@ -1851,7 +1851,7 @@ print_insn_alpha (bfd_vma memaddr, struct disassemble_info *info)
     }
 
   /* No instruction found */
-  (*info->fprintf_func) (info->stream, ".long %#08x", insn);
+  (*info->fprintf_func) (info->stream, ".long 0x%08x", insn);
 
   return 4;
 
diff --git a/disas/sparc.c b/disas/sparc.c
index 5689533ce1..adc8438aa2 100644
--- a/disas/sparc.c
+++ b/disas/sparc.c
@@ -3231,6 +3231,6 @@ print_insn_sparc (bfd_vma memaddr, disassemble_info *info)
     }
 
   info->insn_type = dis_noninsn;        /* Mark as non-valid instruction.  */
-  (*info->fprintf_func) (stream, ".long %#08lx", insn);
+  (*info->fprintf_func) (stream, ".long 0x%08lx", insn);
   return sizeof (buffer);
 }
diff --git a/hw/arm/omap1.c b/hw/arm/omap1.c
index 9852c2a07e..c9e0ae87b1 100644
--- a/hw/arm/omap1.c
+++ b/hw/arm/omap1.c
@@ -42,7 +42,7 @@
 
 static inline void omap_log_badwidth(const char *funcname, hwaddr addr, int sz)
 {
-    qemu_log_mask(LOG_GUEST_ERROR, "%s: %d-bit register %#08" HWADDR_PRIx "\n",
+    qemu_log_mask(LOG_GUEST_ERROR, "%s: %d-bit register 0x%08" HWADDR_PRIx "\n",
                   funcname, 8 * sz, addr);
 }
 
diff --git a/hw/timer/a9gtimer.c b/hw/timer/a9gtimer.c
index 7233068a37..2ea51a137e 100644
--- a/hw/timer/a9gtimer.c
+++ b/hw/timer/a9gtimer.c
@@ -169,7 +169,7 @@ static uint64_t a9_gtimer_read(void *opaque, hwaddr addr, unsigned size)
         return 0;
     }
 
-    DB_PRINT("addr:%#x data:%#08" PRIx64 "\n", (unsigned)addr, ret);
+    DB_PRINT("addr:0x%x data:0x%08" PRIx64 "\n", (unsigned)addr, ret);
     return ret;
 }
 
@@ -180,7 +180,7 @@ static void a9_gtimer_write(void *opaque, hwaddr addr, uint64_t value,
     A9GTimerState *s = gtb->parent;
     int shift = 0;
 
-    DB_PRINT("addr:%#x data:%#08" PRIx64 "\n", (unsigned)addr, value);
+    DB_PRINT("addr:0x%x data:0x%08" PRIx64 "\n", (unsigned)addr, value);
 
     switch (addr) {
     case R_COUNTER_HI:
diff --git a/include/hw/arm/omap.h b/include/hw/arm/omap.h
index ff6a173f8a..8b1c2365d5 100644
--- a/include/hw/arm/omap.h
+++ b/include/hw/arm/omap.h
@@ -1003,10 +1003,11 @@ void omap_badwidth_write32(void *opaque, hwaddr addr,
 void omap_mpu_wakeup(void *opaque, int irq, int req);
 
 # define OMAP_BAD_REG(paddr)		\
-        qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad register %#08"HWADDR_PRIx"\n", \
+        qemu_log_mask(LOG_GUEST_ERROR,  \
+                      "%s: Bad register 0x%08"HWADDR_PRIx"\n", \
                       __func__, paddr)
 # define OMAP_RO_REG(paddr)		\
-        qemu_log_mask(LOG_GUEST_ERROR, "%s: Read-only register %#08" \
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: Read-only register 0x%08" \
                                        HWADDR_PRIx "\n", \
                       __func__, paddr)
 
diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
index 6ca3fad285..338e1c03fd 100644
--- a/softmmu/device_tree.c
+++ b/softmmu/device_tree.c
@@ -369,7 +369,7 @@ int qemu_fdt_setprop_cell(void *fdt, const char *node_path,
 
     r = fdt_setprop_cell(fdt, findnode_nofail(fdt, node_path), property, val);
     if (r < 0) {
-        error_report("%s: Couldn't set %s/%s = %#08x: %s", __func__,
+        error_report("%s: Couldn't set %s/%s = 0x%08x: %s", __func__,
                      node_path, property, val, fdt_strerror(r));
         exit(1);
     }
diff --git a/target/openrisc/disas.c b/target/openrisc/disas.c
index dc025bd64d..72f1476a13 100644
--- a/target/openrisc/disas.c
+++ b/target/openrisc/disas.c
@@ -45,7 +45,7 @@ int print_insn_or1k(bfd_vma addr, disassemble_info *info)
     insn = bfd_getb32(buffer);
 
     if (!decode(info, insn)) {
-        output(".long", "%#08x", insn);
+        output(".long", "0x%08x", insn);
     }
     return 4;
 }
-- 
2.34.1


Re: [PATCH v2] Fix %#0 misuses
Posted by Philippe Mathieu-Daudé via 2 years, 3 months ago
On 2/2/22 19:34, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Joe Perches pointed out on lkml [1] that the format special %# (which
> adds 0x) is included in the character count, i.e.:
> 
>    printf("0: %#08x\n0: %#08x\n", 0xabcdef01,1);
> gives:
> 0: 0xabcdef01
> 0: 0x000001
> 
> rather than padding to the expected 8 data characters.
> 
> Replace all the '%#08' cases by '0x%08' and
> also handle a '%#02'; there are some other cases mostly
> in testing and a few that look like the authors have
> thought about the size, that I've ignored for now.
> 
> (Note I've not managed to test most of these)
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> [1] https://lore.kernel.org/lkml/9499203f1e993872b384aabdec59ac223a8ab931.camel@perches.com/
> 
> Fixup %#
> ---
>   chardev/baum.c          | 2 +-
>   disas/alpha.c           | 2 +-
>   disas/sparc.c           | 2 +-
>   hw/arm/omap1.c          | 2 +-
>   hw/timer/a9gtimer.c     | 4 ++--
>   include/hw/arm/omap.h   | 5 +++--
>   softmmu/device_tree.c   | 2 +-
>   target/openrisc/disas.c | 2 +-
>   8 files changed, 11 insertions(+), 10 deletions(-)

Few more:

$ git grep -E '\%#[0-9]{1,2}[dDxX]'
chardev/baum.c:369:            DPRINTF("Broken packet %#2x, tossing\n", 
req); \
disas/alpha.c:1854:  (*info->fprintf_func) (info->stream, ".long %#08x", 
insn);
hw/audio/ac97.c:244:    dolog ("bd %2d addr=%#x ctl=%#06x len=%#x(%d 
bytes)\n",
hw/dma/i8257.c:265:        linfo ("write_cont: nport %#06x, ichan % 2d, 
val %#06x\n",
hw/dma/i8257.c:290:    ldebug ("read_cont: nport %#06x, iport %#04x val 
%#x\n", nport, iport, val);
softmmu/device_tree.c:372:        error_report("%s: Couldn't set %s/%s = 
%#08x: %s", __func__,
target/openrisc/disas.c:48:        output(".long", "%#08x", insn);
tests/qemu-iotests/nbd-fault-injector.py:187: 
print('unrecognized command type %#02x' % req.type)
tests/tcg/arm/fcvt.c:60:    printf("%02d   HALF: %#04x  (%#x => %s)\n",
tests/tcg/multiarch/libs/float_helpers.c:72:    asprintf(&fmt, 
"f16(%#04x)", num);

In mood to add a regexp to checkpatch? :)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v2] Fix %#0 misuses
Posted by Dr. David Alan Gilbert 2 years, 2 months ago
* Philippe Mathieu-Daudé (f4bug@amsat.org) wrote:
> On 2/2/22 19:34, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Joe Perches pointed out on lkml [1] that the format special %# (which
> > adds 0x) is included in the character count, i.e.:
> > 
> >    printf("0: %#08x\n0: %#08x\n", 0xabcdef01,1);
> > gives:
> > 0: 0xabcdef01
> > 0: 0x000001
> > 
> > rather than padding to the expected 8 data characters.
> > 
> > Replace all the '%#08' cases by '0x%08' and
> > also handle a '%#02'; there are some other cases mostly
> > in testing and a few that look like the authors have
> > thought about the size, that I've ignored for now.
> > 
> > (Note I've not managed to test most of these)
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > 
> > [1] https://lore.kernel.org/lkml/9499203f1e993872b384aabdec59ac223a8ab931.camel@perches.com/
> > 
> > Fixup %#
> > ---
> >   chardev/baum.c          | 2 +-
> >   disas/alpha.c           | 2 +-
> >   disas/sparc.c           | 2 +-
> >   hw/arm/omap1.c          | 2 +-
> >   hw/timer/a9gtimer.c     | 4 ++--
> >   include/hw/arm/omap.h   | 5 +++--
> >   softmmu/device_tree.c   | 2 +-
> >   target/openrisc/disas.c | 2 +-
> >   8 files changed, 11 insertions(+), 10 deletions(-)
> 
> Few more:
> 
> $ git grep -E '\%#[0-9]{1,2}[dDxX]'

Thanks, actually 1,3 is worth it :-)

> chardev/baum.c:369:            DPRINTF("Broken packet %#2x, tossing\n",
> req); \

Interestingly without the 0

> disas/alpha.c:1854:  (*info->fprintf_func) (info->stream, ".long %#08x",
> insn);
> hw/audio/ac97.c:244:    dolog ("bd %2d addr=%#x ctl=%#06x len=%#x(%d
> bytes)\n",
> hw/dma/i8257.c:265:        linfo ("write_cont: nport %#06x, ichan % 2d, val
> %#06x\n",
> hw/dma/i8257.c:290:    ldebug ("read_cont: nport %#06x, iport %#04x val
> %#x\n", nport, iport, val);

So some of these are a bit weird and might need thinking about; I
*think* they're 06 because the author realised the 0x was counted and
bumped it up from 04; but that actually means I'd need to understand the
device code before changing it.

> softmmu/device_tree.c:372:        error_report("%s: Couldn't set %s/%s =
> %#08x: %s", __func__,
> target/openrisc/disas.c:48:        output(".long", "%#08x", insn);
> tests/qemu-iotests/nbd-fault-injector.py:187: print('unrecognized command
> type %#02x' % req.type)
> tests/tcg/arm/fcvt.c:60:    printf("%02d   HALF: %#04x  (%#x => %s)\n",
> tests/tcg/multiarch/libs/float_helpers.c:72:    asprintf(&fmt, "f16(%#04x)",
> num);
> 
> In mood to add a regexp to checkpatch? :)

I think it already has one for new patches.

> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Thanks!

Dave

-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK