Notes on -Wshadow patches

Notes on -Wshadow patches

[Andrey Smirnov]

I finally finished squashing and reordering patches so here are some
notes, before I start sending them.

* Patches format and quantity

- To preserve a backward compatibility in numbering, I decided not to change
  anything about previously sent 34 patches.

- Since the number of people expressed concern about the number of
  patches they will have to review, I augmented the criteria I used to
  split or condense all the other patches. Now all of them must(ideally)
  satisfy following criteria:
        1. Patch never crosses the boundary of a file
        2. Patch is either a series of identical re-namings across
           multiple functions or a series of disparate changes within a
           boundaries of a single function.
  This "algorithm" still yielded 195[1] patches. Because no one provided me
  with any specific upper boundary, this unintentionally round number
  will have to do for now. Taking into account that all this patch
  juggling is quite tedious and time consuming business, I ask anyone,
  who wants me to do another round of it, to provide either a new
  criterion or an augmentation to the old one from the aforementioned
  list, or, if he or she so pleases, they can go grab the sources from 
  https://github.com/ndreys/gdb/tree/Wshadow-compilation and do it
  themselves. These are the only two choices, any other option I will,
  most likely, decline.

- To not completely loose my mind while reordering and merging the
  patches I allowed myself a luxury of not making an entry in ChangeLog
  file itself, but rather in a git commit message(which will still be a
  part of an e-mail but not the actual diff). This allowed me to
  perform reordering of 300+ patches without it turning into a swearing
  fest. Since, in my understanding, due to constant flow of changes to
  ChangeLog file, it is impossible to generate correct diff and one will
  have to solve merging problems, and put an appropriate date, by hands
  anyway, I don't see any problem with my decision. If there's something
  I'm missing, and the inclusion of ChangeLog contents in a diff is
  unavoidable, let me know, and I will make appropriate changes.

* Some statistics

  The contents below is an excerpt from a larger document available at
  https://gist.github.com/1397264. It you want all the gory details, go
  grab that gist. It is an org-mode file, so I recommend using Emacs to
  view it.

  Since Mark questioned the usability of -Wshadow option I decided to
  look at the full set of data and figure out if it was actually the
  case. I decided to investigate -Wshadow generate errors from two
  aspects: the cause of the clash and the type of it. So I devised the
  following groups of clashes:
  
   Clashes by cause:
   - INDX: Local variable shadows `index'
   - SGNL: Local variable shadows `signal'
   - OIND: Local variables shadow `optind' and `optparse'
   - TEEE: Local variable shadows `tee'
   - LINK: Local variable shadows `link'
   - READ: Local variable shadows `read'
   - WRTE: Local variable shadows `write'
   - BSNM: Local variable shadows `basename'
   - EXPP: Local variable shadows `exp'
   - DUPP: Local variable shadows `dup'
   - SYSC: Local variable shadows `syscall'
   - FSTT: Local variable shadows `fstat'
   - STAT: Local variable shadows `stat'
   - TTNM: Local variable shadows `ttyname'
   - FORK: Local variable shadows `fork'
   - WCTP: Local variable shadows `wctype'
   - ISTY: Local variable shadows `isatty'
   - RWND: Local variable shadows `rewind'
   - FPTS: Local variable shadows `fputs'
   - SYSN: Local variable shadows `sysinfo'
   - ACCS: Local variable shadows `access'
   - MISC: All the other name clashes, local to the GDB code.

   Clashes by type:
   - TYP1: Local variable in a nested scope shadows local variable from outer
     scope. Types do not match.
   - TYP2: Local variable in a nested scope shadows a function(except
     when variable is a pointer to function).
   - TYP3: Local variable or function parameter shadows global variable
     of different type
   - TYP4: Local variable or function parameter shadows type defined with typedef
   - ALBW: Local variable in a nested scope shadows local variable from outer
     scope. Types match or types are very close(i. e. `int'  and `unsigned').
   - BWDH: Local variable or function parameter shadows global variable
     of the same type

   Here's the numbers my investigation produced:

   Table 1: Clashes by cause                     
   |-----------+-----+------------|
   | Cause     |   # |          % |
   |-----------+-----+------------|
   | INDX      |  28 |      17.07 |
   | SGNL      |   7 |       4.27 |
   | OIND      |   7 |       4.27 |
   | ERRR      |   8 |       4.88 |
   | TEEE      |   1 |       0.61 |
   | LINK      |   3 |       1.83 |
   | READ      |   5 |       3.05 |
   | WRTE      |  10 |       6.10 |
   | BSNM      |   3 |       1.83 |
   | EXPP      |   3 |       1.83 | 
   | DUPP      |   1 |       0.61 |
   | SYSC      |   2 |       1.22 |
   | FSTT      |   2 |       1.22 |
   | STAT      |   1 |       0.61 |
   | TTNM      |   1 |       0.61 |
   | FORK      |   1 |       0.61 |
   | WCTP      |   1 |       0.61 |
   | ISTY      |   1 |       0.61 |
   | RWND      |   1 |       0.61 |
   | FPTS      |   1 |       0.61 |
   | SYSN      |   2 |       1.22 |
   | ACCS      |   1 |       0.61 |
   |-----------+-----+------------|
   | Subtotal: |  90 |      32.93 |
   |-----------+-----+------------|
   | MISC      | 110 |      67.07 |
   |-----------+-----+------------|
   | Total:    | 200 |        100 |
   |-----------+-----+------------|

   Table 2: Clashes by type     
   |-----------+-----+------------|
   | Cause     |   # |          % |
   |-----------+-----+------------|
   | TYP1      |  10 |       5.13 |
   | TYP2      | 109 |      55.90 |
   | TYP3      |   2 |       1.03 |
   | TYP4      |   2 |       1.03 |
   | ALBW      |  59 |      30.26 |
   | BWDH      |  13 |       6.67 |
   |-----------+-----+------------|
   | Total:    | 195 |        100 |
   |-----------+-----+------------|


* My conclusions from the data
  - As it can be seen from Table 1, half of the time -Werror reports
    errors related to the GDB source code itself, and has nothing to do
    with header files provided by the platform.
  - At least 1 in 3 detected conflicts was a conflict of either type ALBW or
    BWHD, both of which, I hope we can all agree, are either
    unintentional mistake or a hack too clever for it's own good, which
    makes the later editing of the code into "Operation" game.


Andrey Smirnov

[1] This makes total of 238 patches: 34 previously sent ones + 195
    patches against sources in gdb directory + 8 patches unrelated to
    gdb directory + 1 patch to add -Wshadow to default set of flags.

Re: Notes on -Wshadow patches

[Eli Zaretskii]

> From: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: eliz@gnu.org,
>     brobecker@adacore.com,
>     mark.kettenis@xs4all.nl,
>     tromey@redhat.com
> Date: Sun, 27 Nov 2011 19:27:22 +0700
> 
>   If there's something I'm missing, and the inclusion of ChangeLog
>   contents in a diff is unavoidable, let me know, and I will make
>   appropriate changes.

Doesn't git support generation of ChangeLog entries from commit
messages?  I thought it did.  If it does, you should be able to
generate ChangeLog entries automatically.

> * Some statistics

Thanks for the footwork.

>   - At least 1 in 3 detected conflicts was a conflict of either type ALBW or
>     BWHD, both of which, I hope we can all agree, are either
>     unintentional mistake or a hack too clever for it's own good, which
>     makes the later editing of the code into "Operation" game.

I actually think that both ALBW and BWHD are perfectly valid C code,
and neither unintentional mistakes nor "too clever hacks".  The only
type of clashes that should be fixed are TYP2.

Re: Notes on -Wshadow patches

[Mark Kettenis]

> Date: Sun, 27 Nov 2011 08:07:42 -0500
> From: Eli Zaretskii <eliz@gnu.org>

> >   - At least 1 in 3 detected conflicts was a conflict of either type ALBW or
> >     BWHD, both of which, I hope we can all agree, are either
> >     unintentional mistake or a hack too clever for it's own good, which
> >     makes the later editing of the code into "Operation" game.
> 
> I actually think that both ALBW and BWHD are perfectly valid C code,
> and neither unintentional mistakes nor "too clever hacks".  The only
> type of clashes that should be fixed are TYP2.

It's all perfectly valid C code.  That's not the point.  The point is
that certain types of shadowing are indicative of bugs in the code.
In case of the ALBW type, the potential bug is that the programmer
intends to modify the variable in the outer scope, which doesn't
happen because a variable with the same name exists in a nested scope.
I can remember a few bugs like that in the past decade of GDB
development.  So those are worth fixing.  It's somewhat less likely to
be a bug in the BWDH case, but I think those should be fixed as well
(by renaming the global variable, turning it into a static variable,
or perhaps by getting rid of the global variable completely).  And
fixing the TYP1 and TYP3 cases is probably a good idea as well, since
it will help to make the code more readable.  A -Wshadow option that
only warns about these 4 types would actually be useful.

The TYP2 case will never run the risk of being a bug since the
distinction between a function invocation and variable usage in the C
language is unambigious.

Re: Notes on -Wshadow patches

[Joel Brobecker]

Andrey,

> I finally finished squashing and reordering patches so here are some
> notes, before I start sending them.

It's really great that you've done all this work of giving us a better
overview of where the various problems lie.  It feel a little bad asking
you for more work, but on the othe hand, it would be nice if you said
for each future patch what other entity causes the collision you're
trying to fix. I presume you already know, and that'll make our job
reviewing your changes a little faster.

-- 
Joel

Re: Notes on -Wshadow patches

[Andrey Smirnov]

On Sun, Nov 27, 2011 at 9:14 PM, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>> Date: Sun, 27 Nov 2011 08:07:42 -0500
>> From: Eli Zaretskii <eliz@gnu.org>
>
>> >   - At least 1 in 3 detected conflicts was a conflict of either type ALBW or
>> >     BWHD, both of which, I hope we can all agree, are either
>> >     unintentional mistake or a hack too clever for it's own good, which
>> >     makes the later editing of the code into "Operation" game.
>>
>> I actually think that both ALBW and BWHD are perfectly valid C code,
>> and neither unintentional mistakes nor "too clever hacks".  The only
>> type of clashes that should be fixed are TYP2.
>

> It's all perfectly valid C code.

I completely agree with you, Mark, but my argument was never about
standard compliance of the code. If -Wshadow was about uncovering the
code that is not valid according to the standard, then both not
enabling and enabling of it by default could have potentially broken
the build.

> That's not the point. The point is that certain types of shadowing
> are indicative of bugs in the code. In case of the ALBW type, the
> potential bug is that the programmer intends to modify the variable
> in the outer scope, which doesn't happen because a variable with the
> same name exists in a nested scope. I can remember a few bugs like
> that in the past decade of GDB development.  So those are worth
> fixing.  It's somewhat less likely to be a bug in the BWDH case, but
> I think those should be fixed as well (by renaming the global
> variable, turning it into a static variable, or perhaps by getting
> rid of the global variable completely).  And fixing the TYP1 and
> TYP3 cases is probably a good idea as well, since it will help to
> make the code more readable.  A -Wshadow option that only warns
> about these 4 types would actually be useful.

And again I completely agree with you on all of the above
points(except the -Wshadow hating part :), although I would argue that
both are bugs waiting to happen when someone decides to
re-write/augment those pieces of code and remove inner variable,
because in that case compiler won't say anything about any leftover
references to it. And the case of BWDH is worse from that point of
view since with ALBW you will probably be aware of shadowing.

> The TYP2 case will never run the risk of being a bug since the
> distinction between a function invocation and variable usage in the C
> language is unambigious.

The danger I see in TYP2 clashes is not about accidental "invocation"
of variable. Of course it would be reported be compiler even without
-Wshadow. But what about the following example:

#include <stdlib.h>
#include <stdio.h>

void foo(void)
{
  printf ("And now I am a function\n");
}

struct container_element {
  /*
     ...


     Some stuff related to
     inner workings of a container

     ...
  */
  void *data;
};

int main(int argc, char *argv[])
{
  struct container_element elmnt;
  {
    char foo[42];

    /*
      ...

      Lots and lots of code

      ....
    */

    /* Now I am a pointer to a buffer */
    elmnt.data = foo;
  }

  elmnt.data = foo;

  ((void (*) (void))elmnt.data)();

  return 0;
}

How is it not of a TYP2 clash and if it is, is it not as dangerous as BWDH
type?

Andrey Smirnov

Re: Notes on -Wshadow patches

[Andrey Smirnov]

On Mon, Nov 28, 2011 at 12:59 AM, Joel Brobecker <brobecker@adacore.com> wrote:
> Andrey,
>
>> I finally finished squashing and reordering patches so here are some
>> notes, before I start sending them.
>
> It's really great that you've done all this work of giving us a better
> overview of where the various problems lie.  It feel a little bad asking
> you for more work,

Well if it has to be done than it has to be done. I started the whole
-Wshadow revolution, I hope I'll be the one finishing it. Besides
there's also a chance of guilt-tripping you into accepting -Wshadow by
default :-D

> but on the othe hand, it would be nice if you said
> for each future patch what other entity causes the collision you're
> trying to fix.

The ones that related to collisions with definitions in platform
provided *.h, in other words all non MISC commits, they are grouped
and tagged and will have it(tag) in a message subject, there are 90 of
those plus there are 34 previously sent patches so I think we have
enough to deal with, for now. For all MISC commits, I'll probably add a
short description in commit message later.

> I presume you already know, and that'll make our job
> reviewing your changes a little faster.

There are still 195 of them, so oftentimes, I just don't remeber. I have
all the data on which type TYP1..TYP4, BWDH, ALBW, they belong, but the
exact reason of a clash for MISC commits, no, I don't. I guess I'll
remember for some and the others I'll revert and hope gcc will give me
clear enough message.

Andrey Smirnov

Re: Notes on -Wshadow patches

[Andrey Smirnov]

> Doesn't git support generation of ChangeLog entries from commit
> messages?  I thought it did.  If it does, you should be able to
> generate ChangeLog entries automatically.
>

Not that that I'm an exprt on git, but, no, I am unaware of such
functionality. But it doesn't matter, since, as I said, commit
messages already contain what is to be later added to the changelog, i
just didn't add that to the actual ChangeLog file and therefore it is
not in the diff.

Andrey Smirnov

Re: Notes on -Wshadow patches

[Joel Brobecker]

> Not that that I'm an exprt on git, but, no, I am unaware of such
> functionality. But it doesn't matter, since, as I said, commit
> messages already contain what is to be later added to the changelog, i
> just didn't add that to the actual ChangeLog file and therefore it is
> not in the diff.

You don't need to worry about this - ChangeLog entries do not need
to be included in the patch. All they do is create conflicts when
someone tries to apply your patch to his tree.

-- 
Joel