[PATCH v2 01/16] x86: Convert conditional compilation of debug printfs to regular ifs

[PATCH v2 01/16] x86: Convert conditional compilation of debug printfs to regular ifs

[Marc Marí]

Modify debug macros to have the same format through the codebase and use regular
ifs instead of ifdef.

As the debug printf is always put in code, some casting had to be added to avoid
warnings treated as errors at compile time.

Signed-off-by: Marc Marí <marc.mari.barcelo@gmail.com>
---
 hw/i386/kvm/pci-assign.c |    9 ++++-----
 hw/i386/multiboot.c      |    6 ++++--
 target-i386/kvm.c        |    8 ++++----
 xen-hvm.c                |   12 ++++++------
 xen-mapcache.c           |    8 ++++----
 5 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index e55421a..35757ae 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -51,14 +51,13 @@
 //#define DEVICE_ASSIGNMENT_DEBUG
 
 #ifdef DEVICE_ASSIGNMENT_DEBUG
-#define DEBUG(fmt, ...)                                       \
-    do {                                                      \
-        fprintf(stderr, "%s: " fmt, __func__ , __VA_ARGS__);  \
-    } while (0)
+#define DEVICE_ASSIGNMENT_DEBUG_ENABLED 1
 #else
-#define DEBUG(fmt, ...)
+#define DEVICE_ASSIGNMENT_DEBUG_ENABLED 0
 #endif
 
+#define DEBUG(fmt, ...) QEMU_DPRINTF(DEVICE_ASSIGNMENT_DEBUG_ENABLED, "pci_assign", fmt, ## __VA_ARGS__)
+
 typedef struct PCIRegion {
     int type;           /* Memory or port I/O */
     int valid;
diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
index 985ca1e..cd215dc 100644
--- a/hw/i386/multiboot.c
+++ b/hw/i386/multiboot.c
@@ -33,11 +33,13 @@
 //#define DEBUG_MULTIBOOT
 
 #ifdef DEBUG_MULTIBOOT
-#define mb_debug(a...) fprintf(stderr, ## a)
+#define DEBUG_MULTIBOOT_ENABLED 1
 #else
-#define mb_debug(a...)
+#define DEBUG_MULTIBOOT_ENABLED 0
 #endif
 
+#define mb_debug(a...) QEMU_DPRINTF(DEBUG_MULTIBOOT_ENABLED, "i386 multiboot", a)
+
 #define MULTIBOOT_STRUCT_ADDR 0x9000
 
 #if MULTIBOOT_STRUCT_ADDR > 0xf0000
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 4389959..d6cd89c 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -37,13 +37,13 @@
 //#define DEBUG_KVM
 
 #ifdef DEBUG_KVM
-#define DPRINTF(fmt, ...) \
-    do { fprintf(stderr, fmt, ## __VA_ARGS__); } while (0)
+#define DEBUG_KVM_ENABLED 1
 #else
-#define DPRINTF(fmt, ...) \
-    do { } while (0)
+#define DEBUG_KVM_ENABLED 0
 #endif
 
+#define DPRINTF(fmt, ...) QEMU_DPRINTF(DEBUG_KVM_ENABLED, "i386 kvm", fmt, ## __VA_ARGS__)
+
 #define MSR_KVM_WALL_CLOCK  0x11
 #define MSR_KVM_SYSTEM_TIME 0x12
 
diff --git a/xen-hvm.c b/xen-hvm.c
index a64486c..ccce342 100644
--- a/xen-hvm.c
+++ b/xen-hvm.c
@@ -26,16 +26,16 @@
 #include <xen/hvm/params.h>
 #include <xen/hvm/e820.h>
 
-//#define DEBUG_XEN_HVM
+//#define DEBUG_XEN
 
-#ifdef DEBUG_XEN_HVM
-#define DPRINTF(fmt, ...) \
-    do { fprintf(stderr, "xen: " fmt, ## __VA_ARGS__); } while (0)
+#ifdef DEBUG_XEN
+#define DEBUG_XEN_ENABLED 1
 #else
-#define DPRINTF(fmt, ...) \
-    do { } while (0)
+#define DEBUG_XEN_ENABLED 0
 #endif
 
+#define DPRINTF(fmt, ...) QEMU_DPRINTF(DEBUG_XEN_ENABLED, "xen", fmt, ## __VA_ARGS__)
+
 static MemoryRegion ram_memory, ram_640k, ram_lo, ram_hi;
 static MemoryRegion *framebuffer;
 static bool xen_in_migration;
diff --git a/xen-mapcache.c b/xen-mapcache.c
index eda914a..a50bb80 100644
--- a/xen-mapcache.c
+++ b/xen-mapcache.c
@@ -26,13 +26,13 @@
 //#define MAPCACHE_DEBUG
 
 #ifdef MAPCACHE_DEBUG
-#  define DPRINTF(fmt, ...) do { \
-    fprintf(stderr, "xen_mapcache: " fmt, ## __VA_ARGS__); \
-} while (0)
+#define MAPCACHE_DEBUG_ENABLED 1
 #else
-#  define DPRINTF(fmt, ...) do { } while (0)
+#define MAPCACHE_DEBUG_ENABLED 0
 #endif
 
+#define DPRINTF(fmt, ...) QEMU_DPRINTF(MAPCACHE_DEBUG_ENABLED, "xen_mapcache", fmt, ## __VA_ARGS__)
+
 #if defined(__i386__)
 #  define MCACHE_BUCKET_SHIFT 16
 #  define MCACHE_MAX_SIZE     (1UL<<31) /* 2GB Cap */
-- 
1.7.10.4

Re: [PATCH v2 01/16] x86: Convert conditional compilation of debug printfs to regular ifs

[Andreas Färber]

Am 13.05.2014 09:02, schrieb Marc Marí:
> Modify debug macros to have the same format through the codebase and use regular
> ifs instead of ifdef.
> 
> As the debug printf is always put in code, some casting had to be added to avoid
> warnings treated as errors at compile time.
> 
> Signed-off-by: Marc Marí <marc.mari.barcelo@gmail.com>
> ---
>  hw/i386/kvm/pci-assign.c |    9 ++++-----
>  hw/i386/multiboot.c      |    6 ++++--
>  target-i386/kvm.c        |    8 ++++----
>  xen-hvm.c                |   12 ++++++------
>  xen-mapcache.c           |    8 ++++----
>  5 files changed, 22 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
> index e55421a..35757ae 100644
> --- a/hw/i386/kvm/pci-assign.c
> +++ b/hw/i386/kvm/pci-assign.c
> @@ -51,14 +51,13 @@
>  //#define DEVICE_ASSIGNMENT_DEBUG
>  
>  #ifdef DEVICE_ASSIGNMENT_DEBUG
> -#define DEBUG(fmt, ...)                                       \
> -    do {                                                      \
> -        fprintf(stderr, "%s: " fmt, __func__ , __VA_ARGS__);  \
> -    } while (0)
> +#define DEVICE_ASSIGNMENT_DEBUG_ENABLED 1
>  #else
> -#define DEBUG(fmt, ...)
> +#define DEVICE_ASSIGNMENT_DEBUG_ENABLED 0
>  #endif
>  
> +#define DEBUG(fmt, ...) QEMU_DPRINTF(DEVICE_ASSIGNMENT_DEBUG_ENABLED, "pci_assign", fmt, ## __VA_ARGS__)

This is broken, QEMU_DPRINTF() is not defined yet. Looks like an
ordering issue with 16/16.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v2 01/16] x86: Convert conditional compilation of debug printfs to regular ifs

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 2320 bytes --]

On 05/13/2014 01:02 AM, Marc Marí wrote:
> Modify debug macros to have the same format through the codebase and use regular
> ifs instead of ifdef.
> 
> As the debug printf is always put in code, some casting had to be added to avoid
> warnings treated as errors at compile time.

Umm, where in this patch did you add casting?  Don't add bogus lines to
the commit message (I was assuming that it might be a case where the
code has type mismatches, and where something like PRId32 would be
better than adding a cast - but couldn't find where that was. Maybe
other patches in the series are affected?)

> 
> Signed-off-by: Marc Marí <marc.mari.barcelo@gmail.com>
> ---

> -#define DEBUG(fmt, ...)                                       \
> -    do {                                                      \
> -        fprintf(stderr, "%s: " fmt, __func__ , __VA_ARGS__);  \
> -    } while (0)

The old code was line-wrapped to fit in 80 columns...

> +#define DEVICE_ASSIGNMENT_DEBUG_ENABLED 1
>  #else
> -#define DEBUG(fmt, ...)
> +#define DEVICE_ASSIGNMENT_DEBUG_ENABLED 0
>  #endif
>  
> +#define DEBUG(fmt, ...) QEMU_DPRINTF(DEVICE_ASSIGNMENT_DEBUG_ENABLED, "pci_assign", fmt, ## __VA_ARGS__)

the new code should probably try to do similar:

#define DEBUG(fmt, ...)						\
    QEMU_DPRINTF(DEVICE_ASSIGNMENT_DEBUG_ENABLED,		\
                 "pci_assign", fmt, ## __VA_ARGS__)

Although __VA_ARGS__ is required by C99, the use of ##__VA_ARGS__ is a
gcc extension; are you sure that all other supported compilers handle
it?  (I guess that's just clang)

If you want something portable to C99, just use one fewer macro
argument, so that you are guaranteed that __VA_ARGS__ will be non-empty
(that is, subsume fmt into the ...):

#define DEBUG(...)						\
    QEMU_DPRINTF(DEVICE_ASSIGNMENT_DEBUG_ENABLED,		\
                 "pci_assign", __VA_ARGS__)


>  
> +#define mb_debug(a...) QEMU_DPRINTF(DEBUG_MULTIBOOT_ENABLED, "i386 multiboot", a)

Use of 'a...' as a named var-arg parameter is a gcc extension, not
portable to C99.  Again, will this compile on non-gcc?  Unnamed ...
coupled with __VA_ARGS__ is the only fully portable way to do var-args.

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


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

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Qemu-devel] [PATCH v2 01/16] x86: Convert conditional compilation of debug printfs to regular ifs

[Marc Marí]

El Tue, 13 May 2014 08:38:26 -0600
Eric Blake <eblake@redhat.com> escribió:
> Although __VA_ARGS__ is required by C99, the use of ##__VA_ARGS__ is a
> gcc extension; are you sure that all other supported compilers handle
> it?  (I guess that's just clang)
> 
> If you want something portable to C99, just use one fewer macro
> argument, so that you are guaranteed that __VA_ARGS__ will be
> non-empty (that is, subsume fmt into the ...):
> 
> #define DEBUG(...)						\
>     QEMU_DPRINTF(DEVICE_ASSIGNMENT_DEBUG_ENABLED,		\
>                  "pci_assign", __VA_ARGS__)
> 

I found some problems to convert from ## __VA_ARGS__ to __VA_ARGS__ in
some parts, and as I asked in IRC, it was pointed that "our HACKING
document recommends the fmt, ## __VA_ARGS__ approach, and it obviously
works on all the compilers we care about". So I will leave this as it
is now.

Marc

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Qemu-devel] [PATCH v2 01/16] x86: Convert conditional compilation of debug printfs to regular ifs

[Eric Blake]

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

On 05/13/2014 01:02 AM, Marc Marí wrote:
> Modify debug macros to have the same format through the codebase and use regular
> ifs instead of ifdef.
> 
> As the debug printf is always put in code, some casting had to be added to avoid
> warnings treated as errors at compile time.

Umm, where in this patch did you add casting?  Don't add bogus lines to
the commit message (I was assuming that it might be a case where the
code has type mismatches, and where something like PRId32 would be
better than adding a cast - but couldn't find where that was. Maybe
other patches in the series are affected?)

> 
> Signed-off-by: Marc Marí <marc.mari.barcelo@gmail.com>
> ---

> -#define DEBUG(fmt, ...)                                       \
> -    do {                                                      \
> -        fprintf(stderr, "%s: " fmt, __func__ , __VA_ARGS__);  \
> -    } while (0)

The old code was line-wrapped to fit in 80 columns...

> +#define DEVICE_ASSIGNMENT_DEBUG_ENABLED 1
>  #else
> -#define DEBUG(fmt, ...)
> +#define DEVICE_ASSIGNMENT_DEBUG_ENABLED 0
>  #endif
>  
> +#define DEBUG(fmt, ...) QEMU_DPRINTF(DEVICE_ASSIGNMENT_DEBUG_ENABLED, "pci_assign", fmt, ## __VA_ARGS__)

the new code should probably try to do similar:

#define DEBUG(fmt, ...)						\
    QEMU_DPRINTF(DEVICE_ASSIGNMENT_DEBUG_ENABLED,		\
                 "pci_assign", fmt, ## __VA_ARGS__)

Although __VA_ARGS__ is required by C99, the use of ##__VA_ARGS__ is a
gcc extension; are you sure that all other supported compilers handle
it?  (I guess that's just clang)

If you want something portable to C99, just use one fewer macro
argument, so that you are guaranteed that __VA_ARGS__ will be non-empty
(that is, subsume fmt into the ...):

#define DEBUG(...)						\
    QEMU_DPRINTF(DEVICE_ASSIGNMENT_DEBUG_ENABLED,		\
                 "pci_assign", __VA_ARGS__)


>  
> +#define mb_debug(a...) QEMU_DPRINTF(DEBUG_MULTIBOOT_ENABLED, "i386 multiboot", a)

Use of 'a...' as a named var-arg parameter is a gcc extension, not
portable to C99.  Again, will this compile on non-gcc?  Unnamed ...
coupled with __VA_ARGS__ is the only fully portable way to do var-args.

-- 
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 --]