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

[Stefan Hajnoczi]

On Fri, Feb 15, 2013 at 10:50:16AM +0100, Andreas Färber wrote:
> Am 15.02.2013 10:06, schrieb Stefan Hajnoczi:
> > 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.
> 
> Thanks for that feedback. If you look at the previous discussion, that's
> part of what I did in my RFC, and it was rejected in favor of keeping
> #ifdef'able defines. Using inline functions to avoid some nasty
> macro-is-not-function issues, there seemed to be no need to combine both
> approaches due to the format string being checked one level above.

Redefining debug functions in every file is nasty.  I am also not a fan
of modifying a #define, it always need to be reverted before committing
changes.

> > 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.
> 
> This series or patch is not against tracing. It might be an option to
> use them for tmp105. But not being the author of the targets and all of
> the devices my CPU refactorings potentially touch, it is infeasable for
> me to determine an appropriate set of tracepoints everywhere. We'd also
> need to decide whether we want per-target tracepoints or per-aspect
> tracepoints, since it's a global list. Independent of that question,
> some kind of include mechanism (like for the default-configs) would be
> nice to decouple adding tracepoints in different areas.

Not sure I understand.  You don't need to come up with new trace events
in code you didn't write.  DPRINTF() can be converted mechanically to
trace_foo(arg1, arg).  Then we get rid of all the DPRINTF() definitions.

The ./trace-events list is informal and can change at any time.  We
don't need to coordinate between different subsystems or targets in
QEMU.

Stefan