[Qemu-devel] [PATCH 02/43] arm: made printf always compile in debug output

[Qemu-devel] [PATCH 02/43] arm: made printf always compile in debug output

[Danil Antonov]

>From 1671b92e5a4a59d5e38ce754d1d0dde2401c8236 Mon Sep 17 00:00:00 2001
From: Danil Antonov <g.danil.anto@gmail.com>
Date: Wed, 29 Mar 2017 02:09:33 +0300
Subject: [PATCH 02/43] arm: made printf always compile in debug output

Wrapped printf calls inside debug macros (DPRINTF) in `if` statement.
This will ensure that printf function will always compile even if debug
output is turned off and, in turn, will prevent bitrot of the format
strings.

Signed-off-by: Danil Antonov <g.danil.anto@gmail.com>
---
 hw/arm/strongarm.c | 17 +++++++++++------
 hw/arm/z2.c        | 16 ++++++++++------
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/hw/arm/strongarm.c b/hw/arm/strongarm.c
index 3311cc3..88368ac 100644
--- a/hw/arm/strongarm.c
+++ b/hw/arm/strongarm.c
@@ -59,11 +59,16 @@
  - Enhance UART with modem signals
  */

-#ifdef DEBUG
-# define DPRINTF(format, ...) printf(format , ## __VA_ARGS__)
-#else
-# define DPRINTF(format, ...) do { } while (0)
-#endif
+#ifndef DEBUG
+#define DEBUG 0
+#endif
+
+#define DPRINTF(fmt, ...)                         \
+    do {                                          \
+        if (DEBUG) {                              \
+            fprintf(stderr, fmt, ## __VA_ARGS__); \
+        }                                         \
+    } while (0)

 static struct {
     hwaddr io_base;
@@ -1022,7 +1027,7 @@ static void
strongarm_uart_update_parameters(StrongARMUARTState
*s)
     s->char_transmit_time =  (NANOSECONDS_PER_SECOND / speed) * frame_size;
     qemu_chr_fe_ioctl(&s->chr, CHR_IOCTL_SERIAL_SET_PARAMS, &ssp);

-    DPRINTF(stderr, "%s speed=%d parity=%c data=%d stop=%d\n",
s->chr->label,
+    DPRINTF("%s speed=%d parity=%c data=%d stop=%d\n", s->chr.chr->label,
             speed, parity, data_bits, stop_bits);
 }

diff --git a/hw/arm/z2.c b/hw/arm/z2.c
index 1607cbd..1ee8ed9 100644
--- a/hw/arm/z2.c
+++ b/hw/arm/z2.c
@@ -27,12 +27,16 @@
 #include "exec/address-spaces.h"
 #include "sysemu/qtest.h"

-#ifdef DEBUG_Z2
-#define DPRINTF(fmt, ...) \
-        printf(fmt, ## __VA_ARGS__)
-#else
-#define DPRINTF(fmt, ...)
-#endif
+#ifndef DEBUG_Z2
+#define DEBUG_Z2 0
+#endif
+
+#define DPRINTF(fmt, ...)                                \
+    do {                                                 \
+        if (DEBUG_Z2) {                                  \
+            fprintf(stderr, "z2: " fmt, ## __VA_ARGS__); \
+        }                                                \
+    } while (0)

 static const struct keymap map[0x100] = {
     [0 ... 0xff] = { -1, -1 },
-- 
2.8.0.rc3

Re: [Qemu-devel] [PATCH 02/43] arm: made printf always compile in debug output

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3881 bytes --]

On 04/01/2017 08:32 AM, Danil Antonov wrote:
>>From 1671b92e5a4a59d5e38ce754d1d0dde2401c8236 Mon Sep 17 00:00:00 2001
> From: Danil Antonov <g.danil.anto@gmail.com>
> Date: Wed, 29 Mar 2017 02:09:33 +0300
> Subject: [PATCH 02/43] arm: made printf always compile in debug output
> 
> Wrapped printf calls inside debug macros (DPRINTF) in `if` statement.
> This will ensure that printf function will always compile even if debug
> output is turned off and, in turn, will prevent bitrot of the format
> strings.

Again, prefer present tense over past tense in the commit message.

> 
> Signed-off-by: Danil Antonov <g.danil.anto@gmail.com>
> ---
>  hw/arm/strongarm.c | 17 +++++++++++------
>  hw/arm/z2.c        | 16 ++++++++++------
>  2 files changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/arm/strongarm.c b/hw/arm/strongarm.c
> index 3311cc3..88368ac 100644
> --- a/hw/arm/strongarm.c
> +++ b/hw/arm/strongarm.c
> @@ -59,11 +59,16 @@
>   - Enhance UART with modem signals
>   */
> 
> -#ifdef DEBUG
> -# define DPRINTF(format, ...) printf(format , ## __VA_ARGS__)
> -#else
> -# define DPRINTF(format, ...) do { } while (0)
> -#endif
> +#ifndef DEBUG
> +#define DEBUG 0
> +#endif
> +
> +#define DPRINTF(fmt, ...)                         \
> +    do {                                          \
> +        if (DEBUG) {                              \
> +            fprintf(stderr, fmt, ## __VA_ARGS__); \

Again, changing from stdout to stderr should be mentioned as intentional
in the commit message.

Note that your change breaks the case of someone that used to do:

make CFLAGS=-DDEBUG=

because now it results in 'if ()' which is not valid syntax; likewise
for someone that used to do CFLAGS=-DDEBUG=yes.  (Or put another way, as
written, your patch forces someone to use -DDEBUG=0 or -DDEBUG=1).  A
safer way is to make the 'if' condition a separate variable than the
command-line trigger, as in the following, so that regardless of what
DEBUG is defined to (including the empty string), you are only using
DEBUG in the preprocessor, while the if conditional is under your
complete control:

#ifdef DEBUG
# define DEBUG_PRINT 1
#else
# define DEBUG_PRINT 0
#endif

#definde DPRINTF()... if (DEBUG_PRINT)

> @@ -1022,7 +1027,7 @@ static void
> strongarm_uart_update_parameters(StrongARMUARTState
> *s)
>      s->char_transmit_time =  (NANOSECONDS_PER_SECOND / speed) * frame_size;
>      qemu_chr_fe_ioctl(&s->chr, CHR_IOCTL_SERIAL_SET_PARAMS, &ssp);
> 
> -    DPRINTF(stderr, "%s speed=%d parity=%c data=%d stop=%d\n",
> s->chr->label,

Oh gross - the old code couldn't even compile correctly when DEBUG was
defined (since printf(stderr) tries to treat the stderr pointer as a
format string)!  This is a definite cleanup, and an argument that your
switch from stdout to stderr is correct.


> +++ b/hw/arm/z2.c
> @@ -27,12 +27,16 @@
>  #include "exec/address-spaces.h"
>  #include "sysemu/qtest.h"
> 
> -#ifdef DEBUG_Z2
> -#define DPRINTF(fmt, ...) \
> -        printf(fmt, ## __VA_ARGS__)
> -#else
> -#define DPRINTF(fmt, ...)
> -#endif
> +#ifndef DEBUG_Z2
> +#define DEBUG_Z2 0
> +#endif
> +
> +#define DPRINTF(fmt, ...)                                \
> +    do {                                                 \
> +        if (DEBUG_Z2) {                                  \

Again, it's best to separate your conditional to be something completely
under you control, while still allowing the command line freedom to
define DEBUG_Z2 to anything (whether an empty string or a non-numeric
value).

These types of comments probably apply throughout your entire series, so
it may be better if I wait for a v2 before reviewing too many more.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 02/43] arm: made printf always compile in debug output

[Danil Antonov]

Thanks for review. I'll update and resend these pathces (probably next
week).

2017-04-01 17:51 GMT+03:00 Eric Blake <eblake@redhat.com>:

> On 04/01/2017 08:32 AM, Danil Antonov wrote:
> >>From 1671b92e5a4a59d5e38ce754d1d0dde2401c8236 Mon Sep 17 00:00:00 2001
> > From: Danil Antonov <g.danil.anto@gmail.com>
> > Date: Wed, 29 Mar 2017 02:09:33 +0300
> > Subject: [PATCH 02/43] arm: made printf always compile in debug output
> >
> > Wrapped printf calls inside debug macros (DPRINTF) in `if` statement.
> > This will ensure that printf function will always compile even if debug
> > output is turned off and, in turn, will prevent bitrot of the format
> > strings.
>
> Again, prefer present tense over past tense in the commit message.
>
> >
> > Signed-off-by: Danil Antonov <g.danil.anto@gmail.com>
> > ---
> >  hw/arm/strongarm.c | 17 +++++++++++------
> >  hw/arm/z2.c        | 16 ++++++++++------
> >  2 files changed, 21 insertions(+), 12 deletions(-)
> >
> > diff --git a/hw/arm/strongarm.c b/hw/arm/strongarm.c
> > index 3311cc3..88368ac 100644
> > --- a/hw/arm/strongarm.c
> > +++ b/hw/arm/strongarm.c
> > @@ -59,11 +59,16 @@
> >   - Enhance UART with modem signals
> >   */
> >
> > -#ifdef DEBUG
> > -# define DPRINTF(format, ...) printf(format , ## __VA_ARGS__)
> > -#else
> > -# define DPRINTF(format, ...) do { } while (0)
> > -#endif
> > +#ifndef DEBUG
> > +#define DEBUG 0
> > +#endif
> > +
> > +#define DPRINTF(fmt, ...)                         \
> > +    do {                                          \
> > +        if (DEBUG) {                              \
> > +            fprintf(stderr, fmt, ## __VA_ARGS__); \
>
> Again, changing from stdout to stderr should be mentioned as intentional
> in the commit message.
>
> Note that your change breaks the case of someone that used to do:
>
> make CFLAGS=-DDEBUG=
>
> because now it results in 'if ()' which is not valid syntax; likewise
> for someone that used to do CFLAGS=-DDEBUG=yes.  (Or put another way, as
> written, your patch forces someone to use -DDEBUG=0 or -DDEBUG=1).  A
> safer way is to make the 'if' condition a separate variable than the
> command-line trigger, as in the following, so that regardless of what
> DEBUG is defined to (including the empty string), you are only using
> DEBUG in the preprocessor, while the if conditional is under your
> complete control:
>
> #ifdef DEBUG
> # define DEBUG_PRINT 1
> #else
> # define DEBUG_PRINT 0
> #endif
>
> #definde DPRINTF()... if (DEBUG_PRINT)
>
> > @@ -1022,7 +1027,7 @@ static void
> > strongarm_uart_update_parameters(StrongARMUARTState
> > *s)
> >      s->char_transmit_time =  (NANOSECONDS_PER_SECOND / speed) *
> frame_size;
> >      qemu_chr_fe_ioctl(&s->chr, CHR_IOCTL_SERIAL_SET_PARAMS, &ssp);
> >
> > -    DPRINTF(stderr, "%s speed=%d parity=%c data=%d stop=%d\n",
> > s->chr->label,
>
> Oh gross - the old code couldn't even compile correctly when DEBUG was
> defined (since printf(stderr) tries to treat the stderr pointer as a
> format string)!  This is a definite cleanup, and an argument that your
> switch from stdout to stderr is correct.
>
>
> > +++ b/hw/arm/z2.c
> > @@ -27,12 +27,16 @@
> >  #include "exec/address-spaces.h"
> >  #include "sysemu/qtest.h"
> >
> > -#ifdef DEBUG_Z2
> > -#define DPRINTF(fmt, ...) \
> > -        printf(fmt, ## __VA_ARGS__)
> > -#else
> > -#define DPRINTF(fmt, ...)
> > -#endif
> > +#ifndef DEBUG_Z2
> > +#define DEBUG_Z2 0
> > +#endif
> > +
> > +#define DPRINTF(fmt, ...)                                \
> > +    do {                                                 \
> > +        if (DEBUG_Z2) {                                  \
>
> Again, it's best to separate your conditional to be something completely
> under you control, while still allowing the command line freedom to
> define DEBUG_Z2 to anything (whether an empty string or a non-numeric
> value).
>
> These types of comments probably apply throughout your entire series, so
> it may be better if I wait for a v2 before reviewing too many more.
>
> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
>