[PATCH v2] do not depend on signed integer overflow

[PATCH v2] do not depend on signed integer overflow

[Erik Faye-Lund]

Signed integer overflow is not defined in C, so do not depend on it.

This fixes a problem with GCC 4.4.0 and -O3 where the optimizer would
consider "consumed_bytes > consumed_bytes + bytes" as a constant
expression, and never execute the die()-call.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 builtin/index-pack.c     |    2 +-
 builtin/unpack-objects.c |    2 +-
 git-compat-util.h        |    9 +++++++++
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 2e680d7..e243d9d 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -161,7 +161,7 @@ static void use(int bytes)
 	input_offset += bytes;
 
 	/* make sure off_t is sufficiently large not to wrap */
-	if (consumed_bytes > consumed_bytes + bytes)
+	if (signed_add_overflows(consumed_bytes, bytes))
 		die("pack too large for current definition of off_t");
 	consumed_bytes += bytes;
 }
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 685566e..f63973c 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -83,7 +83,7 @@ static void use(int bytes)
 	offset += bytes;
 
 	/* make sure off_t is sufficiently large not to wrap */
-	if (consumed_bytes > consumed_bytes + bytes)
+	if (signed_add_overflows(consumed_bytes, bytes))
 		die("pack too large for current definition of off_t");
 	consumed_bytes += bytes;
 }
diff --git a/git-compat-util.h b/git-compat-util.h
index 81883e7..dfb5565 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -28,6 +28,15 @@
 #define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0]))
 #define bitsizeof(x)  (CHAR_BIT * sizeof(x))
 
+/*
+ * Signed integer overflow is undefined in C, so here's a helper macro
+ * to detect if the sum of two integers will overflow. The bitsize to
+ * overflow at is taken from the first parameter, which must be zero
+ * or positive.
+ */
+#define signed_add_overflows(a, b) \
+    ((b) > ((INTMAX_MAX >> (bitsizeof(intmax_t) - bitsizeof(a))) - (a)))
+
 #ifdef __GNUC__
 #define TYPEOF(x) (__typeof__(x))
 #else
-- 
1.7.3.1.51.g3f36d

Re: [PATCH v2] do not depend on signed integer overflow

[Jonathan Nieder]

Erik Faye-Lund wrote:

> +++ b/git-compat-util.h
> @@ -28,6 +28,15 @@
>  #define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0]))
>  #define bitsizeof(x)  (CHAR_BIT * sizeof(x))
>  
> +/*
> + * Signed integer overflow is undefined in C, so here's a helper macro
> + * to detect if the sum of two integers will overflow. The bitsize to
> + * overflow at is taken from the first parameter, which must be zero
> + * or positive.
> + */
> +#define signed_add_overflows(a, b) \
> +    ((b) > ((INTMAX_MAX >> (bitsizeof(intmax_t) - bitsizeof(a))) - (a)))

Yes, I still like it.  This could be made closer to self-documenting
like so:

#define maximum_signed_value_of_type(a) \
	(INTMAX_MAX >> (bitsizeof(intmax_t) - bitsizeof(a)))

/*
 * Signed overflow is undefined in C, so here's a helper macro
 * to detect if the sum of two signed integers will overflow.
 *
 * Requires: a >= 0, typeof(a) equals typeof(b)
 */
#define signed_add_overflows(a, b) \
	((b) > maximum_signed_value_of_type(a) - (a))

Re: [PATCH v2] do not depend on signed integer overflow

[Erik Faye-Lund]

On Tue, Oct 5, 2010 at 12:33 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Erik Faye-Lund wrote:
>
>> +++ b/git-compat-util.h
>> @@ -28,6 +28,15 @@
>>  #define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0]))
>>  #define bitsizeof(x)  (CHAR_BIT * sizeof(x))
>>
>> +/*
>> + * Signed integer overflow is undefined in C, so here's a helper macro
>> + * to detect if the sum of two integers will overflow. The bitsize to
>> + * overflow at is taken from the first parameter, which must be zero
>> + * or positive.
>> + */
>> +#define signed_add_overflows(a, b) \
>> +    ((b) > ((INTMAX_MAX >> (bitsizeof(intmax_t) - bitsizeof(a))) - (a)))
>
> Yes, I still like it.  This could be made closer to self-documenting
> like so:
>
> #define maximum_signed_value_of_type(a) \
>        (INTMAX_MAX >> (bitsizeof(intmax_t) - bitsizeof(a)))
>
> /*
>  * Signed overflow is undefined in C, so here's a helper macro
>  * to detect if the sum of two signed integers will overflow.
>  *
>  * Requires: a >= 0, typeof(a) equals typeof(b)
>  */
> #define signed_add_overflows(a, b) \
>        ((b) > maximum_signed_value_of_type(a) - (a))
>

I like that. Thanks for the suggestion, I'll roll a new version.