From: Phillip Wood <phillip.wood123@gmail.com>
Date: Thu, 15 Oct 2026 09:27:42 GMT
Subject: Re: [PATCH 4/5] git-compat-util: add `signed_mult_overflows` and `mult_overflows` macros
Message-ID: <373f7186-3da6-4715-a4ce-cd5f56895176@gmail.com>
In-Reply-To: <3dcc3d97570ea7f96628d02941d7424f84f7bf39.1791958171.git.gitgitgadget@gmail.com>

Hi Johannes

On 14/10/2026 07:09, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > `unsigned_mult_overflows()` does not cover signed arithmetic, but types
> such as `time_t` can be signed or unsigned. Add signed and type-generic
> guards so callers can check multiplication overflow in either case.

It would have been nice to mention that the type-generic macro is gcc only so we cannot rely on this for safety checks.

>   #define maximum_signed_value_of_type(a) \
>       (INTMAX_MAX >> (bitsizeof(intmax_t) - bitsizeof(a)))
>   > +/* Account for the extra negative value on two's-complement targets. */
> +#define minimum_signed_value_of_type(a) \
> +    (-maximum_signed_value_of_type(a) - (INTMAX_MIN < -INTMAX_MAX))

Do we support any platforms that do not use two's complement arithmetic? C23 makes it mandatory so presumably there are not may contemporary platforms out there that do not use it.

> +/*
> + * Returns true if the multiplication of "a" and "b" will
> + * overflow or underflow a signed type. The types of "a" and "b"
> + * must match and must be signed. Note that this macro evaluates
> + * both "a" and "b" twice!

At least twice I think

> + */
> +#define signed_mult_overflows(a, b) \

As the macro relies on both operands being the same width, can we use BUILD_ASSERT_OR_ZERO() to ensure sizeof(a) == sizeof(b) here?

> +    (((a) > 0 && (b) > 0 && (a) > maximum_signed_value_of_type(a) / (b)) || \
> +     ((a) < 0 && (b) < 0 && (a) < maximum_signed_value_of_type(a) / (b)) || \
> +     ((a) > 0 && (b) < 0 && (b) < minimum_signed_value_of_type(a) / (a)) || \
> +     ((a) < 0 && (b) > 0 && (a) < minimum_signed_value_of_type(b) / (b)))

Thanks

Phillip

> +
> +/*
> + * Returns true if the product of "a" and "b", after the usual arithmetic
> + * conversions, cannot be represented in the multiplication expression's
> + * type. The operands must be integers and may be evaluated more than once.
> + */
> +#if GIT_GNUC_PREREQ(3, 1)
> +#define mult_overflows(a, b) \
> +    (is_unsigned_type((a) * (b)) \
> +     ? unsigned_mult_overflows((__typeof__((a) * (b)))(a), \
> +			       (__typeof__((a) * (b)))(b)) \
> +     : signed_mult_overflows((__typeof__((a) * (b)))(a), \
> +			     (__typeof__((a) * (b)))(b)))
> +#else
> +/*
> + * This implementation needs GNU `typeof` to convert both operands to the
> + * multiplication's type. Skip the check without that extension.
> + */
> +#define mult_overflows(a, b) 0
> +#endif
> +
>   /*
>    * Returns true if the left shift of "a" by "shift" bits will
>    * overflow. The type of "a" must be unsigned.