Re: [PATCH v2.1 13/12] xen/trace: Introduce new API

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.09.2021 21:29, Andrew Cooper wrote:
> --- a/xen/include/xen/trace.h
> +++ b/xen/include/xen/trace.h
> @@ -74,6 +74,30 @@ static inline void __trace_hypercall(uint32_t event, 
> unsigned long op,
>                                       const xen_ulong_t *args) {}
>  #endif /* CONFIG_TRACEBUFFER */
>  
> +/*
> + * Create a trace record, packaging up to 7 additional parameters into a
> + * uint32_t array.
> + */
> +#define TRACE_INTERNAL(_e, _c, ...)                                     \
> +    do {                                                                \
> +        if ( unlikely(tb_init_done) )                                   \
> +        {                                                               \
> +            uint32_t _d[] = { __VA_ARGS__ };                            \
> +            BUILD_BUG_ON(ARRAY_SIZE(_d) > TRACE_EXTRA_MAX);             \
> +            __trace_var(_e, _c, sizeof(_d), sizeof(_d) ? _d : NULL);    \
> +        }                                                               \
> +    } while ( 0 )

I know we sort of disagree on this aspect, but I would really like
to understand what you (and others) think the leading underscores
are good for in macro parameter names. And if those went away, I'd
like to ask that the local variable also become e.g. d_, like we
have started doing elsewhere.

> +/* Split a uint64_t into two adjacent uint32_t's for a trace record. */
> +#define TRACE_PARAM64(p)    (uint32_t)(p), ((p) >> 32)

You don't have a leading underscore here, for example.

> +/* Create a trace record with time included. */
> +#define TRACE_TIME(_e, ...) TRACE_INTERNAL(_e, true,  ##__VA_ARGS__)
> +
> +/* Create a trace record with no time included. */
> +#define TRACE(_e, ...)      TRACE_INTERNAL(_e, false, ##__VA_ARGS__)

Is , ## __VA_ARGS__ really doing what you expect? So far it has been
my understanding that the special behavior concatenating with a
comma only applies to the GNU form of variable macro arguments, e.g.

#define TRACE(_e, args...)      TRACE_INTERNAL(_e, false, ## args)

As a minor aspect (nit) - iirc it was you who had been asking me in a
few cases to treat ## like a normal binary operator when considering
style, requesting me to have a blank on each side of it.

> +
> +

Nit: Please can you avoid introducing double blank lines?

Jan