{"thread":{"id":"49676","subject":"[PATCH v1 2/2] curl_off_t xcurl_off_t is not portable","startedAt":"2018-10-25T16:13:15Z","lastAt":"2018-11-12T08:03:47Z","messageCount":8,"participants":["tboegi@web.de","Junio C Hamano","Torsten Bögershausen"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"361521","messageId":"20181025161310.29249-1-tboegi@web.de","threadId":"49676","inReplyTo":null,"subject":"[PATCH v1 2/2] curl_off_t xcurl_off_t is not portable","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2018-10-25T16:13:10Z","receivedAt":"2018-10-25T16:13:15Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen \n\nComparing signed and unsigned values is not always portable.\nWhen setting\nDEVELOPER = 1\nDEVOPTS = extra-all\n\n\"gcc (Raspbian 6.3.0-18+rpi1+deb9u1) 6.3.0 20170516\" errors out with\n\"comparison is always false due to limited range of data type\"\n\"[-Werror=type-limits]\"\n\nSolution:\nUse a valid cast & compare, similar to xsize_t()\n\nSigned-off-by: Torsten Bögershausen \n---\n remote-curl.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 762a55a75f..c89fd6d1c3 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -618,9 +618,10 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)\n }\n \n static curl_off_t xcurl_off_t(ssize_t len) {\n-\tif (len > maximum_signed_value_of_type(curl_off_t))\n+\tcurl_off_t size = (curl_off_t) len;\n+\tif (len != (ssize_t) size)\n \t\tdie(\"cannot handle pushes this big\");\n-\treturn (curl_off_t) len;\n+\treturn size;\n }\n \n static int post_rpc(struct rpc_state *rpc)\n-- \n2.11.0\n\n"},{"id":"361590","messageId":"xmqq4ld9e83d.fsf@gitster-ct.c.googlers.com","threadId":"49676","inReplyTo":"20181025161310.29249-1-tboegi@web.de","subject":"Re: [PATCH v1 2/2] curl_off_t xcurl_off_t is not portable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-26T02:48:38Z","receivedAt":"2018-10-26T02:48:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"tboegi@web.de writes:\n\n> From: Torsten Bögershausen \n\n> Subject: Re: [PATCH v1 2/2] curl_off_t xcurl_off_t is not portable\n\nThat title is misleading; it sounded as if the are these two\ntypedefs and they do not work correctly on some platforms, but that\nis not what you are doing with the patch.\n\n> Comparing signed and unsigned values is not always portable.\n\nIs that what the compiler is complaining about? There is this bit\nin git-compat-util.h:\n\n/*\n * Signed integer overflow is undefined in C, so here's a helper macro\n * to detect if the sum of two integers will overflow.\n *\n * Requires: a >= 0, typeof(a) equals typeof(b)\n */\n#define signed_add_overflows(a, b) \\\n ((b) > maximum_signed_value_of_type(a) - (a))\n\nwhich is designed to be fed signed a and signed b. The macro is\nused in packfile codepaths to compare int, off_t, etc..\n\nSo the statement may be true but it does not seem to have much to do\nwith the problem you are seeing with maximum_signed_value_of_type().\n\n> When setting\n> DEVELOPER = 1\n> DEVOPTS = extra-all\n>\n> \"gcc (Raspbian 6.3.0-18+rpi1+deb9u1) 6.3.0 20170516\" errors out with\n> \"comparison is always false due to limited range of data type\"\n> \"[-Werror=type-limits]\"\n\nThen this sounds a bit different from \"comparison between signed\nssize_t len and unsigned maximum_signed_value_of_type() is bad\".\nIsn't it saying that \"No matter how big you make len, you can never\ngo beyond maximum_signed_value_of_type(curl_off_t)\"?\n\n> diff --git a/remote-curl.c b/remote-curl.c\n> index 762a55a75f..c89fd6d1c3 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -618,9 +618,10 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)\n> }\n> \n> static curl_off_t xcurl_off_t(ssize_t len) {\n> -\tif (len > maximum_signed_value_of_type(curl_off_t))\n\nIs the issue that len is signed and maximum_signed_value_of_type()\ngives an unsigned value, and these two are compared? As we saw\nearlier, signed_add_overflows() is another example that wants a\nmixed comparison.\n\nI am just wondering if casting len to uintmax_t before comparing\nwith maximum_signed_value_of_type() is a simpler solution that can\nsafely be cargo-culted to other places without much thinking.\n\n\"git grep maximum_signed_value_of_type\" reports a handful\ncomparisons in vcs-svn/, all of which does\n\n\tif (var > maximum_signed_value_of_type(off_t))\n\nwith var of type uintmax_t, which sounds like a sane thing to do.\n\nThanks.\n\n> +\tcurl_off_t size = (curl_off_t) len;\n> +\tif (len != (ssize_t) size)\n> \t\tdie(\"cannot handle pushes this big\");\n> -\treturn (curl_off_t) len;\n> +\treturn size;\n> }\n\n"},{"id":"361626","messageId":"20181026151533.GA15410@tor.lan","threadId":"49676","inReplyTo":"xmqq4ld9e83d.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v1 2/2] curl_off_t xcurl_off_t is not portable","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2018-10-26T15:15:33Z","receivedAt":"2018-10-26T15:15:39Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Fri, Oct 26, 2018 at 11:48:38AM +0900, Junio C Hamano wrote:\n> tboegi@web.de writes:\n> \n> > From: Torsten Bögershausen \n> \n> > Subject: Re: [PATCH v1 2/2] curl_off_t xcurl_off_t is not portable\n> \n> That title is misleading; it sounded as if the are these two\n> typedefs and they do not work correctly on some platforms, but that\n> is not what you are doing with the patch.\n\nOK.\n\n> \n> > Comparing signed and unsigned values is not always portable.\n> \n> Is that what the compiler is complaining about? There is this bit\n> in git-compat-util.h:\n\nNo, not that either, see below.\n\n> \n> /*\n> * Signed integer overflow is undefined in C, so here's a helper macro\n> * to detect if the sum of two integers will overflow.\n> *\n> * Requires: a >= 0, typeof(a) equals typeof(b)\n> */\n> #define signed_add_overflows(a, b) \\\n> ((b) > maximum_signed_value_of_type(a) - (a))\n> \n> which is designed to be fed signed a and signed b. The macro is\n> used in packfile codepaths to compare int, off_t, etc..\n> \n> So the statement may be true but it does not seem to have much to do\n> with the problem you are seeing with maximum_signed_value_of_type().\n> \n> > When setting\n> > DEVELOPER = 1\n> > DEVOPTS = extra-all\n> >\n> > \"gcc (Raspbian 6.3.0-18+rpi1+deb9u1) 6.3.0 20170516\" errors out with\n> > \"comparison is always false due to limited range of data type\"\n> > \"[-Werror=type-limits]\"\n> \n> Then this sounds a bit different from \"comparison between signed\n> ssize_t len and unsigned maximum_signed_value_of_type() is bad\".\n> Isn't it saying that \"No matter how big you make len, you can never\n> go beyond maximum_signed_value_of_type(curl_off_t)\"?\n\nI digged a little bit deeper into the raspi, and this is what I find\nunder\n/usr/include/arm-linux-gnueabihf/curl\n\ncurlbuild.h:#define CURL_TYPEOF_CURL_OFF_T int64_t\ncurlbuild.h:typedef CURL_TYPEOF_CURL_OFF_T curl_off_t;\n\n> \n> > diff --git a/remote-curl.c b/remote-curl.c\n> > index 762a55a75f..c89fd6d1c3 100644\n> > --- a/remote-curl.c\n> > +++ b/remote-curl.c\n> > @@ -618,9 +618,10 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)\n> > }\n> > \n> > static curl_off_t xcurl_off_t(ssize_t len) {\n> > -\tif (len > maximum_signed_value_of_type(curl_off_t))\n> \n> Is the issue that len is signed and maximum_signed_value_of_type()\n> gives an unsigned value, and these two are compared? As we saw\n> earlier, signed_add_overflows() is another example that wants a\n> mixed comparison.\n> \n> I am just wondering if casting len to uintmax_t before comparing\n> with maximum_signed_value_of_type() is a simpler solution that can\n> safely be cargo-culted to other places without much thinking.\n\nI don't know.\nSince ssize_t is 32 bit on the raspi, and curl_off_t is 64 bit,\nthe test seems not to be needed at all ;-)\nI don't know if it makes sense to stop thinking here and if\ncasting to uintmax_t is the right solution here.\n\nAnd, I like the easy-to-read xsize_t, which is safe and warm.\nAgreed that the commit message is wrong.\nI would like to keep the xsize_t aproach, are there more thoughts ?\n\n> \n> \"git grep maximum_signed_value_of_type\" reports a handful\n> comparisons in vcs-svn/, all of which does\n> \n> \tif (var > maximum_signed_value_of_type(off_t))\n> \n> with var of type uintmax_t, which sounds like a sane thing to do.\n> \n> Thanks.\n> \n> > +\tcurl_off_t size = (curl_off_t) len;\n> > +\tif (len != (ssize_t) size)\n> > \t\tdie(\"cannot handle pushes this big\");\n> > -\treturn (curl_off_t) len;\n> > +\treturn size;\n> > }\n> \n"},{"id":"361923","messageId":"20181029165914.2677-1-tboegi@web.de","threadId":"49676","inReplyTo":"20181025161310.29249-1-tboegi@web.de","subject":"[PATCH v2 1/1] remote-curl.c: xcurl_off_t is not portable (on 32 bit platfoms)","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2018-10-29T16:59:14Z","receivedAt":"2018-10-29T16:59:19Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen \n\nWhen setting\nDEVELOPER = 1\nDEVOPTS = extra-all\n\n\"gcc (Raspbian 6.3.0-18+rpi1+deb9u1) 6.3.0 20170516\" errors out with\n\"comparison is always false due to limited range of data type\"\n\"[-Werror=type-limits]\"\n\nIt turns out that the function xcurl_off_t() has 2 flavours:\n- It gives a warning 32 bit systems, like Linux\n- It takes the signed ssize_t as a paramter, but the only caller is using\n a size_t (which is typically unsigned these days)\n\nThe original motivation of this function is to make sure that sizes > 2GiB\nare handled correctly. The curl documentation says:\n\"For any given platform/compiler curl_off_t must be typedef'ed to a 64-bit\n wide signed integral data type\"\nOn a 32 bit system \"size_t\" can be promoted into a 64 bit signed value\nwithout loss of data, and therefore we may see the\n\"comparison is always false\" warning.\nOn a 64 bit system it may happen, at least in theory, that size_t is > 2^63,\nand then the promotion from an unsigned \"size_t\" into a signed \"curl_off_t\"\nmay be a problem.\n\nOne solution to suppress a possible compiler warning could be to remove\nthe function xcurl_off_t().\n\nHowever, to be on the very safe side, we keep it and improve it:\n- The len parameter is changed from ssize_t to size_t\n- A temporally variable \"size\" is used, promoted int uintmax_t and the comopared\n with \"maximum_signed_value_of_type(curl_off_t)\".\n Thanks to Junio C Hamano for this hint.\n\nSigned-off-by: Torsten Bögershausen \n---\n remote-curl.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 762a55a75f..1220dffcdc 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -617,10 +617,11 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)\n \treturn err;\n }\n \n-static curl_off_t xcurl_off_t(ssize_t len) {\n-\tif (len > maximum_signed_value_of_type(curl_off_t))\n+static curl_off_t xcurl_off_t(size_t len) {\n+\tuintmax_t size = len;\n+\tif (size > maximum_signed_value_of_type(curl_off_t))\n \t\tdie(\"cannot handle pushes this big\");\n-\treturn (curl_off_t) len;\n+\treturn (curl_off_t)size;\n }\n \n static int post_rpc(struct rpc_state *rpc)\n-- \n2.19.0.271.gfe8321ec05\n\n"},{"id":"362835","messageId":"20181109174110.27630-1-tboegi@web.de","threadId":"49676","inReplyTo":"20181025161310.29249-1-tboegi@web.de","subject":"[PATCH v2 1/1] remote-curl.c: xcurl_off_t is not portable (on 32 bit platfoms)","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2018-11-09T17:41:10Z","receivedAt":"2018-11-09T17:41:15Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen \n\nWhen setting\nDEVELOPER = 1\nDEVOPTS = extra-all\n\n\"gcc (Raspbian 6.3.0-18+rpi1+deb9u1) 6.3.0 20170516\" errors out with\n\"comparison is always false due to limited range of data type\"\n\"[-Werror=type-limits]\"\n\nIt turns out that the function xcurl_off_t() has 2 flavours:\n- It gives a warning 32 bit systems, like Linux\n- It takes the signed ssize_t as a paramter, but the only caller is using\n a size_t (which is typically unsigned these days)\n\nThe original motivation of this function is to make sure that sizes > 2GiB\nare handled correctly. The curl documentation says:\n\"For any given platform/compiler curl_off_t must be typedef'ed to a 64-bit\n wide signed integral data type\"\nOn a 32 bit system \"size_t\" can be promoted into a 64 bit signed value\nwithout loss of data, and therefore we may see the\n\"comparison is always false\" warning.\nOn a 64 bit system it may happen, at least in theory, that size_t is > 2^63,\nand then the promotion from an unsigned \"size_t\" into a signed \"curl_off_t\"\nmay be a problem.\n\nOne solution to suppress a possible compiler warning could be to remove\nthe function xcurl_off_t().\n\nHowever, to be on the very safe side, we keep it and improve it:\n- The len parameter is changed from ssize_t to size_t\n- A temporally variable \"size\" is used, promoted int uintmax_t and the comopared\n with \"maximum_signed_value_of_type(curl_off_t)\".\n Thanks to Junio C Hamano for this hint.\n\nSigned-off-by: Torsten Bögershausen \n---\n\nThis is a re-semd, the orignal patch was part of a 2\npatch-series.\nThis patch needed some rework, and here should be\nthe polished version.\n\nremote-curl.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 762a55a75f..1220dffcdc 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -617,10 +617,11 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)\n \treturn err;\n }\n \n-static curl_off_t xcurl_off_t(ssize_t len) {\n-\tif (len > maximum_signed_value_of_type(curl_off_t))\n+static curl_off_t xcurl_off_t(size_t len) {\n+\tuintmax_t size = len;\n+\tif (size > maximum_signed_value_of_type(curl_off_t))\n \t\tdie(\"cannot handle pushes this big\");\n-\treturn (curl_off_t) len;\n+\treturn (curl_off_t)size;\n }\n \n static int post_rpc(struct rpc_state *rpc)\n-- \n2.19.0.271.gfe8321ec05\n\n"},{"id":"362958","messageId":"xmqqbm6v3qex.fsf@gitster-ct.c.googlers.com","threadId":"49676","inReplyTo":"20181109174110.27630-1-tboegi@web.de","subject":"Re: [PATCH v2 1/1] remote-curl.c: xcurl_off_t is not portable (on 32 bit platfoms)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-12T03:50:30Z","receivedAt":"2018-11-12T03:50:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"tboegi@web.de writes:\n\n>\n> This is a re-semd, the orignal patch was part of a 2\n> patch-series.\n> This patch needed some rework, and here should be\n> the polished version.\n\nWill queue. Next time, please refrain from saying \"re-send\", if you\nchanged anything in the patch (or the log message), as the phrase\nimplies you are sending the same thing as before (just in case the\nearlier one was not seen, for example). Marking it as vN+1 like you\ndid for this patch and having a reference to the original would make\nit clear, though ;-)\n\nThanks.\n\n> remote-curl.c | 7 ++++---\n> 1 file changed, 4 insertions(+), 3 deletions(-)\n>\n> diff --git a/remote-curl.c b/remote-curl.c\n> index 762a55a75f..1220dffcdc 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -617,10 +617,11 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)\n> \treturn err;\n> }\n> \n> -static curl_off_t xcurl_off_t(ssize_t len) {\n> -\tif (len > maximum_signed_value_of_type(curl_off_t))\n> +static curl_off_t xcurl_off_t(size_t len) {\n> +\tuintmax_t size = len;\n> +\tif (size > maximum_signed_value_of_type(curl_off_t))\n> \t\tdie(\"cannot handle pushes this big\");\n> -\treturn (curl_off_t) len;\n> +\treturn (curl_off_t)size;\n> }\n> \n> static int post_rpc(struct rpc_state *rpc)\n"},{"id":"362963","messageId":"20181112052039.GA12275@tor.lan","threadId":"49676","inReplyTo":"xmqqbm6v3qex.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/1] remote-curl.c: xcurl_off_t is not portable (on 32 bit platfoms)","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2018-11-12T05:20:39Z","receivedAt":"2018-11-12T05:20:45Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Mon, Nov 12, 2018 at 12:50:30PM +0900, Junio C Hamano wrote:\n> tboegi@web.de writes:\n> \n> >\n> > This is a re-semd, the orignal patch was part of a 2\n> > patch-series.\n> > This patch needed some rework, and here should be\n> > the polished version.\n> \n> Will queue.\n\nThanks, is there a chance to kill a typo here ?\ns/comopared/compared/\n- A temporally variable \"size\" is used, promoted int uintmax_t and the comopared\n\n\n> Next time, please refrain from saying \"re-send\", if you\n> changed anything in the patch (or the log message), as the phrase\n> implies you are sending the same thing as before (just in case the\n> earlier one was not seen, for example). Marking it as vN+1 like you\n> did for this patch and having a reference to the original would make\n> it clear, though ;-)\n\nSorry for the confusion.\nThe next time I will not send unrelated patches as a series,\nso that we have a better \"Message-ID:\" and \"In-Reply-To:\"\nflow (which should make live 3% easier).\n\nhttps://public-inbox.org/git/20181029165914.2677-1-tboegi@web.de/\nhttps://public-inbox.org/git/20181109174110.27630-1-tboegi@web.de/\n\n"},{"id":"362970","messageId":"xmqqy39y204h.fsf@gitster-ct.c.googlers.com","threadId":"49676","inReplyTo":"20181112052039.GA12275@tor.lan","subject":"Re: [PATCH v2 1/1] remote-curl.c: xcurl_off_t is not portable (on 32 bit platfoms)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-12T08:03:42Z","receivedAt":"2018-11-12T08:03:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> Thanks, is there a chance to kill a typo here ?\n> s/comopared/compared/\n> - A temporally variable \"size\" is used, promoted int uintmax_t and the comopared\n\nDone. Thanks.\n"}]}