[PATCH] test: parenthesize assertion macro parameters

[PATCH] test: parenthesize assertion macro parameters

[Weijun Pan]

Some test assertion macros use parameters directly in expressions,
which can lead to unexpected evaluation due to operator precedence
after macro substitution.

Fix this by parenthesizing macro parameters and the resulting
expressions in rte_test.h and app/test/test.h.

Bugzilla ID: 1925

Signed-off-by: Weijun Pan <wpan36@wisc.edu>
---
 app/test/test.h            | 34 +++++++++++++++++-----------------
 lib/eal/include/rte_test.h | 12 ++++++------
 2 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/app/test/test.h b/app/test/test.h
index 1f12fc5397..b1174e4973 100644
--- a/app/test/test.h
+++ b/app/test/test.h
@@ -34,7 +34,7 @@
 
 /* Compare two buffers (length in bytes) */
 #define TEST_ASSERT_BUFFERS_ARE_EQUAL(a, b, len,  msg, ...) do {	\
-	if (memcmp(a, b, len)) {                                        \
+	if (memcmp((a), (b), (len))) {                                        \
 		printf("TestCase %s() line %d failed: "              \
 			msg "\n", __func__, __LINE__, ##__VA_ARGS__);    \
 		TEST_TRACE_FAILURE(__FILE__, __LINE__, __func__);    \
@@ -44,21 +44,21 @@
 
 /* Compare two buffers with offset (length and offset in bytes) */
 #define TEST_ASSERT_BUFFERS_ARE_EQUAL_OFFSET(a, b, len, off, msg, ...) do { \
-	const uint8_t *_a_with_off = (const uint8_t *)a + off;              \
-	const uint8_t *_b_with_off = (const uint8_t *)b + off;              \
-	TEST_ASSERT_BUFFERS_ARE_EQUAL(_a_with_off, _b_with_off, len, msg);  \
+	const uint8_t *_a_with_off = (const uint8_t *)(a) + (off);              \
+	const uint8_t *_b_with_off = (const uint8_t *)(b) + (off);              \
+	TEST_ASSERT_BUFFERS_ARE_EQUAL(_a_with_off, _b_with_off, (len), msg);  \
 } while (0)
 
 /* Compare two buffers (length in bits) */
 #define TEST_ASSERT_BUFFERS_ARE_EQUAL_BIT(a, b, len, msg, ...) do {	\
 	uint8_t _last_byte_a, _last_byte_b;                       \
 	uint8_t _last_byte_mask, _last_byte_bits;                  \
-	TEST_ASSERT_BUFFERS_ARE_EQUAL(a, b, (len >> 3), msg);     \
-	if (len % 8) {                                              \
-		_last_byte_bits = len % 8;                   \
+	TEST_ASSERT_BUFFERS_ARE_EQUAL((a), (b), ((len) >> 3), msg);     \
+	if ((len) % 8) {                                              \
+		_last_byte_bits = (len) % 8;                   \
 		_last_byte_mask = ~((1 << (8 - _last_byte_bits)) - 1); \
-		_last_byte_a = ((const uint8_t *)a)[len >> 3];            \
-		_last_byte_b = ((const uint8_t *)b)[len >> 3];            \
+		_last_byte_a = ((const uint8_t *)(a))[(len) >> 3];            \
+		_last_byte_b = ((const uint8_t *)(b))[(len) >> 3];            \
 		_last_byte_a &= _last_byte_mask;                     \
 		_last_byte_b &= _last_byte_mask;                    \
 		if (_last_byte_a != _last_byte_b) {                  \
@@ -74,16 +74,16 @@
 #define TEST_ASSERT_BUFFERS_ARE_EQUAL_BIT_OFFSET(a, b, len, off, msg, ...) do {	\
 	uint8_t _first_byte_a, _first_byte_b;                                 \
 	uint8_t _first_byte_mask, _first_byte_bits;                           \
-	uint32_t _len_without_first_byte = (off % 8) ?                       \
-				len - (8 - (off % 8)) :                       \
-				len;                                          \
-	uint32_t _off_in_bytes = (off % 8) ? (off >> 3) + 1 : (off >> 3);     \
-	const uint8_t *_a_with_off = (const uint8_t *)a + _off_in_bytes;      \
-	const uint8_t *_b_with_off = (const uint8_t *)b + _off_in_bytes;      \
+	uint32_t _len_without_first_byte = ((off) % 8) ?                       \
+				((len) - (8 - ((off) % 8))) :                       \
+				(len);                                          \
+	uint32_t _off_in_bytes = ((off) % 8) ? (((off) >> 3) + 1) : ((off) >> 3);     \
+	const uint8_t *_a_with_off = (const uint8_t *)(a) + _off_in_bytes;      \
+	const uint8_t *_b_with_off = (const uint8_t *)(b) + _off_in_bytes;      \
 	TEST_ASSERT_BUFFERS_ARE_EQUAL_BIT(_a_with_off, _b_with_off,           \
 				_len_without_first_byte, msg);                \
-	if (off % 8) {                                                        \
-		_first_byte_bits = 8 - (off % 8);                             \
+	if ((off) % 8) {                                                        \
+		_first_byte_bits = 8 - ((off) % 8);                             \
 		_first_byte_mask = (1 << _first_byte_bits) - 1;               \
 		_first_byte_a = *(_a_with_off - 1);                           \
 		_first_byte_b = *(_b_with_off - 1);                           \
diff --git a/lib/eal/include/rte_test.h b/lib/eal/include/rte_test.h
index 62c8f165af..b73d4e75c2 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__)
 
 #endif /* _RTE_TEST_H_ */
-- 
2.34.1

RE: [PATCH] test: parenthesize assertion macro parameters

[Marat Khalili]

Hi, thanks for doing this! Please see some comments inline below.

> -----Original Message-----
> From: Weijun Pan <wpan3636@gmail.com>
> Sent: Sunday 19 April 2026 17:48
> Cc: dev@dpdk.org; Weijun Pan <wpan36@wisc.edu>
> Subject: [PATCH] test: parenthesize assertion macro parameters
> 
> Some test assertion macros use parameters directly in expressions,
> which can lead to unexpected evaluation due to operator precedence
> after macro substitution.
> 
> Fix this by parenthesizing macro parameters and the resulting
> expressions in rte_test.h and app/test/test.h.
> 
> Bugzilla ID: 1925
> 
> Signed-off-by: Weijun Pan <wpan36@wisc.edu>
> ---
>  app/test/test.h            | 34 +++++++++++++++++-----------------
>  lib/eal/include/rte_test.h | 12 ++++++------
>  2 files changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/app/test/test.h b/app/test/test.h
> index 1f12fc5397..b1174e4973 100644
> --- a/app/test/test.h
> +++ b/app/test/test.h
> @@ -34,7 +34,7 @@
> 
>  /* Compare two buffers (length in bytes) */
>  #define TEST_ASSERT_BUFFERS_ARE_EQUAL(a, b, len,  msg, ...) do {	\
> -	if (memcmp(a, b, len)) {                                        \
> +	if (memcmp((a), (b), (len))) {                                        \
>  		printf("TestCase %s() line %d failed: "              \
>  			msg "\n", __func__, __LINE__, ##__VA_ARGS__);    \
>  		TEST_TRACE_FAILURE(__FILE__, __LINE__, __func__);    \
> @@ -44,21 +44,21 @@
> 
>  /* Compare two buffers with offset (length and offset in bytes) */
>  #define TEST_ASSERT_BUFFERS_ARE_EQUAL_OFFSET(a, b, len, off, msg, ...) do { \
> -	const uint8_t *_a_with_off = (const uint8_t *)a + off;              \
> -	const uint8_t *_b_with_off = (const uint8_t *)b + off;              \
> -	TEST_ASSERT_BUFFERS_ARE_EQUAL(_a_with_off, _b_with_off, len, msg);  \
> +	const uint8_t *_a_with_off = (const uint8_t *)(a) + (off);              \
> +	const uint8_t *_b_with_off = (const uint8_t *)(b) + (off);              \
> +	TEST_ASSERT_BUFFERS_ARE_EQUAL(_a_with_off, _b_with_off, (len), msg);  \

Probably needed a comment somewhere in the commit message or cover letter (part
after --- for a single commit) what the strategy was to help reviewers:
parenthesize all substitutions even in unambiguous places like surrounded by
commas (which is fine to me personally, finding places where parentheses are
not strictly necessary needs effort), but leave msg alone since it has to be a
string literal and parenthesizing it would break literal concatenation.

>  } while (0)
> 
>  /* Compare two buffers (length in bits) */
>  #define TEST_ASSERT_BUFFERS_ARE_EQUAL_BIT(a, b, len, msg, ...) do {	\
>  	uint8_t _last_byte_a, _last_byte_b;                       \
>  	uint8_t _last_byte_mask, _last_byte_bits;                  \
> -	TEST_ASSERT_BUFFERS_ARE_EQUAL(a, b, (len >> 3), msg);     \
> -	if (len % 8) {                                              \
> -		_last_byte_bits = len % 8;                   \
> +	TEST_ASSERT_BUFFERS_ARE_EQUAL((a), (b), ((len) >> 3), msg);     \
> +	if ((len) % 8) {                                              \
> +		_last_byte_bits = (len) % 8;                   \
>  		_last_byte_mask = ~((1 << (8 - _last_byte_bits)) - 1); \
> -		_last_byte_a = ((const uint8_t *)a)[len >> 3];            \
> -		_last_byte_b = ((const uint8_t *)b)[len >> 3];            \
> +		_last_byte_a = ((const uint8_t *)(a))[(len) >> 3];            \
> +		_last_byte_b = ((const uint8_t *)(b))[(len) >> 3];            \
>  		_last_byte_a &= _last_byte_mask;                     \
>  		_last_byte_b &= _last_byte_mask;                    \
>  		if (_last_byte_a != _last_byte_b) {                  \
> @@ -74,16 +74,16 @@
>  #define TEST_ASSERT_BUFFERS_ARE_EQUAL_BIT_OFFSET(a, b, len, off, msg, ...) do {	\
>  	uint8_t _first_byte_a, _first_byte_b;                                 \
>  	uint8_t _first_byte_mask, _first_byte_bits;                           \
> -	uint32_t _len_without_first_byte = (off % 8) ?                       \
> -				len - (8 - (off % 8)) :                       \
> -				len;                                          \
> -	uint32_t _off_in_bytes = (off % 8) ? (off >> 3) + 1 : (off >> 3);     \
> -	const uint8_t *_a_with_off = (const uint8_t *)a + _off_in_bytes;      \
> -	const uint8_t *_b_with_off = (const uint8_t *)b + _off_in_bytes;      \
> +	uint32_t _len_without_first_byte = ((off) % 8) ?                       \
> +				((len) - (8 - ((off) % 8))) :                       \
> +				(len);                                          \
> +	uint32_t _off_in_bytes = ((off) % 8) ? (((off) >> 3) + 1) : ((off) >> 3);     \
> +	const uint8_t *_a_with_off = (const uint8_t *)(a) + _off_in_bytes;      \
> +	const uint8_t *_b_with_off = (const uint8_t *)(b) + _off_in_bytes;      \
>  	TEST_ASSERT_BUFFERS_ARE_EQUAL_BIT(_a_with_off, _b_with_off,           \
>  				_len_without_first_byte, msg);                \
> -	if (off % 8) {                                                        \
> -		_first_byte_bits = 8 - (off % 8);                             \
> +	if ((off) % 8) {                                                        \
> +		_first_byte_bits = 8 - ((off) % 8);                             \
>  		_first_byte_mask = (1 << _first_byte_bits) - 1;               \
>  		_first_byte_a = *(_a_with_off - 1);                           \
>  		_first_byte_b = *(_b_with_off - 1);                           \
> diff --git a/lib/eal/include/rte_test.h b/lib/eal/include/rte_test.h
> index 62c8f165af..b73d4e75c2 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__)

Parenthesizing both arguments passed and arguments substituted is not strictly
necessary, although not technically wrong.

> 
>  #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__)
> 
>  #endif /* _RTE_TEST_H_ */
> --
> 2.34.1

Patchwork has detected that your From email is different from your
Signed-off-by email. I believe this is a formal requirement that may prevent
your patch from getting merged. If you plan to send patches from your personal
email please add it as an alias to the .mailmap .

The patch is technically correct and would work, neither of my comments is a
blocker (although last one might be a blocker from maintainer), but given that
there are three of them maybe you would want to send a v2 to practice the
process. Don't forget --in-reply-to when sending it.

RE: [PATCH] test: parenthesize assertion macro parameters

[Marat Khalili]

> The patch is technically correct and would work, neither of my comments is a
> blocker (although last one might be a blocker from maintainer), but given that
> there are three of them maybe you would want to send a v2 to practice the
> process. Don't forget --in-reply-to when sending it.

P.S. Please also consider if this patch needs Fixes: and Cc: stable@dpdk.org headers and mention why in the cover letter if you think it doesn't.

[PATCH v2] test: parenthesize assertion macro parameters

[Weijun Pan]

Some test assertion macros use parameters directly in expressions,
which can lead to unexpected evaluation due to operator precedence
after macro substitution.

Fix this by parenthesizing macro parameters and the resulting
expressions in rte_test.h and app/test/test.h.

This is a test macro robustness fix and does not fix a runtime issue
in DPDK, so no Fixes or stable tags are added.

Bugzilla ID: 1925

Signed-off-by: Weijun Pan <wpan3636@gmail.com>

---
v2:
- Remove unnecessary parentheses surrounded by commas
- Keep parenthesize substitutions even in unambiguous places in rte_test.h
  to maintain coding style: parenthesize a condition
- Keep msg unparenthesized to preserve string literal concatenation
---
 app/test/test.h            | 30 +++++++++++++++---------------
 lib/eal/include/rte_test.h | 12 ++++++------
 2 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/app/test/test.h b/app/test/test.h
index 1f12fc5397..e7539418a8 100644
--- a/app/test/test.h
+++ b/app/test/test.h
@@ -44,8 +44,8 @@
 
 /* Compare two buffers with offset (length and offset in bytes) */
 #define TEST_ASSERT_BUFFERS_ARE_EQUAL_OFFSET(a, b, len, off, msg, ...) do { \
-	const uint8_t *_a_with_off = (const uint8_t *)a + off;              \
-	const uint8_t *_b_with_off = (const uint8_t *)b + off;              \
+	const uint8_t *_a_with_off = (const uint8_t *)(a) + (off);              \
+	const uint8_t *_b_with_off = (const uint8_t *)(b) + (off);              \
 	TEST_ASSERT_BUFFERS_ARE_EQUAL(_a_with_off, _b_with_off, len, msg);  \
 } while (0)
 
@@ -53,12 +53,12 @@
 #define TEST_ASSERT_BUFFERS_ARE_EQUAL_BIT(a, b, len, msg, ...) do {	\
 	uint8_t _last_byte_a, _last_byte_b;                       \
 	uint8_t _last_byte_mask, _last_byte_bits;                  \
-	TEST_ASSERT_BUFFERS_ARE_EQUAL(a, b, (len >> 3), msg);     \
-	if (len % 8) {                                              \
-		_last_byte_bits = len % 8;                   \
+	TEST_ASSERT_BUFFERS_ARE_EQUAL(a, b, ((len) >> 3), msg);     \
+	if ((len) % 8) {                                              \
+		_last_byte_bits = (len) % 8;                   \
 		_last_byte_mask = ~((1 << (8 - _last_byte_bits)) - 1); \
-		_last_byte_a = ((const uint8_t *)a)[len >> 3];            \
-		_last_byte_b = ((const uint8_t *)b)[len >> 3];            \
+		_last_byte_a = ((const uint8_t *)(a))[(len) >> 3];            \
+		_last_byte_b = ((const uint8_t *)(b))[(len) >> 3];            \
 		_last_byte_a &= _last_byte_mask;                     \
 		_last_byte_b &= _last_byte_mask;                    \
 		if (_last_byte_a != _last_byte_b) {                  \
@@ -74,16 +74,16 @@
 #define TEST_ASSERT_BUFFERS_ARE_EQUAL_BIT_OFFSET(a, b, len, off, msg, ...) do {	\
 	uint8_t _first_byte_a, _first_byte_b;                                 \
 	uint8_t _first_byte_mask, _first_byte_bits;                           \
-	uint32_t _len_without_first_byte = (off % 8) ?                       \
-				len - (8 - (off % 8)) :                       \
-				len;                                          \
-	uint32_t _off_in_bytes = (off % 8) ? (off >> 3) + 1 : (off >> 3);     \
-	const uint8_t *_a_with_off = (const uint8_t *)a + _off_in_bytes;      \
-	const uint8_t *_b_with_off = (const uint8_t *)b + _off_in_bytes;      \
+	uint32_t _len_without_first_byte = ((off) % 8) ?                       \
+				((len) - (8 - ((off) % 8))) :                       \
+				(len);                                          \
+	uint32_t _off_in_bytes = ((off) % 8) ? (((off) >> 3) + 1) : ((off) >> 3);     \
+	const uint8_t *_a_with_off = (const uint8_t *)(a) + _off_in_bytes;      \
+	const uint8_t *_b_with_off = (const uint8_t *)(b) + _off_in_bytes;      \
 	TEST_ASSERT_BUFFERS_ARE_EQUAL_BIT(_a_with_off, _b_with_off,           \
 				_len_without_first_byte, msg);                \
-	if (off % 8) {                                                        \
-		_first_byte_bits = 8 - (off % 8);                             \
+	if ((off) % 8) {                                                        \
+		_first_byte_bits = 8 - ((off) % 8);                             \
 		_first_byte_mask = (1 << _first_byte_bits) - 1;               \
 		_first_byte_a = *(_a_with_off - 1);                           \
 		_first_byte_b = *(_b_with_off - 1);                           \
diff --git a/lib/eal/include/rte_test.h b/lib/eal/include/rte_test.h
index 62c8f165af..b73d4e75c2 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__)
 
 #endif /* _RTE_TEST_H_ */
-- 
2.34.1

RE: [PATCH v2] test: parenthesize assertion macro parameters

[Marat Khalili]

Acked-by: Marat Khalili <marat.khalili@huawei.com>

> -----Original Message-----
> From: Weijun Pan <wpan3636@gmail.com>
> Sent: Monday 27 April 2026 18:17
> Cc: dev@dpdk.org; Weijun Pan <wpan3636@gmail.com>
> Subject: [PATCH v2] test: parenthesize assertion macro parameters
> 
> Some test assertion macros use parameters directly in expressions,
> which can lead to unexpected evaluation due to operator precedence
> after macro substitution.
> 
> Fix this by parenthesizing macro parameters and the resulting
> expressions in rte_test.h and app/test/test.h.
> 
> This is a test macro robustness fix and does not fix a runtime issue
> in DPDK, so no Fixes or stable tags are added.
> 
> Bugzilla ID: 1925
> 
> Signed-off-by: Weijun Pan <wpan3636@gmail.com>
> 
> ---
> v2:
> - Remove unnecessary parentheses surrounded by commas
> - Keep parenthesize substitutions even in unambiguous places in rte_test.h
>   to maintain coding style: parenthesize a condition
> - Keep msg unparenthesized to preserve string literal concatenation
> ---
>  app/test/test.h            | 30 +++++++++++++++---------------
>  lib/eal/include/rte_test.h | 12 ++++++------
>  2 files changed, 21 insertions(+), 21 deletions(-)
> 

Re: [PATCH v2] test: parenthesize assertion macro parameters

[Stephen Hemminger]

On Mon, 27 Apr 2026 12:17:13 -0500
Weijun Pan <wpan3636@gmail.com> wrote:

> Some test assertion macros use parameters directly in expressions,
> which can lead to unexpected evaluation due to operator precedence
> after macro substitution.
> 
> Fix this by parenthesizing macro parameters and the resulting
> expressions in rte_test.h and app/test/test.h.
> 
> This is a test macro robustness fix and does not fix a runtime issue
> in DPDK, so no Fixes or stable tags are added.
> 
> Bugzilla ID: 1925
> 
> Signed-off-by: Weijun Pan <wpan3636@gmail.com>

This is not critical path code, and there is secondary problem
of arguements being evaluated multiple times. Perhaps use helper
functions instead. 

For test.h do this. The rte_test.h part is correct.

Let me split them and send a followup

[PATCH 0/2] test: clean up assertion macros

[Stephen Hemminger]

Two patches addressing Bugzilla 1925, which reported missing
parentheses around macro parameters in the DPDK test assertion
macros.

The TEST_ASSERT_BUFFERS_ARE_EQUAL family in app/test/test.h turned
out to have additional issues beyond missing parens -- multiple
evaluation of arguments, no type checking, and a dropped
__VA_ARGS__ in the _OFFSET wrappers -- so patch 1 moves the
comparison logic into static inline helpers.  The macros are now
thin wrappers around the printf / TEST_TRACE_FAILURE / return
TEST_FAILED boilerplate.  Existing call sites need no changes.

Patch 2 is Weijun Pan's original rte_test.h paren fix carried
through unchanged.

Stephen Hemminger (1):
  test: use inline helpers in buffer comparison macros

Weijun Pan (1):
  test: parenthesize assertion macro parameters

 .mailmap                   |   1 +
 app/test/test.h            | 147 ++++++++++++++++++++++++-------------
 lib/eal/include/rte_test.h |  12 +--
 3 files changed, 102 insertions(+), 58 deletions(-)

-- 
2.53.0

[PATCH 1/2] test: use inline helpers in buffer comparison macros

[Stephen Hemminger]

The TEST_ASSERT_BUFFERS_ARE_EQUAL family of macros had several
problems stemming from doing all the work inside the macro body:

 - Macro parameters were used directly in arithmetic expressions
   without parentheses.
 - Arguments were evaluated multiple times.
 - There was no type checking.

In addition, the _OFFSET and _BIT_OFFSET wrappers chained to the
inner assertion macro passing only "msg" and dropped the variadic
arguments, so any printf parameters after the format string were
silently lost on failure.

Move the comparison logic into static inline helpers and keep the
macros as thin wrappers around the printf / TEST_TRACE_FAILURE /
return TEST_FAILED boilerplate (which must remain a macro to
capture __func__ / __LINE__ and to return from the caller).
Each argument is now evaluated exactly once where the macro hands
it to the helper, the size_t parameters give the compiler real
types to check against, and the bit-mask arithmetic lives in C
rather than the preprocessor.  Existing call sites need no
changes.

Bugzilla ID: 1925
Suggested-by: Weijun Pan <wpan3636@gmail.com>
Fixes: db4faf469816 ("app/test: add new buffer comparison macros")
Cc: stable@dpdk.org

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test.h | 147 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 95 insertions(+), 52 deletions(-)

diff --git a/app/test/test.h b/app/test/test.h
index 1f12fc5397..ebf6727639 100644
--- a/app/test/test.h
+++ b/app/test/test.h
@@ -6,8 +6,12 @@
 #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>
@@ -32,70 +36,109 @@
 
 #define TEST_ASSERT_EQUAL RTE_TEST_ASSERT_EQUAL
 
+/*
+ * 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(const void *a, const void *b, size_t len)
+{
+	return memcmp(a, b, len) == 0;
+}
+
+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;
+}
+
+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 whole = len >> 3;
+	size_t extra = len & 7;
+
+	if (memcmp(pa, pb, whole) != 0)
+		return false;
+	if (extra != 0) {
+		uint8_t mask = (uint8_t)~((1U << (8 - extra)) - 1);
+
+		if ((pa[whole] & mask) != (pb[whole] & mask))
+			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 = (1U << first_bits) - 1;
+
+		if ((pa[off_bytes] & mask) != (pb[off_bytes] & mask))
+			return false;
+		off_bytes++;
+		len -= first_bits;
+	}
+	return test_buffers_equal_bit(pa + off_bytes, pb + off_bytes, len);
+}
+
 /* Compare two buffers (length in bytes) */
-#define TEST_ASSERT_BUFFERS_ARE_EQUAL(a, b, len,  msg, ...) do {	\
-	if (memcmp(a, b, len)) {                                        \
-		printf("TestCase %s() line %d failed: "              \
-			msg "\n", __func__, __LINE__, ##__VA_ARGS__);    \
-		TEST_TRACE_FAILURE(__FILE__, __LINE__, __func__);    \
-		return TEST_FAILED;                                  \
-	}                                                        \
+#define TEST_ASSERT_BUFFERS_ARE_EQUAL(a, b, len, msg, ...) do {		\
+	if (!test_buffers_equal((a), (b), (len))) {			\
+		printf("TestCase %s() line %d failed: " msg "\n",	\
+			__func__, __LINE__, ##__VA_ARGS__);		\
+		TEST_TRACE_FAILURE(__FILE__, __LINE__, __func__);	\
+		return TEST_FAILED;					\
+	}								\
 } while (0)
 
 /* Compare two buffers with offset (length and offset in bytes) */
 #define TEST_ASSERT_BUFFERS_ARE_EQUAL_OFFSET(a, b, len, off, msg, ...) do { \
-	const uint8_t *_a_with_off = (const uint8_t *)a + off;              \
-	const uint8_t *_b_with_off = (const uint8_t *)b + off;              \
-	TEST_ASSERT_BUFFERS_ARE_EQUAL(_a_with_off, _b_with_off, len, msg);  \
+	if (!test_buffers_equal_offset((a), (b), (len), (off))) {	\
+		printf("TestCase %s() line %d failed: " msg "\n",	\
+			__func__, __LINE__, ##__VA_ARGS__);		\
+		TEST_TRACE_FAILURE(__FILE__, __LINE__, __func__);	\
+		return TEST_FAILED;					\
+	}								\
 } while (0)
 
 /* Compare two buffers (length in bits) */
 #define TEST_ASSERT_BUFFERS_ARE_EQUAL_BIT(a, b, len, msg, ...) do {	\
-	uint8_t _last_byte_a, _last_byte_b;                       \
-	uint8_t _last_byte_mask, _last_byte_bits;                  \
-	TEST_ASSERT_BUFFERS_ARE_EQUAL(a, b, (len >> 3), msg);     \
-	if (len % 8) {                                              \
-		_last_byte_bits = len % 8;                   \
-		_last_byte_mask = ~((1 << (8 - _last_byte_bits)) - 1); \
-		_last_byte_a = ((const uint8_t *)a)[len >> 3];            \
-		_last_byte_b = ((const uint8_t *)b)[len >> 3];            \
-		_last_byte_a &= _last_byte_mask;                     \
-		_last_byte_b &= _last_byte_mask;                    \
-		if (_last_byte_a != _last_byte_b) {                  \
-			printf("TestCase %s() line %d failed: "              \
-				msg "\n", __func__, __LINE__, ##__VA_ARGS__);\
-			TEST_TRACE_FAILURE(__FILE__, __LINE__, __func__);    \
-			return TEST_FAILED;                                  \
-		}                                                        \
-	}                                                            \
+	if (!test_buffers_equal_bit((a), (b), (len))) {			\
+		printf("TestCase %s() line %d failed: " msg "\n",	\
+			__func__, __LINE__, ##__VA_ARGS__);		\
+		TEST_TRACE_FAILURE(__FILE__, __LINE__, __func__);	\
+		return TEST_FAILED;					\
+	}								\
 } while (0)
 
 /* Compare two buffers with offset (length and offset in bits) */
-#define TEST_ASSERT_BUFFERS_ARE_EQUAL_BIT_OFFSET(a, b, len, off, msg, ...) do {	\
-	uint8_t _first_byte_a, _first_byte_b;                                 \
-	uint8_t _first_byte_mask, _first_byte_bits;                           \
-	uint32_t _len_without_first_byte = (off % 8) ?                       \
-				len - (8 - (off % 8)) :                       \
-				len;                                          \
-	uint32_t _off_in_bytes = (off % 8) ? (off >> 3) + 1 : (off >> 3);     \
-	const uint8_t *_a_with_off = (const uint8_t *)a + _off_in_bytes;      \
-	const uint8_t *_b_with_off = (const uint8_t *)b + _off_in_bytes;      \
-	TEST_ASSERT_BUFFERS_ARE_EQUAL_BIT(_a_with_off, _b_with_off,           \
-				_len_without_first_byte, msg);                \
-	if (off % 8) {                                                        \
-		_first_byte_bits = 8 - (off % 8);                             \
-		_first_byte_mask = (1 << _first_byte_bits) - 1;               \
-		_first_byte_a = *(_a_with_off - 1);                           \
-		_first_byte_b = *(_b_with_off - 1);                           \
-		_first_byte_a &= _first_byte_mask;                            \
-		_first_byte_b &= _first_byte_mask;                            \
-		if (_first_byte_a != _first_byte_b) {                         \
-			printf("TestCase %s() line %d failed: "               \
-				msg "\n", __func__, __LINE__, ##__VA_ARGS__); \
-			TEST_TRACE_FAILURE(__FILE__, __LINE__, __func__);     \
-			return TEST_FAILED;                                   \
-		}                                                             \
-	}                                                                     \
+#define TEST_ASSERT_BUFFERS_ARE_EQUAL_BIT_OFFSET(a, b, len, off, msg, ...) \
+do {									\
+	if (!test_buffers_equal_bit_offset((a), (b), (len), (off))) {	\
+		printf("TestCase %s() line %d failed: " msg "\n",	\
+			__func__, __LINE__, ##__VA_ARGS__);		\
+		TEST_TRACE_FAILURE(__FILE__, __LINE__, __func__);	\
+		return TEST_FAILED;					\
+	}								\
 } while (0)
 
 #define TEST_ASSERT_NOT_EQUAL RTE_TEST_ASSERT_NOT_EQUAL
-- 
2.53.0

RE: [PATCH 1/2] test: use inline helpers in buffer comparison macros

[Marat Khalili]

With or without addressing inline comments below,

Acked-by: Marat Khalili <marat.khalili@huawei.com>

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Wednesday 29 April 2026 15:42
> To: dev@dpdk.org
> Cc: Stephen Hemminger <stephen@networkplumber.org>; Weijun Pan <wpan3636@gmail.com>; stable@dpdk.org;
> Deepak Kumar Jain <deepak.k.jain@intel.com>; Pablo de Lara <pablo.de.lara.guarch@intel.com>
> Subject: [PATCH 1/2] test: use inline helpers in buffer comparison macros
> 
> The TEST_ASSERT_BUFFERS_ARE_EQUAL family of macros had several
> problems stemming from doing all the work inside the macro body:
> 
>  - Macro parameters were used directly in arithmetic expressions
>    without parentheses.
>  - Arguments were evaluated multiple times.
>  - There was no type checking.
> 
> In addition, the _OFFSET and _BIT_OFFSET wrappers chained to the
> inner assertion macro passing only "msg" and dropped the variadic
> arguments, so any printf parameters after the format string were
> silently lost on failure.
> 
> Move the comparison logic into static inline helpers and keep the
> macros as thin wrappers around the printf / TEST_TRACE_FAILURE /
> return TEST_FAILED boilerplate (which must remain a macro to
> capture __func__ / __LINE__ and to return from the caller).
> Each argument is now evaluated exactly once where the macro hands
> it to the helper, the size_t parameters give the compiler real
> types to check against, and the bit-mask arithmetic lives in C
> rather than the preprocessor.  Existing call sites need no
> changes.
> 
> Bugzilla ID: 1925
> Suggested-by: Weijun Pan <wpan3636@gmail.com>
> Fixes: db4faf469816 ("app/test: add new buffer comparison macros")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  app/test/test.h | 147 +++++++++++++++++++++++++++++++-----------------
>  1 file changed, 95 insertions(+), 52 deletions(-)
> 
> diff --git a/app/test/test.h b/app/test/test.h
> index 1f12fc5397..ebf6727639 100644
> --- a/app/test/test.h
> +++ b/app/test/test.h
> @@ -6,8 +6,12 @@
>  #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>
> @@ -32,70 +36,109 @@
> 
>  #define TEST_ASSERT_EQUAL RTE_TEST_ASSERT_EQUAL
> 
> +/*
> + * 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(const void *a, const void *b, size_t len)
> +{
> +	return memcmp(a, b, len) == 0;
> +}

Not sure it's really justified to define a separate function for this, could
either use memcmp directly or if striving for symmetry call
test_buffers_equal_offset with zero last argument.

> +
> +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;
> +}
> +
> +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 whole = len >> 3;
> +	size_t extra = len & 7;
> +
> +	if (memcmp(pa, pb, whole) != 0)
> +		return false;
> +	if (extra != 0) {
> +		uint8_t mask = (uint8_t)~((1U << (8 - extra)) - 1);

I know this is from the original, but I think we had some macros for this like
RTE_GENMASK32.

> +
> +		if ((pa[whole] & mask) != (pb[whole] & mask))
> +			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;

Would be nice to have some consistency in naming these.

> +
> +	if (off_bits != 0) {
> +		uint8_t first_bits = 8 - off_bits;
> +		uint8_t mask = (1U << first_bits) - 1;

Could use a standard macro here as well.

> +
> +		if ((pa[off_bytes] & mask) != (pb[off_bytes] & mask))
> +			return false;
> +		off_bytes++;
> +		len -= first_bits;

Would an assert that len >= first_bits make sense before this line?

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

// skip the rest

[PATCH 2/2] test: parenthesize assertion macro parameters

[Stephen Hemminger]

From: Weijun Pan <wpan3636@gmail.com>

Some test assertion macros use parameters directly in expressions,
which can lead to unexpected evaluation due to operator precedence
after macro substitution.

Fix this by parenthesizing macro parameters and the resulting
expressions in rte_test.h

Bugzilla ID: 1925
Fixes: 5afc521eac6a ("eal: add test assert macros")
Cc: stable@dpdk.org

Signed-off-by: Weijun Pan <wpan3636@gmail.com>
---
 .mailmap                   |  1 +
 lib/eal/include/rte_test.h | 12 ++++++------
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/.mailmap b/.mailmap
index 0e0d83e1c6..a72bba5fb8 100644
--- a/.mailmap
+++ b/.mailmap
@@ -1771,6 +1771,7 @@ Weichun Chen <weichunx.chen@intel.com>
 Weifeng Li <liweifeng96@126.com>
 Weiguo Li <liweiguo@xencore.cn> <liwg06@foxmail.com>
 WeiJie Zhuang <zhuangwj@gmail.com>
+Weijun Pan <wpan3636@gmail.com> <wpan36@wisc.edu>
 Weiliang Luo <droidluo@gmail.com>
 Weiyuan Li <weiyuanx.li@intel.com>
 Wen Chiu <wchiu@brocade.com>
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__)
 
 #endif /* _RTE_TEST_H_ */
-- 
2.53.0

RE: [PATCH 2/2] test: parenthesize assertion macro parameters

[Marat Khalili]

Acked-by: Marat Khalili <marat.khalili@huawei.com>

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Wednesday 29 April 2026 15:42
> To: dev@dpdk.org
> Cc: Weijun Pan <wpan3636@gmail.com>; stable@dpdk.org; Thomas Monjalon <thomas@monjalon.net>; Jerin
> Jacob <jerinj@marvell.com>; Pavan Nikhilesh <pbhagavatula@marvell.com>
> Subject: [PATCH 2/2] test: parenthesize assertion macro parameters
> 
> From: Weijun Pan <wpan3636@gmail.com>
> 
> Some test assertion macros use parameters directly in expressions,
> which can lead to unexpected evaluation due to operator precedence
> after macro substitution.
> 
> Fix this by parenthesizing macro parameters and the resulting
> expressions in rte_test.h
> 
> Bugzilla ID: 1925
> Fixes: 5afc521eac6a ("eal: add test assert macros")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Weijun Pan <wpan3636@gmail.com>
> ---
>  .mailmap                   |  1 +
>  lib/eal/include/rte_test.h | 12 ++++++------
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/.mailmap b/.mailmap
> index 0e0d83e1c6..a72bba5fb8 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -1771,6 +1771,7 @@ Weichun Chen <weichunx.chen@intel.com>
>  Weifeng Li <liweifeng96@126.com>
>  Weiguo Li <liweiguo@xencore.cn> <liwg06@foxmail.com>
>  WeiJie Zhuang <zhuangwj@gmail.com>
> +Weijun Pan <wpan3636@gmail.com> <wpan36@wisc.edu>
>  Weiliang Luo <droidluo@gmail.com>
>  Weiyuan Li <weiyuanx.li@intel.com>
>  Wen Chiu <wchiu@brocade.com>
> 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__)
> 
>  #endif /* _RTE_TEST_H_ */
> --
> 2.53.0

[PATCH v2 0/2] test: cleanup assertion macros

[Stephen Hemminger]

Two patches addressing Bugzilla 1925, which reported missing
parentheses around macro parameters in the DPDK test assertion
macros.

The TEST_ASSERT_BUFFERS_ARE_EQUAL family in app/test/test.h turned
out to have additional issues beyond missing parens -- multiple
evaluation of arguments, no type checking, and a dropped
__VA_ARGS__ in the _OFFSET wrappers -- so patch 1 moves the
comparison logic into static inline helpers.  The macros are now
thin wrappers around the printf / TEST_TRACE_FAILURE / return
TEST_FAILED boilerplate.  Existing call sites need no changes.

Patch 2 is Weijun Pan's original rte_test.h paren fix carried
through unchanged.

v2 - incorporate review feedback

Stephen Hemminger (1):
  test: use inline helpers in buffer comparison macros

Weijun Pan (1):
  test: parenthesize assertion macro parameters

 .mailmap                   |   1 +
 app/test/test.h            | 146 +++++++++++++++++++++++--------------
 lib/eal/include/rte_test.h |  12 +--
 3 files changed, 100 insertions(+), 59 deletions(-)

-- 
2.53.0

[PATCH v2 1/2] test: use inline helpers in buffer comparison macros

[Stephen Hemminger]

The TEST_ASSERT_BUFFERS_ARE_EQUAL family of macros had several
problems stemming from doing all the work inside the macro body:

 - Macro parameters were used directly in arithmetic expressions
   without parentheses, so callers passing expressions like
   "len = x | y" or "off = i + 1" could hit operator precedence
   bugs.

 - Arguments were evaluated multiple times.  For example
   TEST_ASSERT_BUFFERS_ARE_EQUAL_BIT expanded "len" six times and
   "a" / "b" twice each, so passing any expression with a side
   effect (i++, a function call, ...) would misbehave silently.

 - There was no type checking; memcmp accepts any pointer, and
   nothing constrained len/off to unsigned integers.

In addition, the _OFFSET and _BIT_OFFSET wrappers chained to the
inner assertion macro passing only "msg" and dropped the variadic
arguments, so any printf parameters after the format string were
silently lost on failure.

Move the comparison logic into static inline helpers and keep the
macros as thin wrappers around the printf / TEST_TRACE_FAILURE /
return TEST_FAILED boilerplate (which must remain a macro to
capture __func__ / __LINE__ and to return from the caller).
Each argument is now evaluated exactly once where the macro hands
it to the helper, the size_t parameters give the compiler real
types to check against, and the bit-mask arithmetic lives in C
rather than the preprocessor.  Existing call sites need no
changes.

Bugzilla ID: 1925
Suggested-by: Weijun Pan <wpan3636@gmail.com>

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Marat Khalili <marat.khalili@huawei.com>
Acked-by: Marat Khalili <marat.khalili@huawei.com>
---
 app/test/test.h | 146 ++++++++++++++++++++++++++++++------------------
 1 file changed, 93 insertions(+), 53 deletions(-)

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>
 
@@ -32,70 +38,104 @@
 
 #define TEST_ASSERT_EQUAL RTE_TEST_ASSERT_EQUAL
 
+/*
+ * 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;
+}
+
+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);
+
+		if ((pa[len_bytes] & mask) != (pb[len_bytes] & mask))
+			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);
+
+		if ((pa[off_bytes] & mask) != (pb[off_bytes] & mask))
+			return false;
+		RTE_ASSERT(len >= first_bits);
+		off_bytes++;
+		len -= first_bits;
+	}
+	return test_buffers_equal_bit(pa + off_bytes, pb + off_bytes, len);
+}
+
 /* Compare two buffers (length in bytes) */
-#define TEST_ASSERT_BUFFERS_ARE_EQUAL(a, b, len,  msg, ...) do {	\
-	if (memcmp(a, b, len)) {                                        \
-		printf("TestCase %s() line %d failed: "              \
-			msg "\n", __func__, __LINE__, ##__VA_ARGS__);    \
-		TEST_TRACE_FAILURE(__FILE__, __LINE__, __func__);    \
-		return TEST_FAILED;                                  \
-	}                                                        \
+#define TEST_ASSERT_BUFFERS_ARE_EQUAL(a, b, len, msg, ...) do {		\
+	if (memcmp((a), (b), (len)) != 0) {				\
+		printf("TestCase %s() line %d failed: " msg "\n",	\
+			__func__, __LINE__, ##__VA_ARGS__);		\
+		TEST_TRACE_FAILURE(__FILE__, __LINE__, __func__);	\
+		return TEST_FAILED;					\
+	}								\
 } while (0)
 
 /* Compare two buffers with offset (length and offset in bytes) */
 #define TEST_ASSERT_BUFFERS_ARE_EQUAL_OFFSET(a, b, len, off, msg, ...) do { \
-	const uint8_t *_a_with_off = (const uint8_t *)a + off;              \
-	const uint8_t *_b_with_off = (const uint8_t *)b + off;              \
-	TEST_ASSERT_BUFFERS_ARE_EQUAL(_a_with_off, _b_with_off, len, msg);  \
+	if (!test_buffers_equal_offset((a), (b), (len), (off))) {	\
+		printf("TestCase %s() line %d failed: " msg "\n",	\
+			__func__, __LINE__, ##__VA_ARGS__);		\
+		TEST_TRACE_FAILURE(__FILE__, __LINE__, __func__);	\
+		return TEST_FAILED;					\
+	}								\
 } while (0)
 
 /* Compare two buffers (length in bits) */
 #define TEST_ASSERT_BUFFERS_ARE_EQUAL_BIT(a, b, len, msg, ...) do {	\
-	uint8_t _last_byte_a, _last_byte_b;                       \
-	uint8_t _last_byte_mask, _last_byte_bits;                  \
-	TEST_ASSERT_BUFFERS_ARE_EQUAL(a, b, (len >> 3), msg);     \
-	if (len % 8) {                                              \
-		_last_byte_bits = len % 8;                   \
-		_last_byte_mask = ~((1 << (8 - _last_byte_bits)) - 1); \
-		_last_byte_a = ((const uint8_t *)a)[len >> 3];            \
-		_last_byte_b = ((const uint8_t *)b)[len >> 3];            \
-		_last_byte_a &= _last_byte_mask;                     \
-		_last_byte_b &= _last_byte_mask;                    \
-		if (_last_byte_a != _last_byte_b) {                  \
-			printf("TestCase %s() line %d failed: "              \
-				msg "\n", __func__, __LINE__, ##__VA_ARGS__);\
-			TEST_TRACE_FAILURE(__FILE__, __LINE__, __func__);    \
-			return TEST_FAILED;                                  \
-		}                                                        \
-	}                                                            \
+	if (!test_buffers_equal_bit((a), (b), (len))) {			\
+		printf("TestCase %s() line %d failed: " msg "\n",	\
+			__func__, __LINE__, ##__VA_ARGS__);		\
+		TEST_TRACE_FAILURE(__FILE__, __LINE__, __func__);	\
+		return TEST_FAILED;					\
+	}								\
 } while (0)
 
 /* Compare two buffers with offset (length and offset in bits) */
-#define TEST_ASSERT_BUFFERS_ARE_EQUAL_BIT_OFFSET(a, b, len, off, msg, ...) do {	\
-	uint8_t _first_byte_a, _first_byte_b;                                 \
-	uint8_t _first_byte_mask, _first_byte_bits;                           \
-	uint32_t _len_without_first_byte = (off % 8) ?                       \
-				len - (8 - (off % 8)) :                       \
-				len;                                          \
-	uint32_t _off_in_bytes = (off % 8) ? (off >> 3) + 1 : (off >> 3);     \
-	const uint8_t *_a_with_off = (const uint8_t *)a + _off_in_bytes;      \
-	const uint8_t *_b_with_off = (const uint8_t *)b + _off_in_bytes;      \
-	TEST_ASSERT_BUFFERS_ARE_EQUAL_BIT(_a_with_off, _b_with_off,           \
-				_len_without_first_byte, msg);                \
-	if (off % 8) {                                                        \
-		_first_byte_bits = 8 - (off % 8);                             \
-		_first_byte_mask = (1 << _first_byte_bits) - 1;               \
-		_first_byte_a = *(_a_with_off - 1);                           \
-		_first_byte_b = *(_b_with_off - 1);                           \
-		_first_byte_a &= _first_byte_mask;                            \
-		_first_byte_b &= _first_byte_mask;                            \
-		if (_first_byte_a != _first_byte_b) {                         \
-			printf("TestCase %s() line %d failed: "               \
-				msg "\n", __func__, __LINE__, ##__VA_ARGS__); \
-			TEST_TRACE_FAILURE(__FILE__, __LINE__, __func__);     \
-			return TEST_FAILED;                                   \
-		}                                                             \
-	}                                                                     \
+#define TEST_ASSERT_BUFFERS_ARE_EQUAL_BIT_OFFSET(a, b, len, off, msg, ...) \
+do {									\
+	if (!test_buffers_equal_bit_offset((a), (b), (len), (off))) {	\
+		printf("TestCase %s() line %d failed: " msg "\n",	\
+			__func__, __LINE__, ##__VA_ARGS__);		\
+		TEST_TRACE_FAILURE(__FILE__, __LINE__, __func__);	\
+		return TEST_FAILED;					\
+	}								\
 } while (0)
 
 #define TEST_ASSERT_NOT_EQUAL RTE_TEST_ASSERT_NOT_EQUAL
-- 
2.53.0

[PATCH v2 2/2] test: parenthesize assertion macro parameters

[Stephen Hemminger]

From: Weijun Pan <wpan3636@gmail.com>

Some test assertion macros use parameters directly in expressions,
which can lead to unexpected evaluation due to operator precedence
after macro substitution.

Fix this by parenthesizing macro parameters and the resulting
expressions in rte_test.h

Bugzilla ID: 1925

Signed-off-by: Weijun Pan <wpan3636@gmail.com>
Acked-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Marat Khalili <marat.khalili@huawei.com>
---
 .mailmap                   |  1 +
 lib/eal/include/rte_test.h | 12 ++++++------
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/.mailmap b/.mailmap
index 895412e568..3c08fe9aa3 100644
--- a/.mailmap
+++ b/.mailmap
@@ -1772,6 +1772,7 @@ Weichun Chen <weichunx.chen@intel.com>
 Weifeng Li <liweifeng96@126.com>
 Weiguo Li <liweiguo@xencore.cn> <liwg06@foxmail.com>
 WeiJie Zhuang <zhuangwj@gmail.com>
+Weijun Pan <wpan3636@gmail.com> <wpan36@wisc.edu>
 Weiliang Luo <droidluo@gmail.com>
 Weiyuan Li <weiyuanx.li@intel.com>
 Wen Chiu <wchiu@brocade.com>
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__)
 
 #endif /* _RTE_TEST_H_ */
-- 
2.53.0