Re: Testsuite failure on s390x and sparc64 after 6840fe9ee2

From: Jeff King <peff@peff.net>
Date: Apr 1, 2025, 02:33 UTC
Message-ID: <20250401023358.GA1087913@coredump.intra.peff.net>
In-Reply-To: <Z+rcVY7KqEuF1wFw@szeder.dev>

On Mon, Mar 31, 2025 at 08:17:57PM +0200, SZEDER Gábor wrote:

Show 10 quoted lines

> On Mon, Mar 31, 2025 at 02:27:06PM +0200, Patrick Steinhardt wrote:
> > 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:

That would be nice. I think we've discussed type safety for parse-options before, but IIRC none of the solutions were very satisfying. But this sounds like a relatively low-effort approach that buys us something, at least. I wonder if it could even be extended to use __builtin_types_compatible() on platforms that support it.

+cc René as our resident expert on gross C hacks. ;)

Show 21 quoted lines

> 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.

We do have BUILD_ASSERT_OR_ZERO(). It produces similarly arcane errors, but at least the presence of the macro name helps a bit. E.g., doing this:

diff --git a/parse-options.h b/parse-options.h
index 997ffbee80..5303ad6bcf 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) + BUILD_ASSERT_OR_ZERO(sizeof(*v) == sizeof(int)), \
 	.argh = N_("n"), \
 	.help = (h), \
 	.flags = (f), \

yields:

      CC builtin/backfill.o
  In file included from ./builtin.h:4,
                   from builtin/backfill.c:4:
  builtin/backfill.c: In function ‘cmd_backfill’:
  ./git-compat-util.h:103:22: error: size of unnamed array is negative
    103 |         (sizeof(char [1 - 2*!(cond)]) - 1)
        |                      ^
  ./parse-options.h:216:24: note: in expansion of macro ‘BUILD_ASSERT_OR_ZERO’
    216 |         .value = (v) + BUILD_ASSERT_OR_ZERO(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,
        |                 ^~~~~~~~~~~
  make: *** [Makefile:2810: builtin/backfill.o] Error 1

-Peff