[PATCH] vcs-svn: Fix some compiler warnings

[PATCH] vcs-svn: Fix some compiler warnings

[Ramsay Jones]

In particular, some versions of gcc complains as follows:

        CC vcs-svn/sliding_window.o
    vcs-svn/sliding_window.c: In function `check_overflow':
    vcs-svn/sliding_window.c:36: warning: comparison is always false \
        due to limited range of data type

        CC vcs-svn/fast_export.o
    vcs-svn/fast_export.c: In function `fast_export_blob_delta':
    vcs-svn/fast_export.c:303: warning: comparison is always false due \
        to limited range of data type

Simply casting the (limited range unsigned) variable in the comparison
to an uintmax_t does not suppress the warning, however, since gcc is
"smart" enough to know that the cast does not change anything regarding
the value of the casted expression. In order to suppress the warning, we
replace the variable with a new uintmax_t variable initialized with the
value of the original variable.

Note that the "some versions of gcc" which complain includes 3.4.4 and
4.1.2, whereas gcc version 4.4.0 compiles the code without complaint.

Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---

Hi Jonathan,

When I wrote this patch, your "jn/svn-fe" branch was still in pu, so I was
going to ask you to squash this, or some variant, into any re-roll ...

Note, in the original version of this patch, the change to check_overflow()
was much simpler; it was effectively the same 2-line change (modulo some
variable names) which is applied to fast_export_blob_delta(). But it just
didn't look right (not sure why!), which prompted the renaming of the
function parameter names. This is, obviously, an unrelated change, so maybe
the change to sliding_window.c should be reverted to the 2-line change ...

ATB,
Ramsay Jones

 vcs-svn/fast_export.c    |    3 ++-
 vcs-svn/sliding_window.c |   11 ++++++-----
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
index 19d7c34..6edd37e 100644
--- a/vcs-svn/fast_export.c
+++ b/vcs-svn/fast_export.c
@@ -300,7 +300,8 @@ void fast_export_blob_delta(uint32_t mode,
 				uint32_t len, struct line_buffer *input)
 {
 	long postimage_len;
-	if (len > maximum_signed_value_of_type(off_t))
+	uintmax_t delta_len = (uintmax_t) len;
+	if (delta_len > maximum_signed_value_of_type(off_t))
 		die("enormous delta");
 	postimage_len = apply_delta((off_t) len, input, old_data, old_mode);
 	if (mode == REPO_MODE_LNK) {
diff --git a/vcs-svn/sliding_window.c b/vcs-svn/sliding_window.c
index 1bac7a4..49a7293 100644
--- a/vcs-svn/sliding_window.c
+++ b/vcs-svn/sliding_window.c
@@ -31,15 +31,16 @@ static int read_to_fill_or_whine(struct line_buffer *file,
 	return 0;
 }
 
-static int check_overflow(off_t a, size_t b)
+static int check_overflow(off_t offset, size_t len)
 {
-	if (b > maximum_signed_value_of_type(off_t))
+	uintmax_t delta_len = (uintmax_t) len;
+	if (delta_len > maximum_signed_value_of_type(off_t))
 		return error("unrepresentable length in delta: "
-				"%"PRIuMAX" > OFF_MAX", (uintmax_t) b);
-	if (signed_add_overflows(a, (off_t) b))
+				"%"PRIuMAX" > OFF_MAX", delta_len);
+	if (signed_add_overflows(offset, (off_t) len))
 		return error("unrepresentable offset in delta: "
 				"%"PRIuMAX" + %"PRIuMAX" > OFF_MAX",
-				(uintmax_t) a, (uintmax_t) b);
+				(uintmax_t) offset, delta_len);
 	return 0;
 }
 
-- 
1.7.9

Re: [PATCH] vcs-svn: Fix some compiler warnings

[Jonathan Nieder]

Hi,

Ramsay Jones wrote:

> In particular, some versions of gcc complains as follows:
> 
>         CC vcs-svn/sliding_window.o
>     vcs-svn/sliding_window.c: In function `check_overflow':
>     vcs-svn/sliding_window.c:36: warning: comparison is always false \
>         due to limited range of data type

Yuck.  Suppressing this warning would presumably also suppress the
optimization that notices the comparison is always false.

The -Wtype-limits warning also triggers in some other perfectly
reasonable situations: see <http://gcc.gnu.org/PR51712>.  I wonder if
we should keep a list of unreliable warnings somewhere (e.g.,
Meta/Make).

[...]
> Note that the "some versions of gcc" which complain includes 3.4.4 and
> 4.1.2, whereas gcc version 4.4.0 compiles the code without complaint.

Thanks for tracking this down.  Interesting.  -Wtype-limits was split
out from the default set of warnings (!) in gcc 4.3 to address
<http://gcc.gnu.org/PR12963>, among other bugs (r124875, 2007-05-20).

[...]
> --- a/vcs-svn/fast_export.c
> +++ b/vcs-svn/fast_export.c
> @@ -300,7 +300,8 @@ void fast_export_blob_delta(uint32_t mode,
>  				uint32_t len, struct line_buffer *input)
>  {
>  	long postimage_len;
> -	if (len > maximum_signed_value_of_type(off_t))
> +	uintmax_t delta_len = (uintmax_t) len;
> +	if (delta_len > maximum_signed_value_of_type(off_t))
>  		die("enormous delta");
>  	postimage_len = apply_delta((off_t) len, input, old_data, old_mode);

Is there some less ugly way to write the condition "if this value is
not representable in this type"?

I guess I could live with something like the following (please don't
take the names too seriously):

	static inline off_t off_t_or_die(uintmax_t val, const char *msg_if_bad)
	{
		if (val > maximum_signed_value_of_type(off_t))
			die("%s", msg_if_bad);
		return (off_t) val;
	}

	...

		off_t delta_len = off_t_or_die(len, "enormous delta");
		postimage_len = apply_delta(delta_len, input, ...);

What do you think?

Jonathan

Re: [PATCH] vcs-svn: Fix some compiler warnings

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> 		off_t delta_len = off_t_or_die(len, "enormous delta");
> 		postimage_len = apply_delta(delta_len, input, ...);
>
> What do you think?

Another possibility would be to make the "die" part responsibility of the
caller of the helper function, e.g.

	if (value_out_of_range(off_t, len))
		die("enormous delta");

which may make the caller easier to follow and the helper easier to reuse.

Re: [PATCH] vcs-svn: Fix some compiler warnings

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Thanks for tracking this down.  Interesting.  -Wtype-limits was split
> out from the default set of warnings (!) in gcc 4.3 to address
> <http://gcc.gnu.org/PR12963>, among other bugs (r124875, 2007-05-20).
>
> [...]
>> --- a/vcs-svn/fast_export.c
>> +++ b/vcs-svn/fast_export.c
>> @@ -300,7 +300,8 @@ void fast_export_blob_delta(uint32_t mode,
>>  				uint32_t len, struct line_buffer *input)
>>  {
>>  	long postimage_len;
>> -	if (len > maximum_signed_value_of_type(off_t))
>> +	uintmax_t delta_len = (uintmax_t) len;
>> +	if (delta_len > maximum_signed_value_of_type(off_t))
>>  		die("enormous delta");
>>  	postimage_len = apply_delta((off_t) len, input, old_data, old_mode);
>
> Is there some less ugly way to write the condition "if this value is
> not representable in this type"?
> 
> I guess I could live with something like the following (please don't
> take the names too seriously):
> ...
> What do you think?

I'd hold the branch in 'next' for now, until this gets resolved (one
possible resolution is to declare Ramsey's patch is good enough for now,
and do the follow-up later).

[PATCH/RFC 0/3] Re: [PATCH] vcs-svn: Fix some compiler warnings

[Jonathan Nieder]

Junio C Hamano wrote:

> I'd hold the branch in 'next' for now, until this gets resolved (one
> possible resolution is to declare Ramsey's patch is good enough for now,
> and do the follow-up later).

Sounds sensible.  How about the following?  Intended to replace
Ramsay's patch.  Not well tested yet.

By the way, wouldn't the (p->field < 0) test in grep.c line 330
trigger the same compiler bug?

Jonathan Nieder (2):
  vcs-svn: allow import of > 4GiB files
  vcs-svn: suppress a -Wtype-limits warning

Ramsay Allan Jones (1):
  vcs-svn: rename check_overflow arguments for clarity

 vcs-svn/fast_export.c    |   15 +++++++++------
 vcs-svn/fast_export.h    |    4 ++--
 vcs-svn/sliding_window.c |   10 +++++-----
 vcs-svn/svndump.c        |   21 +++++++++++++++------
 4 files changed, 31 insertions(+), 19 deletions(-)

[PATCH 1/3] vcs-svn: rename check_overflow arguments for clarity

[Jonathan Nieder]

From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>

Code using the argument names a and b just doesn't look right (not
sure why!).  Use more explicit names "offset" and "len" to make their
type and function clearer.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Split out from Ramsay's patch.

 vcs-svn/sliding_window.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/vcs-svn/sliding_window.c b/vcs-svn/sliding_window.c
index 1bac7a4c..fafa4a63 100644
--- a/vcs-svn/sliding_window.c
+++ b/vcs-svn/sliding_window.c
@@ -31,15 +31,15 @@ static int read_to_fill_or_whine(struct line_buffer *file,
 	return 0;
 }
 
-static int check_overflow(off_t a, size_t b)
+static int check_overflow(off_t offset, size_t len)
 {
-	if (b > maximum_signed_value_of_type(off_t))
+	if (len > maximum_signed_value_of_type(off_t))
 		return error("unrepresentable length in delta: "
-				"%"PRIuMAX" > OFF_MAX", (uintmax_t) b);
+				"%"PRIuMAX" > OFF_MAX", (uintmax_t) len);
-	if (signed_add_overflows(a, (off_t) b))
+	if (signed_add_overflows(offset, (off_t) len))
 		return error("unrepresentable offset in delta: "
 				"%"PRIuMAX" + %"PRIuMAX" > OFF_MAX",
-				(uintmax_t) a, (uintmax_t) b);
+				(uintmax_t) offset, (uintmax_t) len);
 	return 0;
 }
 
-- 
1.7.9

Re: [PATCH 1/3] vcs-svn: rename check_overflow arguments for clarity

[Dmitry Ivankov]

Hi

On Thu, Feb 2, 2012 at 4:59 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
>
> Code using the argument names a and b just doesn't look right (not
> sure why!).  Use more explicit names "offset" and "len" to make their
> type and function clearer.
Well, it's still not clear. Given off_t a, size_t b, check that a+b
fits into type... which type?
"offset" and "length" don't imply that it's "type of offset" or maybe
"type of length".

>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Split out from Ramsay's patch.
>
>  vcs-svn/sliding_window.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/vcs-svn/sliding_window.c b/vcs-svn/sliding_window.c
> index 1bac7a4c..fafa4a63 100644
> --- a/vcs-svn/sliding_window.c
> +++ b/vcs-svn/sliding_window.c
> @@ -31,15 +31,15 @@ static int read_to_fill_or_whine(struct line_buffer *file,
>        return 0;
>  }
>
> -static int check_overflow(off_t a, size_t b)
> +static int check_overflow(off_t offset, size_t len)
>  {
> -       if (b > maximum_signed_value_of_type(off_t))
> +       if (len > maximum_signed_value_of_type(off_t))
>                return error("unrepresentable length in delta: "
> -                               "%"PRIuMAX" > OFF_MAX", (uintmax_t) b);
> +                               "%"PRIuMAX" > OFF_MAX", (uintmax_t) len);
> -       if (signed_add_overflows(a, (off_t) b))
> +       if (signed_add_overflows(offset, (off_t) len))
>                return error("unrepresentable offset in delta: "
>                                "%"PRIuMAX" + %"PRIuMAX" > OFF_MAX",
> -                               (uintmax_t) a, (uintmax_t) b);
> +                               (uintmax_t) offset, (uintmax_t) len);
>        return 0;
>  }
>
> --
> 1.7.9
>

Re: [PATCH 1/3] vcs-svn: rename check_overflow arguments for clarity

[Jonathan Nieder]

Dmitry Ivankov wrote:
> On Thu, Feb 2, 2012 at 4:59 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

>> From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
>>
>> Code using the argument names a and b just doesn't look right (not
>> sure why!).  Use more explicit names "offset" and "len" to make their
>> type and function clearer.
>
> Well, it's still not clear. Given off_t a, size_t b, check that a+b
> fits into type... which type?
> "offset" and "length" don't imply that it's "type of offset" or maybe
> "type of length".

Hmm... in vector arithmetic, position (i.e., file offset) + displacement
(i.e., size of chunk) = position (i.e., new file offset).  Any ideas
for making this clearer?

Re: [PATCH 1/3] vcs-svn: rename check_overflow arguments for clarity

[David Barr]

On Thu, Feb 2, 2012 at 10:16 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Dmitry Ivankov wrote:
>> On Thu, Feb 2, 2012 at 4:59 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>
>>> From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
>>>
>>> Code using the argument names a and b just doesn't look right (not
>>> sure why!).  Use more explicit names "offset" and "len" to make their
>>> type and function clearer.
>>
>> Well, it's still not clear. Given off_t a, size_t b, check that a+b
>> fits into type... which type?
>> "offset" and "length" don't imply that it's "type of offset" or maybe
>> "type of length".
>
> Hmm... in vector arithmetic, position (i.e., file offset) + displacement
> (i.e., size of chunk) = position (i.e., new file offset).  Any ideas
> for making this clearer?

Maybe rename to check_offset_overflow to make it explicit?
--
David Barr

Re: [PATCH 1/3] vcs-svn: rename check_overflow arguments for clarity

[Jonathan Nieder]

David Barr wrote:

> Maybe rename to check_offset_overflow to make it explicit?

Ok, here's the patch I'll squash in. ;-)

diff --git i/vcs-svn/sliding_window.c w/vcs-svn/sliding_window.c
index 2f4ae60f..ec2707c9 100644
--- i/vcs-svn/sliding_window.c
+++ w/vcs-svn/sliding_window.c
@@ -31,7 +31,7 @@ static int read_to_fill_or_whine(struct line_buffer *file,
 	return 0;
 }
 
-static int check_overflow(off_t offset, uintmax_t len)
+static int check_offset_overflow(off_t offset, uintmax_t len)
 {
 	if (len > maximum_signed_value_of_type(off_t))
 		return error("unrepresentable length in delta: "
@@ -48,9 +48,9 @@ int move_window(struct sliding_view *view, off_t off, size_t width)
 	off_t file_offset;
 	assert(view);
 	assert(view->width <= view->buf.len);
-	assert(!check_overflow(view->off, view->buf.len));
+	assert(!check_offset_overflow(view->off, view->buf.len));
 
-	if (check_overflow(off, width))
+	if (check_offset_overflow(off, width))
 		return -1;
 	if (off < view->off || off + width < view->off + view->width)
 		return error("invalid delta: window slides left");

Re: [PATCH 1/3] vcs-svn: rename check_overflow arguments for clarity

[Junio C Hamano]

Ok, I've tentatively queued this.

-- >8 --
From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>

Code using the argument names a and b just doesn't look right (not
sure why!).  Use more explicit names "offset" and "len" to make their
type and meaning clearer.

Also rename check_overflow() to check_offset_overflow() to clarify
that we are making sure that "len" bytes beyond "offset" still fits
the type to represent an offset.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 vcs-svn/sliding_window.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/vcs-svn/sliding_window.c b/vcs-svn/sliding_window.c
index 1bac7a4..c6c2eff 100644
--- a/vcs-svn/sliding_window.c
+++ b/vcs-svn/sliding_window.c
@@ -31,15 +31,15 @@ static int read_to_fill_or_whine(struct line_buffer *file,
 	return 0;
 }
 
-static int check_overflow(off_t a, size_t b)
+static int check_offset_overflow(off_t offset, size_t len)
 {
-	if (b > maximum_signed_value_of_type(off_t))
+	if (len > maximum_signed_value_of_type(off_t))
 		return error("unrepresentable length in delta: "
-				"%"PRIuMAX" > OFF_MAX", (uintmax_t) b);
-	if (signed_add_overflows(a, (off_t) b))
+				"%"PRIuMAX" > OFF_MAX", (uintmax_t) len);
+	if (signed_add_overflows(offset, (off_t) len))
 		return error("unrepresentable offset in delta: "
 				"%"PRIuMAX" + %"PRIuMAX" > OFF_MAX",
-				(uintmax_t) a, (uintmax_t) b);
+				(uintmax_t) offset, (uintmax_t) len);
 	return 0;
 }
 
@@ -48,9 +48,9 @@ int move_window(struct sliding_view *view, off_t off, size_t width)
 	off_t file_offset;
 	assert(view);
 	assert(view->width <= view->buf.len);
-	assert(!check_overflow(view->off, view->buf.len));
+	assert(!check_offset_overflow(view->off, view->buf.len));
 
-	if (check_overflow(off, width))
+	if (check_offset_overflow(off, width))
 		return -1;
 	if (off < view->off || off + width < view->off + view->width)
 		return error("invalid delta: window slides left");
-- 
1.7.9.172.ge26ae

[PATCH 2/3] vcs-svn: allow import of > 4GiB files

[Jonathan Nieder]

There is no reason in principle that an svn-format dump would not be
able to represent a file whose length does not fit in a 32-bit
integer.  Use off_t consistently to represent file lengths (in place
of using uint32_t in some contexts) so we can handle that.

Most svn-fe code is already ready to do that without this patch and
passes values of type off_t around.  The type mismatch from stragglers
was noticed with gcc -Wtype-limits.

While at it, tighten the parsing of the Text-content-length field to
make sure it is a number and does not overflow, and tighten other
overflow checks as that value is passed around and manipulated.

Inspired-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 vcs-svn/fast_export.c |   15 +++++++++------
 vcs-svn/fast_export.h |    4 ++--
 vcs-svn/svndump.c     |   21 +++++++++++++++------
 3 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
index 19d7c34c..b823b851 100644
--- a/vcs-svn/fast_export.c
+++ b/vcs-svn/fast_export.c
@@ -227,15 +227,18 @@ static long apply_delta(off_t len, struct line_buffer *input,
 	return ret;
 }
 
-void fast_export_data(uint32_t mode, uint32_t len, struct line_buffer *input)
+void fast_export_data(uint32_t mode, off_t len, struct line_buffer *input)
 {
+	assert(len >= 0);
 	if (mode == REPO_MODE_LNK) {
 		/* svn symlink blobs start with "link " */
+		if (len < 5)
+			die("invalid dump: symlink too short for \"link\" prefix");
 		len -= 5;
 		if (buffer_skip_bytes(input, 5) != 5)
 			die_short_read(input);
 	}
-	printf("data %"PRIu32"\n", len);
+	printf("data %"PRIuMAX"\n", (uintmax_t) len);
 	if (buffer_copy_bytes(input, len) != len)
 		die_short_read(input);
 	fputc('\n', stdout);
@@ -297,12 +300,12 @@ int fast_export_ls(const char *path, uint32_t *mode, struct strbuf *dataref)
 
 void fast_export_blob_delta(uint32_t mode,
 				uint32_t old_mode, const char *old_data,
-				uint32_t len, struct line_buffer *input)
+				off_t len, struct line_buffer *input)
 {
 	long postimage_len;
-	if (len > maximum_signed_value_of_type(off_t))
-		die("enormous delta");
-	postimage_len = apply_delta((off_t) len, input, old_data, old_mode);
+
+	assert(len >= 0);
+	postimage_len = apply_delta(len, input, old_data, old_mode);
 	if (mode == REPO_MODE_LNK) {
 		buffer_skip_bytes(&postimage, strlen("link "));
 		postimage_len -= strlen("link ");
diff --git a/vcs-svn/fast_export.h b/vcs-svn/fast_export.h
index 43d05b65..aa629f54 100644
--- a/vcs-svn/fast_export.h
+++ b/vcs-svn/fast_export.h
@@ -14,10 +14,10 @@ void fast_export_begin_commit(uint32_t revision, const char *author,
 			const struct strbuf *log, const char *uuid,
 			const char *url, unsigned long timestamp);
 void fast_export_end_commit(uint32_t revision);
-void fast_export_data(uint32_t mode, uint32_t len, struct line_buffer *input);
+void fast_export_data(uint32_t mode, off_t len, struct line_buffer *input);
 void fast_export_blob_delta(uint32_t mode,
 			uint32_t old_mode, const char *old_data,
-			uint32_t len, struct line_buffer *input);
+			off_t len, struct line_buffer *input);
 
 /* If there is no such file at that rev, returns -1, errno == ENOENT. */
 int fast_export_ls_rev(uint32_t rev, const char *path,
diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index ca63760f..644fdc71 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -40,7 +40,8 @@
 static struct line_buffer input = LINE_BUFFER_INIT;
 
 static struct {
-	uint32_t action, propLength, textLength, srcRev, type;
+	uint32_t action, propLength, srcRev, type;
+	off_t text_length;
 	struct strbuf src, dst;
 	uint32_t text_delta, prop_delta;
 } node_ctx;
@@ -61,7 +62,7 @@ static void reset_node_ctx(char *fname)
 	node_ctx.type = 0;
 	node_ctx.action = NODEACT_UNKNOWN;
 	node_ctx.propLength = LENGTH_UNKNOWN;
-	node_ctx.textLength = LENGTH_UNKNOWN;
+	node_ctx.text_length = -1;
 	strbuf_reset(&node_ctx.src);
 	node_ctx.srcRev = 0;
 	strbuf_reset(&node_ctx.dst);
@@ -209,7 +210,7 @@ static void handle_node(void)
 {
 	const uint32_t type = node_ctx.type;
 	const int have_props = node_ctx.propLength != LENGTH_UNKNOWN;
-	const int have_text = node_ctx.textLength != LENGTH_UNKNOWN;
+	const int have_text = node_ctx.text_length != -1;
 	/*
 	 * Old text for this node:
 	 *  NULL	- directory or bug
@@ -291,12 +292,12 @@ static void handle_node(void)
 	}
 	if (!node_ctx.text_delta) {
 		fast_export_modify(node_ctx.dst.buf, node_ctx.type, "inline");
-		fast_export_data(node_ctx.type, node_ctx.textLength, &input);
+		fast_export_data(node_ctx.type, node_ctx.text_length, &input);
 		return;
 	}
 	fast_export_modify(node_ctx.dst.buf, node_ctx.type, "inline");
 	fast_export_blob_delta(node_ctx.type, old_mode, old_data,
-				node_ctx.textLength, &input);
+				node_ctx.text_length, &input);
 }
 
 static void begin_revision(void)
@@ -409,7 +410,15 @@ void svndump_read(const char *url)
 			break;
 		case sizeof("Text-content-length"):
 			if (!constcmp(t, "Text-content-length")) {
-				node_ctx.textLength = atoi(val);
+				char *end;
+				uintmax_t textlen;
+
+				textlen = strtoumax(val, &end, 10);
+				if (!isdigit(*val) || *end)
+					die("invalid dump: non-numeric length %s", val);
+				if (textlen > maximum_signed_value_of_type(off_t))
+					die("unrepresentable length in dump: %s", val);
+				node_ctx.text_length = (off_t) textlen;
 				break;
 			}
 			if (constcmp(t, "Prop-content-length"))
-- 
1.7.9

[PATCH 3/3] vcs-svn: suppress a -Wtype-limits warning

[Jonathan Nieder]

On 32-bit architectures with 64-bit file offsets, gcc 4.3 and earlier
produce the following warning:

	    CC vcs-svn/sliding_window.o
	vcs-svn/sliding_window.c: In function `check_overflow':
	vcs-svn/sliding_window.c:36: warning: comparison is always false \
	    due to limited range of data type

The warning appears even when gcc is run without any warning flags
(this is gcc bug 12963).  In later versions the same warning can be
reproduced with -Wtype-limits, which is implied by -Wextra.

On 64-bit architectures it really is possible for a size_t not to be
representable as an off_t so the check this is warning about is not
actually redundant.  But even false positives are distracting.  Avoid
the warning by making the "len" argument to check_overflow a
uintmax_t; no functional change intended.

Reported-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
That's the end of the series.  I hope it was entertaining.

Thoughts of all kinds welcome, as usual.

Jonathan

 vcs-svn/sliding_window.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/vcs-svn/sliding_window.c b/vcs-svn/sliding_window.c
index fafa4a63..2f4ae60f 100644
--- a/vcs-svn/sliding_window.c
+++ b/vcs-svn/sliding_window.c
@@ -31,15 +31,15 @@ static int read_to_fill_or_whine(struct line_buffer *file,
 	return 0;
 }
 
-static int check_overflow(off_t offset, size_t len)
+static int check_overflow(off_t offset, uintmax_t len)
 {
 	if (len > maximum_signed_value_of_type(off_t))
 		return error("unrepresentable length in delta: "
-				"%"PRIuMAX" > OFF_MAX", (uintmax_t) len);
+				"%"PRIuMAX" > OFF_MAX", len);
 	if (signed_add_overflows(offset, (off_t) len))
 		return error("unrepresentable offset in delta: "
 				"%"PRIuMAX" + %"PRIuMAX" > OFF_MAX",
-				(uintmax_t) offset, (uintmax_t) len);
+				(uintmax_t) offset, len);
 	return 0;
 }
 
-- 
1.7.9

Re: [PATCH 3/3] vcs-svn: suppress a -Wtype-limits warning

[Ramsay Jones]

Jonathan Nieder wrote:
> ---
> That's the end of the series.  I hope it was entertaining.
> 
> Thoughts of all kinds welcome, as usual.

Yes, this is a much better approach! Thanks!

I've only compile tested (on cygwin and mingw) so far, but
I don't expect any problems ...

So, please disregard my earlier v2 patch. [If it's not already
obvious, I often don't read the list every day and I missed
all of the discussion which resulted in this series (while, at
the same time, writing testing and sending the v2 patch!).]

ATB,
Ramsay Jones

Re: [PATCH] vcs-svn: Fix some compiler warnings

[Ramsay Jones]

Jonathan Nieder wrote:
> Ramsay Jones wrote:
> 
>> In particular, some versions of gcc complains as follows:
>>
>>         CC vcs-svn/sliding_window.o
>>     vcs-svn/sliding_window.c: In function `check_overflow':
>>     vcs-svn/sliding_window.c:36: warning: comparison is always false \
>>         due to limited range of data type
> 
> Yuck.  Suppressing this warning would presumably also suppress the
> optimization that notices the comparison is always false.

I didn't check, but I would assume so.  I would prefer not to loose any
chance at optimizing the code, but in this case, given that we are talking
about a fatal error path, I don't think it is much of a loss.

> The -Wtype-limits warning also triggers in some other perfectly
> reasonable situations: see <http://gcc.gnu.org/PR51712>.
[...]
>> Note that the "some versions of gcc" which complain includes 3.4.4 and
>> 4.1.2, whereas gcc version 4.4.0 compiles the code without complaint.
> 
> Thanks for tracking this down.  Interesting.  -Wtype-limits was split
> out from the default set of warnings (!) in gcc 4.3 to address
> <http://gcc.gnu.org/PR12963>, among other bugs (r124875, 2007-05-20).

Thanks for the above references, and for taking the time to track them
down.

> Is there some less ugly way to write the condition "if this value is
> not representable in this type"?
> 
> I guess I could live with something like the following (please don't
> take the names too seriously):
> 
> 	static inline off_t off_t_or_die(uintmax_t val, const char *msg_if_bad)
> 	{
> 		if (val > maximum_signed_value_of_type(off_t))
> 			die("%s", msg_if_bad);
> 		return (off_t) val;
> 	}
> 
> 	...
> 
> 		off_t delta_len = off_t_or_die(len, "enormous delta");
> 		postimage_len = apply_delta(delta_len, input, ...);
> 
> What do you think?

An static inline function was actually my first thought (although I had
something more like Junio's suggestion [elsewhere in this thread] in mind),
but I didn't want to place it in git-compat-util.h and could not find a
suitable place in the vcs-svn directory.

Hmm, I will send a v2 patch along these lines ...

ATB,
Ramsay Jones

Re: [PATCH] vcs-svn: Fix some compiler warnings

[Jonathan Nieder]

Ramsay Jones wrote:

> An static inline function was actually my first thought (although I had
> something more like Junio's suggestion [elsewhere in this thread] in mind),
> but I didn't want to place it in git-compat-util.h and could not find a
> suitable place in the vcs-svn directory.
>
> Hmm, I will send a v2 patch along these lines ...

Well, your v2 patch looks good to me.  The three-patch series I
sent[1] also look good to me.  I guess I could queue your v2 and put
the rest on top --- what say you?

[1] http://thread.gmane.org/gmane.comp.version-control.git/189618
http://repo.or.cz/w/git/jrn.git/log/refs/topics/rj/svn-fe-type-limits