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

[Andreas Färber]

Am 15.02.2013 13:51, schrieb 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.

If you want to convert the code to tracepoints, feel free to go at it!
But that hasn't been done since introducing that infrastructure, so my
hopes are low. ;)

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

I didn't write those, but I am the one that would like them to work for
my purposes. So it looks like I need to change them or nobody will.

>  DPRINTF() can be converted mechanically to
> trace_foo(arg1, arg).  Then we get rid of all the DPRINTF() definitions.

What is "foo" though and what "arg1, arg"? That needs to be invented AFAIU.

Following up on Anthony's original thought, the question is whether to
convert a LOG_EXCP macro in target-ppc to ppc_log_excp tracepoint or
whether to homogenize it as log_excp and use that across targets, to
save tracepoint definitions. That's not purely mechanical.

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

I'm assuming that changes to ppc logging go through ppc-next, changes to
sparc code go through Blue etc. All those potentially conflict since
they're adding to the bottom of the same text file, thus coordination.
It's a long-standing criticism of our centralistic tracing
infrastructure compared to the totally decentral and inhomogeneous
DPRINTFs and what-nots on the other hand.

Cheers,
Andreas

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