[PATCH 0/2] enable dmadev lib to be compiled with MSVC

[PATCH 0/2] enable dmadev lib to be compiled with MSVC

[Andre Muezerie]

The first patch eliminates undefined behavior in the dmadev lib:

lib\dmadev\rte_dmadev_trace_fp.h(36):
    warning C5101: use of preprocessor directive in
    function-like macro argument list is undefined behavior

The second patch enables the dmadev library to be compiled with MSVC.

Andre Muezerie (2):
  lib/dmadev: eliminate undefined behavior
  lib/dmadev: enable dmadev lib to be compiled with MSVC

 lib/dmadev/meson.build           |  6 ----
 lib/dmadev/rte_dmadev_trace.h    | 62 ++++++++++++++++++++++++++++----
 lib/dmadev/rte_dmadev_trace_fp.h | 52 +++++++++++++++++++++++----
 3 files changed, 102 insertions(+), 18 deletions(-)

--
2.47.0.vfs.0.3

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

[Andre Muezerie]

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_ */
 
+#ifdef _RTE_TRACE_POINT_REGISTER_H_
 RTE_TRACE_POINT(
 	rte_dma_trace_configure,
 	RTE_TRACE_POINT_ARGS(int16_t dev_id, const struct rte_dma_conf *dev_conf,
 			     int ret),
-#ifdef _RTE_TRACE_POINT_REGISTER_H_
 	const struct rte_dma_conf __dev_conf = {0};
 	dev_conf = &__dev_conf;
-#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
 	int enable_silent = (int)dev_conf->enable_silent;
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(dev_conf->nb_vchans);
@@ -53,6 +67,19 @@ RTE_TRACE_POINT(
 	rte_trace_point_emit_int(enable_silent);
 	rte_trace_point_emit_int(ret);
 )
+#else
+RTE_TRACE_POINT(
+	rte_dma_trace_configure,
+	RTE_TRACE_POINT_ARGS(int16_t dev_id, const struct rte_dma_conf *dev_conf,
+			     int ret),
+	int enable_silent = (int)dev_conf->enable_silent;
+	rte_trace_point_emit_i16(dev_id);
+	rte_trace_point_emit_u16(dev_conf->nb_vchans);
+	rte_trace_point_emit_u16(dev_conf->priority);
+	rte_trace_point_emit_int(enable_silent);
+	rte_trace_point_emit_int(ret);
+)
+#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
 
 RTE_TRACE_POINT(
 	rte_dma_trace_start,
@@ -75,14 +102,13 @@ RTE_TRACE_POINT(
 	rte_trace_point_emit_int(ret);
 )
 
+#ifdef _RTE_TRACE_POINT_REGISTER_H_
 RTE_TRACE_POINT(
 	rte_dma_trace_vchan_setup,
 	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
 			     const struct rte_dma_vchan_conf *conf, int ret),
-#ifdef _RTE_TRACE_POINT_REGISTER_H_
 	const struct rte_dma_vchan_conf __conf = {0};
 	conf = &__conf;
-#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
 	int src_port_type = conf->src_port.port_type;
 	int dst_port_type = conf->dst_port.port_type;
 	int direction = conf->direction;
@@ -101,6 +127,30 @@ RTE_TRACE_POINT(
 	rte_trace_point_emit_ptr(conf->auto_free.m2d.pool);
 	rte_trace_point_emit_int(ret);
 )
+#else
+RTE_TRACE_POINT(
+	rte_dma_trace_vchan_setup,
+	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
+			     const struct rte_dma_vchan_conf *conf, int ret),
+	int src_port_type = conf->src_port.port_type;
+	int dst_port_type = conf->dst_port.port_type;
+	int direction = conf->direction;
+	uint64_t src_pcie_cfg;
+	uint64_t dst_pcie_cfg;
+	rte_trace_point_emit_i16(dev_id);
+	rte_trace_point_emit_u16(vchan);
+	rte_trace_point_emit_int(direction);
+	rte_trace_point_emit_u16(conf->nb_desc);
+	rte_trace_point_emit_int(src_port_type);
+	memcpy(&src_pcie_cfg, &conf->src_port.pcie, sizeof(uint64_t));
+	rte_trace_point_emit_u64(src_pcie_cfg);
+	memcpy(&dst_pcie_cfg, &conf->dst_port.pcie, sizeof(uint64_t));
+	rte_trace_point_emit_int(dst_port_type);
+	rte_trace_point_emit_u64(dst_pcie_cfg);
+	rte_trace_point_emit_ptr(conf->auto_free.m2d.pool);
+	rte_trace_point_emit_int(ret);
+)
+#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
 
 RTE_TRACE_POINT(
 	rte_dma_trace_stats_reset,
diff --git a/lib/dmadev/rte_dmadev_trace_fp.h b/lib/dmadev/rte_dmadev_trace_fp.h
index f5b96838bc..0f26f71c80 100644
--- a/lib/dmadev/rte_dmadev_trace_fp.h
+++ b/lib/dmadev/rte_dmadev_trace_fp.h
@@ -29,20 +29,31 @@ RTE_TRACE_POINT_FP(
 	rte_trace_point_emit_int(ret);
 )
 
+#ifdef _RTE_TRACE_POINT_REGISTER_H_
 RTE_TRACE_POINT_FP(
 	rte_dma_trace_vchan_status,
 	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
 			     enum rte_dma_vchan_status *status, int ret),
-#ifdef _RTE_TRACE_POINT_REGISTER_H_
 	enum rte_dma_vchan_status __status = 0;
 	status = &__status;
-#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
 	int vchan_status = *status;
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(vchan);
 	rte_trace_point_emit_int(vchan_status);
 	rte_trace_point_emit_int(ret);
 )
+#else
+RTE_TRACE_POINT_FP(
+	rte_dma_trace_vchan_status,
+	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
+			     enum rte_dma_vchan_status *status, int ret),
+	int vchan_status = *status;
+	rte_trace_point_emit_i16(dev_id);
+	rte_trace_point_emit_u16(vchan);
+	rte_trace_point_emit_int(vchan_status);
+	rte_trace_point_emit_int(ret);
+)
+#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
 
 RTE_TRACE_POINT_FP(
 	rte_dma_trace_copy,
@@ -96,17 +107,16 @@ RTE_TRACE_POINT_FP(
 	rte_trace_point_emit_int(ret);
 )
 
+#ifdef _RTE_TRACE_POINT_REGISTER_H_
 RTE_TRACE_POINT_FP(
 	rte_dma_trace_completed,
 	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
 			     const uint16_t nb_cpls, uint16_t *last_idx,
 			     bool *has_error, uint16_t ret),
-#ifdef _RTE_TRACE_POINT_REGISTER_H_
 	uint16_t __last_idx = 0;
 	bool __has_error = false;
 	last_idx = &__last_idx;
 	has_error = &__has_error;
-#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
 	int has_error_val = *has_error;
 	int last_idx_val = *last_idx;
 	rte_trace_point_emit_i16(dev_id);
@@ -116,16 +126,31 @@ RTE_TRACE_POINT_FP(
 	rte_trace_point_emit_int(has_error_val);
 	rte_trace_point_emit_u16(ret);
 )
+#else
+RTE_TRACE_POINT_FP(
+	rte_dma_trace_completed,
+	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
+			     const uint16_t nb_cpls, uint16_t *last_idx,
+			     bool *has_error, uint16_t ret),
+	int has_error_val = *has_error;
+	int last_idx_val = *last_idx;
+	rte_trace_point_emit_i16(dev_id);
+	rte_trace_point_emit_u16(vchan);
+	rte_trace_point_emit_u16(nb_cpls);
+	rte_trace_point_emit_int(last_idx_val);
+	rte_trace_point_emit_int(has_error_val);
+	rte_trace_point_emit_u16(ret);
+)
+#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
 
+#ifdef _RTE_TRACE_POINT_REGISTER_H_
 RTE_TRACE_POINT_FP(
 	rte_dma_trace_completed_status,
 	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
 			     const uint16_t nb_cpls, uint16_t *last_idx,
 			     enum rte_dma_status_code *status, uint16_t ret),
-#ifdef _RTE_TRACE_POINT_REGISTER_H_
 	uint16_t __last_idx = 0;
 	last_idx = &__last_idx;
-#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
 	int last_idx_val = *last_idx;
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(vchan);
@@ -134,6 +159,21 @@ RTE_TRACE_POINT_FP(
 	rte_trace_point_emit_ptr(status);
 	rte_trace_point_emit_u16(ret);
 )
+#else
+RTE_TRACE_POINT_FP(
+	rte_dma_trace_completed_status,
+	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
+			     const uint16_t nb_cpls, uint16_t *last_idx,
+			     enum rte_dma_status_code *status, uint16_t ret),
+	int last_idx_val = *last_idx;
+	rte_trace_point_emit_i16(dev_id);
+	rte_trace_point_emit_u16(vchan);
+	rte_trace_point_emit_u16(nb_cpls);
+	rte_trace_point_emit_int(last_idx_val);
+	rte_trace_point_emit_ptr(status);
+	rte_trace_point_emit_u16(ret);
+)
+#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
 
 RTE_TRACE_POINT_FP(
 	rte_dma_trace_burst_capacity,
-- 
2.47.0.vfs.0.3

[PATCH 2/2] lib/dmadev: enable dmadev lib to be compiled with MSVC

[Andre Muezerie]

With the outstanding issues preventing dmadev from being compiled
with MSVC being fixed by this series, the lib can be enabled to
be compiled with MSVC.

Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
---
 lib/dmadev/meson.build | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/lib/dmadev/meson.build b/lib/dmadev/meson.build
index e66dcb66b0..62b0650b9b 100644
--- a/lib/dmadev/meson.build
+++ b/lib/dmadev/meson.build
@@ -1,12 +1,6 @@
 # SPDX-License-Identifier: BSD-3-Clause
 # Copyright(c) 2021 HiSilicon Limited.
 
-if is_ms_compiler
-    build = false
-    reason = 'not supported building with Visual Studio Toolset'
-    subdir_done()
-endif
-
 sources = files('rte_dmadev.c', 'rte_dmadev_trace_points.c')
 headers = files('rte_dmadev.h')
 indirect_headers += files('rte_dmadev_core.h', 'rte_dmadev_trace_fp.h')
-- 
2.47.0.vfs.0.3

Re: [PATCH 0/2] enable dmadev lib to be compiled with MSVC

[fengchengwen]

Series-acked-by: Chengwen Feng <fengchengwen@huawei.com>

Thanks

On 2024/12/7 3:27, Andre Muezerie wrote:
> The first patch eliminates undefined behavior in the dmadev lib:
> 
> lib\dmadev\rte_dmadev_trace_fp.h(36):
>     warning C5101: use of preprocessor directive in
>     function-like macro argument list is undefined behavior
> 
> The second patch enables the dmadev library to be compiled with MSVC.
> 
> Andre Muezerie (2):
>   lib/dmadev: eliminate undefined behavior
>   lib/dmadev: enable dmadev lib to be compiled with MSVC
> 
>  lib/dmadev/meson.build           |  6 ----
>  lib/dmadev/rte_dmadev_trace.h    | 62 ++++++++++++++++++++++++++++----
>  lib/dmadev/rte_dmadev_trace_fp.h | 52 +++++++++++++++++++++++----
>  3 files changed, 102 insertions(+), 18 deletions(-)
> 
> --
> 2.47.0.vfs.0.3
> 

Re: [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@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

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

[Jerin Jacob]

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

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

[fengchengwen]

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
>