threads / patch / 29492

patchvcs-svn: Fix some compiler warnings

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

## tl;dr

16 messages between Jan 31, 2012 and Feb 2, 2012. Diffs are folded; open one to read it.

replies: 15people: 5as markdown or json

Ramsay Jones· Jan 31, 2012, 18:48 UTC · lore
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(-)
Show changes to 2 files +8 −6

vcs-svn/fast_export.c, vcs-svn/sliding_window.c

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
Jonathan Nieder· Jan 31, 2012, 19:20 UTC · re: Ramsay Jones · lore

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

Hi,
Ramsay Jones wrote:
Show 6 quoted lines
> 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).

[...]
Show 11 quoted lines
> --- 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
Junio C Hamano· Jan 31, 2012, 20:14 UTC · re: Jonathan Nieder · lore

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

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.
Junio C Hamano· Feb 2, 2012, 04:14 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 24 quoted lines
> 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).

Jonathan Nieder· Feb 2, 2012, 10:41 UTC · re: Junio C Hamano · lore

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

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(-)
Jonathan Nieder· Feb 2, 2012, 10:59 UTC · re: Jonathan Nieder · lore

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

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(-)
Show changes to vcs-svn/sliding_window.c +5 −5
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
Dmitry Ivankov· Feb 2, 2012, 11:05 UTC · re: Jonathan Nieder · lore

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

Hi
On Thu, Feb 2, 2012 at 4:59 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 5 quoted lines
> 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".

Show 36 quoted lines
>
> 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
>
Jonathan Nieder· Feb 2, 2012, 11:16 UTC · re: Dmitry Ivankov · lore

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

Dmitry Ivankov wrote:
> On Thu, Feb 2, 2012 at 4:59 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 10 quoted lines
>> 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?

David Barr· Feb 2, 2012, 11:25 UTC · re: Jonathan Nieder · lore

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

On Thu, Feb 2, 2012 at 10:16 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 17 quoted lines
> 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

Jonathan Nieder· Feb 2, 2012, 11:27 UTC · re: David Barr · lore

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

David Barr wrote:
> Maybe rename to check_offset_overflow to make it explicit?
Ok, here's the patch I'll squash in. ;-)
Show changes to diff +3 −3
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");
Junio C Hamano· Feb 2, 2012, 18:56 UTC · re: Jonathan Nieder · lore

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

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(-)
Show changes to vcs-svn/sliding_window.c +7 −7
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
Jonathan Nieder· Feb 2, 2012, 11:03 UTC · re: Jonathan Nieder · lore

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

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(-)
Show changes to 3 files +26 −14

vcs-svn/fast_export.c, vcs-svn/fast_export.h, vcs-svn/svndump.c

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
Jonathan Nieder· Feb 2, 2012, 11:06 UTC · re: Jonathan Nieder · lore

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

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(-)
Show changes to vcs-svn/sliding_window.c +3 −3
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
Ramsay Jones· Feb 2, 2012, 22:18 UTC · re: Jonathan Nieder · lore

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

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

Ramsay Jones· Feb 2, 2012, 18:24 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder wrote:
Show 11 quoted lines
> 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>.
[...]
Show 6 quoted lines
>> 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.

Show 19 quoted lines
> 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

Jonathan Nieder· Feb 2, 2012, 18:53 UTC · re: Ramsay Jones · lore

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

Ramsay Jones wrote:
Show 6 quoted lines
> 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

← back to recent threads