Re: [PATCH] mem-pool: fix big allocations

[Phillip Wood <phillip.wood123@gmail.com>]

Hi René

On 21/12/2023 23:13, René Scharfe wrote:
> +#define check_ptr(a, op, b) check_int(((a) op (b)), ==, 1)
> [...]
> +static void t_calloc_100(struct mem_pool *pool)
> +{
> +	size_t size = 100;
> +	char *buffer = mem_pool_calloc(pool, 1, size);
> +	for (size_t i = 0; i < size; i++)
> +		check_int(buffer[i], ==, 0);
> +	if (!check_ptr(pool->mp_block, !=, NULL))
> +		return;
> +	check_ptr(pool->mp_block->next_free, <=, pool->mp_block->end);
> +	check_ptr(pool->mp_block->next_free, !=, NULL);
> +	check_ptr(pool->mp_block->end, !=, NULL);
> +}

It's great to see the unit test framework being used here. I wonder
though if it would be simpler just to use

	check(ptr != NULL)

as I'm not sure what the check_ptr() macro adds. The diff at the end of
this email shows a possible implementation of a check_ptr() macro for
the unit test library. I'm wary of adding it though because I'm not sure
printing the pointer values is actually very useful most of the
time. I'm also concerned that the rules around pointer arithmetic and
comparisons mean that many pointer tests such as

     check_ptr(pool->mp_block->next_free, <=, pool->mp_block->end);

will be undefined if they fail. The documentation for check_ptr() below
tries to illustrate that concern. If the compiler can prove that a check
is undefined when that check fails it is at liberty to hard code the
test as passing. In practice I think most failing pointer comparisons
would fall into the category of "this is undefined but the compiler
can't prove it" but that doesn't really make me any happier.

Best Wishes

Phillip

---- >8 ----
diff --git a/t/unit-tests/test-lib.h b/t/unit-tests/test-lib.h
index a8f07ae0b7..ecd1fce17d 100644
--- a/t/unit-tests/test-lib.h
+++ b/t/unit-tests/test-lib.h
@@ -99,6 +99,39 @@ int check_int_loc(const char *loc, const char *check, int ok,
  int check_uint_loc(const char *loc, const char *check, int ok,
  		   uintmax_t a, uintmax_t b);
  
+/*
+ * Compare two pointers. Prints a message with the two values if the
+ * comparison fails. NB this is not thread safe.
+ *
+ * Use this with care. The rules around pointer arithmetic and comparison
+ * in C are quite strict and violating them results in undefined behavior
+ * To avoid a failing comparison resulting undefined behavior we compare
+ * the integer value of the pointers. While this avoids undefined
+ * behavior in the comparison in many cases a failing test will be the
+ * result of creating an invalid pointer in a way that violates the
+ * rules on pointer arithmetic. For example if `start` and `end` are
+ * pointers to the beginning and end of an allocation and `offset` is an
+ * integer then
+ *
+ *     check_ptr(start + offset, <=, end)
+ *
+ * is undefined when `offset` is larger than `end - start`. Rewriting the
+ * comparison as
+ *
+ *     check_uint(offset, <=, end - start)
+ *
+ * avoids undefined behavior when offset is too large, but is still
+ * undefined if there is a bug that means `start` and `end` do not point
+ * to the same allocation.
+ */
+#define check_ptr(a, op, b)						\
+	(test__tmp[0].p = (a), test__tmp[1].p = (b),			\
+	 check_ptr_loc(TEST_LOCATION(), #a" "#op" "#b,			\
+		       (uintptr_t)test__tmp[0].p op (uintptr_t)test__tmp[1].p,	\
+			test__tmp[0].p, test__tmp[1].p))
+
+int check_ptr_loc(const char *loc, const char *check, int ok, void *a, void *b);
+
  /*
   * Compare two chars. Prints a message with the two values if the
   * comparison fails. NB this is not thread safe.
@@ -133,6 +166,7 @@ int check_str_loc(const char *loc, const char *check,
  #define TEST__MAKE_LOCATION(line) __FILE__ ":" TEST__STR(line)
  
  union test__tmp {
+	void *p;
  	intmax_t i;
  	uintmax_t u;
  	char c;
diff --git a/t/unit-tests/test-lib.c b/t/unit-tests/test-lib.c
index 7bf9dfdb95..cb757edbd8 100644
--- a/t/unit-tests/test-lib.c
+++ b/t/unit-tests/test-lib.c
@@ -311,6 +311,18 @@ int check_uint_loc(const char *loc, const char *check, int ok,
  	return ret;
  }
  
+int check_ptr_loc(const char *loc, const char *check, int ok, void *a, void *b)
+{
+	int ret = test_assert(loc, check, ok);
+
+	if (!ret) {
+		test_msg("   left: %p", a);
+		test_msg("  right: %p", b);
+	}
+
+	return ret;
+}
+
  static void print_one_char(char ch, char quote)
  {
  	if ((unsigned char)ch < 0x20u || ch == 0x7f) {