Re: Testsuite failure on s390x and sparc64 after 6840fe9ee2

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Mar 31, 2025, 18:17 UTC
Message-ID: <Z+rcVY7KqEuF1wFw@szeder.dev>
In-Reply-To: <Z-qKGqpbdaW9WCrP@pks.im>

On Mon, Mar 31, 2025 at 02:27:06PM +0200, Patrick Steinhardt wrote:

Show 5 quoted lines

> One thing I stumbled over: the `--min-batch-size` parameter is parsed
> using `OPT_INTEGER()`, which expects the value pointer to point to an
> integer. But we pass `struct backfill_context::min_batch_size`, which is
> of type `size_t`. Maybe that's causing us to end up with an invalid
> value?

We could teach parse-options to verify at compile time that it got a 'value' pointer to an appropriately sized variable with a simple trick:

diff --git a/parse-options.h b/parse-options.h
index 997ffbee80..ac63f9548a 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -213,7 +213,7 @@ struct option {
 	.type = OPTION_INTEGER, \
 	.short_name = (s), \
 	.long_name = (l), \
-	.value = (v), \
+	.value = (v) + 0/(sizeof(*(v)) == sizeof(int)), \
 	.argh = N_("n"), \
 	.help = (h), \
 	.flags = (f), \

This bug would then cause a compiler error like this:

      CC builtin/backfill.o
  In file included from builtin/backfill.c:7:
  builtin/backfill.c: In function ‘cmd_backfill’:
  ./parse-options.h:216:25: error: division by zero [-Werror=div-by-zero]
    216 |         .value = (v) + 0/(sizeof(*v) == sizeof(int)), \
        |                         ^
  ./parse-options.h:272:37: note: in expansion of macro ‘OPT_INTEGER_F’
    272 | #define OPT_INTEGER(s, l, v, h)     OPT_INTEGER_F(s, l, v, h, 0)
        |                                     ^~~~~~~~~~~~~
  builtin/backfill.c:126:17: note: in expansion of macro ‘OPT_INTEGER’
    126 |                 OPT_INTEGER(0, "min-batch-size", &ctx.min_batch_size,
        |                 ^~~~~~~~~~~
  cc1: all warnings being treated as errors
  make: *** [Makefile:2811: builtin/backfill.o] Error 1

Alas, the change is ugly (and we should do the same for many other
OPT_* macros as well) and the error message is far from
to-the-point...  Turning this into something usable would require a
more clever trick, and that's more than I can devote to this issue.