{"thread":{"id":"48086","subject":"[PATCH 0/2] -Wuninitialized","startedAt":"2018-03-19T17:54:06Z","lastAt":"2018-03-20T23:02:40Z","messageCount":5,"participants":["Ramsay Jones","Jeff King","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"342216","messageId":"8f6d8e2e-aba4-128c-f17c-6c1b9c12436c@ramsayjones.plus.com","threadId":"48086","inReplyTo":null,"subject":"[PATCH 0/2] -Wuninitialized","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-03-19T17:53:09Z","receivedAt":"2018-03-19T17:54:06Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nThis series removes all 'self-initialised' variables (ie. <type> var = var;).\nThis construct has been used to silence gcc '-W[maybe-]uninitialized' warnings\nin the past [1]. Unfortunately, this construct causes warnings to be issued by\nMSVC [2], along with clang static analysis complaining about an 'Assigned value\nis garbage or undefined'. The number of these constructs has dropped over the\nyears (eg. see [3] and [4]), so there are currently only 6 remaining in the\ncurrent codebase. As demonstrated below, 5 of these no longer cause gcc to\nissue warnings.\n\nThese patches were developed on v2.16.0, but a test merge to current 'master',\n'next' and 'pu' branches complete without conflict.\n\nIf you add '-Winit-self' to CFLAGS and compile v2.16.0, then:\n\n  $ vim config.mak # add -Winit-self to CFLAGS\n  $ make >g.out-warn-init 2>&1\n  $ grep warning g.out-warn-init\n  merge-recursive.c:2064:15: warning: ‘mrtree’ is used uninitialized in this function [-Wuninitialized]\n  read-cache.c:2107:6: warning: ‘saved_namelen’ is used uninitialized in this function [-Wuninitialized]\n  fast-import.c:3006:23: warning: ‘oe’ is used uninitialized in this function [-Wuninitialized]\n  fast-import.c:3023:23: warning: ‘oe’ is used uninitialized in this function [-Wuninitialized]\n  $ \n\nThis misses the self-initialization of the 'reaches' and 'all' symbols in\nbuiltin/rev-list.c, which is somewhat surprising! ;-)\n\nThe commits in which these self-initializations were introduced are noted\nbelow:\n\n  a. builtin/rev-list.c: 'reaches' and 'all', see commit 457f08a030\n     (\"git-rev-list: add --bisect-vars option.\", 2007-03-21).\n\n  b. merge-recursive.c:2064 'mrtree', see commit f120ae2a8e (\"merge-\n     recursive.c: mrtree in merge() is not used before set\", 2007-10-29).\n\n  c. fast-import.c:3023 'oe', see commit 85c62395b1 (\"fast-import: let\n     importers retrieve blobs\", 2010-11-28).\n\n  d. read-cache.c:2107 'saved_namelen', see commit b3c96fb158 (\"split-index:\n     strip pathname of on-disk replaced entries\", 2014-06-13).\n\n  e. fast-import.c:3006 'oe', see commit 28c7b1f7b7 (\"fast-import: add a\n     get-mark command\", 2015-07-01).\n\nClang static analysis marks the self-initialization as a 'Logic error' with\nthe name 'Assigned value is garbage or undefined'. clang 3.8.0 notes b,c,d\nand e, but not a. clang 5.0.1 notes a(reaches),b,c,d and e, but not a(all).\n\nIf we now add a patch to remove all self-initialization, which would be the\nfirst patch plus the obvious change to 'saved_namelen' in read-cache.c, then\nnote the warnings issued by various compilers at various optimization levels\non several different platforms [5]:\n\n                    O0      O1      O2      O3      Os       Og\n 1) gcc 4.8.3   |   -      1,20     1    1,18-19  1-4,21-23  1,5-17\n 2) gcc 4.8.4   |   -      1,20     1       1     1-4,21-23  1,5-8,10-13,15-16 \n 3) clang 3.4   |   -       -       -       -        -       n/a \n 4) gcc 5.4.0   |   -       1       1       1     1,3-4,21   1,5-8,10-13,16-16\n 5) clang 3.8.0 |   -       -       -       -        -       n/a \n 6) gcc 5.4.0   |   -       1       1       1       1-4     1,5-17 \n 7) clang 3.8.0 |   -       -       -       -        -       n/a \n 8) gcc 6.4.0   |   -       1       1    1,18-19    1,4     1,5-17\n 9) clang 5.0.1 |   -       -       -       -        -        -\n10) gcc 7.2.1   |   -       1       1       1       1,4     1,5-17\n\n[Not -Wmaybe-uninitialized table]\n 1) gcc 4.8.3   |   27      27      27      27       27       27\n 3) clang 3.4   |   26      26      26      26       26      n/a \n 5) clang 3.8.0 | 24-26   24-26   24-26   24-26    24-26     n/a \n\n[clang 3.4 and 3.8.0 do not support -Og, but clang 5.0.1 does.]\n[warnings 18-19 issued on cygwin since it builds with NO_REGEX.]\n\nCompiler/Platforms:\n  1: 32-bit Windows XP - Cygwin 1.7.30 2014-05-23 i686, Core2 Duo T2050\n2,3: 32-bit Linux Mint 17.3, i686, Intel Core2 Duo T2050\n4,5: 32-bit Linux Mint 18.3, i686, Virtual Box VM\n6,7: 64-bit Linux Mint 18.3, x86_64, Intel Core i5-4200M\n8,9: 64-bit Windows 10 - Cygwin 2.10.0 2018-02-02 x86_64, Intel Core i5-4200M\n 10: 64-bit Fedora Linux 27, x86_64, Virtual Box VM\n\nNote that warnings 1-23 [6] are all -Wmaybe-uninitialized, and 24-27 do not\nconcern us here [7]. Outside of -Os and -Og, the only warnings are 1, 18\nand 19. Warnings 18 and 19 are only issued against the compat regex routines\nwhile building with NO_REGEX set. Warning 1 is suppressed by applying the\nsecond patch in this series.\n\nSo, as noted above, with the exception of the 'saved_namelen' symbol, none\nof the 'self-initialised' variables cause gcc to complain. Thus the first\npatch does not make any difference to the table(s) above. The second patch\nremoves the warning #1 from the above table(s).\n\nNotes:\n[1] My memory is not what it was, but my recollection is that, circa 2007,\nthese warnings were -Wuninitialized. I suspect that, due to too many false\npositives, many have been moved to -Wmaybe-uninitialized. ;-)\n\n[2] At least it used to cause (actually linker) warnings, but maybe more\nup-to-date versions of MSVC no longer complain? I haven't tried compiling\nwith MSVC for many years (I haven't had an MSVC installation for at least\n6 years now).\n\n[3] https://public-inbox.org/git/4CFA8D4D.2020500@ramsay1.demon.co.uk/\n\n[4] https://public-inbox.org/git/d2e97e800910021440q46bd46c4y8a5af987620ffc5c@mail.gmail.com/#r \n\n[5] I installed fedora 27 so that I could get a more up-to-date compiler,\nsince I tend to stick with conservative distros that are based on an LTS\nbase (e.g. Linux Mint). (I also found that sparse was totaly useless on\nfedora - something else for my TODO list)!\n\n[6] I have not studied the warnings 1-23. I have only briefly looked at a\nfew of these warnings, but they all look (at first blush) to be false\npositives. (Also, note that this is not a clang problem)! ;-)\n\n[7] A patch has already been sent for warning 27. I am preparing a patch for\nwarnings 24-25 (call imaxabs() rather than labs()). As for warning 26, well\nyes, I agree! ;-)\n\n\nwarnings:\n 1. read-cache.c:2154:18: warning: ‘saved_namelen’ may be used uninitialized in this function [-Wmaybe-uninitialized]\n 2. config.c:904:9: warning: ‘ret’ may be used uninitialized in this function [-Wmaybe-uninitialized]\n 3. config.c:928:9: warning: ‘ret’ may be used uninitialized in this function [-Wmaybe-uninitialized]\n 4. fast-import.c:2378:8: warning: ‘mode’ may be used uninitialized in this function [-Wmaybe-uninitialized]\n 5. connect.c:126:3: warning: ‘arg’ may be used uninitialized in this function [-Wmaybe-uninitialized]\n 6. diff.c:2938:9: warning: ‘orig_size’ may be used uninitialized in this function [-Wmaybe-uninitialized]\n 7. fetch-pack.c:283:3: warning: ‘arg’ may be used uninitialized in this function [-Wmaybe-uninitialized]\n 8. fetch-pack.c:427:9: warning: ‘arg’ may be used uninitialized in this function [-Wmaybe-uninitialized]\n 9. list-objects-filter-options.c:64:39: warning: ‘v0’ may be used uninitialized in this function [-Wmaybe-uninitialized]\n10. refs/packed-backend.c:638:3: warning: ‘p’ may be used uninitialized in this function [-Wmaybe-uninitialized]\n11. trailer.c:560:3: warning: ‘type’ may be used uninitialized in this function [-Wmaybe-uninitialized]\n12. git-compat-util.h:483:18: warning: ‘data’ may be used uninitialized in this function [-Wmaybe-uninitialized]\n13. fast-import.c:2876:4: warning: ‘from’ may be used uninitialized in this function [-Wmaybe-uninitialized]\n14. http-backend.c:261:8: warning: ‘svc_name’ may be used uninitialized in this function [-Wmaybe-uninitialized]\n15. git-compat-util.h:535:23: warning: ‘tail’ may be used uninitialized in this function [-Wmaybe-uninitialized]\n16. remote-curl.c:875:5: warning: ‘p’ may be used uninitialized in this function [-Wmaybe-uninitialized]\n17. t/helper/test-ref-store.c:46:16: warning: ‘gitdir’ may be used uninitialized in this function [-Wmaybe-uninitialized]\n18. compat/regex/regcomp.c:2634:6: warning: ‘end_elem’ may be used uninitialized in this function [-Wmaybe-uninitialized]\n19. compat/regex/regcomp.c:3138:41: warning: ‘start_elem’ may be used uninitialized in this function [-Wmaybe-uninitialized]\n20. fsck.c:768:5: warning: ‘err’ may be used uninitialized in this function [-Wmaybe-uninitialized]\n21. config.c:912:9: warning: ‘ret’ may be used uninitialized in this function [-Wmaybe-uninitialized]\n22. diff.c:1729:56: warning: ‘j’ may be used uninitialized in this function [-Wmaybe-uninitialized]\n23. builtin/update-index.c:853:19: warning: ‘mode’ may be used uninitialized in this function [-Wmaybe-uninitialized]\n24. config.c:788:10: warning: absolute value function 'labs' given an argument of type 'intmax_t' (aka 'long long') but has parameter of type 'long' which may cause truncation of value [-Wabsolute-value]\n25. config.c:790:21: warning: absolute value function 'labs' given an argument of type 'intmax_t' (aka 'long long') but has parameter of type 'long' which may cause truncation of value [-Wabsolute-value]\n26. remote-curl.c:536:10: warning: comparison of constant 9223372036854775807 with expression of type 'ssize_t' (aka 'int') is always false [-Wtautological-constant-out-of-range-compare]\n27. http.c:77:20: warning: ‘curl_no_proxy’ defined but not used [-Wunused-variable]\n\n\n\nRamsay Jones (2):\n  -Wuninitialized: remove some 'init-self' workarounds\n  read-cache: fix an -Wmaybe-uninitialized warning\n\n builtin/rev-list.c | 2 +-\n fast-import.c      | 4 ++--\n merge-recursive.c  | 2 +-\n read-cache.c       | 6 ++++--\n 4 files changed, 8 insertions(+), 6 deletions(-)\n\n-- \n2.16.0\n"},{"id":"342285","messageId":"20180320043225.GB13302@sigill.intra.peff.net","threadId":"48086","inReplyTo":"8f6d8e2e-aba4-128c-f17c-6c1b9c12436c@ramsayjones.plus.com","subject":"Re: [PATCH 0/2] -Wuninitialized","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-20T04:32:25Z","receivedAt":"2018-03-20T04:32:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 19, 2018 at 05:53:09PM +0000, Ramsay Jones wrote:\n\n> This series removes all 'self-initialised' variables (ie. <type> var = var;).\n> This construct has been used to silence gcc '-W[maybe-]uninitialized' warnings\n> in the past [1]. Unfortunately, this construct causes warnings to be issued by\n> MSVC [2], along with clang static analysis complaining about an 'Assigned value\n> is garbage or undefined'. The number of these constructs has dropped over the\n> years (eg. see [3] and [4]), so there are currently only 6 remaining in the\n> current codebase. As demonstrated below, 5 of these no longer cause gcc to\n> issue warnings.\n\nGreat. I'm happy to see these going away, and thanks for all the careful\ndigging.\n\n> If we now add a patch to remove all self-initialization, which would be the\n> first patch plus the obvious change to 'saved_namelen' in read-cache.c, then\n> note the warnings issued by various compilers at various optimization levels\n> on several different platforms [5]:\n> \n>                     O0      O1      O2      O3      Os       Og\n>  1) gcc 4.8.3   |   -      1,20     1    1,18-19  1-4,21-23  1,5-17\n>  2) gcc 4.8.4   |   -      1,20     1       1     1-4,21-23  1,5-8,10-13,15-16 \n>  3) clang 3.4   |   -       -       -       -        -       n/a \n>  4) gcc 5.4.0   |   -       1       1       1     1,3-4,21   1,5-8,10-13,16-16\n>  5) clang 3.8.0 |   -       -       -       -        -       n/a \n>  6) gcc 5.4.0   |   -       1       1       1       1-4     1,5-17 \n>  7) clang 3.8.0 |   -       -       -       -        -       n/a \n>  8) gcc 6.4.0   |   -       1       1    1,18-19    1,4     1,5-17\n>  9) clang 5.0.1 |   -       -       -       -        -        -\n> 10) gcc 7.2.1   |   -       1       1       1       1,4     1,5-17\n\nSo I guess this could create headaches for people using DEVELOPER=1 on\nas ancient a compiler as 4.8.4, but most other people should be OK. I\nthink I can live with that as a cutoff, and the Travis builds should\nwork there.\n\n(And if we do the detect-compiler stuff from the other nearby thread,\nsomebody who cares can even loosen the warnings for those old gcc\nversions).\n\nI'm neglecting anybody doing -O3 or -Os here, but IMHO those are\nsufficiently rare that the builder can tweak their own settings.\n\nI wonder if people use -Og, though? I don't (I usually do -O0 for my\nedit-compile-debug cycle).\n\n-Peff\n"},{"id":"342319","messageId":"nycvar.QRO.7.76.6.1803201514490.55@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","threadId":"48086","inReplyTo":"8f6d8e2e-aba4-128c-f17c-6c1b9c12436c@ramsayjones.plus.com","subject":"Re: [PATCH 0/2] -Wuninitialized","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-03-20T14:46:05Z","receivedAt":"2018-03-20T14:46:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ramsay,\n\nOn Mon, 19 Mar 2018, Ramsay Jones wrote:\n\n> This series removes all 'self-initialised' variables (ie. <type> var =\n> var;).  This construct has been used to silence gcc\n> '-W[maybe-]uninitialized' warnings in the past [1]. Unfortunately, this\n> construct causes warnings to be issued by MSVC [2], along with clang\n> static analysis complaining about an 'Assigned value is garbage or\n> undefined'. The number of these constructs has dropped over the years\n> (eg. see [3] and [4]), so there are currently only 6 remaining in the\n> current codebase. As demonstrated below, 5 of these no longer cause gcc\n> to issue warnings.\n\nThank you so much for working on this!\n\nIn Git for Windows, to work around the MSVC issues you mention, I have\nthis ugly work-around (essentially, it has a FAKE_INIT() macro that either\nperforms that GCC workaround or initializes the variable to NULL/0):\n\n\thttps://github.com/git-for-windows/git/commit/474155f32a\n\nFWIW I just tested your patches with Visual Studio 2017 and there are no\nC4700 warnings (the equivalent of GCC's \"may be uninitialized\" warning)\n[*1*].\n\nYou can find the patches (your patches rebased onto Git for Windows'\nmaster, plus a patch adding the project files for Visual Studio) here:\n\nhttps://github.com/git-for-windows/git/compare/master...dscho:msvc-uninitialized-warning-test\n\nSo here is my ACK, in case you want it ;-)\n\nCiao,\nDscho\n\nFootnote *1*: there actually was one, but in a Windows-only patch. That's\nwhat that last (fixup!) patch on my branch is all about.\n"},{"id":"342371","messageId":"ce46b318-2c37-4562-7e57-526f511cd6a8@ramsayjones.plus.com","threadId":"48086","inReplyTo":"20180320043225.GB13302@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] -Wuninitialized","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-03-20T22:41:05Z","receivedAt":"2018-03-20T22:41:13Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 20/03/18 04:32, Jeff King wrote:\n> On Mon, Mar 19, 2018 at 05:53:09PM +0000, Ramsay Jones wrote:\n \n>> If we now add a patch to remove all self-initialization, which would be the\n>> first patch plus the obvious change to 'saved_namelen' in read-cache.c, then\n>> note the warnings issued by various compilers at various optimization levels\n>> on several different platforms [5]:\n>>\n>>                     O0      O1      O2      O3      Os       Og\n>>  1) gcc 4.8.3   |   -      1,20     1    1,18-19  1-4,21-23  1,5-17\n>>  2) gcc 4.8.4   |   -      1,20     1       1     1-4,21-23  1,5-8,10-13,15-16 \n>>  3) clang 3.4   |   -       -       -       -        -       n/a \n>>  4) gcc 5.4.0   |   -       1       1       1     1,3-4,21   1,5-8,10-13,16-16\n>>  5) clang 3.8.0 |   -       -       -       -        -       n/a \n>>  6) gcc 5.4.0   |   -       1       1       1       1-4     1,5-17 \n>>  7) clang 3.8.0 |   -       -       -       -        -       n/a \n>>  8) gcc 6.4.0   |   -       1       1    1,18-19    1,4     1,5-17\n>>  9) clang 5.0.1 |   -       -       -       -        -        -\n>> 10) gcc 7.2.1   |   -       1       1       1       1,4     1,5-17\n> \n> So I guess this could create headaches for people using DEVELOPER=1 on\n> as ancient a compiler as 4.8.4, but most other people should be OK. I\n> think I can live with that as a cutoff, and the Travis builds should\n> work there.\n\nYeah, I have an even older laptop (Windows 95 era), but I'm not\nsure if it will even boot these days. (It does have gcc on it\nIIRC, but who knows which version). ;-)\n\n> (And if we do the detect-compiler stuff from the other nearby thread,\n> somebody who cares can even loosen the warnings for those old gcc\n> versions).\n> \n> I'm neglecting anybody doing -O3 or -Os here, but IMHO those are\n> sufficiently rare that the builder can tweak their own settings.\n\nI have occasionally used -O3, never used -Os (except for this\nexercise) and have been meaning to try -Og (in anger) for a while. ;-)\n\n> I wonder if people use -Og, though? I don't (I usually do -O0 for my\n> edit-compile-debug cycle).\n\nIn the gcc documentation, the -Og description says:\n\n  \"... It should be the optimization level of choice for the standard\n   edit-compile-debug cycle, ...\"\n\nLike you, I use -O0 (very old habits). As I said above, I have been\nmeaning to try -Og, but, well round tuits ... ;-)\n\n[BTW, gcc also supports -Ofast, but I don't think we want to go\nthere ...]\n\nATB,\nRamsay Jones\n\n\n\n\n"},{"id":"342374","messageId":"b939f3e7-9ccf-9a26-9ccf-2b2c13853e42@ramsayjones.plus.com","threadId":"48086","inReplyTo":"nycvar.QRO.7.76.6.1803201514490.55@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","subject":"Re: [PATCH 0/2] -Wuninitialized","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-03-20T23:02:29Z","receivedAt":"2018-03-20T23:02:40Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 20/03/18 14:46, Johannes Schindelin wrote:\n> Hi Ramsay,\n> \n> On Mon, 19 Mar 2018, Ramsay Jones wrote:\n> \n>> This series removes all 'self-initialised' variables (ie. <type> var =\n>> var;).  This construct has been used to silence gcc\n>> '-W[maybe-]uninitialized' warnings in the past [1]. Unfortunately, this\n>> construct causes warnings to be issued by MSVC [2], along with clang\n>> static analysis complaining about an 'Assigned value is garbage or\n>> undefined'. The number of these constructs has dropped over the years\n>> (eg. see [3] and [4]), so there are currently only 6 remaining in the\n>> current codebase. As demonstrated below, 5 of these no longer cause gcc\n>> to issue warnings.\n> \n> Thank you so much for working on this!\n\nThese patches are based on a very old branch (that goes back\nat least as far as 2010, see [1]). (I have too many in my repo,\nso it will be good to remove this one)!\n\n> In Git for Windows, to work around the MSVC issues you mention, I have\n> this ugly work-around (essentially, it has a FAKE_INIT() macro that either\n> performs that GCC workaround or initializes the variable to NULL/0):\n> \n> \thttps://github.com/git-for-windows/git/commit/474155f32a\n\nOh, wow! (Hmm, actually that doesn't look too bad :-D )\n\n> FWIW I just tested your patches with Visual Studio 2017 and there are no\n> C4700 warnings (the equivalent of GCC's \"may be uninitialized\" warning)\n> [*1*].\n> \n> You can find the patches (your patches rebased onto Git for Windows'\n> master, plus a patch adding the project files for Visual Studio) here:\n> \n> https://github.com/git-for-windows/git/compare/master...dscho:msvc-uninitialized-warning-test\n\nThanks for testing the patches.\n\n> So here is my ACK, in case you want it ;-)\n\nThanks!\n\nATB,\nRamsay Jones\n\n[1] https://public-inbox.org/git/4CFA8D4D.2020500@ramsay1.demon.co.uk/\n"}]}