[PATCH 1/2] lib/dmadev: eliminate undefined behavior

[Bruce Richardson]

On Fri, Dec 06, 2024 at 11:27:52AM -0800, Andre Muezerie wrote:
> MSVC compiler issues warnings like the one below:
> 
> lib\dmadev\rte_dmadev_trace_fp.h(36):
>     warning C5101: use of preprocessor directive in
>     function-like macro argument list is undefined behavior
> 
> Indeed, looking at C99 section 6.10.3 Macro replacement, paragraph 11:
> "If there are sequences of preprocessing tokens within the list of
> arguments that would otherwise act as preprocessing directives, the
> behavior is undefined."
> 
> The fix proposed in this patch moves the ifdef to the outside.
> This results in no perf impact, but some lines end up being
> duplicated, which seems to be a reasonable trade-off.
> 
> Signed-off-by: Andre Muezerie <andremue at linux.microsoft.com>
> ---
>  lib/dmadev/rte_dmadev_trace.h    | 62 ++++++++++++++++++++++++++++----
>  lib/dmadev/rte_dmadev_trace_fp.h | 52 +++++++++++++++++++++++----
>  2 files changed, 102 insertions(+), 12 deletions(-)
> 
> diff --git a/lib/dmadev/rte_dmadev_trace.h b/lib/dmadev/rte_dmadev_trace.h
> index be089c065c..264a464928 100644
> --- a/lib/dmadev/rte_dmadev_trace.h
> +++ b/lib/dmadev/rte_dmadev_trace.h
> @@ -19,13 +19,12 @@
>  extern "C" {
>  #endif
>  
> +#ifdef _RTE_TRACE_POINT_REGISTER_H_
>  RTE_TRACE_POINT(
>  	rte_dma_trace_info_get,
>  	RTE_TRACE_POINT_ARGS(int16_t dev_id, struct rte_dma_info *dev_info),
> -#ifdef _RTE_TRACE_POINT_REGISTER_H_
>  	struct rte_dma_info __dev_info = {0};
>  	dev_info = &__dev_info;
> -#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
>  	rte_trace_point_emit_i16(dev_id);
>  	rte_trace_point_emit_string(dev_info->dev_name);
>  	rte_trace_point_emit_u64(dev_info->dev_capa);
> @@ -37,15 +36,30 @@ RTE_TRACE_POINT(
>  	rte_trace_point_emit_u16(dev_info->nb_vchans);
>  	rte_trace_point_emit_u16(dev_info->nb_priorities);
>  )
> +#else
> +RTE_TRACE_POINT(
> +	rte_dma_trace_info_get,
> +	RTE_TRACE_POINT_ARGS(int16_t dev_id, struct rte_dma_info *dev_info),
> +	rte_trace_point_emit_i16(dev_id);
> +	rte_trace_point_emit_string(dev_info->dev_name);
> +	rte_trace_point_emit_u64(dev_info->dev_capa);
> +	rte_trace_point_emit_u16(dev_info->max_vchans);
> +	rte_trace_point_emit_u16(dev_info->max_desc);
> +	rte_trace_point_emit_u16(dev_info->min_desc);
> +	rte_trace_point_emit_u16(dev_info->max_sges);
> +	rte_trace_point_emit_i16(dev_info->numa_node);
> +	rte_trace_point_emit_u16(dev_info->nb_vchans);
> +	rte_trace_point_emit_u16(dev_info->nb_priorities);
> +)
> +#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
>  

+Jerin

I'm unfortunately not familiar with the internals of the traceing library.
Can someone perhaps explain (and maybe add in the code as a comment), why
we need conditional compilation here? Specifically, when would
_RTE_TRACE_POINT_REGISTER_H_ be defined, and when not defined? (or why not
defined?) How does having it defined or not affect the expansion of the
macro here?

Thanks,
/Bruce