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

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@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(-)
  

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

> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Monday, December 9, 2024 4:58 AM
> To: Andre Muezerie <andremue@linux.microsoft.com>; Jerin Jacob
> <jerinj@marvell.com>
> Cc: Chengwen Feng <fengchengwen@huawei.com>; Kevin Laatz
> <kevin.laatz@intel.com>; dev@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@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
  

On 2024/12/10 7:21, Jerin Jacob wrote:
> 
> 
>> -----Original Message-----
>> From: Bruce Richardson <bruce.richardson@intel.com>
>> Sent: Monday, December 9, 2024 4:58 AM
>> To: Andre Muezerie <andremue@linux.microsoft.com>; Jerin Jacob
>> <jerinj@marvell.com>
>> Cc: Chengwen Feng <fengchengwen@huawei.com>; Kevin Laatz
>> <kevin.laatz@intel.com>; dev@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@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

It's strange that no other library doesn't have this problem.

When I first add tracepoints support for dmadev, there is no such macro (just like other library),
but the CI report ASAN error.

The rootcause is that register:
	RTE_TRACE_POINT_REGISTER(rte_dma_trace_info_get,
		lib.dmadev.info_get)
it will invoke :
	__rte_trace_point_register(&__rte_dma_trace_info_get, __rte_dma_trace_info_get_lib.dmadev.info_get,
				(void (*)(void)rte_dma_trace_info_get) {
            rte_dma_trace_info_get();
        }

But rte_dma_trace_info_get() it was defined with parameters: int16_t dev_id, struct rte_dma_info *dev_info
If we force invoke rte_dma_trace_info_get() without pass any parameter, it may lead to ASAN problem because
the parameter's corresponding register was not set (and it's value undefine).


Because it was happened only when register, so add such macro for it.

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

On Tue, Dec 10, 2024 at 1:58 AM fengchengwen <fengchengwen@huawei.com> wrote:
> > + @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
>
> It's strange that no other library doesn't have this problem.
>
> When I first add tracepoints support for dmadev, there is no such macro (just like other library),
> but the CI report ASAN error.
>
> The rootcause is that register:
>         RTE_TRACE_POINT_REGISTER(rte_dma_trace_info_get,
>                 lib.dmadev.info_get)
> it will invoke :
>         __rte_trace_point_register(&__rte_dma_trace_info_get, __rte_dma_trace_info_get_lib.dmadev.info_get,
>                                 (void (*)(void)rte_dma_trace_info_get) {
>             rte_dma_trace_info_get();
>         }
>
> But rte_dma_trace_info_get() it was defined with parameters: int16_t dev_id, struct rte_dma_info *dev_info
> If we force invoke rte_dma_trace_info_get() without pass any parameter, it may lead to ASAN problem because
> the parameter's corresponding register was not set (and it's value undefine).

I remember of an issue with tracepoint and *UB*SAN, but I fail to see
how ASAN is affected (plus I see that the CI runs the tracepoint
autotests with ASAN).
Can you clarify?

In any case, this looks like something that should be handled at the
tracepoint framework level, and not silenced/wrapped around in the
dmadev library.
  

On Fri, Jan 24, 2025 at 12:13 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Tue, Dec 10, 2024 at 1:58 AM fengchengwen <fengchengwen@huawei.com> wrote:
> > > + @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
> >
> > It's strange that no other library doesn't have this problem.
> >
> > When I first add tracepoints support for dmadev, there is no such macro (just like other library),
> > but the CI report ASAN error.
> >
> > The rootcause is that register:
> >         RTE_TRACE_POINT_REGISTER(rte_dma_trace_info_get,
> >                 lib.dmadev.info_get)
> > it will invoke :
> >         __rte_trace_point_register(&__rte_dma_trace_info_get, __rte_dma_trace_info_get_lib.dmadev.info_get,
> >                                 (void (*)(void)rte_dma_trace_info_get) {
> >             rte_dma_trace_info_get();
> >         }
> >
> > But rte_dma_trace_info_get() it was defined with parameters: int16_t dev_id, struct rte_dma_info *dev_info
> > If we force invoke rte_dma_trace_info_get() without pass any parameter, it may lead to ASAN problem because
> > the parameter's corresponding register was not set (and it's value undefine).
>
> I remember of an issue with tracepoint and *UB*SAN, but I fail to see
> how ASAN is affected (plus I see that the CI runs the tracepoint
> autotests with ASAN).
> Can you clarify?
>
> In any case, this looks like something that should be handled at the
> tracepoint framework level, and not silenced/wrapped around in the
> dmadev library.

Fengcheng, Jerin, André, could you please have a look at this patch:
https://patchwork.dpdk.org/project/dpdk/patch/20250130145849.82003-3-david.marchand@redhat.com/


Thanks!