Re: [Qemu-devel] [PATCH for-next v3 2/5] tmp105: Add debug output

[Stefan Hajnoczi]

On Thu, Feb 14, 2013 at 04:40:29PM +0100, Andreas Färber wrote:
> CC'ing some more people from the "debug output revamp" RFC discussion.
> 
> Am 11.02.2013 20:01, schrieb Andreas Färber:
> > From: Andreas Färber <address@hidden>
> > 
> > Signed-off-by: Andreas Färber <address@hidden>
> > ---
> >  hw/tmp105.c |   27 +++++++++++++++++++++++++--
> >  1 Datei geändert, 25 Zeilen hinzugefügt(+), 2 Zeilen entfernt(-)
> > 
> > diff --git a/hw/tmp105.c b/hw/tmp105.c
> > index 3ad2d2f..5dafa37 100644
> > --- a/hw/tmp105.c
> > +++ b/hw/tmp105.c
> > @@ -23,6 +23,18 @@
> >  #include "tmp105.h"
> >  #include "qapi/visitor.h"
> >  
> > +#undef DEBUG_TMP105
> > +
> > +static inline void GCC_FMT_ATTR(1, 2) DPRINTF(const char *fmt, ...)
> 
> Being an inline function, I think it would be better to name this
> tmp105_dprintf() and alias via: #define DPRINTF tmp105_dprintf
> Assuming we want an uppercase conditional-output statement in the first
> place?
> 
> dprintf() as used in some files (sheepdog, sPAPR, SPICE,
> target-{i386,ppc,s390x}/kvm.c) is already used by system headers on some
> platforms, so I'd rather avoid it.
> 
> Any other comments or suggestions? I'm preparing to respin the above
> series in a similar, less-invasive style.

Here is a radical departure from the zoo of DPRINTF() approaches that
QEMU has today.  I know not everyone is comfortable using tracing, even
though you can use --enable-trace-backend=stderr to get good old stderr
output.

In iPXE they use a clever compile-time debug macro:
https://git.ipxe.org/ipxe.git/blob/HEAD:/src/include/compiler.h#l204

Basically you do DBG("hello world\n") and it gets compiled out by
default using:
  if (DBG_LOG) {
      printf("hello world\n");
  }

Here's the really cool part, debugging can be toggled on a per-object
file basis at compile-time:

make DEBUG=e1000  # build QEMU with debug printfs only in e1000.o

The iPXE implementation is fancier than this.  It supports different log
levels, hex dumping, MD5 hashing, etc.

But the core idea is:
1. Debug printfs are always if (0 or 1) { printf(...); } so that the
   compiler syntax checks them - no more bitrot in debug printfs.
2. A single debug.h header included from qemu-common.h with Makefile
   support that lets you say "make DEBUG=e1000,rtl8139,net".

It would be a big step up from the mess we have today.

Personally, I think we should use tracing instead of debug printfs
though.  Tracing allows you to use not just fprintf(stderr) but also
DTrace, if you like.  You can mark debug trace events "disabled" in
./trace-events so they will not be compiled in by default - zero
performance overhead.

Stefan