|SUCCESS| [v2,2/2] test: parenthesize assertion macro parameters

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163547

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-01

git sha: (from patch 1/2) 20260501163533.2689152-2-stephen at networkplumber.org
git sha: (from patch 2/2) 20260501163533.2689152-3-stephen at networkplumber.org

Author: Stephen Hemminger <stephen at networkplumber.org>
Author: Weijun Pan <wpan3636 at gmail.com>

Subject: [PATCH v2 1/2] test: use inline helpers in buffer comparison macros
Subject: [PATCH v2 2/2] test: parenthesize assertion macro parameters

This two-patch series refactors the TEST_ASSERT_BUFFERS_ARE_EQUAL family
of macros to fix operator precedence and multiple evaluation bugs by
moving comparison logic into static inline helper functions, and adds
parentheses around macro parameters in the RTE_TEST_ASSERT family to
prevent operator precedence issues after macro substitution.

> diff --git a/app/test/test.h b/app/test/test.h
> index 1f12fc5397..b29233bb32 100644
> --- a/app/test/test.h
> +++ b/app/test/test.h
> @@ -6,12 +6,18 @@
>  #define _TEST_H_
>  
>  #include <errno.h>
> +#include <stdbool.h>
>  #include <stddef.h>
> +#include <stdint.h>
> +#include <stdio.h>
>  #include <stdlib.h>
> +#include <string.h>
>  #include <sys/queue.h>
>  
> -#include <rte_hexdump.h>
> +#include <rte_bitops.h>
>  #include <rte_common.h>
> +#include <rte_debug.h>
> +#include <rte_hexdump.h>
>  #include <rte_lcore.h>
>  #include <rte_os_shim.h>

[ ... ]

> +/*
> + * Helpers backing the TEST_ASSERT_BUFFERS_ARE_EQUAL* macros.
> + *
> + * Keeping the comparison logic in inline functions ensures each macro
> + * argument is evaluated exactly once and gives the compiler real types
> + * to check against, which the original all-macro implementation could
> + * not provide.
> + */
> +static inline bool
> +test_buffers_equal_offset(const void *a, const void *b,
> +		size_t len, size_t off)
> +{
> +	const uint8_t *pa = (const uint8_t *)a + off;
> +	const uint8_t *pb = (const uint8_t *)b + off;
> +
> +	return memcmp(pa, pb, len) == 0;
> +}

Does this code overflow pa or pb if the caller passes an offset that
exceeds the buffer size?  The old macro logic did pointer arithmetic on
(const uint8_t *)a + off without any size checks, and this inline helper
preserves the same pattern.

> +
> +static inline bool
> +test_buffers_equal_bit(const void *a, const void *b, size_t len)
> +{
> +	const uint8_t *pa = a;
> +	const uint8_t *pb = b;
> +	size_t len_bytes = len >> 3;
> +	size_t len_bits = len & 7;
> +
> +	if (memcmp(pa, pb, len_bytes) != 0)
> +		return false;
> +	if (len_bits != 0) {
> +		uint8_t mask = (uint8_t)RTE_GENMASK32(7, 8 - len_bits);

Does this code overflow the shift in RTE_GENMASK32 when len_bits is 0?
The old macro code used _last_byte_bits = len % 8 and then
~((1 << (8 - _last_bit_bits)) - 1).  Does RTE_GENMASK32(7, 8) produce the
same result as ~((1 << 0) - 1)?  The RTE_GENMASK32 macro does
(~0u - (1u << (low)) + 1) & (~0u >> (32 - 1 - (high))).  When low=8 and
high=7, that shifts (1u << 8), which is 256, and (~0u >> 24), which is
255. The old code with len_bits=0 would shift (1 << 8) yielding 256, then
~(256 - 1) = ~255 = 0xff00 truncated to uint8_t gives 0x00.  Does
RTE_GENMASK32(7, 8) give 0x00?

> +
> +		if ((pa[len_bytes] & mask) != (pb[len_bytes] & mask))

Can this code overflow pa or pb when len_bytes exceeds the buffer size?

> +			return false;
> +	}
> +	return true;
> +}
> +
> +static inline bool
> +test_buffers_equal_bit_offset(const void *a, const void *b,
> +		size_t len, size_t off)
> +{
> +	const uint8_t *pa = a;
> +	const uint8_t *pb = b;
> +	size_t off_bits = off & 7;
> +	size_t off_bytes = off >> 3;
> +
> +	if (off_bits != 0) {
> +		uint8_t first_bits = 8 - off_bits;
> +		uint8_t mask = (uint8_t)RTE_GENMASK32(first_bits - 1, 0);

Does this code work correctly when off_bits is 7?  That makes
first_bits = 1, and RTE_GENMASK32(0, 0) should give 1.  The old code used
(1 << _first_byte_bits) - 1, which with _first_byte_bits=1 gives
(1 << 1) - 1 = 1.  Does RTE_GENMASK32(0, 0) produce the same result?

> +
> +		if ((pa[off_bytes] & mask) != (pb[off_bytes] & mask))

Can this code overflow pa or pb?

> +			return false;
> +		RTE_ASSERT(len >= first_bits);

Does this code properly handle the case where len < first_bits?  The
RTE_ASSERT will fire in debug builds, but in release builds
(when RTE_ENABLE_ASSERT is not defined) does the code proceed with
incorrect arithmetic?  The old macro code did not have this assertion.

> +		off_bytes++;
> +		len -= first_bits;
> +	}
> +	return test_buffers_equal_bit(pa + off_bytes, pb + off_bytes, len);

Can this code overflow pa or pb when (off >> 3) + 1 exceeds the buffer
size?

[ ... ]

> diff --git a/lib/eal/include/rte_test.h b/lib/eal/include/rte_test.h
> index 62c8f165af..d132d3156b 100644
> --- a/lib/eal/include/rte_test.h
> +++ b/lib/eal/include/rte_test.h
> @@ -26,21 +26,21 @@
>  } while (0)
>  
>  #define RTE_TEST_ASSERT_EQUAL(a, b, msg, ...) \
> -	RTE_TEST_ASSERT(a == b, msg, ##__VA_ARGS__)
> +	RTE_TEST_ASSERT((a) == (b), msg, ##__VA_ARGS__)
>  
>  #define RTE_TEST_ASSERT_NOT_EQUAL(a, b, msg, ...) \
> -	RTE_TEST_ASSERT(a != b, msg, ##__VA_ARGS__)
> +	RTE_TEST_ASSERT((a) != (b), msg, ##__VA_ARGS__)
>  
>  #define RTE_TEST_ASSERT_SUCCESS(val, msg, ...) \
> -	RTE_TEST_ASSERT(val == 0, msg, ##__VA_ARGS__)
> +	RTE_TEST_ASSERT((val) == 0, msg, ##__VA_ARGS__)
>  
>  #define RTE_TEST_ASSERT_FAIL(val, msg, ...) \
> -	RTE_TEST_ASSERT(val != 0, msg, ##__VA_ARGS__)
> +	RTE_TEST_ASSERT((val) != 0, msg, ##__VA_ARGS__)
>  
>  #define RTE_TEST_ASSERT_NULL(val, msg, ...) \
> -	RTE_TEST_ASSERT(val == NULL, msg, ##__VA_ARGS__)
> +	RTE_TEST_ASSERT((val) == NULL, msg, ##__VA_ARGS__)
>  
>  #define RTE_TEST_ASSERT_NOT_NULL(val, msg, ...) \
> -	RTE_TEST_ASSERT(val != NULL, msg, ##__VA_ARGS__)
> +	RTE_TEST_ASSERT((val) != NULL, msg, ##__VA_ARGS__)

This change looks good and fixes the operator precedence issue described
in the commit message.