[PATCH v5 1/6] eal: trace: add trace point emit for blob

[Morten Brørup]

> From: Ankur Dwivedi [mailto:adwivedi at marvell.com]
> Sent: Thursday, 12 January 2023 12.22
> 
> Adds a trace point emit function for emitting a blob. The maximum blob
> bytes which can be captured is maximum value contained in uint16_t,
> which is 65535.
> 
> Also adds test case for emit array tracepoint function.
> 
> Signed-off-by: Ankur Dwivedi <adwivedi at marvell.com>
> ---

Excellent, thank you.

[...]

> +#define rte_trace_point_emit_blob(in, len) \
> +do { \
> +	if (unlikely(in == NULL)) \
> +		return; \
> +	__rte_trace_point_emit(len, uint16_t); \
> +	memcpy(mem, in, len); \
> +	mem = RTE_PTR_ADD(mem, len); \
> +} while (0)

I am somewhat unsure about my concerns here, so please forgive me if they are invalid...

Is rte_trace_point_emit_blob() always called with "len" being a variable, then this is OK.

If "len" can be a non-constant formula (e.g. len++), you need a temporary variable:

#define rte_trace_point_emit_blob(in, len) \
do { \
	uint16_t _len = len; \
	if (unlikely(in == NULL)) \
		return; \
	__rte_trace_point_emit(_len, uint16_t); \
	memcpy(mem, in, _len); \
	mem = RTE_PTR_ADD(_mem, _len); \
} while (0)

But I don't think this can ever happen.

However, if "len" can be a constant (e.g. 6), does __rte_trace_point_emit(len, uint16_t) accept it? (The __rte_trace_point_emit() macro is shown below.) A constant has no pointer to it (i.e. &6 does not exist).

Looking deeper at it, I'm afraid this question can be generalized to all the existing macros/functions calling __rte_trace_point_emit().

And now that I have hijacked your patch with a generalized question, I wonder if the existing __rte_trace_point_emit() has a bug? It uses sizeof(in), but I think it should use sizeof(type).

It looks like this:

#define __rte_trace_point_emit(in, type) \
do { \
	memcpy(mem, &(in), sizeof(in)); \
	mem = RTE_PTR_ADD(mem, sizeof(in)); \
} while (0)

Alternatively, __rte_trace_point_emit() should RTE_BUILD_BUG_ON(typeof(in) != type).


If my concerns above are invalid, then:

Acked-by: Morten Brørup <mb at smartsharesystems.com>