Re: [PATCH 2/2] use __builtin_add_overflow() in st_add() with Clang

["René Scharfe" <l.s.r@web.de>]

On 5/14/26 10:17 PM, René Scharfe wrote:
> On 5/14/26 9:12 PM, Junio C Hamano wrote:
>> René Scharfe <l.s.r@web.de> writes:
>>
>>> Provide a variant of st_add() that wraps __builtin_add_overflow() to
>>> help Clang optimize it.  Use it on all platforms for simplicity.
>>> ...
>>> +/* Help Clang; GCC generates the same code for both variants. */
>>> +#if defined(__clang__)
>>> +static inline size_t st_add(size_t a, size_t b)
>>> +{
>>> +	size_t sum;
>>> +	if (__builtin_add_overflow(a, b, &sum))
>>> +		die("size_t overflow: %"PRIuMAX" + %"PRIuMAX,
>>> +		    (uintmax_t)a, (uintmax_t)b);
>>> +	return sum;
>>> +}
>>> +#else
>>>  static inline size_t st_add(size_t a, size_t b)
>>>  {
>>>  	if (unsigned_add_overflows(a, b))
>>> @@ -621,6 +632,7 @@ static inline size_t st_add(size_t a, size_t b)
>>>  		    (uintmax_t)a, (uintmax_t)b);
>>>  	return a + b;
>>>  }
>>> +#endif
>>
>> Makes me wonder if we tweaked unsigned_add_overflows() to take an
>> extra *dst parameter to match __builtin_add_overflow(), which of
>> course requires us to all of 18 callsites, it might make the whole
>> thing a bit simpler.  New uses of unsigned_add_overflows(), if we
>> ever add them, would automatically benefit, right?
> 
> Hmm.  It sounds like a lot of churn, but it would make sure that
> we use the checked result and not check a + b and then go on and
> use x + y because the code de-synced at some point.
> 
> How to do it, though?  It needs to be generic and evaluate its
> arguments only once.  Perhaps like this?
> 
> 
> diff --git a/git-compat-util.h b/git-compat-util.h
> index ca89cfb0b3..27fbb622d7 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -103,6 +103,21 @@ struct strbuf;
>  #define unsigned_add_overflows(a, b) \
>      ((b) > maximum_unsigned_value_of_type(a) - (a))
>  
> +static bool uint_add_overflow(uintmax_t a, uintmax_t b,
> +			      uintmax_t *out, size_t out_size)
> +{
> +	if (b > UINTMAX_MAX - a)
> +		return true;
> +	a += b;
> +	if (a > (UINTMAX_MAX >> (bitsizeof(uintmax_t) - CHAR_BIT * out_size)))
> +		return true;
> +	*out = a;
> +	return false;
> +}
> +
> +#define UINT_ADD_OVERFLOW(a, b, out) \
> +	uint_add_overflow((a), (b), (out), sizeof(a))
> +
>  /*
>   * Returns true if the multiplication of "a" and "b" will
>   * overflow. The types of "a" and "b" must match and must be unsigned.
> @@ -616,10 +631,11 @@ int git_open_cloexec(const char *name, int flags);
>  
>  static inline size_t st_add(size_t a, size_t b)
>  {
> -	if (unsigned_add_overflows(a, b))
> +	size_t ret;
> +	if (UINT_ADD_OVERFLOW(a, b, &ret))

Type mismatch of third argument: pointer to size_t given, pointer to
uintmax_t expected.

>  		die("size_t overflow: %"PRIuMAX" + %"PRIuMAX,
>  		    (uintmax_t)a, (uintmax_t)b);
> -	return a + b;
> +	return ret;
>  }
>  #define st_add3(a,b,c)   st_add(st_add((a),(b)),(c))
>  #define st_add4(a,b,c,d) st_add(st_add3((a),(b),(c)),(d))
Perhaps like this instead?


diff --git a/git-compat-util.h b/git-compat-util.h
index ae1bdc90a4..23ea42f373 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -103,6 +103,25 @@ struct strbuf;
 #define unsigned_add_overflows(a, b) \
     ((b) > maximum_unsigned_value_of_type(a) - (a))
 
+static inline uintmax_t uint_add_overflow(uintmax_t a, uintmax_t b,
+					  uintmax_t max, bool *overflow)
+{
+	*overflow = a > max || b > max - a;
+	return a + b;
+}
+
+#ifdef __clang__
+#define UINT_ADD_OVERFLOW(a, b, out, overflow) \
+	(*(overflow) = __builtin_add_overflow((a), (b), (out)))
+#else
+#define UINT_ADD_OVERFLOW(a, b, out, overflow) ( \
+	*(out) = uint_add_overflow((a), (b), \
+				   maximum_unsigned_value_of_type(*(out)), \
+				   (overflow)), \
+	*(overflow) \
+)
+#endif
+
 /*
  * Returns true if the multiplication of "a" and "b" will
  * overflow. The types of "a" and "b" must match and must be unsigned.
@@ -616,10 +635,12 @@ int git_open_cloexec(const char *name, int flags);
 
 static inline size_t st_add(size_t a, size_t b)
 {
-	if (unsigned_add_overflows(a, b))
+	bool overflow;
+	size_t ret;
+	if (UINT_ADD_OVERFLOW(a, b, &ret, &overflow))
 		die("size_t overflow: %"PRIuMAX" + %"PRIuMAX,
 		    (uintmax_t)a, (uintmax_t)b);
-	return a + b;
+	return ret;
 }
 #define st_add3(a,b,c)   st_add(st_add((a),(b)),(c))
 #define st_add4(a,b,c,d) st_add(st_add3((a),(b),(c)),(d))