Re: [Qemu-devel] [RFC 16/19] target-ppc: Refactor debug output macros

[Andreas Färber]

Am 27.01.2013 15:14, schrieb Anthony Liguori:
> Andreas Färber <address@hidden> writes:
>> diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
>> index 0a1ac86..54722c4 100644
>> --- a/target-ppc/excp_helper.c
>> +++ b/target-ppc/excp_helper.c
>> @@ -21,14 +21,14 @@
>>  
>>  #include "helper_regs.h"
>>  
>> -//#define DEBUG_OP
>> -//#define DEBUG_EXCEPTIONS
>> +#define DEBUG_OP 0
>> +#define DEBUG_EXCEPTIONS 0
>>  
>> -#ifdef DEBUG_EXCEPTIONS
>> -#  define LOG_EXCP(...) qemu_log(__VA_ARGS__)
>> -#else
>> -#  define LOG_EXCP(...) do { } while (0)
>> -#endif
>> +#define LOG_EXCP(...) G_STMT_START \
>> +    if (DEBUG_EXCEPTIONS) { \
>> +        qemu_log(__VA_ARGS__); \
>> +    } \
>> +    G_STMT_END
> 
> Just thinking out loud a bit..  This form becomes pretty common and it's
> ashame to use a macro here if we don't have to.
> 
> I think:
> 
> static inline void LOG_EXCP(const char *fmt, ...)
> {
>     if (debug_exceptions) {
>        va_list ap;
>        va_start(ap, fmt);
>        qemu_logv(fmt, ap);
>        va_end(ap);
>     }
> }
> 
> Probably would have equivalent performance.  debug_exception would be
> read-mostly and ought to be very predictable as a result.  I strongly
> expect that the compiler would actually inline LOG_EXCP too.

Thanks for your early feedback. I merely tried to stay close to the
original code. I wouldn't mind inline functions either. Or even more
harmonization for that matter.

> I see LOG_EXCP and LOG_DIS in this series.  Perhaps we could just
> introduce these functions and then make these flags run-time
> controllable?

I was feeling conservative during that series in light of compile-time
decided conditional; if we want to go down that route we should probably
sprinkle quite some unlikely()s for optimization.

I think the if (0) { ... } approach would already catch a few things. As
a next step, some mechanism as proposed by Peter C. (?) to enable things
at configure-time could be built on top. Run-time would need some
stabilization phase to avoid command line compatibility issues.

> BTW, one advantage of this over your original proposal back to your
> point is that you still won't catch linker errors with your proposal.
> Dead code eliminate will kill off those branches before the linker ever
> sees them.

Linker errors would be limited to renamed/dropped/#ifdef'ed functions,
wouldn't they? In the past I caught that using existing --enable-debug.

My recurring issue is overlooking env->something after removing fields
from CPU_COMMON/CPUArchState. I was hoping that to be caught inside
if (0) { ... } during my 3x KVM + BSD + MinGW builds rather than
patching individual files.

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