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

[Jerin Jacob]

> -----Original Message-----
> From: Bruce Richardson <bruce.richardson at intel.com>
> Sent: Monday, December 9, 2024 4:58 AM
> To: Andre Muezerie <andremue at linux.microsoft.com>; Jerin Jacob
> <jerinj at marvell.com>
> Cc: Chengwen Feng <fengchengwen at huawei.com>; Kevin Laatz
> <kevin.laatz at intel.com>; dev at dpdk.org
> Subject: [EXTERNAL] Re: [PATCH 1/2] lib/dmadev: eliminate undefined behavior
> 
> 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 
> 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_


+ @Chengwen Feng

This kind of patten is not used other places like ethdev traces, Why we need this kind of pattern in dmadev?
Looks like, it can be fixed by caller of this function by initializing struct rte_dma_info. So may not need a fixup patch to begin with


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