[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 at gmail.com>
Fixes: db4faf469816 ("app/test: add new buffer comparison macros")
Cc: stable at dpdk.org

Signed-off-by: Stephen Hemminger <stephen at 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