Re: Testsuite failure on s390x and sparc64 after 6840fe9ee2

From

Jeff King <peff@peff.net>

Date

Apr 1, 2025, 03:10 UTC

Message-ID

<20250401031030.GB1087913@coredump.intra.peff.net>

In-Reply-To

<20250401023358.GA1087913@coredump.intra.peff.net>

On Mon, Mar 31, 2025 at 10:33:58PM -0400, Jeff King wrote:

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

diff --git a/git-compat-util.h b/git-compat-util.h
index 8560c89374..7bcbe0b4ac 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -110,11 +110,17 @@ DISABLE_WARNING(-Wsign-compare)
 # define BARF_UNLESS_COPYABLE(dst, src) \
 	BUILD_ASSERT_OR_ZERO(__builtin_types_compatible_p(__typeof__(*(dst)), \
 							  __typeof__(*(src))))
+
+# define BARF_UNLESS_TYPE_MATCH(var, type) \
+	BUILD_ASSERT_OR_ZERO(__builtin_types_compatible_p(__typeof__(*(var)), type))
+
 #else
 # define BARF_UNLESS_AN_ARRAY(arr) 0
 # define BARF_UNLESS_COPYABLE(dst, src) \
 	BUILD_ASSERT_OR_ZERO(0 ? ((*(dst) = *(src)), 0) : \
 				 sizeof(*(dst)) == sizeof(*(src)))
+# define BARF_UNLESS_TYPE_MATCH(var, type) \
+	BUILD_ASSERT_OR_ZERO(sizeof(*(var)) == sizeof(type))
 #endif
 /*
  * ARRAY_SIZE - get the number of elements in a visible array
diff --git a/parse-options.h b/parse-options.h
index 997ffbee80..b38a852a8b 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) + BARF_UNLESS_TYPE_MATCH((v), int), \
 	.argh = N_("n"), \
 	.help = (h), \
 	.flags = (f), \

That turns up several more hits, which all seem to be related to
signed-ness (mostly passing a pointer to unsigned). E.g.:

  git grep --after-context=-1 foo -- builtin/checkout.c

ends up assigning "-1" to an "unsigned" via pointer casting. I think
that's probably technically undefined behavior, but works OK in practice
to give you UINT_MAX.  I'd have thought that would give you infinite
context, so it might even be doing something useful (or at least
something that users might rely upon), but strangely it doesn't seem to
(I'd guess very large context values aren't handled in the grep code
somehow).

So it might be possible to clean these up without hurting anything else.

-Peff