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