[v1,2/2] curl_off_t xcurl_off_t is not portable

Commit Message

Torsten Bögershausen Oct. 25, 2018, 4:13 p.m. UTC

From: Torsten Bögershausen <[e-mail]>

Comparing signed and unsigned values is not always portable.
When  setting
DEVELOPER = 1
DEVOPTS = extra-all

"gcc (Raspbian 6.3.0-18+rpi1+deb9u1) 6.3.0 20170516" errors out with
"comparison is always false due to limited range of data type"
"[-Werror=type-limits]"

Solution:
Use a valid cast & compare, similar to xsize_t()

Signed-off-by: Torsten Bögershausen <[e-mail]>
---
 remote-curl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Junio C Hamano Oct. 26, 2018, 2:48 a.m. UTC | #1

[e-mail] writes:

> From: Torsten Bögershausen <[e-mail]>

> Subject: Re: [PATCH v1 2/2] curl_off_t xcurl_off_t is not portable

That title is misleading; it sounded as if the are these two
typedefs and they do not work correctly on some platforms, but that
is not what you are doing with the patch.

> Comparing signed and unsigned values is not always portable.

Is that what the compiler is complaining about?  There is this bit
in git-compat-util.h:

/*
 * Signed integer overflow is undefined in C, so here's a helper macro
 * to detect if the sum of two 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))

which is designed to be fed signed a and signed b.  The macro is
used in packfile codepaths to compare int, off_t, etc..

So the statement may be true but it does not seem to have much to do
with the problem you are seeing with maximum_signed_value_of_type().

> When  setting
> DEVELOPER = 1
> DEVOPTS = extra-all
>
> "gcc (Raspbian 6.3.0-18+rpi1+deb9u1) 6.3.0 20170516" errors out with
> "comparison is always false due to limited range of data type"
> "[-Werror=type-limits]"

Then this sounds a bit different from "comparison between signed
ssize_t len and unsigned maximum_signed_value_of_type() is bad".
Isn't it saying that "No matter how big you make len, you can never
go beyond maximum_signed_value_of_type(curl_off_t)"?

> diff --git a/remote-curl.c b/remote-curl.c
> index 762a55a75f..c89fd6d1c3 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -618,9 +618,10 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
>  }
>  
>  static curl_off_t xcurl_off_t(ssize_t len) {
> -	if (len > maximum_signed_value_of_type(curl_off_t))

Is the issue that len is signed and maximum_signed_value_of_type()
gives an unsigned value, and these two are compared?  As we saw
earlier, signed_add_overflows() is another example that wants a
mixed comparison.

I am just wondering if casting len to uintmax_t before comparing
with maximum_signed_value_of_type() is a simpler solution that can
safely be cargo-culted to other places without much thinking.

"git grep maximum_signed_value_of_type" reports a handful
comparisons in vcs-svn/, all of which does

	if (var > maximum_signed_value_of_type(off_t))

with var of type uintmax_t, which sounds like a sane thing to do.

Thanks.

> +	curl_off_t size = (curl_off_t) len;
> +	if (len != (ssize_t) size)
>  		die("cannot handle pushes this big");
> -	return (curl_off_t) len;
> +	return size;
>  }

Torsten Bögershausen Oct. 26, 2018, 3:15 p.m. UTC | #2

On Fri, Oct 26, 2018 at 11:48:38AM +0900, Junio C Hamano wrote:
> [e-mail] writes:
> 
> > From: Torsten Bögershausen <[e-mail]>
> 
> > Subject: Re: [PATCH v1 2/2] curl_off_t xcurl_off_t is not portable
> 
> That title is misleading; it sounded as if the are these two
> typedefs and they do not work correctly on some platforms, but that
> is not what you are doing with the patch.

OK.

> 
> > Comparing signed and unsigned values is not always portable.
> 
> Is that what the compiler is complaining about?  There is this bit
> in git-compat-util.h:

No, not that either, see below.

> 
> /*
>  * Signed integer overflow is undefined in C, so here's a helper macro
>  * to detect if the sum of two 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))
> 
> which is designed to be fed signed a and signed b.  The macro is
> used in packfile codepaths to compare int, off_t, etc..
> 
> So the statement may be true but it does not seem to have much to do
> with the problem you are seeing with maximum_signed_value_of_type().
> 
> > When  setting
> > DEVELOPER = 1
> > DEVOPTS = extra-all
> >
> > "gcc (Raspbian 6.3.0-18+rpi1+deb9u1) 6.3.0 20170516" errors out with
> > "comparison is always false due to limited range of data type"
> > "[-Werror=type-limits]"
> 
> Then this sounds a bit different from "comparison between signed
> ssize_t len and unsigned maximum_signed_value_of_type() is bad".
> Isn't it saying that "No matter how big you make len, you can never
> go beyond maximum_signed_value_of_type(curl_off_t)"?

I digged a little bit deeper into the raspi, and this is what I find
under
/usr/include/arm-linux-gnueabihf/curl

curlbuild.h:#define CURL_TYPEOF_CURL_OFF_T int64_t
curlbuild.h:typedef CURL_TYPEOF_CURL_OFF_T curl_off_t;

> 
> > diff --git a/remote-curl.c b/remote-curl.c
> > index 762a55a75f..c89fd6d1c3 100644
> > --- a/remote-curl.c
> > +++ b/remote-curl.c
> > @@ -618,9 +618,10 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
> >  }
> >  
> >  static curl_off_t xcurl_off_t(ssize_t len) {
> > -	if (len > maximum_signed_value_of_type(curl_off_t))
> 
> Is the issue that len is signed and maximum_signed_value_of_type()
> gives an unsigned value, and these two are compared?  As we saw
> earlier, signed_add_overflows() is another example that wants a
> mixed comparison.
> 
> I am just wondering if casting len to uintmax_t before comparing
> with maximum_signed_value_of_type() is a simpler solution that can
> safely be cargo-culted to other places without much thinking.

I don't know.
Since ssize_t is 32 bit on the raspi, and curl_off_t is 64 bit,
the test seems not to be needed at all ;-)
I don't know if it makes sense to stop thinking here and if
casting to uintmax_t is the right solution here.

And, I like the easy-to-read xsize_t, which is safe and warm.
Agreed that the commit message is wrong.
I would like to keep the xsize_t aproach, are there more thoughts ?

> 
> "git grep maximum_signed_value_of_type" reports a handful
> comparisons in vcs-svn/, all of which does
> 
> 	if (var > maximum_signed_value_of_type(off_t))
> 
> with var of type uintmax_t, which sounds like a sane thing to do.
> 
> Thanks.
> 
> > +	curl_off_t size = (curl_off_t) len;
> > +	if (len != (ssize_t) size)
> >  		die("cannot handle pushes this big");
> > -	return (curl_off_t) len;
> > +	return size;
> >  }
>

Patch

diff --git a/remote-curl.c b/remote-curl.c
index 762a55a75f..c89fd6d1c3 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -618,9 +618,10 @@  static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
 }
 
 static curl_off_t xcurl_off_t(ssize_t len) {
-	if (len > maximum_signed_value_of_type(curl_off_t))
+	curl_off_t size = (curl_off_t) len;
+	if (len != (ssize_t) size)
 		die("cannot handle pushes this big");
-	return (curl_off_t) len;
+	return size;
 }
 
 static int post_rpc(struct rpc_state *rpc)