[PATCH 18/348] Fix -Wsahdow warnings

[PATCH 18/348] Fix -Wsahdow warnings

[Andrey Smirnov]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Fix -Wshadow warnings --]
[-- Type: text/x-diff, Size: 1313 bytes --]

From 6347add7f352c5e9994f7a5de938e8a7dacc98f9 Mon Sep 17 00:00:00 2001
From: Andrey Smirnov <andrew.smirnov@gmail.com>
Date: Tue, 22 Nov 2011 17:25:56 +0700
Subject: [PATCH 18/39] Fix -Wshadow warnings.

* amd64-linux-tdep.c (amd64_canonicalize_syscall): Fix -Wshadow
	warnings.
---
 gdb/ChangeLog          |    5 +++++
 gdb/amd64-linux-tdep.c |    4 ++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 54284cf..7f049a8 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2011-11-22  Andrey Smirnov <andrew.smirnov@gmail.com>
+
+	* amd64-linux-tdep.c (amd64_canonicalize_syscall): Fix -Wshadow
+	warnings.
+
 2011-11-19  Andrey Smirnov <andrew.smirnov@gmail.com>
 
 	* ada-valprint.c (ada_val_print_1): Fix -Wshadow warnings.
diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
index 0119838..84b58a0 100644
--- a/gdb/amd64-linux-tdep.c
+++ b/gdb/amd64-linux-tdep.c
@@ -339,9 +339,9 @@ amd64_all_but_ip_registers_record (struct regcache *regcache)
    process record.  */
 
 static enum gdb_syscall
-amd64_canonicalize_syscall (enum amd64_syscall syscall)
+amd64_canonicalize_syscall (enum amd64_syscall syscall_number)
 {
-  switch (syscall) {
+  switch (syscall_number) {
   case amd64_sys_read:
     return gdb_sys_read;
 
-- 
1.7.5.4

Re: [PATCH 18/348] Fix -Wsahdow warnings

[Tom Tromey]

>>>>> "Andrey" == Andrey Smirnov <andrew.smirnov@gmail.com> writes:

Andrey> * amd64-linux-tdep.c (amd64_canonicalize_syscall): Fix -Wshadow
Andrey> 	warnings.

Ok.

Tom

Re: [PATCH 18/348] Fix -Wsahdow warnings

[Mark Kettenis]

> From: Andrey Smirnov <andrew.smirnov@gmail.com>
> Date: Tue, 22 Nov 2011 20:01:18 +0700
> 
> >From 6347add7f352c5e9994f7a5de938e8a7dacc98f9 Mon Sep 17 00:00:00 2001
> From: Andrey Smirnov <andrew.smirnov@gmail.com>
> Date: Tue, 22 Nov 2011 17:25:56 +0700
> Subject: [PATCH 18/39] Fix -Wshadow warnings.
> 
> * amd64-linux-tdep.c (amd64_canonicalize_syscall): Fix -Wshadow
> 	warnings.

Why the hell does -Wshadow complain here?

> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 54284cf..7f049a8 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,8 @@
> +2011-11-22  Andrey Smirnov <andrew.smirnov@gmail.com>
> +
> +	* amd64-linux-tdep.c (amd64_canonicalize_syscall): Fix -Wshadow
> +	warnings.
> +
>  2011-11-19  Andrey Smirnov <andrew.smirnov@gmail.com>
>  
>  	* ada-valprint.c (ada_val_print_1): Fix -Wshadow warnings.
> diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
> index 0119838..84b58a0 100644
> --- a/gdb/amd64-linux-tdep.c
> +++ b/gdb/amd64-linux-tdep.c
> @@ -339,9 +339,9 @@ amd64_all_but_ip_registers_record (struct regcache *regcache)
>     process record.  */
>  
>  static enum gdb_syscall
> -amd64_canonicalize_syscall (enum amd64_syscall syscall)
> +amd64_canonicalize_syscall (enum amd64_syscall syscall_number)
>  {
> -  switch (syscall) {
> +  switch (syscall_number) {
>    case amd64_sys_read:
>      return gdb_sys_read;
>  
> -- 
> 1.7.5.4
> 
> 

Re: [PATCH 18/348] Fix -Wsahdow warnings

[Ulrich Weigand]

Mark Kettenis wrote:

> > From: Andrey Smirnov <andrew.smirnov@gmail.com>
> > Date: Tue, 22 Nov 2011 17:25:56 +0700
> > Subject: [PATCH 18/39] Fix -Wshadow warnings.
> > 
> > * amd64-linux-tdep.c (amd64_canonicalize_syscall): Fix -Wshadow
> > 	warnings.
> 
> Why the hell does -Wshadow complain here?

> > -amd64_canonicalize_syscall (enum amd64_syscall syscall)
> > +amd64_canonicalize_syscall (enum amd64_syscall syscall_number)

I'd expect this is because the parameter "syscall" shadows the global
function declaration "syscall" provided by glibc headers:

/usr/include/unistd.h:extern long int syscall (long int __sysno, ...) __THROW;

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [PATCH 18/348] Fix -Wsahdow warnings

[Pedro Alves]

On Wednesday 23 November 2011 16:40:41, Ulrich Weigand wrote:
> Mark Kettenis wrote:
> 
> > > From: Andrey Smirnov <andrew.smirnov@gmail.com>
> > > Date: Tue, 22 Nov 2011 17:25:56 +0700
> > > Subject: [PATCH 18/39] Fix -Wshadow warnings.
> > > 
> > > * amd64-linux-tdep.c (amd64_canonicalize_syscall): Fix -Wshadow
> > > 	warnings.
> > 
> > Why the hell does -Wshadow complain here?
> 
> > > -amd64_canonicalize_syscall (enum amd64_syscall syscall)
> > > +amd64_canonicalize_syscall (enum amd64_syscall syscall_number)
> 
> I'd expect this is because the parameter "syscall" shadows the global
> function declaration "syscall" provided by glibc headers:
> 
> /usr/include/unistd.h:extern long int syscall (long int __sysno, ...) __THROW;

Yeah, this is unfortunate because it means you trigger different 
shadows on different hosts, or by configuring gdb differently.

There was this gcc patch

 http://comments.gmane.org/gmane.comp.gcc.patches/244771

to stop -Wshadow from complaning about shadowing of symbols in system
headers, but it doesn't seem to have been applied, though it was okayed.

-- 
Pedro Alves

Re: [PATCH 18/348] Fix -Wsahdow warnings

[Joel Brobecker]

> There was this gcc patch
> 
>  http://comments.gmane.org/gmane.comp.gcc.patches/244771
> 
> to stop -Wshadow from complaning about shadowing of symbols in system
> headers, but it doesn't seem to have been applied, though it was okayed.

I'll send Alan a ping...

That being said, because some compilers can warn, I think it might be
best to avoid the clash anyways.

-- 
Joel

Re: [PATCH 18/348] Fix -Wsahdow warnings

[Mark Kettenis]

> From: Pedro Alves <pedro@codesourcery.com>
> Date: Wed, 23 Nov 2011 18:20:40 +0000
> 
> On Wednesday 23 November 2011 16:40:41, Ulrich Weigand wrote:
> > Mark Kettenis wrote:
> > 
> > > > From: Andrey Smirnov <andrew.smirnov@gmail.com>
> > > > Date: Tue, 22 Nov 2011 17:25:56 +0700
> > > > Subject: [PATCH 18/39] Fix -Wshadow warnings.
> > > > 
> > > > * amd64-linux-tdep.c (amd64_canonicalize_syscall): Fix -Wshadow
> > > > 	warnings.
> > > 
> > > Why the hell does -Wshadow complain here?
> > 
> > > > -amd64_canonicalize_syscall (enum amd64_syscall syscall)
> > > > +amd64_canonicalize_syscall (enum amd64_syscall syscall_number)
> > 
> > I'd expect this is because the parameter "syscall" shadows the global
> > function declaration "syscall" provided by glibc headers:
> > 
> > /usr/include/unistd.h:extern long int syscall (long int __sysno, ...) __THROW;
> 
> Yeah, this is unfortunate because it means you trigger different 
> shadows on different hosts, or by configuring gdb differently.

Indeed.  And I'd say this means we can't add -Wshadow to the set of
default flags for compiling gdb.

> There was this gcc patch
> 
>  http://comments.gmane.org/gmane.comp.gcc.patches/244771
> 
> to stop -Wshadow from complaning about shadowing of symbols in system
> headers, but it doesn't seem to have been applied, though it was okayed.

I'd argue that -Wshadow should never warn about a local variable
shadowing a function.  There'sno chance such shadowing is
unintentional.

It seems a lot of the changes posted up until now are dealing with
that type of "conflict".  I'd really like to see those dropped from
this set.

Re: [PATCH 18/348] Fix -Wsahdow warnings

[Andrey Smirnov]

On Thu, Nov 24, 2011 at 3:23 AM, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>> From: Pedro Alves <pedro@codesourcery.com>
>> Date: Wed, 23 Nov 2011 18:20:40 +0000
>>
>> On Wednesday 23 November 2011 16:40:41, Ulrich Weigand wrote:
>> > Mark Kettenis wrote:
>> >
>> > > > From: Andrey Smirnov <andrew.smirnov@gmail.com>
>> > > > Date: Tue, 22 Nov 2011 17:25:56 +0700
>> > > > Subject: [PATCH 18/39] Fix -Wshadow warnings.
>> > > >
>> > > > * amd64-linux-tdep.c (amd64_canonicalize_syscall): Fix -Wshadow
>> > > >         warnings.
>> > >
>> > > Why the hell does -Wshadow complain here?
>> >
>> > > > -amd64_canonicalize_syscall (enum amd64_syscall syscall)
>> > > > +amd64_canonicalize_syscall (enum amd64_syscall syscall_number)
>> >
>> > I'd expect this is because the parameter "syscall" shadows the global
>> > function declaration "syscall" provided by glibc headers:
>> >
>> > /usr/include/unistd.h:extern long int syscall (long int __sysno, ...) __THROW;
>>
>> Yeah, this is unfortunate because it means you trigger different
>> shadows on different hosts, or by configuring gdb differently.
>
> Indeed.  And I'd say this means we can't add -Wshadow to the set of
> default flags for compiling gdb.

IMHO,
not setting the -Wshadow variable to the default set would only be
inviting for such warnings to accumulate and grow into snowball of
little instances where aforementioned rule has been violated, that
would require yet another 300+ patches to clean it up. Doing so would
be plugging the wholes instead of fixing the problem.

I'll admit you annoyed me real good when you started kicking all my
toys and called them stupid, and I apologize if my behavior is
childish, but now I see your argument as this:
"We can't have all the pretty names on for our variables on all the
platforms because of the -Wshadow, let's not use -Wshadow", maybe I'll
cool down and see the merit of your argument. Now I don't.

>> There was this gcc patch
>>
>>  http://comments.gmane.org/gmane.comp.gcc.patches/244771
>>
>> to stop -Wshadow from complaning about shadowing of symbols in system
>> headers, but it doesn't seem to have been applied, though it was okayed.
>
> I'd argue that -Wshadow should never warn about a local variable
> shadowing a function.  There'sno chance such shadowing is
> unintentional.

"There'sno chance such shadowing is unintentional." is either an
assumed conclusion or an argument from incredulity both of which are
logical fallacies, but even if the shadowing is always intentional I
still fail to see your point. You are trying to use the name that has
already been taken, since both the names of the functions and
variables reside in the same namespace it is an name clash. There is,
to the best of my knowledge, no means in the C language to help
compiler to resolve this ambiguity and when you ask compiler to slap
you for any naming ambiguities with -Wshadow you get what you asked
for. Why shouldn't it be so?

> It seems a lot of the changes posted up until now are dealing with
> that type of "conflict".  I'd really like to see those dropped from
> this set.
>

It is not a "conflict". It is a conflict. Dropping such changes would
make it impossible to test wither the whole goal of the patch-set,
that is building with -Wshadow enabled, has been achieved.

Andrey Smirnov

Re: [PATCH 18/348] Fix -Wsahdow warnings

[Eli Zaretskii]

> Date: Thu, 24 Nov 2011 10:16:14 +0600
> From: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: gdb-patches <gdb-patches@sourceware.org>
> 
> > It seems a lot of the changes posted up until now are dealing with
> > that type of "conflict".  I'd really like to see those dropped from
> > this set.
> >
> 
> It is not a "conflict". It is a conflict.

I'm with Andrey on this one.  Catching variables that clash with
well-known global identifiers, such as library functions, are about
the only good reason for using -Wsahdow; all the other kinds of
"shadowing" it flags are usually perfectly correct usage of C.

(I was actually bitten once by a mysterious bug caused by a variable
whose name was identical to an external symbol that came from a
library.  I don't wish anybody to get into such a conundrum.)

Re: [PATCH 18/348] Fix -Wsahdow warnings

[Joel Brobecker]

Andrey,

I understand your fustration towards the tone of some of the messages.
Hopefully things will be better from now on.

And I agree that not enabling -Wshadow by default will let the number
of such conflict increase again over time. But at the same time, now
that I am seeing the changes that are required to fix these, I am not
very happy either.  I mean, I understand that "index" might be part of
a system's include. But "block_found" (or was it "found_block") seems
quite surprising. Add the fact that includes and compiler vary from
system to system, and we're not sure that once clean on one machine,
it'll be clean everywhere else. All of this to fix warnings that,
as far as I could tell for the most part, did not indicate an actual
bug in the code.

This is why I am left wondering (meaning I haven't decided yet)
whether the idea of enabling -Wshadow was such a good idea after
all. I know that looking at the warnings allowed you to spot some
areas where there definitely is a mistake, and so that's useful.
I'm not disputing that. But I'm not convinced by a good number of
the patches I've seen, and I still haven't decided whether to
accept the situation and approve them, or not. For that, I asked
everyone else' opinion.

-- 
Joel

Re: [PATCH 18/348] Fix -Wsahdow warnings

[Andrey Smirnov]

On Fri, Nov 25, 2011 at 5:00 AM, Joel Brobecker <brobecker@adacore.com> wrote:
> Andrey,
>
> I understand your fustration towards the tone of some of the messages.
> Hopefully things will be better from now on.
>
> And I agree that not enabling -Wshadow by default will let the number
> of such conflict increase again over time. But at the same time, now
> that I am seeing the changes that are required to fix these, I am not
> very happy either.  I mean, I understand that "index" might be part of
> a system's include.

> But "block_found" (or was it "found_block") seems
> quite surprising.

Thanks for bringing that up, that is, IMHO, an excellent example.
The `block_found' in function conflicts with global variable declared
in symtab.c and it is not that just their names match, types are very
similar too one is `struct block **' another is `const struct *block'.

But for the sake of argument let's assume that this will never cause
any bugs. I say this shadowing still should be resolved because it
makes source code, at least in my experience, harder to understand: on
one hand in `ada_lookup_encoded_symbol' `block_found' is function
parameter used to return result, on the other hand in
`standard_lookup' it is a global variable from symtab.c, rather confusing.

I say the whole situation is exactly why -Wshadow should be enabled.

> Add the fact that includes and compiler vary from
> system to system, and we're not sure that once clean on one machine,
> it'll be clean everywhere else. All of this to fix warnings that,
> as far as I could tell for the most part, did not indicate an actual
> bug in the code.

I've been humbled by the compiler and the machine too many times to
think my bug spotting capabilities for all intents and purposes are
any greater than zero, so my perspective at this is buggy until
formally proven to be bug-free. And from my view your argument sounds
really different: It means that we potentially can have a bug that is
only reproducible on a certain mix of a compiler and the platform, now
I don't mind fixing very well reproducible bugs, it this kind of
once-in-a-blue-moon kind of bugs that I hate very passionately.

> This is why I am left wondering (meaning I haven't decided yet)
> whether the idea of enabling -Wshadow was such a good idea after
> all. I know that looking at the warnings allowed you to spot some
> areas where there definitely is a mistake, and so that's useful.
> I'm not disputing that.

But that tool is only available if the source tree in question can be
compiled with -Wshadow, otherwise it is "checking for possible mistakes
up until, in the order of source file compilation, the first
legitimate shadowing".

> But I'm not convinced by a good number of the patches I've seen, and
> I still haven't decided whether to accept the situation and approve
> them, or not. For that, I asked everyone else' opinion.

Well, now that I made my case, I hope you reserve negative judgment
until the whole patchset is presented.

Andrey Smirnov.

Re: [PATCH 18/348] Fix -Wsahdow warnings

[Joel Brobecker]

> Thanks for bringing that up, that is, IMHO, an excellent example.
> The `block_found' in function conflicts with global variable declared
> in symtab.c and it is not that just their names match, types are very
> similar too one is `struct block **' another is `const struct *block'.

Hmmm, I don't know how I missed that, as I grep'ed the source code.
Or so I thought. Maybe a typo.

In this particular case, yes, I agree, we should do something. We might
prefer to rename the global variable, though; I think that a global
variable with that name is bizarre.

As to the final decision of enabling -Wshadow by default, I agree
a little more to the idea, although not quite convinced yet. So far,
Eli is pro. Mark is against. I'm 50/50. Unless we get more feedback
from other GMs, you've done the work, we could at least try it and
see where that gets us.

-- 
Joel

About adding -Wshadow option by default (was Re: [PATCH 18/348] Fix -Wsahdow warnings)

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Joel Brobecker
> Envoyé : vendredi 25 novembre 2011 15:26
> À : Andrey Smirnov
> Cc : gdb-patches
> Objet : Re: [PATCH 18/348] Fix -Wsahdow warnings
> 
> > Thanks for bringing that up, that is, IMHO, an excellent example.
> > The `block_found' in function conflicts with global variable declared
> > in symtab.c and it is not that just their names match, types are very
> > similar too one is `struct block **' another is `const struct *block'.
> 
> Hmmm, I don't know how I missed that, as I grep'ed the source code.
> Or so I thought. Maybe a typo.
> 
> In this particular case, yes, I agree, we should do something. We might
> prefer to rename the global variable, though; I think that a global
> variable with that name is bizarre.
> 
> As to the final decision of enabling -Wshadow by default, I agree
> a little more to the idea, although not quite convinced yet. So far,
> Eli is pro. Mark is against. I'm 50/50. Unless we get more feedback
> from other GMs, you've done the work, we could at least try it and
> see where that gets us.

As you might have noticed from my last errorneous email,
-Wshadow is already used by default for Binutils
and the cases where it leads to problems are not that common
(even though my email was just about one of those cases
where a system specific function shadows a local variable...)

  Anyhow, as it seems to indeed avoid some overlap that could lead
to hard to debug errors, I would also be in favor of enabling this,
even though my vote shouldn't really have any weight here...

Pierre Muller

Re: About adding -Wshadow option by default (was Re: [PATCH 18/348] Fix -Wsahdow warnings)

[Mark Kettenis]

> From: "Pierre Muller" <pierre.muller@ics-cnrs.unistra.fr>
> Date: Fri, 25 Nov 2011 16:52:24 +0100
> 
> As you might have noticed from my last errorneous email,
> -Wshadow is already used by default for Binutils
> and the cases where it leads to problems are not that common
> (even though my email was just about one of those cases
> where a system specific function shadows a local variable...)

But binutils has far less code that pulls in far-less non standard
system-specific headers than GDB.

Re: [PATCH 18/348] Fix -Wsahdow warnings

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> As to the final decision of enabling -Wshadow by default, I agree
Joel> a little more to the idea, although not quite convinced yet. So far,
Joel> Eli is pro. Mark is against. I'm 50/50. Unless we get more feedback
Joel> from other GMs, you've done the work, we could at least try it and
Joel> see where that gets us.

I'm generally in favor of it.

I think it is unfortunate that it causes us to rename variables when
they clash in a way that is unimportant in practice.  This is the
function-version-scalar issue that Mark points out.

However, I have also run across code in gdb where there is local
shadowing which has confused me (no example at hand though, sorry).  I
would like to see these cleaned up.

On balance I don't really care that we have to rename some variables in
order to get rid of the bad code.  There are plenty of names that are
equally clear as the ones already in the code.

Could we possibly mandate that -Wshadow only be used with a GCC that has
Alan Modra's patch in it?

Joel pinged it, and it went in, though I didn't see the actual patch:

    http://gcc.gnu.org/ml/gcc-patches/2011-11/msg02340.html

Still, what it does is prevent the warning when shadowing something from
a system header.  This seems decent to me and in particular will, I
think, largely address Mark's concerns.

All we'd need then is a bit of configury.

Tom

Re: [PATCH 18/348] Fix -Wsahdow warnings

[Andrey Smirnov]

On Wed, Nov 30, 2011 at 2:17 AM, Tom Tromey <tromey@redhat.com> wrote:
> Could we possibly mandate that -Wshadow only be used with a GCC that has
> Alan Modra's patch in it?
>
> Joel pinged it, and it went in, though I didn't see the actual patch:
>
>    http://gcc.gnu.org/ml/gcc-patches/2011-11/msg02340.html
>

I know I probably should go to GCC mailing list and ask that question
there, but anyways, would this patch cause gcc to stop generating the
warning about local variable shadowing global one from system headers?

The reason I'm asking is because there are several instances of said
shadowing in GDB, for example
https://github.com/ndreys/gdb/commit/bf87d6034f7093aa207a1b14233be48214a6c3d8
(please ignore the erroneous description, for some unknown reason I
call `optarg' `optparse')

I hope that kinds of shadowing would still be detectable even with this
patch applied.


> Still, what it does is prevent the warning when shadowing something from
> a system header.  This seems decent to me and in particular will, I
> think, largely address Mark's concerns.
>

It would pretty much solve that problem, yes, but still it would
divide patch submitters into two groups those who have newest gcc and
-Wshadow enabled by default, and those who don't. And the people
without -Wshadow enabled compilers would be, on occasion, breaking the
build because they have no means to check for -Wshadow caused errors.
I hope I missing something and it is not the case, but that how the
things seems to me now.

Andrey Smirnov

Re: [PATCH 18/348] Fix -Wsahdow warnings

[Tom Tromey]

>>>>> "Andrey" == Andrey Smirnov <andrew.smirnov@gmail.com> writes:

Andrey> I know I probably should go to GCC mailing list and ask that question
Andrey> there, but anyways, would this patch cause gcc to stop generating the
Andrey> warning about local variable shadowing global one from system headers?

Yeah, that's what it does.

Andrey> I hope that kinds of shadowing would still be detectable even with this
Andrey> patch applied.

Why is that?

I do think the GCC patch could probably be better.  That is, I think a
slightly different rule would be an improvement, something like
"generally do not warn about shadowing things declared in system
headers, except if a local function pointer variable shadows a system
function".  That would eliminate the potential for some kinds of bugs.

Tom> Still, what it does is prevent the warning when shadowing something from
Tom> a system header.  This seems decent to me and in particular will, I
Tom> think, largely address Mark's concerns.

Andrey> It would pretty much solve that problem, yes, but still it would
Andrey> divide patch submitters into two groups those who have newest gcc and
Andrey> -Wshadow enabled by default, and those who don't. And the people
Andrey> without -Wshadow enabled compilers would be, on occasion, breaking the
Andrey> build because they have no means to check for -Wshadow caused errors.
Andrey> I hope I missing something and it is not the case, but that how the
Andrey> things seems to me now.

Yes, I think it would result in some periodic breakage until the newer
GCC is widely distributed.  I'm willing to put up with that.  We already
put up with it, in a way, due to other GCC differences... see the
uninitialized variable patches or FORTIFY_SOURCE patches that go in from
time to time.

If the configury part is set up properly, then this warning will simply
auto-enable for people when they upgrade GCC.  So, it isn't like we'll
just forget about it; more like at some point we'll all be joining in :)

Tom

Re: [PATCH 18/348] Fix -Wsahdow warnings

[Andrey Smirnov]

On Wed, Nov 30, 2011 at 9:59 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Andrey" == Andrey Smirnov <andrew.smirnov@gmail.com> writes:
>
> Andrey> I hope that kinds of shadowing would still be detectable even with this
> Andrey> patch applied.
>
> Why is that?

Because when somebody decides to modify such a code and rename/remove
local variables, compiler will not warn them about any leftover pieces
of code referencing now renamed/non-existent variable. And on some
occasions that would lead to introduction of weird, not so easy to
reproduce bugs.

Although generally I think functionality introduced by that patch
should have been optional. In my opinion C gives one enough of an
arsenal to shoot their own foot, so while Mark is adamant that scalar
variables will never be the cause of a bug, I remain unconvinced and
still think that even such unlikely cause is still a cause. But then
again it is just my opinion, I do not have real world examples, and it
looks like my synthetic ones are not very persuasive.

> Andrey> It would pretty much solve that problem, yes, but still it would
> Andrey> divide patch submitters into two groups those who have newest gcc and
> Andrey> -Wshadow enabled by default, and those who don't. And the people
> Andrey> without -Wshadow enabled compilers would be, on occasion, breaking the
> Andrey> build because they have no means to check for -Wshadow caused errors.
> Andrey> I hope I missing something and it is not the case, but that how the
> Andrey> things seems to me now.
>
> Yes, I think it would result in some periodic breakage until the newer
> GCC is widely distributed.  I'm willing to put up with that.  We already
> put up with it, in a way, due to other GCC differences... see the
> uninitialized variable patches or FORTIFY_SOURCE patches that go in from
> time to time.
>

But those are pretty much the results we would get with enabling
-Wshadow on all versions of gcc but on a per-platform basis.

BTW I'm just playing Devil's advocate here, IMHO, your solution is
still better than no -Wshadow at all.

> If the configury part is set up properly, then this warning will simply
> auto-enable for people when they upgrade GCC.  So, it isn't like we'll
> just forget about it; more like at some point we'll all be joining in :)

I wish this world would have less small, city-local companies, doing
embedded development whose version of GCC is fixed in stone because
the whole building system is so obscure to anyone that no one dares to
introduce any changes to it, and usually no one even cares. I used to
work in such a place, so I know that some people will be left behind.
:-)

Andrey Smirnov

Re: [PATCH 18/348] Fix -Wsahdow warnings

[Tom Tromey]

>>>>> "Andrey" == Andrey Smirnov <andrew.smirnov@gmail.com> writes:

>> If the configury part is set up properly, then this warning will simply
>> auto-enable for people when they upgrade GCC.  So, it isn't like we'll
>> just forget about it; more like at some point we'll all be joining in :)

Andrey> I wish this world would have less small, city-local companies, doing
Andrey> embedded development whose version of GCC is fixed in stone because
Andrey> the whole building system is so obscure to anyone that no one dares to
Andrey> introduce any changes to it, and usually no one even cares. I used to
Andrey> work in such a place, so I know that some people will be left behind.
Andrey> :-)

Yeah, but due to the configure check, this warning won't be enabled for
them.  Maybe it would be a problem if they submitted a patch, but I
don't think it is really worth worrying about.

Tom

Re: [PATCH 18/348] Fix -Wsahdow warnings

[Eli Zaretskii]

> Date: Thu, 24 Nov 2011 14:00:57 -0800
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: Mark Kettenis <mark.kettenis@xs4all.nl>,	gdb-patches <gdb-patches@sourceware.org>
> 
> I mean, I understand that "index" might be part of a system's
> include. But "block_found" (or was it "found_block") seems quite
> surprising.

If it surprised you, it means -Wsahdow did its job well.

IOW, it's easy to avoid well-known names like `printf' or `strchr'.
It's the not-so-well-known names that give you hell and high water.

> Add the fact that includes and compiler vary from system to system,
> and we're not sure that once clean on one machine, it'll be clean
> everywhere else.

But this is true of any other non-trivial piece of our code.  E.g., if
you declare a variable `long' expecting it to be a 64-bit type, this
will break on MS-Windows, and you will never no until you actually try
such a compilation.

In general, we have no bullet-proof way of making sure our code works
on all supported platforms, except by actually compiling it on those
platforms.  That's why platforms which lose their area maintainers
bit-rot quite quickly.  This compiler switch doesn't change this
situation in any way.

> All of this to fix warnings that, as far as I could tell for the
> most part, did not indicate an actual bug in the code.

It's a bug waiting to happen, though.  That it didn't happen yet is
just sheer luck.

Re: [PATCH 18/348] Fix -Wsahdow warnings

[Mark Kettenis]

> Date: Fri, 25 Nov 2011 14:02:55 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > Date: Thu, 24 Nov 2011 14:00:57 -0800
> > From: Joel Brobecker <brobecker@adacore.com>
> > Cc: Mark Kettenis <mark.kettenis@xs4all.nl>,	gdb-patches <gdb-patches@sourceware.org>
> > 
> > I mean, I understand that "index" might be part of a system's
> > include. But "block_found" (or was it "found_block") seems quite
> > surprising.
> 
> If it surprised you, it means -Wsahdow did its job well.

No it didn't.  It does a poor job, by flagging cases that can never
ever cause problems.  A local variable "shadowing" a global function
name will never be a problem, because something like:

void foo(void);

void
bar(void)
{
	int foo;

	foo();
}

simply won't compile, even if you don't use -Werror.

> IOW, it's easy to avoid well-known names like `printf' or `strchr'.
> It's the not-so-well-known names that give you hell and high water.

Exactly.  And since we enable -Werror by default, -Wshadow will break
GDB builds for no good reason.  And remove some perfectly usable and
meaningful variable names.

> > Add the fact that includes and compiler vary from system to system,
> > and we're not sure that once clean on one machine, it'll be clean
> > everywhere else.
> 
> But this is true of any other non-trivial piece of our code.  E.g., if
> you declare a variable `long' expecting it to be a 64-bit type, this
> will break on MS-Windows, and you will never no until you actually try
> such a compilation.

Well, that would be a wrong assumption on any sane 32-bit platform as
well, so that issue could be caught on most platforms that GDB runs on.

> In general, we have no bullet-proof way of making sure our code works
> on all supported platforms, except by actually compiling it on those
> platforms.  That's why platforms which lose their area maintainers
> bit-rot quite quickly.  This compiler switch doesn't change this
> situation in any way.

I disagree.  -Wshadow is a game changer because actually compiling on
*all* plaforms is the only way to make sure that I don't actually
break anything.  That's extremely bad!

> > All of this to fix warnings that, as far as I could tell for the
> > most part, did not indicate an actual bug in the code.

Absolutely!

> It's a bug waiting to happen, though.  That it didn't happen yet is
> just sheer luck.

Not really.  Most people that contribute to GDB are fairly competent
programmers.

Re: [PATCH 18/348] Fix -Wsahdow warnings

[Eli Zaretskii]

> Date: Fri, 25 Nov 2011 16:11:11 +0100 (CET)
> From: Mark Kettenis <mark.kettenis@xs4all.nl>
> CC: brobecker@adacore.com, andrew.smirnov@gmail.com,
>         gdb-patches@sourceware.org
> 
> > If it surprised you, it means -Wsahdow did its job well.
> 
> No it didn't.  It does a poor job, by flagging cases that can never
> ever cause problems.  A local variable "shadowing" a global function
> name will never be a problem, because something like:
> 
> void foo(void);
> 
> void
> bar(void)
> {
> 	int foo;
> 
> 	foo();
> }
> 
> simply won't compile, even if you don't use -Werror.

But the patches suggested by Andrey didn't find any instances of the
above.  So I submit that this danger is purely theoretical at this
point, or at least sufficiently rare in GDB to render this
consideration not important in practice for us.

> > IOW, it's easy to avoid well-known names like `printf' or `strchr'.
> > It's the not-so-well-known names that give you hell and high water.
> 
> Exactly.  And since we enable -Werror by default, -Wshadow will break
> GDB builds for no good reason.

Clashing with library symbols _is_ a good reason to prevent GDB from
building.

> And remove some perfectly usable and meaningful variable names.

They can be easily replaced by no less meaningful and usable ones.

> > > Add the fact that includes and compiler vary from system to system,
> > > and we're not sure that once clean on one machine, it'll be clean
> > > everywhere else.
> > 
> > But this is true of any other non-trivial piece of our code.  E.g., if
> > you declare a variable `long' expecting it to be a 64-bit type, this
> > will break on MS-Windows, and you will never no until you actually try
> > such a compilation.
> 
> Well, that would be a wrong assumption on any sane 32-bit platform as
> well, so that issue could be caught on most platforms that GDB runs on.

What about the assumption that a long is as wide as a pointer?  Don't
dismiss a principle just because one of its examples can be easily
refuted.

> > In general, we have no bullet-proof way of making sure our code works
> > on all supported platforms, except by actually compiling it on those
> > platforms.  That's why platforms which lose their area maintainers
> > bit-rot quite quickly.  This compiler switch doesn't change this
> > situation in any way.
> 
> I disagree.  -Wshadow is a game changer because actually compiling on
> *all* plaforms is the only way to make sure that I don't actually
> break anything.  That's extremely bad!

The same is true of any code you add or change, with the possible
exception of very trivial changes.

> Most people that contribute to GDB are fairly competent programmers.

Being competent doesn't mean being clairvoyant.  How can we trust
ourselves to know by heart every single symbol in the standard
libraries?

Re: [PATCH 18/348] Fix -Wsahdow warnings

[Mark Kettenis]

> Date: Fri, 25 Nov 2011 17:41:02 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > Date: Fri, 25 Nov 2011 16:11:11 +0100 (CET)
> > From: Mark Kettenis <mark.kettenis@xs4all.nl>
> > 
> > > If it surprised you, it means -Wsahdow did its job well.
> > 
> > No it didn't.  It does a poor job, by flagging cases that can never
> > ever cause problems.  A local variable "shadowing" a global function
> > name will never be a problem, because something like:
> > 
> > void foo(void);
> > 
> > void
> > bar(void)
> > {
> > 	int foo;
> > 
> > 	foo();
> > }
> > 
> > simply won't compile, even if you don't use -Werror.
> 
> But the patches suggested by Andrey didn't find any instances of the
> above.  So I submit that this danger is purely theoretical at this
> point, or at least sufficiently rare in GDB to render this
> consideration not important in practice for us.

You're missing my point.

A significant number of Andrey's changes (all-but-one of the ones I
looked at) rename *local* variables because they have the same name as
a function.  My example above tries to show you that there is
absolutely no problem with that, because if one wouldf accidentally
try to invoke the function that has the same name as the local
variable, the compiler would already generate an error.

> > > IOW, it's easy to avoid well-known names like `printf' or `strchr'.
> > > It's the not-so-well-known names that give you hell and high water.
> > 
> > Exactly.  And since we enable -Werror by default, -Wshadow will break
> > GDB builds for no good reason.
> 
> Clashing with library symbols _is_ a good reason to prevent GDB from
> building.

For *local* variables?  

> > And remove some perfectly usable and meaningful variable names.
> 
> They can be easily replaced by no less meaningful and usable ones.

But such a change requires one to actually read and understand the
code.  Such changes can therefore never be obvious, so they can't be
fixed quickly.

> > > In general, we have no bullet-proof way of making sure our code works
> > > on all supported platforms, except by actually compiling it on those
> > > platforms.  That's why platforms which lose their area maintainers
> > > bit-rot quite quickly.  This compiler switch doesn't change this
> > > situation in any way.
> > 
> > I disagree.  -Wshadow is a game changer because actually compiling on
> > *all* plaforms is the only way to make sure that I don't actually
> > break anything.  That's extremely bad!
> 
> The same is true of any code you add or change, with the possible
> exception of very trivial changes.

For most changes, compiling on one system gives you reasonable
confidence that things work on other systems as well.

> > Most people that contribute to GDB are fairly competent programmers.
> 
> Being competent doesn't mean being clairvoyant.  How can we trust
> ourselves to know by heart every single symbol in the standard
> libraries?

Exactly!  That's why -Wshadow is so bad.  Our "defs.h" pulls in lots
of system headers.  And on top of that we use options like
_GNU_SOURCE, which invite the system to pollute the global namespace
as much as it want.

Since there is absolutely no problem with a local variable that has
the same name as a library function, this is bad.

Re: [PATCH 18/348] Fix -Wsahdow warnings

[Eli Zaretskii]

> Date: Fri, 25 Nov 2011 17:26:04 +0100 (CET)
> From: Mark Kettenis <mark.kettenis@xs4all.nl>
> CC: brobecker@adacore.com, andrew.smirnov@gmail.com,
>         gdb-patches@sourceware.org
> 
> Since there is absolutely no problem with a local variable that has
> the same name as a library function, this is bad.

You are, in effect, saying that the wisdom of coding "down the middle
of a programming language" is wrong.  We will have to disagree on
that, sorry.  FWIW, I don't think a large project should live
dangerously just because some versions of some compiler will flag
these clashes even without -Wshadow.

Re: [PATCH 18/348] Fix -Wsahdow warnings

[Mark Kettenis]

> Date: Fri, 25 Nov 2011 20:19:55 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > Date: Fri, 25 Nov 2011 17:26:04 +0100 (CET)
> > From: Mark Kettenis <mark.kettenis@xs4all.nl>
> > CC: brobecker@adacore.com, andrew.smirnov@gmail.com,
> >         gdb-patches@sourceware.org
> > 
> > Since there is absolutely no problem with a local variable that has
> > the same name as a library function, this is bad.
> 
> You are, in effect, saying that the wisdom of coding "down the middle
> of a programming language" is wrong.  We will have to disagree on
> that, sorry.  FWIW, I don't think a large project should live
> dangerously just because some versions of some compiler will flag
> these clashes even without -Wshadow.

I think you're still missing my point.

What I'm saying is that a local variable shadowing a function is never
a problem.  It would only be a problem if inside the function that has
the local variable, you'd (accidentally) try to invoke the function.
That's why I came up with the example:

void foo(void);

void
bar(void)
{
	int foo;

	foo();
}

where the function foo() is being called when it is being shadowed by
a local variable.  This won't compile on *any* C compiler, simply
because it isn't legal C.

Re: [PATCH 18/348] Fix -Wsahdow warnings

[Pedro Alves]

On Sunday 27 November 2011 13:52:45, Mark Kettenis wrote:

> What I'm saying is that a local variable shadowing a function is never
> a problem.  It would only be a problem if inside the function that has
> the local variable, you'd (accidentally) try to invoke the function.
> That's why I came up with the example:
> 
> void foo(void);
> 
> void
> bar(void)
> {
> 	int foo;
> 
> 	foo();
> }
> 
> where the function foo() is being called when it is being shadowed by
> a local variable.  This won't compile on *any* C compiler, simply
> because it isn't legal C.
> 

Unless the variable is a function pointer.

-- 
Pedro Alves

Re: [PATCH 18/348] Fix -Wsahdow warnings

[Eli Zaretskii]

> Date: Sun, 27 Nov 2011 14:52:45 +0100 (CET)
> From: Mark Kettenis <mark.kettenis@xs4all.nl>
> CC: brobecker@adacore.com, andrew.smirnov@gmail.com,
>         gdb-patches@sourceware.org
> 
> > Date: Fri, 25 Nov 2011 20:19:55 +0200
> > From: Eli Zaretskii <eliz@gnu.org>
> > 
> > > Date: Fri, 25 Nov 2011 17:26:04 +0100 (CET)
> > > From: Mark Kettenis <mark.kettenis@xs4all.nl>
> > > CC: brobecker@adacore.com, andrew.smirnov@gmail.com,
> > >         gdb-patches@sourceware.org
> > > 
> > > Since there is absolutely no problem with a local variable that has
> > > the same name as a library function, this is bad.
> > 
> > You are, in effect, saying that the wisdom of coding "down the middle
> > of a programming language" is wrong.  We will have to disagree on
> > that, sorry.  FWIW, I don't think a large project should live
> > dangerously just because some versions of some compiler will flag
> > these clashes even without -Wshadow.
> 
> I think you're still missing my point.

No, I'm not.  I understood perfectly what you are saying.  We just
don't agree on whether having local variables by the name of an
external function is or isn't a practice to be avoided.