1. Patchew
  2. QEMU
  3. View series

[Qemu-devel] [PATCH] hw/timer/mt48t59: Fix bit-rotten NVRAM_PRINTF format strings

Thomas Huth posted 1 patch 8 years ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1517559331-6388-1-git-send-email-thuth@redhat.com
Test checkpatch passed
Test docker-build@min-glib passed
Test docker-mingw@fedora passed
Test docker-quick@centos6 passed
Test ppc passed
Test s390x passed
hw/timer/m48t59-internal.h | 9 +++------
hw/timer/m48t59.c          | 4 ++--
2 files changed, 5 insertions(+), 8 deletions(-)
[Qemu-devel] [PATCH] hw/timer/mt48t59: Fix bit-rotten NVRAM_PRINTF format strings
Posted by Thomas Huth 8 years ago 
When compiling with NVRAM_PRINTF enabled, gcc currently bails out with:

  CC      hw/timer/m48t59.o
  CC      hw/timer/m48t59-isa.o
hw/timer/m48t59.c: In function ‘NVRAM_writeb’:
hw/timer/m48t59.c:460:5: error: format ‘%x’ expects argument of type ‘unsigned int’, but argument 3 has type ‘hwaddr’ [-Werror=format=]
     NVRAM_PRINTF("%s: 0x%08x => 0x%08x\n", __func__, addr, val);
     ^
hw/timer/m48t59.c:460:5: error: format ‘%x’ expects argument of type ‘unsigned int’, but argument 4 has type ‘uint64_t’ [-Werror=format=]
hw/timer/m48t59.c: In function ‘NVRAM_readb’:
hw/timer/m48t59.c:492:5: error: format ‘%x’ expects argument of type ‘unsigned int’, but argument 3 has type ‘hwaddr’ [-Werror=format=]
     NVRAM_PRINTF("%s: 0x%08x <= 0x%08x\n", __func__, addr, retval);

Fix it by using the correct format strings and while we're at it,
also change the definition of NVRAM_PRINTF so that this can not
bit-rot so easily again.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/timer/m48t59-internal.h | 9 +++------
 hw/timer/m48t59.c          | 4 ++--
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/hw/timer/m48t59-internal.h b/hw/timer/m48t59-internal.h
index 32ae957..d0f0caf 100644
--- a/hw/timer/m48t59-internal.h
+++ b/hw/timer/m48t59-internal.h
@@ -25,13 +25,10 @@
 #ifndef HW_M48T59_INTERNAL_H
 #define HW_M48T59_INTERNAL_H 1
 
-//#define DEBUG_NVRAM
+#define M48T59_DEBUG 0
 
-#if defined(DEBUG_NVRAM)
-#define NVRAM_PRINTF(fmt, ...) do { printf(fmt , ## __VA_ARGS__); } while (0)
-#else
-#define NVRAM_PRINTF(fmt, ...) do { } while (0)
-#endif
+#define NVRAM_PRINTF(fmt, ...) do { \
+    if (M48T59_DEBUG) { printf(fmt , ## __VA_ARGS__); } } while (0)
 
 /*
  * The M48T02, M48T08 and M48T59 chips are very similar. The newer '59 has
diff --git a/hw/timer/m48t59.c b/hw/timer/m48t59.c
index 844aad5..4abb4ac 100644
--- a/hw/timer/m48t59.c
+++ b/hw/timer/m48t59.c
@@ -457,7 +457,7 @@ static void NVRAM_writeb(void *opaque, hwaddr addr, uint64_t val,
 {
     M48t59State *NVRAM = opaque;
 
-    NVRAM_PRINTF("%s: 0x%08x => 0x%08x\n", __func__, addr, val);
+    NVRAM_PRINTF("%s: 0x%"HWADDR_PRIx" => 0x%"PRIx64"\n", __func__, addr, val);
     switch (addr) {
     case 0:
         NVRAM->addr &= ~0x00FF;
@@ -489,7 +489,7 @@ static uint64_t NVRAM_readb(void *opaque, hwaddr addr, unsigned size)
         retval = -1;
         break;
     }
-    NVRAM_PRINTF("%s: 0x%08x <= 0x%08x\n", __func__, addr, retval);
+    NVRAM_PRINTF("%s: 0x%"HWADDR_PRIx" <= 0x%08x\n", __func__, addr, retval);
 
     return retval;
 }
-- 
1.8.3.1
Re: [Qemu-devel] [PATCH] hw/timer/mt48t59: Fix bit-rotten NVRAM_PRINTF format strings
Posted by Philippe Mathieu-Daudé 8 years ago 
Hi Thomas,

On 02/02/2018 05:15 AM, Thomas Huth wrote:
> When compiling with NVRAM_PRINTF enabled, gcc currently bails out with:
> 
>   CC      hw/timer/m48t59.o
>   CC      hw/timer/m48t59-isa.o
> hw/timer/m48t59.c: In function ‘NVRAM_writeb’:
> hw/timer/m48t59.c:460:5: error: format ‘%x’ expects argument of type ‘unsigned int’, but argument 3 has type ‘hwaddr’ [-Werror=format=]
>      NVRAM_PRINTF("%s: 0x%08x => 0x%08x\n", __func__, addr, val);
>      ^
> hw/timer/m48t59.c:460:5: error: format ‘%x’ expects argument of type ‘unsigned int’, but argument 4 has type ‘uint64_t’ [-Werror=format=]
> hw/timer/m48t59.c: In function ‘NVRAM_readb’:
> hw/timer/m48t59.c:492:5: error: format ‘%x’ expects argument of type ‘unsigned int’, but argument 3 has type ‘hwaddr’ [-Werror=format=]
>      NVRAM_PRINTF("%s: 0x%08x <= 0x%08x\n", __func__, addr, retval);
> 
> Fix it by using the correct format strings and while we're at it,
> also change the definition of NVRAM_PRINTF so that this can not
> bit-rot so easily again.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/timer/m48t59-internal.h | 9 +++------
>  hw/timer/m48t59.c          | 4 ++--
>  2 files changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/timer/m48t59-internal.h b/hw/timer/m48t59-internal.h
> index 32ae957..d0f0caf 100644
> --- a/hw/timer/m48t59-internal.h
> +++ b/hw/timer/m48t59-internal.h
> @@ -25,13 +25,10 @@
>  #ifndef HW_M48T59_INTERNAL_H
>  #define HW_M48T59_INTERNAL_H 1
>  
> -//#define DEBUG_NVRAM
> +#define M48T59_DEBUG 0
>  
> -#if defined(DEBUG_NVRAM)
> -#define NVRAM_PRINTF(fmt, ...) do { printf(fmt , ## __VA_ARGS__); } while (0)
> -#else
> -#define NVRAM_PRINTF(fmt, ...) do { } while (0)
> -#endif
> +#define NVRAM_PRINTF(fmt, ...) do { \
> +    if (M48T59_DEBUG) { printf(fmt , ## __VA_ARGS__); } } while (0)

While not use tracepoints directly?

>  
>  /*
>   * The M48T02, M48T08 and M48T59 chips are very similar. The newer '59 has
> diff --git a/hw/timer/m48t59.c b/hw/timer/m48t59.c
> index 844aad5..4abb4ac 100644
> --- a/hw/timer/m48t59.c
> +++ b/hw/timer/m48t59.c
> @@ -457,7 +457,7 @@ static void NVRAM_writeb(void *opaque, hwaddr addr, uint64_t val,
>  {
>      M48t59State *NVRAM = opaque;
>  
> -    NVRAM_PRINTF("%s: 0x%08x => 0x%08x\n", __func__, addr, val);
> +    NVRAM_PRINTF("%s: 0x%"HWADDR_PRIx" => 0x%"PRIx64"\n", __func__, addr, val);
>      switch (addr) {
>      case 0:
>          NVRAM->addr &= ~0x00FF;
> @@ -489,7 +489,7 @@ static uint64_t NVRAM_readb(void *opaque, hwaddr addr, unsigned size)
>          retval = -1;
>          break;
>      }
> -    NVRAM_PRINTF("%s: 0x%08x <= 0x%08x\n", __func__, addr, retval);
> +    NVRAM_PRINTF("%s: 0x%"HWADDR_PRIx" <= 0x%08x\n", __func__, addr, retval);
>  
>      return retval;
>  }
>
Re: [Qemu-devel] [PATCH] hw/timer/mt48t59: Fix bit-rotten NVRAM_PRINTF format strings
Posted by Mark Cave-Ayland 8 years ago 
On 02/02/18 08:15, Thomas Huth wrote:

> When compiling with NVRAM_PRINTF enabled, gcc currently bails out with:
> 
>    CC      hw/timer/m48t59.o
>    CC      hw/timer/m48t59-isa.o
> hw/timer/m48t59.c: In function ‘NVRAM_writeb’:
> hw/timer/m48t59.c:460:5: error: format ‘%x’ expects argument of type ‘unsigned int’, but argument 3 has type ‘hwaddr’ [-Werror=format=]
>       NVRAM_PRINTF("%s: 0x%08x => 0x%08x\n", __func__, addr, val);
>       ^
> hw/timer/m48t59.c:460:5: error: format ‘%x’ expects argument of type ‘unsigned int’, but argument 4 has type ‘uint64_t’ [-Werror=format=]
> hw/timer/m48t59.c: In function ‘NVRAM_readb’:
> hw/timer/m48t59.c:492:5: error: format ‘%x’ expects argument of type ‘unsigned int’, but argument 3 has type ‘hwaddr’ [-Werror=format=]
>       NVRAM_PRINTF("%s: 0x%08x <= 0x%08x\n", __func__, addr, retval);
> 
> Fix it by using the correct format strings and while we're at it,
> also change the definition of NVRAM_PRINTF so that this can not
> bit-rot so easily again.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   hw/timer/m48t59-internal.h | 9 +++------
>   hw/timer/m48t59.c          | 4 ++--
>   2 files changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/timer/m48t59-internal.h b/hw/timer/m48t59-internal.h
> index 32ae957..d0f0caf 100644
> --- a/hw/timer/m48t59-internal.h
> +++ b/hw/timer/m48t59-internal.h
> @@ -25,13 +25,10 @@
>   #ifndef HW_M48T59_INTERNAL_H
>   #define HW_M48T59_INTERNAL_H 1
>   
> -//#define DEBUG_NVRAM
> +#define M48T59_DEBUG 0
>   
> -#if defined(DEBUG_NVRAM)
> -#define NVRAM_PRINTF(fmt, ...) do { printf(fmt , ## __VA_ARGS__); } while (0)
> -#else
> -#define NVRAM_PRINTF(fmt, ...) do { } while (0)
> -#endif
> +#define NVRAM_PRINTF(fmt, ...) do { \
> +    if (M48T59_DEBUG) { printf(fmt , ## __VA_ARGS__); } } while (0)
>   
>   /*
>    * The M48T02, M48T08 and M48T59 chips are very similar. The newer '59 has
> diff --git a/hw/timer/m48t59.c b/hw/timer/m48t59.c
> index 844aad5..4abb4ac 100644
> --- a/hw/timer/m48t59.c
> +++ b/hw/timer/m48t59.c
> @@ -457,7 +457,7 @@ static void NVRAM_writeb(void *opaque, hwaddr addr, uint64_t val,
>   {
>       M48t59State *NVRAM = opaque;
>   
> -    NVRAM_PRINTF("%s: 0x%08x => 0x%08x\n", __func__, addr, val);
> +    NVRAM_PRINTF("%s: 0x%"HWADDR_PRIx" => 0x%"PRIx64"\n", __func__, addr, val);
>       switch (addr) {
>       case 0:
>           NVRAM->addr &= ~0x00FF;
> @@ -489,7 +489,7 @@ static uint64_t NVRAM_readb(void *opaque, hwaddr addr, unsigned size)
>           retval = -1;
>           break;
>       }
> -    NVRAM_PRINTF("%s: 0x%08x <= 0x%08x\n", __func__, addr, retval);
> +    NVRAM_PRINTF("%s: 0x%"HWADDR_PRIx" <= 0x%08x\n", __func__, addr, retval);
>   
>       return retval;
>   }

Hi Thomas,

I agree with Philippe here that given you've got this far then is it not 
worth going all the way with a trace-events conversion?

Then again fixing this is enough of an improvement that I'm happy to 
give my R-B regardless:

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.