From: Sunil Kumar Kori <skori@marvell.com>
To: David Marchand <david.marchand@redhat.com>,
"dev@dpdk.org" <dev@dpdk.org>
Cc: Chengwen Feng <fengchengwen@huawei.com>,
Kevin Laatz <kevin.laatz@intel.com>,
Bruce Richardson <bruce.richardson@intel.com>,
Jerin Jacob <jerinj@marvell.com>,
Tyler Retzlaff <roretzla@linux.microsoft.com>
Subject: RE: [EXTERNAL] [PATCH v4 5/5] trace: fix undefined behavior in register
Date: Wed, 5 Mar 2025 05:20:14 +0000 [thread overview]
Message-ID: <CO6PR18MB386044D9FC76FB057D6040FAB4CB2@CO6PR18MB3860.namprd18.prod.outlook.com> (raw)
In-Reply-To: <20250304160633.385185-6-david.marchand@redhat.com>
> Registering a tracepoint handler was resulting so far in undefined behavior at
> runtime.
>
> The RTE_TRACE_POINT_REGISTER() macro was casting the tracepoint handler
> (which expects arguments) to a void (*)(void).
> At runtime, calling this handler while registering resulted in reading the
> current stack with no relation to this function prototype.
>
> Instead, declare an additional inline _register() handler for each tracepoint
> and make sure that the emitting macros in rte_trace_point_register.h only
> work on arguments name and type.
>
> The original tracepoint handler prototype is adjusted by adding a
> __rte_unused for each argument (since emitting macros do nothing with
> them).
> This last part introduces an implementation limit of 15 arguments.
>
> With this change in place, the workaround in dmadev tracepoints can be
> removed.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> lib/dmadev/rte_dmadev_trace.h | 12 ------
> lib/dmadev/rte_dmadev_trace_fp.h | 14 -------
> lib/eal/include/rte_trace_point.h | 4 ++
> lib/eal/include/rte_trace_point_register.h | 44 +++++++++++++++++++---
> 4 files changed, 42 insertions(+), 32 deletions(-)
>
> diff --git a/lib/dmadev/rte_dmadev_trace.h
> b/lib/dmadev/rte_dmadev_trace.h index 1beb938168..1de92655f2 100644
> --- a/lib/dmadev/rte_dmadev_trace.h
> +++ b/lib/dmadev/rte_dmadev_trace.h
> @@ -22,10 +22,6 @@ extern "C" {
> 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);
> @@ -42,10 +38,6 @@ 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_ */
> rte_trace_point_emit_i16(dev_id);
> rte_trace_point_emit_u16(dev_conf->nb_vchans);
> rte_trace_point_emit_u16(dev_conf->priority);
> @@ -78,10 +70,6 @@ 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_ */
> rte_trace_point_emit_i16(dev_id);
> rte_trace_point_emit_u16(vchan);
> rte_trace_point_emit_int(conf->direction);
> diff --git a/lib/dmadev/rte_dmadev_trace_fp.h
> b/lib/dmadev/rte_dmadev_trace_fp.h
> index f2656c6726..a1374e78b7 100644
> --- a/lib/dmadev/rte_dmadev_trace_fp.h
> +++ b/lib/dmadev/rte_dmadev_trace_fp.h
> @@ -33,10 +33,6 @@ 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_ */
> rte_trace_point_emit_i16(dev_id);
> rte_trace_point_emit_u16(vchan);
> rte_trace_point_emit_int_ptr(status);
> @@ -100,12 +96,6 @@ RTE_TRACE_POINT_FP(
> 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_ */
> rte_trace_point_emit_i16(dev_id);
> rte_trace_point_emit_u16(vchan);
> rte_trace_point_emit_u16(nb_cpls);
> @@ -119,10 +109,6 @@ RTE_TRACE_POINT_FP(
> 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_ */
> rte_trace_point_emit_i16(dev_id);
> rte_trace_point_emit_u16(vchan);
> rte_trace_point_emit_u16(nb_cpls);
> diff --git a/lib/eal/include/rte_trace_point.h
> b/lib/eal/include/rte_trace_point.h
> index 84de233d59..8a317d31d2 100644
> --- a/lib/eal/include/rte_trace_point.h
> +++ b/lib/eal/include/rte_trace_point.h
> @@ -36,6 +36,8 @@ extern "C" {
> /** The tracepoint object. */
> typedef RTE_ATOMIC(uint64_t) rte_trace_point_t;
>
> +#ifndef _RTE_TRACE_POINT_REGISTER_H_
> +
> /**
> * Macro to define the tracepoint arguments in RTE_TRACE_POINT macro.
>
> @@ -53,6 +55,8 @@ _tp _args \
> __VA_ARGS__ \
> }
>
> +#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
> +
> /**
> * Create a tracepoint.
> *
> diff --git a/lib/eal/include/rte_trace_point_register.h
> b/lib/eal/include/rte_trace_point_register.h
> index ac391eb28f..b036121959 100644
> --- a/lib/eal/include/rte_trace_point_register.h
> +++ b/lib/eal/include/rte_trace_point_register.h
> @@ -18,6 +18,43 @@ extern "C" {
>
> RTE_DECLARE_PER_LCORE(volatile int, trace_point_sz);
>
> +#define
> RTE_TRACE_POINT_ARGS_COUNT_(_0,_1,_2,_3,_4,_5,_6,_7,_8,_9,_10,_11,_1
> 2,_13,_14,_15,N, ...) \
> + N
> +#define RTE_TRACE_POINT_ARGS_COUNT(...) \
> +
> +RTE_TRACE_POINT_ARGS_COUNT_(__VA_ARGS__,15,14,13,12,11,10,9,8,7,6,
> 5,4,3
> +,2,1,0)
> +
> +#define RTE_TRACE_POINT_ARGS_1(a) __rte_unused a #define
> +RTE_TRACE_POINT_ARGS_2(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_1(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_3(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_2(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_4(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_3(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_5(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_4(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_6(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_5(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_7(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_6(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_8(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_7(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_9(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_8(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_10(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_9(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_11(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_10(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_12(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_11(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_13(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_12(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_14(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_13(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_15(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_14(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_FUNC(a) RTE_TRACE_POINT_ARGS_ ## a
> #define
> +RTE_TRACE_POINT_ARGS_EXPAND(...) __VA_ARGS__ #define
> +RTE_TRACE_POINT_ARGS_(N, ...) \
> +
> RTE_TRACE_POINT_ARGS_EXPAND(RTE_TRACE_POINT_ARGS_FUNC(
> N))(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS(...) \
> + (RTE_TRACE_POINT_ARGS_(RTE_TRACE_POINT_ARGS_COUNT(0,
> __VA_ARGS__),
> +__VA_ARGS__))
> +
> +#define __RTE_TRACE_POINT(_mode, _tp, _args, ...) \ extern
> +rte_trace_point_t __##_tp; \ static __rte_always_inline void _tp _args
> +{ } \ static __rte_always_inline void \ _tp ## _register (void) \ { \
> + __rte_trace_point_emit_header_##_mode(&__##_tp); \
> + __VA_ARGS__ \
> +}
> +
> #define RTE_TRACE_POINT_REGISTER(trace, name) \ rte_trace_point_t
> __rte_section("__rte_trace_point") __##trace; \ static const char
> __##trace##_name[] = RTE_STR(name); \ @@ -26,7 +63,7 @@
> RTE_INIT(trace##_init) \
> if (!rte_trace_feature_is_enabled()) \
> return; \
> __rte_trace_point_register(&__##trace, __##trace##_name, \
> - (void (*)(void)) trace); \
> + trace ## _register); \
> }
>
> #define __rte_trace_point_emit_header_generic(t) \ @@ -37,22 +74,17 @@
> RTE_INIT(trace##_init) \
>
> #define __rte_trace_point_emit(name, in, type) \ do { \
> - RTE_BUILD_BUG_ON(sizeof(type) != sizeof(typeof(*in))); \
> __rte_trace_point_emit_field(sizeof(type), name, RTE_STR(type)); \ }
> while (0)
>
> #define rte_trace_point_emit_string(in) \ do { \
> - RTE_SET_USED(in); \
>
> __rte_trace_point_emit_field(__RTE_TRACE_EMIT_STRING_LEN_MAX
> , \
> RTE_STR(in)"[32]", "string_bounded_t"); \ } while (0)
>
> #define rte_trace_point_emit_blob(in, len) \ do { \
> - uint8_t size = len; \
> - RTE_SET_USED(in); \
> - RTE_SET_USED(size); \
> __rte_trace_point_emit_field(sizeof(uint8_t), \
> RTE_STR(in) "_size", \
> RTE_STR(uint8_t)); \
> --
> 2.48.1
Acked-by: Sunil Kumar Kori <skori@marvell.com>
next prev parent reply other threads:[~2025-03-05 5:20 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-01-24 16:14 [PATCH 1/2] trace: support expression for blob length David Marchand
2025-01-24 16:14 ` [PATCH 2/2] dmadev: avoid copies in tracepoints David Marchand
2025-01-27 8:25 ` [EXTERNAL] [PATCH 1/2] trace: support expression for blob length Jerin Jacob
2025-01-30 14:58 ` [PATCH v2 1/3] " David Marchand
2025-01-30 14:58 ` [PATCH v2 2/3] dmadev: avoid copies in tracepoints David Marchand
2025-01-30 14:58 ` [PATCH v2 3/3] trace: fix undefined behavior in register David Marchand
2025-01-30 19:10 ` Stephen Hemminger
2025-01-30 21:06 ` David Marchand
2025-02-07 8:49 ` David Marchand
2025-02-07 11:39 ` [EXTERNAL] " Sunil Kumar Kori
2025-02-10 9:02 ` Sunil Kumar Kori
2025-02-10 9:36 ` David Marchand
2025-02-10 13:37 ` [EXTERNAL] " Jerin Jacob
2025-02-10 14:04 ` David Marchand
2025-02-10 13:38 ` [EXTERNAL] [PATCH v2 1/3] trace: support expression for blob length Jerin Jacob
2025-02-10 17:44 ` [PATCH v3 0/6] Trace point framework enhancement for dmadev David Marchand
2025-02-10 17:44 ` [PATCH v3 1/6] ci: check traces validity David Marchand
2025-02-10 17:44 ` [PATCH v3 2/6] trace: support dereferencing arguments David Marchand
2025-02-11 8:44 ` [EXTERNAL] " Sunil Kumar Kori
2025-02-11 9:53 ` David Marchand
2025-02-12 5:08 ` Sunil Kumar Kori
2025-02-10 17:44 ` [PATCH v3 3/6] trace: support expression for blob length David Marchand
2025-02-10 17:44 ` [PATCH v3 4/6] trace: support dumping binary inside a struct David Marchand
2025-02-11 8:52 ` [EXTERNAL] " Sunil Kumar Kori
2025-02-11 9:54 ` David Marchand
2025-02-12 5:14 ` Sunil Kumar Kori
2025-02-18 14:28 ` David Marchand
2025-02-19 11:17 ` Jerin Jacob
2025-02-10 17:44 ` [PATCH v3 5/6] dmadev: avoid copies in tracepoints David Marchand
2025-02-10 17:44 ` [PATCH v3 6/6] trace: fix undefined behavior in register David Marchand
2025-02-11 8:41 ` [EXTERNAL] [PATCH v3 0/6] Trace point framework enhancement for dmadev Sunil Kumar Kori
2025-03-04 16:06 ` [PATCH v4 0/5] " David Marchand
2025-03-04 16:06 ` [PATCH v4 1/5] ci: check traces validity David Marchand
2025-03-04 16:06 ` [PATCH v4 2/5] dmadev: avoid copies in tracepoints David Marchand
2025-03-04 16:06 ` [PATCH v4 3/5] trace: support dereferencing pointers David Marchand
2025-03-05 5:19 ` [EXTERNAL] " Sunil Kumar Kori
2025-03-04 16:06 ` [PATCH v4 4/5] trace: support expression for blob length David Marchand
2025-03-05 5:20 ` [EXTERNAL] " Sunil Kumar Kori
2025-03-04 16:06 ` [PATCH v4 5/5] trace: fix undefined behavior in register David Marchand
2025-03-05 5:20 ` Sunil Kumar Kori [this message]
2025-03-05 17:49 ` [PATCH v4 0/5] Trace point framework enhancement for dmadev David Marchand
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CO6PR18MB386044D9FC76FB057D6040FAB4CB2@CO6PR18MB3860.namprd18.prod.outlook.com \
--to=skori@marvell.com \
--cc=bruce.richardson@intel.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=fengchengwen@huawei.com \
--cc=jerinj@marvell.com \
--cc=kevin.laatz@intel.com \
--cc=roretzla@linux.microsoft.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).