[PATCH 0/2] -Wuninitialized

[PATCH 0/2] -Wuninitialized

[Ramsay Jones]

This series removes all 'self-initialised' variables (ie. <type> var = var;).
This construct has been used to silence gcc '-W[maybe-]uninitialized' warnings
in the past [1]. Unfortunately, this construct causes warnings to be issued by
MSVC [2], along with clang static analysis complaining about an 'Assigned value
is garbage or undefined'. The number of these constructs has dropped over the
years (eg. see [3] and [4]), so there are currently only 6 remaining in the
current codebase. As demonstrated below, 5 of these no longer cause gcc to
issue warnings.

These patches were developed on v2.16.0, but a test merge to current 'master',
'next' and 'pu' branches complete without conflict.

If you add '-Winit-self' to CFLAGS and compile v2.16.0, then:

  $ vim config.mak # add -Winit-self to CFLAGS
  $ make >g.out-warn-init 2>&1
  $ grep warning g.out-warn-init
  merge-recursive.c:2064:15: warning: ‘mrtree’ is used uninitialized in this function [-Wuninitialized]
  read-cache.c:2107:6: warning: ‘saved_namelen’ is used uninitialized in this function [-Wuninitialized]
  fast-import.c:3006:23: warning: ‘oe’ is used uninitialized in this function [-Wuninitialized]
  fast-import.c:3023:23: warning: ‘oe’ is used uninitialized in this function [-Wuninitialized]
  $ 

This misses the self-initialization of the 'reaches' and 'all' symbols in
builtin/rev-list.c, which is somewhat surprising! ;-)

The commits in which these self-initializations were introduced are noted
below:

  a. builtin/rev-list.c: 'reaches' and 'all', see commit 457f08a030
     ("git-rev-list: add --bisect-vars option.", 2007-03-21).

  b. merge-recursive.c:2064 'mrtree', see commit f120ae2a8e ("merge-
     recursive.c: mrtree in merge() is not used before set", 2007-10-29).

  c. fast-import.c:3023 'oe', see commit 85c62395b1 ("fast-import: let
     importers retrieve blobs", 2010-11-28).

  d. read-cache.c:2107 'saved_namelen', see commit b3c96fb158 ("split-index:
     strip pathname of on-disk replaced entries", 2014-06-13).

  e. fast-import.c:3006 'oe', see commit 28c7b1f7b7 ("fast-import: add a
     get-mark command", 2015-07-01).

Clang static analysis marks the self-initialization as a 'Logic error' with
the name 'Assigned value is garbage or undefined'. clang 3.8.0 notes b,c,d
and e, but not a. clang 5.0.1 notes a(reaches),b,c,d and e, but not a(all).

If we now add a patch to remove all self-initialization, which would be the
first patch plus the obvious change to 'saved_namelen' in read-cache.c, then
note the warnings issued by various compilers at various optimization levels
on several different platforms [5]:

                    O0      O1      O2      O3      Os       Og
 1) gcc 4.8.3   |   -      1,20     1    1,18-19  1-4,21-23  1,5-17
 2) gcc 4.8.4   |   -      1,20     1       1     1-4,21-23  1,5-8,10-13,15-16 
 3) clang 3.4   |   -       -       -       -        -       n/a 
 4) gcc 5.4.0   |   -       1       1       1     1,3-4,21   1,5-8,10-13,16-16
 5) clang 3.8.0 |   -       -       -       -        -       n/a 
 6) gcc 5.4.0   |   -       1       1       1       1-4     1,5-17 
 7) clang 3.8.0 |   -       -       -       -        -       n/a 
 8) gcc 6.4.0   |   -       1       1    1,18-19    1,4     1,5-17
 9) clang 5.0.1 |   -       -       -       -        -        -
10) gcc 7.2.1   |   -       1       1       1       1,4     1,5-17

[Not -Wmaybe-uninitialized table]
 1) gcc 4.8.3   |   27      27      27      27       27       27
 3) clang 3.4   |   26      26      26      26       26      n/a 
 5) clang 3.8.0 | 24-26   24-26   24-26   24-26    24-26     n/a 

[clang 3.4 and 3.8.0 do not support -Og, but clang 5.0.1 does.]
[warnings 18-19 issued on cygwin since it builds with NO_REGEX.]

Compiler/Platforms:
  1: 32-bit Windows XP - Cygwin 1.7.30 2014-05-23 i686, Core2 Duo T2050
2,3: 32-bit Linux Mint 17.3, i686, Intel Core2 Duo T2050
4,5: 32-bit Linux Mint 18.3, i686, Virtual Box VM
6,7: 64-bit Linux Mint 18.3, x86_64, Intel Core i5-4200M
8,9: 64-bit Windows 10 - Cygwin 2.10.0 2018-02-02 x86_64, Intel Core i5-4200M
 10: 64-bit Fedora Linux 27, x86_64, Virtual Box VM

Note that warnings 1-23 [6] are all -Wmaybe-uninitialized, and 24-27 do not
concern us here [7]. Outside of -Os and -Og, the only warnings are 1, 18
and 19. Warnings 18 and 19 are only issued against the compat regex routines
while building with NO_REGEX set. Warning 1 is suppressed by applying the
second patch in this series.

So, as noted above, with the exception of the 'saved_namelen' symbol, none
of the 'self-initialised' variables cause gcc to complain. Thus the first
patch does not make any difference to the table(s) above. The second patch
removes the warning #1 from the above table(s).

Notes:
[1] My memory is not what it was, but my recollection is that, circa 2007,
these warnings were -Wuninitialized. I suspect that, due to too many false
positives, many have been moved to -Wmaybe-uninitialized. ;-)

[2] At least it used to cause (actually linker) warnings, but maybe more
up-to-date versions of MSVC no longer complain? I haven't tried compiling
with MSVC for many years (I haven't had an MSVC installation for at least
6 years now).

[3] https://public-inbox.org/git/4CFA8D4D.2020500@ramsay1.demon.co.uk/

[4] https://public-inbox.org/git/d2e97e800910021440q46bd46c4y8a5af987620ffc5c@mail.gmail.com/#r 

[5] I installed fedora 27 so that I could get a more up-to-date compiler,
since I tend to stick with conservative distros that are based on an LTS
base (e.g. Linux Mint). (I also found that sparse was totaly useless on
fedora - something else for my TODO list)!

[6] I have not studied the warnings 1-23. I have only briefly looked at a
few of these warnings, but they all look (at first blush) to be false
positives. (Also, note that this is not a clang problem)! ;-)

[7] A patch has already been sent for warning 27. I am preparing a patch for
warnings 24-25 (call imaxabs() rather than labs()). As for warning 26, well
yes, I agree! ;-)


warnings:
 1. read-cache.c:2154:18: warning: ‘saved_namelen’ may be used uninitialized in this function [-Wmaybe-uninitialized]
 2. config.c:904:9: warning: ‘ret’ may be used uninitialized in this function [-Wmaybe-uninitialized]
 3. config.c:928:9: warning: ‘ret’ may be used uninitialized in this function [-Wmaybe-uninitialized]
 4. fast-import.c:2378:8: warning: ‘mode’ may be used uninitialized in this function [-Wmaybe-uninitialized]
 5. connect.c:126:3: warning: ‘arg’ may be used uninitialized in this function [-Wmaybe-uninitialized]
 6. diff.c:2938:9: warning: ‘orig_size’ may be used uninitialized in this function [-Wmaybe-uninitialized]
 7. fetch-pack.c:283:3: warning: ‘arg’ may be used uninitialized in this function [-Wmaybe-uninitialized]
 8. fetch-pack.c:427:9: warning: ‘arg’ may be used uninitialized in this function [-Wmaybe-uninitialized]
 9. list-objects-filter-options.c:64:39: warning: ‘v0’ may be used uninitialized in this function [-Wmaybe-uninitialized]
10. refs/packed-backend.c:638:3: warning: ‘p’ may be used uninitialized in this function [-Wmaybe-uninitialized]
11. trailer.c:560:3: warning: ‘type’ may be used uninitialized in this function [-Wmaybe-uninitialized]
12. git-compat-util.h:483:18: warning: ‘data’ may be used uninitialized in this function [-Wmaybe-uninitialized]
13. fast-import.c:2876:4: warning: ‘from’ may be used uninitialized in this function [-Wmaybe-uninitialized]
14. http-backend.c:261:8: warning: ‘svc_name’ may be used uninitialized in this function [-Wmaybe-uninitialized]
15. git-compat-util.h:535:23: warning: ‘tail’ may be used uninitialized in this function [-Wmaybe-uninitialized]
16. remote-curl.c:875:5: warning: ‘p’ may be used uninitialized in this function [-Wmaybe-uninitialized]
17. t/helper/test-ref-store.c:46:16: warning: ‘gitdir’ may be used uninitialized in this function [-Wmaybe-uninitialized]
18. compat/regex/regcomp.c:2634:6: warning: ‘end_elem’ may be used uninitialized in this function [-Wmaybe-uninitialized]
19. compat/regex/regcomp.c:3138:41: warning: ‘start_elem’ may be used uninitialized in this function [-Wmaybe-uninitialized]
20. fsck.c:768:5: warning: ‘err’ may be used uninitialized in this function [-Wmaybe-uninitialized]
21. config.c:912:9: warning: ‘ret’ may be used uninitialized in this function [-Wmaybe-uninitialized]
22. diff.c:1729:56: warning: ‘j’ may be used uninitialized in this function [-Wmaybe-uninitialized]
23. builtin/update-index.c:853:19: warning: ‘mode’ may be used uninitialized in this function [-Wmaybe-uninitialized]
24. 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]
25. 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]
26. 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]
27. http.c:77:20: warning: ‘curl_no_proxy’ defined but not used [-Wunused-variable]



Ramsay Jones (2):
  -Wuninitialized: remove some 'init-self' workarounds
  read-cache: fix an -Wmaybe-uninitialized warning

 builtin/rev-list.c | 2 +-
 fast-import.c      | 4 ++--
 merge-recursive.c  | 2 +-
 read-cache.c       | 6 ++++--
 4 files changed, 8 insertions(+), 6 deletions(-)

-- 
2.16.0

Re: [PATCH 0/2] -Wuninitialized

[Jeff King]

On Mon, Mar 19, 2018 at 05:53:09PM +0000, Ramsay Jones wrote:

> This series removes all 'self-initialised' variables (ie. <type> var = var;).
> This construct has been used to silence gcc '-W[maybe-]uninitialized' warnings
> in the past [1]. Unfortunately, this construct causes warnings to be issued by
> MSVC [2], along with clang static analysis complaining about an 'Assigned value
> is garbage or undefined'. The number of these constructs has dropped over the
> years (eg. see [3] and [4]), so there are currently only 6 remaining in the
> current codebase. As demonstrated below, 5 of these no longer cause gcc to
> issue warnings.

Great. I'm happy to see these going away, and thanks for all the careful
digging.

> If we now add a patch to remove all self-initialization, which would be the
> first patch plus the obvious change to 'saved_namelen' in read-cache.c, then
> note the warnings issued by various compilers at various optimization levels
> on several different platforms [5]:
> 
>                     O0      O1      O2      O3      Os       Og
>  1) gcc 4.8.3   |   -      1,20     1    1,18-19  1-4,21-23  1,5-17
>  2) gcc 4.8.4   |   -      1,20     1       1     1-4,21-23  1,5-8,10-13,15-16 
>  3) clang 3.4   |   -       -       -       -        -       n/a 
>  4) gcc 5.4.0   |   -       1       1       1     1,3-4,21   1,5-8,10-13,16-16
>  5) clang 3.8.0 |   -       -       -       -        -       n/a 
>  6) gcc 5.4.0   |   -       1       1       1       1-4     1,5-17 
>  7) clang 3.8.0 |   -       -       -       -        -       n/a 
>  8) gcc 6.4.0   |   -       1       1    1,18-19    1,4     1,5-17
>  9) clang 5.0.1 |   -       -       -       -        -        -
> 10) gcc 7.2.1   |   -       1       1       1       1,4     1,5-17

So I guess this could create headaches for people using DEVELOPER=1 on
as ancient a compiler as 4.8.4, but most other people should be OK. I
think I can live with that as a cutoff, and the Travis builds should
work there.

(And if we do the detect-compiler stuff from the other nearby thread,
somebody who cares can even loosen the warnings for those old gcc
versions).

I'm neglecting anybody doing -O3 or -Os here, but IMHO those are
sufficiently rare that the builder can tweak their own settings.

I wonder if people use -Og, though? I don't (I usually do -O0 for my
edit-compile-debug cycle).

-Peff

Re: [PATCH 0/2] -Wuninitialized

[Johannes Schindelin]

Hi Ramsay,

On Mon, 19 Mar 2018, Ramsay Jones wrote:

> This series removes all 'self-initialised' variables (ie. <type> var =
> var;).  This construct has been used to silence gcc
> '-W[maybe-]uninitialized' warnings in the past [1]. Unfortunately, this
> construct causes warnings to be issued by MSVC [2], along with clang
> static analysis complaining about an 'Assigned value is garbage or
> undefined'. The number of these constructs has dropped over the years
> (eg. see [3] and [4]), so there are currently only 6 remaining in the
> current codebase. As demonstrated below, 5 of these no longer cause gcc
> to issue warnings.

Thank you so much for working on this!

In Git for Windows, to work around the MSVC issues you mention, I have
this ugly work-around (essentially, it has a FAKE_INIT() macro that either
performs that GCC workaround or initializes the variable to NULL/0):

	https://github.com/git-for-windows/git/commit/474155f32a

FWIW I just tested your patches with Visual Studio 2017 and there are no
C4700 warnings (the equivalent of GCC's "may be uninitialized" warning)
[*1*].

You can find the patches (your patches rebased onto Git for Windows'
master, plus a patch adding the project files for Visual Studio) here:

https://github.com/git-for-windows/git/compare/master...dscho:msvc-uninitialized-warning-test

So here is my ACK, in case you want it ;-)

Ciao,
Dscho

Footnote *1*: there actually was one, but in a Windows-only patch. That's
what that last (fixup!) patch on my branch is all about.

Re: [PATCH 0/2] -Wuninitialized

[Ramsay Jones]

On 20/03/18 04:32, Jeff King wrote:
> On Mon, Mar 19, 2018 at 05:53:09PM +0000, Ramsay Jones wrote:
 
>> If we now add a patch to remove all self-initialization, which would be the
>> first patch plus the obvious change to 'saved_namelen' in read-cache.c, then
>> note the warnings issued by various compilers at various optimization levels
>> on several different platforms [5]:
>>
>>                     O0      O1      O2      O3      Os       Og
>>  1) gcc 4.8.3   |   -      1,20     1    1,18-19  1-4,21-23  1,5-17
>>  2) gcc 4.8.4   |   -      1,20     1       1     1-4,21-23  1,5-8,10-13,15-16 
>>  3) clang 3.4   |   -       -       -       -        -       n/a 
>>  4) gcc 5.4.0   |   -       1       1       1     1,3-4,21   1,5-8,10-13,16-16
>>  5) clang 3.8.0 |   -       -       -       -        -       n/a 
>>  6) gcc 5.4.0   |   -       1       1       1       1-4     1,5-17 
>>  7) clang 3.8.0 |   -       -       -       -        -       n/a 
>>  8) gcc 6.4.0   |   -       1       1    1,18-19    1,4     1,5-17
>>  9) clang 5.0.1 |   -       -       -       -        -        -
>> 10) gcc 7.2.1   |   -       1       1       1       1,4     1,5-17
> 
> So I guess this could create headaches for people using DEVELOPER=1 on
> as ancient a compiler as 4.8.4, but most other people should be OK. I
> think I can live with that as a cutoff, and the Travis builds should
> work there.

Yeah, I have an even older laptop (Windows 95 era), but I'm not
sure if it will even boot these days. (It does have gcc on it
IIRC, but who knows which version). ;-)

> (And if we do the detect-compiler stuff from the other nearby thread,
> somebody who cares can even loosen the warnings for those old gcc
> versions).
> 
> I'm neglecting anybody doing -O3 or -Os here, but IMHO those are
> sufficiently rare that the builder can tweak their own settings.

I have occasionally used -O3, never used -Os (except for this
exercise) and have been meaning to try -Og (in anger) for a while. ;-)

> I wonder if people use -Og, though? I don't (I usually do -O0 for my
> edit-compile-debug cycle).

In the gcc documentation, the -Og description says:

  "... It should be the optimization level of choice for the standard
   edit-compile-debug cycle, ..."

Like you, I use -O0 (very old habits). As I said above, I have been
meaning to try -Og, but, well round tuits ... ;-)

[BTW, gcc also supports -Ofast, but I don't think we want to go
there ...]

ATB,
Ramsay Jones

Re: [PATCH 0/2] -Wuninitialized

[Ramsay Jones]

On 20/03/18 14:46, Johannes Schindelin wrote:
> Hi Ramsay,
> 
> On Mon, 19 Mar 2018, Ramsay Jones wrote:
> 
>> This series removes all 'self-initialised' variables (ie. <type> var =
>> var;).  This construct has been used to silence gcc
>> '-W[maybe-]uninitialized' warnings in the past [1]. Unfortunately, this
>> construct causes warnings to be issued by MSVC [2], along with clang
>> static analysis complaining about an 'Assigned value is garbage or
>> undefined'. The number of these constructs has dropped over the years
>> (eg. see [3] and [4]), so there are currently only 6 remaining in the
>> current codebase. As demonstrated below, 5 of these no longer cause gcc
>> to issue warnings.
> 
> Thank you so much for working on this!

These patches are based on a very old branch (that goes back
at least as far as 2010, see [1]). (I have too many in my repo,
so it will be good to remove this one)!

> In Git for Windows, to work around the MSVC issues you mention, I have
> this ugly work-around (essentially, it has a FAKE_INIT() macro that either
> performs that GCC workaround or initializes the variable to NULL/0):
> 
> 	https://github.com/git-for-windows/git/commit/474155f32a

Oh, wow! (Hmm, actually that doesn't look too bad :-D )

> FWIW I just tested your patches with Visual Studio 2017 and there are no
> C4700 warnings (the equivalent of GCC's "may be uninitialized" warning)
> [*1*].
> 
> You can find the patches (your patches rebased onto Git for Windows'
> master, plus a patch adding the project files for Visual Studio) here:
> 
> https://github.com/git-for-windows/git/compare/master...dscho:msvc-uninitialized-warning-test

Thanks for testing the patches.

> So here is my ACK, in case you want it ;-)

Thanks!

ATB,
Ramsay Jones

[1] https://public-inbox.org/git/4CFA8D4D.2020500@ramsay1.demon.co.uk/