Re: [PATCH] Fix -Wshadow warning in libiberty/cp-demangle.c

[Ian Lance Taylor]

On Sun, Nov 6, 2016 at 10:03 AM, Mark Wielaard  wrote:
>
> We now have -Wshadow=local. So the attached patch uses that for
> libiberty. Is the attached patch OK to commit?

This is OK.

Thanks.

Ian

Re: [PATCH] Fix -Wshadow warning in libiberty/cp-demangle.c

[Mark Wielaard]

On Sun, Nov 06, 2016 at 07:03:34PM +0100, Mark Wielaard wrote:
> On Sat, 2016-09-10 at 09:51 -0400, Eric Gallager wrote:
> > On 9/10/16, Ian Lance Taylor  wrote:
> > > I'm not sure about the patch to configure.ac/configure.  The last I
> > > looked -Wshadow would warn if a local variable shadows a global
> > > variable.  That can cause a pointless build break if some system
> > > header file defines a global variable that happens to have the same
> > > name as a local variable.  It's not a likely scenario but I don't see
> > > a need to court a build breakage.
> > >
> > Maybe if the patch to add -Wshadow-local went in, the configure script
> > could use that instead?
> 
> We now have -Wshadow=local. So the attached patch uses that for
> libiberty. Is the attached patch OK to commit?

Ping?

> From f6f938e8053b6caf393c35c6f96c946073ca9373 Mon Sep 17 00:00:00 2001
> From: Mark Wielaard 
> Date: Sun, 6 Nov 2016 17:36:06 +0100
> Subject: [PATCH] libiberty: Add -Wshadow=local to warning flags (if
>  supported).
> 
> libiberty/ChangeLog:
> 
>* configure.ac (ac_libiberty_warn_cflags): Add -Wshadow=local.
>* configure: Regenerated.
> ---
> diff --git a/libiberty/configure b/libiberty/configure
> index 0f8e9b9..5c4dda5 100755
> --- a/libiberty/configure
> +++ b/libiberty/configure
> @@ -4398,7 +4398,8 @@ ac_compiler_gnu=$ac_cv_c_compiler_gnu
>  ac_libiberty_warn_cflags=
>  save_CFLAGS="$CFLAGS"
>  for real_option in -W -Wall -Wwrite-strings -Wc++-compat \
> -   -Wstrict-prototypes; do
> +   -Wstrict-prototypes \
> +   -Wshadow=local; do
># Do the check with the no- prefix removed since gcc silently
># accepts any -Wno-* option on purpose
>case $real_option in
> diff --git a/libiberty/configure.ac b/libiberty/configure.ac
> index 9d3f298..1aa0c7c 100644
> --- a/libiberty/configure.ac
> +++ b/libiberty/configure.ac
> @@ -160,7 +160,8 @@ AC_SYS_LARGEFILE
>  AC_PROG_CPP_WERROR
>  
>  ACX_PROG_CC_WARNING_OPTS([-W -Wall -Wwrite-strings -Wc++-compat \
> -   -Wstrict-prototypes], [ac_libiberty_warn_cflags])
> +   -Wstrict-prototypes \
> +   -Wshadow=local], [ac_libiberty_warn_cflags])
>  ACX_PROG_CC_WARNING_ALMOST_PEDANTIC([], [ac_libiberty_warn_cflags])
>  
>  AC_PROG_CC_C_O
> -- 
> 1.8.3.1
> 

Re: [PATCH] Fix -Wshadow warning in libiberty/cp-demangle.c

[Mark Wielaard]

On Sat, 2016-09-10 at 09:51 -0400, Eric Gallager wrote:
> On 9/10/16, Ian Lance Taylor  wrote:
> > I'm not sure about the patch to configure.ac/configure.  The last I
> > looked -Wshadow would warn if a local variable shadows a global
> > variable.  That can cause a pointless build break if some system
> > header file defines a global variable that happens to have the same
> > name as a local variable.  It's not a likely scenario but I don't see
> > a need to court a build breakage.
> >
> Maybe if the patch to add -Wshadow-local went in, the configure script
> could use that instead?

We now have -Wshadow=local. So the attached patch uses that for
libiberty. Is the attached patch OK to commit?

Thanks,

Mark
From f6f938e8053b6caf393c35c6f96c946073ca9373 Mon Sep 17 00:00:00 2001
From: Mark Wielaard 
Date: Sun, 6 Nov 2016 17:36:06 +0100
Subject: [PATCH] libiberty: Add -Wshadow=local to warning flags (if
 supported).

libiberty/ChangeLog:

   * configure.ac (ac_libiberty_warn_cflags): Add -Wshadow=local.
   * configure: Regenerated.
---
diff --git a/libiberty/configure b/libiberty/configure
index 0f8e9b9..5c4dda5 100755
--- a/libiberty/configure
+++ b/libiberty/configure
@@ -4398,7 +4398,8 @@ ac_compiler_gnu=$ac_cv_c_compiler_gnu
 ac_libiberty_warn_cflags=
 save_CFLAGS="$CFLAGS"
 for real_option in -W -Wall -Wwrite-strings -Wc++-compat \
-			  -Wstrict-prototypes; do
+			  -Wstrict-prototypes \
+			  -Wshadow=local; do
   # Do the check with the no- prefix removed since gcc silently
   # accepts any -Wno-* option on purpose
   case $real_option in
diff --git a/libiberty/configure.ac b/libiberty/configure.ac
index 9d3f298..1aa0c7c 100644
--- a/libiberty/configure.ac
+++ b/libiberty/configure.ac
@@ -160,7 +160,8 @@ AC_SYS_LARGEFILE
 AC_PROG_CPP_WERROR
 
 ACX_PROG_CC_WARNING_OPTS([-W -Wall -Wwrite-strings -Wc++-compat \
-			  -Wstrict-prototypes], [ac_libiberty_warn_cflags])
+			  -Wstrict-prototypes \
+			  -Wshadow=local], [ac_libiberty_warn_cflags])
 ACX_PROG_CC_WARNING_ALMOST_PEDANTIC([], [ac_libiberty_warn_cflags])
 
 AC_PROG_CC_C_O
-- 
1.8.3.1

Re: [PATCH] Fix -Wshadow warning in libiberty/cp-demangle.c

[Eric Gallager]

On 9/10/16, Ian Lance Taylor  wrote:
> On Fri, Sep 9, 2016 at 5:06 PM, Mark Wielaard  wrote:
>> valgrind contains a copy of the libiberty demangler which gets compiled
>> with -Wshadow. That shows the following warning:
>>
>> cp-demangle.c: In function ‘d_substitution’:
>> cp-demangle.c:3772:35: warning: declaration of ‘c’ shadows a previous
>> local
>> struct demangle_component *c;
>>^
>> cp-demangle.c:3708:8: note: shadowed declaration is here
>>char c;
>> ^
>>
>> Fix that by renaming the struct demangle_component variable to dc
>> and add -Wshadow to ac_libiberty_warn_cflags (the only warning is this
>> one).
>>
>> libiberty/ChangeLog:
>>
>> * cp-demangle.c (d_substitution): Change struct
>> demangle_component
>> variable name from c to dc.
>> * configure.ac (ac_libiberty_warn_cflags): Add -Wshadow.
>> * configure: Regenerate.
>
> The patch to cp-demangle.c is OK.  Thanks.
>
> I'm not sure about the patch to configure.ac/configure.  The last I
> looked -Wshadow would warn if a local variable shadows a global
> variable.  That can cause a pointless build break if some system
> header file defines a global variable that happens to have the same
> name as a local variable.  It's not a likely scenario but I don't see
> a need to court a build breakage.
>
> Ian
>


Maybe if the patch to add -Wshadow-local went in, the configure script
could use that instead? For reference:
https://gcc.gnu.org/ml/gcc-patches/2015-12/msg01317.html

Re: [PATCH] Fix -Wshadow warning in libiberty/cp-demangle.c

[Mark Wielaard]

On Sat, Sep 10, 2016 at 01:57:54AM -0700, Ian Lance Taylor wrote:
> On Fri, Sep 9, 2016 at 5:06 PM, Mark Wielaard  wrote:
> > Fix that by renaming the struct demangle_component variable to dc
> > and add -Wshadow to ac_libiberty_warn_cflags (the only warning is this one).
> >
> > libiberty/ChangeLog:
> >
> > * cp-demangle.c (d_substitution): Change struct demangle_component
> > variable name from c to dc.
> > * configure.ac (ac_libiberty_warn_cflags): Add -Wshadow.
> > * configure: Regenerate.
> 
> The patch to cp-demangle.c is OK.  Thanks.
> 
> I'm not sure about the patch to configure.ac/configure.  The last I
> looked -Wshadow would warn if a local variable shadows a global
> variable.  That can cause a pointless build break if some system
> header file defines a global variable that happens to have the same
> name as a local variable.  It's not a likely scenario but I don't see
> a need to court a build breakage.

OK, pushed with just the cp-demangle.c fix.
We'll pick up and shadow warnings when we update the valgrind copy again.

Cheers,

Mark

Re: [PATCH] Fix -Wshadow warning in libiberty/cp-demangle.c

[Ian Lance Taylor]

On Fri, Sep 9, 2016 at 5:06 PM, Mark Wielaard  wrote:
> valgrind contains a copy of the libiberty demangler which gets compiled
> with -Wshadow. That shows the following warning:
>
> cp-demangle.c: In function ‘d_substitution’:
> cp-demangle.c:3772:35: warning: declaration of ‘c’ shadows a previous local
> struct demangle_component *c;
>^
> cp-demangle.c:3708:8: note: shadowed declaration is here
>char c;
> ^
>
> Fix that by renaming the struct demangle_component variable to dc
> and add -Wshadow to ac_libiberty_warn_cflags (the only warning is this one).
>
> libiberty/ChangeLog:
>
> * cp-demangle.c (d_substitution): Change struct demangle_component
> variable name from c to dc.
> * configure.ac (ac_libiberty_warn_cflags): Add -Wshadow.
> * configure: Regenerate.

The patch to cp-demangle.c is OK.  Thanks.

I'm not sure about the patch to configure.ac/configure.  The last I
looked -Wshadow would warn if a local variable shadows a global
variable.  That can cause a pointless build break if some system
header file defines a global variable that happens to have the same
name as a local variable.  It's not a likely scenario but I don't see
a need to court a build breakage.

Ian

[PATCH] Fix -Wshadow warning in libiberty/cp-demangle.c

[Mark Wielaard]

valgrind contains a copy of the libiberty demangler which gets compiled
with -Wshadow. That shows the following warning:

cp-demangle.c: In function ‘d_substitution’:
cp-demangle.c:3772:35: warning: declaration of ‘c’ shadows a previous local
struct demangle_component *c;
   ^
cp-demangle.c:3708:8: note: shadowed declaration is here
   char c;
^

Fix that by renaming the struct demangle_component variable to dc
and add -Wshadow to ac_libiberty_warn_cflags (the only warning is this one).

libiberty/ChangeLog:

* cp-demangle.c (d_substitution): Change struct demangle_component
variable name from c to dc.
* configure.ac (ac_libiberty_warn_cflags): Add -Wshadow.
* configure: Regenerate.
diff --git a/libiberty/configure b/libiberty/configure
index 0f8e9b9..91a051c 100755
--- a/libiberty/configure
+++ b/libiberty/configure
@@ -4398,7 +4398,8 @@ ac_compiler_gnu=$ac_cv_c_compiler_gnu
 ac_libiberty_warn_cflags=
 save_CFLAGS="$CFLAGS"
 for real_option in -W -Wall -Wwrite-strings -Wc++-compat \
- -Wstrict-prototypes; do
+ -Wstrict-prototypes \
+ -Wshadow; do
   # Do the check with the no- prefix removed since gcc silently
   # accepts any -Wno-* option on purpose
   case $real_option in
diff --git a/libiberty/configure.ac b/libiberty/configure.ac
index 9d3f298..44bed8f 100644
--- a/libiberty/configure.ac
+++ b/libiberty/configure.ac
@@ -160,7 +160,8 @@ AC_SYS_LARGEFILE
 AC_PROG_CPP_WERROR
 
 ACX_PROG_CC_WARNING_OPTS([-W -Wall -Wwrite-strings -Wc++-compat \
- -Wstrict-prototypes], [ac_libiberty_warn_cflags])
+ -Wstrict-prototypes \
+ -Wshadow], [ac_libiberty_warn_cflags])
 ACX_PROG_CC_WARNING_ALMOST_PEDANTIC([], [ac_libiberty_warn_cflags])
 
 AC_PROG_CC_C_O
diff --git a/libiberty/cp-demangle.c b/libiberty/cp-demangle.c
index 1c2bce2..a843dc3 100644
--- a/libiberty/cp-demangle.c
+++ b/libiberty/cp-demangle.c
@@ -3769,7 +3769,7 @@ d_substitution (struct d_info *di, int prefix)
{
  const char *s;
  int len;
- struct demangle_component *c;
+ struct demangle_component *dc;
 
  if (p->set_last_name != NULL)
di->last_name = d_make_sub (di, p->set_last_name,
@@ -3785,15 +3785,15 @@ d_substitution (struct d_info *di, int prefix)
  len = p->simple_len;
}
  di->expansion += len;
- c = d_make_sub (di, s, len);
+ dc = d_make_sub (di, s, len);
  if (d_peek_char (di) == 'B')
{
  /* If there are ABI tags on the abbreviation, it becomes
 a substitution candidate.  */
- c = d_abi_tags (di, c);
- d_add_substitution (di, c);
+ dc = d_abi_tags (di, dc);
+ d_add_substitution (di, dc);
}
- return c;
+ return dc;
}
}
 
-- 
1.8.3.1

Re: -Wshadow warning

[Gabriel Dos Reis]

On Wed, Aug 24, 2011 at 9:32 PM, Alan Modra amo...@gmail.com wrote:
 Wouldn't -Wshadow be more useful if it obeyed -Wno-system-headers?
 For code like

        #include stdlib.h
        int foo (int atof);
        int foo (int atof) { return atof; }

 we currently do not warn on the prototype, but do on the function
 definition, leading to reports such as
 http://sourceware.org/bugzilla/show_bug.cgi?id=13121

 The following has been bootstrapped and regression tested
 powerpc-linux.  OK to apply?

OK.


        * c-decl.c (warn_if_shadowing): Don't warn if shadowed
        identifier is from system header.

 Index: gcc/c-decl.c
 ===
 --- gcc/c-decl.c        (revision 178035)
 +++ gcc/c-decl.c        (working copy)
 @@ -2516,7 +2516,10 @@ warn_if_shadowing (tree new_decl)

   /* Is anything being shadowed?  Invisible decls do not count.  */
   for (b = I_SYMBOL_BINDING (DECL_NAME (new_decl)); b; b = b-shadowed)
 -    if (b-decl  b-decl != new_decl  !b-invisible)
 +    if (b-decl  b-decl != new_decl  !b-invisible
 +        (b-decl == error_mark_node
 +           || diagnostic_report_warnings_p (global_dc,
 +                                            DECL_SOURCE_LOCATION (b-decl
       {
        tree old_decl = b-decl;


 --
 Alan Modra
 Australia Development Lab, IBM

-Wshadow warning

[Alan Modra]

Wouldn't -Wshadow be more useful if it obeyed -Wno-system-headers?
For code like

#include stdlib.h
int foo (int atof);
int foo (int atof) { return atof; }

we currently do not warn on the prototype, but do on the function
definition, leading to reports such as
http://sourceware.org/bugzilla/show_bug.cgi?id=13121

The following has been bootstrapped and regression tested
powerpc-linux.  OK to apply?

* c-decl.c (warn_if_shadowing): Don't warn if shadowed
identifier is from system header.

Index: gcc/c-decl.c
===
--- gcc/c-decl.c(revision 178035)
+++ gcc/c-decl.c(working copy)
@@ -2516,7 +2516,10 @@ warn_if_shadowing (tree new_decl)
 
   /* Is anything being shadowed?  Invisible decls do not count.  */
   for (b = I_SYMBOL_BINDING (DECL_NAME (new_decl)); b; b = b-shadowed)
-if (b-decl  b-decl != new_decl  !b-invisible)
+if (b-decl  b-decl != new_decl  !b-invisible
+(b-decl == error_mark_node
+   || diagnostic_report_warnings_p (global_dc,
+DECL_SOURCE_LOCATION (b-decl
   {
tree old_decl = b-decl;
 

-- 
Alan Modra
Australia Development Lab, IBM