Re: [RFC PATCH 10/10] linear-assignment.c: use "intmax_t" instead of "int"

From: Jeff King <peff@peff.net>
Date: Dec 10, 2021, 04:00 UTC
Message-ID: <YbLQxId7DLkbT7mQ@coredump.intra.peff.net>
In-Reply-To: <RFC-patch-10.10-46395080b64-20211209T191653Z-avarab@gmail.com>

On Thu, Dec 09, 2021 at 08:19:27PM +0100, Ævar Arnfjörð Bjarmason wrote:

Show 11 quoted lines

> Change the "int" type used by compute_assignment() to "intmax_t". On
> 64 bit systems this changes the overflow "die" added in the preceding
> commit (which before that was a segfault) to something that merely
> takes a very long time and a lot of memory to run.
> 
> On my relatively beefy system this completes:
> 
>     git -P range-diff --creation-factor=50 origin/master...git-for-windows/main
> 
> In around 300 seconds, with a reported max RSS of just under 18GB, but
> it does give you correct results for all ~50k commitsin that range.

So here you do what I think is the "real" fix. And at this point I suspect that the overflow checks here:

Show 14 quoted lines

> -static inline int cost_index(int *cost, int a, int b, int c)
> +static inline intmax_t cost_index(intmax_t *cost, intmax_t a, intmax_t b, intmax_t c)
>  {
> -	int r;
> +	intmax_t r;
>  
>  	if (INT_MULTIPLY_WRAPV(a, c, &r))
> -		die(_("integer overflow in cost[%d + %d * %d] multiplication"), b, a, c);
> +		die(_("integer overflow in cost[%"PRIuMAX" + %"PRIuMAX" * %"PRIuMAX"] multiplication"), b, a, c);
>  	if (INT_ADD_WRAPV(b, r, &r))
> -		die(_("integer overflow in cost[%d + ((%d * %d) = %d)] addition"), b, a, c, r);
> +		die(_("integer overflow in cost[%"PRIuMAX" + ((%"PRIuMAX" * %"PRIuMAX") = %"PRIuMAX")] addition"), b, a, c, r);
>  
>  	return r;

cannot be triggered. We are indexing an array that is already limited to size_t. So using that type should be sufficient (or else we have problems even outside of overflow).

And for that reason I'd probably pick size_t (or ssize_t; you don't need it in the hunk above, but presumably some of the other code cares about signedness. And as I said earlier, it's effectively the same from a protection standpoint).

-Peff