Re: [PATCH v2 2/2] hw/timer/hpet: Fix DPRINTF format string

[Dov Murik <dovmurik@linux.vnet.ibm.com>]

Hi Phil,

On 10/09/2020 16:58, Philippe Mathieu-Daudé wrote:
> Fix building with HPET_DEBUG enabled:
> 
>    hw/timer/hpet.c:512:73: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat]
>        DPRINTF("qemu: Enter hpet_ram_writel at %" PRIx64 " = %#x\n", addr, value);
>                                                              ~~~           ^~~~~
>                                                              %#lx
>    hw/timer/hpet.c:655:21: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat]
>                        value, s->hpet_counter);
>                        ^~~~~
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Dov Murik <dovmurik@linux.vnet.ibm.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   hw/timer/hpet.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
> index b683f64f1d3..20bd0388740 100644
> --- a/hw/timer/hpet.c
> +++ b/hw/timer/hpet.c
> @@ -495,7 +495,8 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
>       HPETState *s = opaque;
>       uint64_t old_val, new_val, val, index;
> 
> -    DPRINTF("qemu: Enter hpet_ram_writel at %" PRIx64 " = %#x\n", addr, value);
> +    DPRINTF("qemu: Enter hpet_ram_writel at %#" HWADDR_PRIx " = %#"PRIx64"\n",
> +            addr, value);

You still use "#" in the format string; but qemu's CODING_STYLE.rst says:

//////////////

'#' printf flag
---------------

Do not use printf flag '#', like '%#x'.

Rationale: there are two ways to add a '0x' prefix to printed number: 
'0x%...'
and '%#...'. For consistency the only one way should be used. Arguments for
'0x%' are:

* it is more popular
* '%#' omits the 0x for the value 0 which makes output inconsistent

//////////////



According to that, I think the better solution would be:

DPRINTF("qemu: Enter hpet_ram_writel at 0x%" HWADDR_PRIx
         " = 0x%" PRIx64 "\n", addr, value);




>       index = addr;
>       old_val = hpet_ram_read(opaque, addr, 4);
>       new_val = value;
> @@ -637,7 +638,8 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
>               }
>               s->hpet_counter =
>                   (s->hpet_counter & 0xffffffff00000000ULL) | value;
> -            DPRINTF("qemu: HPET counter written. ctr = %#x -> %" PRIx64 "\n",
> +            DPRINTF("qemu: HPET counter written. ctr = %#"
> +                    PRIx64 " -> %#" PRIx64 "\n",

ditto.

>                       value, s->hpet_counter);
>               break;
>           case HPET_COUNTER + 4:
> @@ -646,7 +648,8 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
>               }
>               s->hpet_counter =
>                   (s->hpet_counter & 0xffffffffULL) | (((uint64_t)value) << 32);
> -            DPRINTF("qemu: HPET counter + 4 written. ctr = %#x -> %" PRIx64 "\n",
> +            DPRINTF("qemu: HPET counter + 4 written. ctr = %#"
> +                    PRIx64 " -> %#" PRIx64 "\n",

ditto.

>                       value, s->hpet_counter);
>               break;
>           default:
> 

-Dov