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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 26, 2018, 02:48 UTC
Message-ID: <xmqq4ld9e83d.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20181025161310.29249-1-tboegi@web.de>

tboegi@web.de writes:

> From: Torsten Bögershausen <tboegi@web.de>

> 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().

Show 7 quoted lines

> 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)"?

Show 9 quoted lines

> 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.

Show 6 quoted lines

> +	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;
>  }