{"thread":{"id":"10495","subject":"[RFH] gcc constant expression warning...","startedAt":"2007-10-28T08:46:47Z","lastAt":"2007-10-29T04:37:29Z","messageCount":8,"participants":["Junio C Hamano","Florian Weimer","Antti-Juhani Kaijanaho","Daniel Barkalow","Linus Torvalds","Nicolas Pitre","Stephen Rothwell"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"57405","messageId":"7vy7dnvd6w.fsf@gitster.siamese.dyndns.org","threadId":"10495","inReplyTo":null,"subject":"[RFH] gcc constant expression warning...","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-10-28T08:46:47Z","receivedAt":"2007-10-28T08:46:47Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"With the recent gcc, we get:\n\nsha1_file.c: In check_packed_git_:\nsha1_file.c:527: warning: assuming signed overflow does not\noccur when assuming that (X + c) < X is always false\nsha1_file.c:527: warning: assuming signed overflow does not\noccur when assuming that (X + c) < X is always false\n\nwhen compiling with\n\n    -O2 -Werror -Wall -Wold-style-definition \\\n    -ansi -pedantic -std=c99 -Wdeclaration-after-statement\n\nThe offending lines are:\n\n        if (idx_size != min_size) {\n                /* make sure we can deal with large pack offsets */\n                off_t x = 0x7fffffffUL, y = 0xffffffffUL;\n                if (x > (x + 1) || y > (y + 1)) {\n                        munmap(idx_map, idx_size);\n"},{"id":"57414","messageId":"87ir4rletv.fsf@mid.deneb.enyo.de","threadId":"10495","inReplyTo":"7vy7dnvd6w.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFH] gcc constant expression warning...","fromName":"Florian Weimer","fromEmail":"fw@deneb.enyo.de","sentAt":"2007-10-28T10:21:32Z","receivedAt":"2007-10-28T10:21:32Z","isPatch":false,"sender":{"key":"fw@deneb.enyo.de","avatar":null},"body":"* Junio C. Hamano:\n\n> The offending lines are:\n>\n>         if (idx_size != min_size) {\n>                 /* make sure we can deal with large pack offsets */\n>                 off_t x = 0x7fffffffUL, y = 0xffffffffUL;\n>                 if (x > (x + 1) || y > (y + 1)) {\n>                         munmap(idx_map, idx_size);\n\nx and y must be unsigned for this test to work (signed overflow is\nundefined).\n"},{"id":"57421","messageId":"slrnfi8pj7.mb4.antti-juhani@kukkaseppele.kaijanaho.fi","threadId":"10495","inReplyTo":"7vy7dnvd6w.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFH] gcc constant expression warning...","fromName":"Antti-Juhani Kaijanaho","fromEmail":"antti-juhani@kaijanaho.fi","sentAt":"2007-10-28T10:37:27Z","receivedAt":"2007-10-28T10:37:27Z","isPatch":false,"sender":{"key":"antti-juhani@kaijanaho.fi","avatar":null},"body":"Junio C Hamano <gitster@pobox.com> kirjoitti 28.10.2007:\n> With the recent gcc, we get:\n>\n> sha1_file.c: In check_packed_git_:\n> sha1_file.c:527: warning: assuming signed overflow does not\n> occur when assuming that (X + c) < X is always false\n> sha1_file.c:527: warning: assuming signed overflow does not\n> occur when assuming that (X + c) < X is always false\n>\n> when compiling with\n>\n>     -O2 -Werror -Wall -Wold-style-definition \\\n>     -ansi -pedantic -std=c99 -Wdeclaration-after-statement\n\n-ansi and -std=c99 in the same command line is a bit weird, BTW :)\n\n> The offending lines are:\n>\n>         if (idx_size != min_size) {\n>                 /* make sure we can deal with large pack offsets */\n>                 off_t x = 0x7fffffffUL, y = 0xffffffffUL;\n>                 if (x > (x + 1) || y > (y + 1)) {\n>                         munmap(idx_map, idx_size);\n\nThe second if line invokes undefined behavior if off_t cannot represent\n0x7fffffffUL + 1.  GCC apparently takes that as a license to ignore\noverflow and rewrite that if as \"if (0) { ...\".\n\nA fast fix is to compile with -fwrapv or with -fno-strict-overflow:\n\n-fstrict-overflow\n    Allow the compiler to assume strict signed overflow rules, depending\n    on the language being compiled. For C (and C++) this means that\n    overflow when doing arithmetic with signed numbers is undefined,\n    which means that the compiler may assume that it will not happen.\n    This permits various optimizations. For example, the compiler will\n    assume that an expression like i + 10 > i will always be true for\n    signed i. This assumption is only valid if signed overflow is\n    undefined, as the expression is false if i + 10 overflows when using\n    twos complement arithmetic. When this option is in effect any\n    attempt to determine whether an operation on signed numbers will\n    overflow must be written carefully to not actually involve overflow.\n\n    See also the -fwrapv option. Using -fwrapv means that signed\n    overflow is fully defined: it wraps. When -fwrapv is used, there is\n    no difference between -fstrict-overflow and -fno-strict-overflow.\n    With -fwrapv certain types of overflow are permitted. For example,\n    if the compiler gets an overflow when doing arithmetic on constants,\n    the overflowed value can still be used with -fwrapv, but not\n    otherwise.\n\n    The -fstrict-overflow option is enabled at levels -O2, -O3, -Os. \n\n-fwrapv\n    This option instructs the compiler to assume that signed arithmetic\n    overflow of addition, subtraction and multiplication wraps around\n    using twos-complement representation. This flag enables some\n    optimizations and disables others. This option is enabled by default\n    for the Java front-end, as required by the Java language\n    specification. \n\n(From the GCC 4.2.2 manual.)\n\nA correct fix would be to check for the size of off_t in some other (and\ndefined) manner, but I don't know off_t well enough to suggest one.\nConsidering that the size of off_t won't change at runtime, the test\nought to be compile (or configure) time.  Reading POSIX, there seem to\nbe some rather cumbersome sysconf stuff one could test for, and of\ncourse CHAR_BIT * sizeof(off_t) also tells one something.  GNU autoconf\nmight also have a solution at hand.\n\n-- \nAntti-Juhani Kaijanaho, Jyväskylä, Finland\n"},{"id":"57433","messageId":"Pine.LNX.4.64.0710281204350.7345@iabervon.org","threadId":"10495","inReplyTo":"87ir4rletv.fsf@mid.deneb.enyo.de","subject":"Re: [RFH] gcc constant expression warning...","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2007-10-28T16:28:59Z","receivedAt":"2007-10-28T16:28:59Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sun, 28 Oct 2007, Florian Weimer wrote:\n\n> * Junio C. Hamano:\n> \n> > The offending lines are:\n> >\n> >         if (idx_size != min_size) {\n> >                 /* make sure we can deal with large pack offsets */\n> >                 off_t x = 0x7fffffffUL, y = 0xffffffffUL;\n> >                 if (x > (x + 1) || y > (y + 1)) {\n> >                         munmap(idx_map, idx_size);\n> \n> x and y must be unsigned for this test to work (signed overflow is\n> undefined).\n\nI believe the test is trying to determine if signed addition on numbers of \na certain size is safe in this environment. Doing the test with unsigned \nvariables would cause the test to give a predictable but irrelevant \nresult. I think gcc is being annoying in assuming that signed overflow \ndoesn't occur (even when it must), rather than assuming that the result of \nsigned overflow is some arbitrary and likely not useful value. If we have \nan overflow possible with off_t in the way we'd use it, then one of those \ntests should be automatically true due to the limited size of the type \n(except that I think the test should be >= instead of >). I think we \nshould be able to assume that the result of a signed overflow, whatever \nundefined value it is, is a possible value of its type and therefore not \nmore than the maximum value of its type, but gcc may be screwing this up.\n\nIt's probably best just to test the size of off_t.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"57438","messageId":"alpine.LFD.0.999.0710281000260.30120@woody.linux-foundation.org","threadId":"10495","inReplyTo":"slrnfi8pj7.mb4.antti-juhani@kukkaseppele.kaijanaho.fi","subject":"Re: [RFH] gcc constant expression warning...","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-10-28T17:09:10Z","receivedAt":"2007-10-28T17:09:10Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 28 Oct 2007, Antti-Juhani Kaijanaho wrote:\n>\n> A correct fix would be to check for the size of off_t in some other (and\n> defined) manner, but I don't know off_t well enough to suggest one.\n\nIn this case, it's trying to make sense that \"off_t\" can hold more than 32\nbits. So I think that test can just be rewritten as\n\n\tif (sizeof(off_t) <= 4) {\n\t\tmunmap(idx_map, idx_size);\n\t\treturn error(\"pack too large for current definition of off_t in %s\", path);\n\t}\n\ninstead.\n\n\t\t\tLinus\n"},{"id":"57456","messageId":"alpine.LFD.0.9999.0710282053590.22100@xanadu.home","threadId":"10495","inReplyTo":"alpine.LFD.0.999.0710281000260.30120@woody.linux-foundation.org","subject":"Re: [RFH] gcc constant expression warning...","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-10-29T00:55:32Z","receivedAt":"2007-10-29T00:55:32Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Sun, 28 Oct 2007, Linus Torvalds wrote:\n\n> \n> \n> On Sun, 28 Oct 2007, Antti-Juhani Kaijanaho wrote:\n> >\n> > A correct fix would be to check for the size of off_t in some other (and\n> > defined) manner, but I don't know off_t well enough to suggest one.\n> \n> In this case, it's trying to make sense that \"off_t\" can hold more than 32\n> bits. So I think that test can just be rewritten as\n> \n> \tif (sizeof(off_t) <= 4) {\n> \t\tmunmap(idx_map, idx_size);\n> \t\treturn error(\"pack too large for current definition of off_t in %s\", path);\n> \t}\n> \n> instead.\n\nThe test must also make sure off_t isn't signed, since in that case it \ncan only hold 31 bits.\n\n\nNicolas\n"},{"id":"57457","messageId":"20071029134135.ed72fe78.sfr@canb.auug.org.au","threadId":"10495","inReplyTo":"alpine.LFD.0.9999.0710282053590.22100@xanadu.home","subject":"Re: [RFH] gcc constant expression warning...","fromName":"Stephen Rothwell","fromEmail":"sfr@canb.auug.org.au","sentAt":"2007-10-29T02:41:35Z","receivedAt":"2007-10-29T02:41:35Z","isPatch":false,"sender":{"key":"sfr@canb.auug.org.au","avatar":null},"body":"On Sun, 28 Oct 2007 20:55:32 -0400 (EDT) Nicolas Pitre <nico@cam.org> wrote:\n>\n> The test must also make sure off_t isn't signed, since in that case it \n> can only hold 31 bits.\n\nPosix says:\n\t\"blkcnt_t and off_t shall be signed integer types.\"\n \n-- \nCheers,\nStephen Rothwell                    sfr@canb.auug.org.au\nhttp://www.canb.auug.org.au/~sfr/\n"},{"id":"57463","messageId":"alpine.LFD.0.999.0710282136410.30120@woody.linux-foundation.org","threadId":"10495","inReplyTo":"alpine.LFD.0.9999.0710282053590.22100@xanadu.home","subject":"Re: [RFH] gcc constant expression warning...","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-10-29T04:37:29Z","receivedAt":"2007-10-29T04:37:29Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 28 Oct 2007, Nicolas Pitre wrote:\n> \n> The test must also make sure off_t isn't signed, since in that case it \n> can only hold 31 bits.\n\nSinc eneither 31 _nor_ 32 bits is really enough, it's perfectly fine to \njust check that the size of \"off_t\" is *bigger* than 4 bytes, which my \npseudo-patch did.\n\n\t\tLinus\n"}]}