Re: [Qemu-devel] [PATCH v2 00/15] Debug output revamp

[Andreas Färber]

Am 22.02.2013 17:00, schrieb Markus Armbruster:
> Andreas Färber <address@hidden> writes:
> 
>> Am 21.02.2013 17:10, schrieb Richard Henderson:
>>> On 2013-02-20 20:24, Andreas Färber wrote:
>>>> v2 replaces macros with static functions, adopting Scott's scheme of
>>>> const
>>>> variables set through #ifdefs (keeping their defined() semantics) and
>>>> adopting
>>>> Anthony's proposal of using va_list for argument passing.
>>>> v1 had changed some #ifdefs into ifs due to the #ifdef -> #if change;
>>>> while not
>>>> strictly necessary anymore, I have not reverted this (mostly sparc).
>>>
>>> Sorry I missed the discussion that must have went into v1, but I'm not
>>> thrilled about this.
>>>
>>> My gcc intuition says that varargs means that inlining won't happen, and
>>> that static const means that the code won't be deleted as dead at
>>> compile time.  Which means that we're going to have more overhead both
>>> when the debugging is disabled and when it is enabled.
>>>
>>> Is there an especially good reason why we're not still using a macro,
>>> but one controlled by an if (0) when debugging is disabled?
>>
>> v1 had that.
>>
>> static const bool: Coming from openpic, suggested by Alex to keep #ifdef
>> rather than #if semantics. Would a #define be any better?
>>
>> if (0) vs. #ifdef: See my tmp105 debug output patch that went in between
>> v1 and v2, it used #ifdef inside the static function.
>> It was suggested that if the functions have no side-effects, GCC would
>> optimize them out in the if (0) case.
>> I checked in one case that a helper function cc_name() passed as vararg
>> to D_LOG() did not show up in objdump in the if (0) case. I admit I
>> probably didn't check for the D_LOG() function itself.
> 
> We'd have to check every compiler we care about, and even then we
> wouldn't really be certain the optimizer gets rid of the debug code
> reliably.
> 
> Relying on dead code elimination is fine in simple cases, but relying on
> automatic inlining + optimizing load of const variable into value of its
> initializer + dead code elminination is asking for quite a bit more.
> 
>> Macros: Need for \ linebreaks and heated discussions about
>> G_STMT_START/END vs. do {...} while (0) vs. functions. Really don't want
>> to repeat that.
> 
> If I remember correctly, that was you vs. everybody else, so an easy way
> not to repeat it is to do it the way everybody else does it ;-P

I was the only one to suggest and prefer use of macros (whether G_ or
not) in place of not-actually-needed loop code, yes.

The suggestion of using real functions came from Anthony.

>> If you want to make a proposal please do, it was a lot of dumb typing
>> work... For example you could use tracepoints to avoid the issue in
> 
> Thanks for that!  I hope we can salvage it.

Thanks for your review.

I would be willing to do a macro-based v3 using do { ... } while (0) if
maintainers can reach agreement on that and on how to do the if (0).

I am NOT willing to do a v4 or more though and keep going back and forth
just because everyone has different preferences. If we don't reach
agreement on how to uniformly tackle the debug-is-not-compile-tested
issue in a not-file-dependent way, then I will abandon this series and
cold-heartedly ignore breaking any maintainer's debug code that doesn't
actively provide some way of their own liking that allows me to test it
with --enable-debug configure option. And I feel under time pressure for
1.5 with my CPU series already due to CPU hot-plug.

Andreas

>> alpha and s390x, but others such as Alex and Peter don't like the
>> tracing infrastructure and it'd require non-mechanical changes so we
>> seem to be stuck with the current macros.
>>
>> Disabled ppc #ifdefs caused issues recently in a target-ppc series
>> removing ppc_def_t.
> 
> Annoying, but not the end of the world.
> 
>> And my QOM CPUState part 9 series needs to do ugly variable definitions
>> in target-cris/helper.c due to the disabled D(x) and D_LOG(x) macros.
> 
> Even more annoying.
> 
> I got one more: debug print macros are defined all over the place, with
> minor variations.

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