A strange gcc behavior, and an argument against -Wno-unused

A strange gcc behavior, and an argument against -Wno-unused

[Michael Snyder]

Hey all,

We have "-Wno-unused" in our Makefile.in, as a result of which
we have accumulated a huge pool of unused variables, which
probably get optimized away and so don't hurt anything, but
which clutter up the code.

I was playing around with the idea of cleaning them up, and
so I removed the "-Wno-unused" from the makefile.

But the first thing that happened was that my compile failed
with the following warning (which of course was treated as an
error, because we also have -Werror):

     i386-tdep.c:4149: warning: statement with no effect

Now here's the line in question, from i386_process_record:

           ir.rm != ir.rex_b;

 From context and other examples, I'm pretty sure that this
is a typo, and was meant to read "|=", not "!=".  No sweat,
I'll fix it, or submit it for review anyway.

BUT!  The fact that this escaped being detected by the compiler
bothers me a lot!  I don't know whether to think of it as a
compiler bug, or to reason "well, we said "don't warn us about
things that are unused, and this is basically an unused
statement".

So, for discussion, should we remove -Wno-unused?

Michael

Re: A strange gcc behavior, and an argument against -Wno-unused

[Dave Korn]

Michael Snyder wrote:

>           ir.rm != ir.rex_b;

> So, for discussion, should we remove -Wno-unused?

  Or append "-Wunused-value"?

    cheers,
      DaveK

Re: A strange gcc behavior, and an argument against -Wno-unused

[Pedro Alves]

On Saturday 10 October 2009 01:43:35, Michael Snyder wrote:
> We have "-Wno-unused" in our Makefile.in, as a result of which
> we have accumulated a huge pool of unused variables, which
> probably get optimized away and so don't hurt anything, but
> which clutter up the code.
> 
> I was playing around with the idea of cleaning them up, 

Yeah.  I think Aleksandar's "-Wall patches" patch
series must have fixed most of these already.  Unfortunately,
the patch was so large, that it ended up dropped on
the floor.  Please, if you have patches already that
properly fix some warnings, let's put those in, even
if we don't enable more warnings by default immediately.

> and 
> so I removed the "-Wno-unused" from the makefile.
> 

(...)

> So, for discussion, should we remove -Wno-unused?

I think that would be nice.  I'll all for having
tighter warnings and leaner code.  But we should try
to be smooth, and first get rid of all the corresponding
warnings in at least an --enable-targets=all build.
It can't hurt.  That discussion you're proposing should
resolve faster if removing the switch doesn't result
in a gazillion warnings and breaking everyone's builds.

On Saturday 10 October 2009 03:39:03, Dave Korn wrote:

>  Or append "-Wunused-value"?

Sounds like a good first step to me.

Curious as I am, I tried a build with that on.  An --enable-targets=all
on x86_64-linux built with

 make WARN_CFLAGS="-Wall -Wdeclaration-after-statement -Wpointer-arith -Wformat-nonliteral -Wno-pointer-sign -Wno-unused -Wno-switch -Wno-char-subscripts -Werror -Wunused-value" -k

has only revealed this extra warning (in addition to the one
you pointed out):

 ../../src/gdb/mi/mi-cmd-stack.c: In function 'list_args_or_locals':
 ../../src/gdb/mi/mi-cmd-stack.c:265: warning: left-hand operand of comma expression has no effect

"Fixed" as below.

-- 
Pedro Alves

2009-10-10  Pedro Alves  <pedro@codesourcery.com>

	* mi/mi-cmd-stack.c (list_args_or_locals): Use internal_error.
	Put "break" statements on their own line.

---
 gdb/mi/mi-cmd-stack.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Index: src/gdb/mi/mi-cmd-stack.c
===================================================================
--- src.orig/gdb/mi/mi-cmd-stack.c	2009-10-10 16:30:30.000000000 +0100
+++ src/gdb/mi/mi-cmd-stack.c	2009-10-10 17:00:14.000000000 +0100
@@ -256,13 +256,17 @@ list_args_or_locals (enum what_to_list w
   switch (what)
     {
     case locals:
-      name_of_result = "locals"; break;
+      name_of_result = "locals";
+      break;
     case arguments:
-      name_of_result = "args"; break;
+      name_of_result = "args";
+      break;
     case all:
-      name_of_result = "variables"; break;
+      name_of_result = "variables";
+      break;
     default:
-      gdb_assert (("unexpected value", 0));
+      internal_error (__FILE__, __LINE__,
+		      "unexpected what_to_list: %d", (int) what);
     }
 
   cleanup_list = make_cleanup_ui_out_list_begin_end (uiout, name_of_result);

Re: A strange gcc behavior, and an argument against -Wno-unused

[Eli Zaretskii]

> From: Pedro Alves <pedro@codesourcery.com>
> Date: Sat, 10 Oct 2009 17:45:26 +0100
> Cc: Dave Korn <dave.korn.cygwin@googlemail.com>,  Michael Snyder <msnyder@vmware.com>
> 
> On Saturday 10 October 2009 03:39:03, Dave Korn wrote:
> 
> >  Or append "-Wunused-value"?
> 
> Sounds like a good first step to me.

What version of GCC introduced this switch?

Re: A strange gcc behavior, and an argument against -Wno-unused

[Pedro Alves]

On Saturday 10 October 2009 19:10:36, Eli Zaretskii wrpte:

> What version of GCC introduced this switch?

If you're worried about breaking the build with older
gcc's, note that gdb/configure.ac filters out -W options
that gcc doesn't recognize.

if test "x${build_warnings}" != x -a "x$GCC" = xyes
then
    AC_MSG_CHECKING(compiler warning flags)
    # Separate out the -Werror flag as some files just cannot be
    # compiled with it enabled.
    for w in ${build_warnings}; do
	case $w in
	-Werr*) WERROR_CFLAGS=-Werror ;;
	*) # Check that GCC accepts it
	    saved_CFLAGS="$CFLAGS"
	    CFLAGS="$CFLAGS $w"
	    AC_TRY_COMPILE([],[],WARN_CFLAGS="${WARN_CFLAGS} $w",)
	    CFLAGS="$saved_CFLAGS"
	esac
    done
    AC_MSG_RESULT(${WARN_CFLAGS} ${WERROR_CFLAGS})
fi

That said, I don't know when the switch was introduced
in gcc.

-- 
Pedro Alves

Re: A strange gcc behavior, and an argument against -Wno-unused

[Tom Tromey]

>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

>> >  Or append "-Wunused-value"?
>> 
>> Sounds like a good first step to me.

Eli> What version of GCC introduced this switch?

It seems to have been added here:

    Wed May 17 17:27:44 2000  Andrew Cagney  <cagney@b1.cygnus.com>

            * flags.h (warn_unused_function, warn_unused_label,
            warn_unused_parameter, warn_unused_variable, warn_unused_value):
            Replace ``warn_unused''.
            (set_Wunused): Add declaration.
    [...]

That would put it in 3.0 and maybe 2.95.3.

Tom

Re: A strange gcc behavior, and an argument against -Wno-unused

[Eli Zaretskii]

> From: Tom Tromey <tromey@redhat.com>
> Cc: Pedro Alves <pedro@codesourcery.com>, gdb@sourceware.org,
>         dave.korn.cygwin@googlemail.com, msnyder@vmware.com
> Date: Sat, 10 Oct 2009 12:25:38 -0600
> 
> >>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> >  Or append "-Wunused-value"?
> >> 
> >> Sounds like a good first step to me.
> 
> Eli> What version of GCC introduced this switch?
> 
> It seems to have been added here:
> 
>     Wed May 17 17:27:44 2000  Andrew Cagney  <cagney@b1.cygnus.com>
> 
>             * flags.h (warn_unused_function, warn_unused_label,
>             warn_unused_parameter, warn_unused_variable, warn_unused_value):
>             Replace ``warn_unused''.
>             (set_Wunused): Add declaration.
>     [...]
> 
> That would put it in 3.0 and maybe 2.95.3.

Thanks, that's old enough to be useful for most, if not all, of those
who build GDB.

Re: A strange gcc behavior, and an argument against -Wno-unused

[Pedro Alves]

On Saturday 10 October 2009 19:39:24, Eli Zaretskii wrote:

> Thanks, that's old enough to be useful for most, if not all, of those
> who build GDB.

To be clear, FRT:  I don't think it really matters which version
a "positive" -W warning switch went in (-Wno-* switches matter,
because otherwise we wouldn't be silencing a build breaking
warning on older gcc's we may care about).  Even if a switch is
found only on more recent gcc's, if it uncovers bugs, it's
useful.  People using older gcc's just don't get the new
warnings, as gdb's build system takes care of confirming the
switch works at all before using it.

-- 
Pedro Alves

Re: A strange gcc behavior, and an argument against -Wno-unused

[Eli Zaretskii]

> From: Pedro Alves <pedro@codesourcery.com>
> Date: Sat, 10 Oct 2009 19:50:26 +0100
> Cc: tromey@redhat.com,
>  dave.korn.cygwin@googlemail.com,
>  msnyder@vmware.com
> 
> On Saturday 10 October 2009 19:39:24, Eli Zaretskii wrote:
> 
> > Thanks, that's old enough to be useful for most, if not all, of those
> > who build GDB.
> 
> To be clear, FRT:  I don't think it really matters which version
> a "positive" -W warning switch went in (-Wno-* switches matter,
> because otherwise we wouldn't be silencing a build breaking
> warning on older gcc's we may care about).  Even if a switch is
> found only on more recent gcc's, if it uncovers bugs, it's
> useful.  People using older gcc's just don't get the new
> warnings, as gdb's build system takes care of confirming the
> switch works at all before using it.

Understood.  However, I was thinking about exotic architectures that
don't get built by the global maintainers, and so it is important that
those builds actually have this switch on and working, rather than
silently turned off due to non-support.

Re: A strange gcc behavior, and an argument against -Wno-unused

[Pedro Alves]

Since Michael has committed his patch meanwhile, in the
interest of moving a bit forward, how 'bout we give this a
try, then?  No new warnings caught on an
'x86_64-linux --enable-targets=all' at -O2 build.

-- 
Pedro Alves

2009-10-13  Pedro Alves  <pedro@codesourcery.com>

	gdb/
	* configure.ac (build_warnings): Add -Wunused-value.
	* configure: Regenerate.

---
 gdb/configure    |    2 +-
 gdb/configure.ac |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Index: src/gdb/configure.ac
===================================================================
--- src.orig/gdb/configure.ac	2009-10-11 15:53:33.000000000 +0100
+++ src/gdb/configure.ac	2009-10-11 15:54:10.000000000 +0100
@@ -1542,7 +1542,7 @@ fi
 # gdb/doc/gdbint.texinfo.
 build_warnings="-Wall -Wdeclaration-after-statement -Wpointer-arith \
 -Wformat-nonliteral -Wno-pointer-sign \
--Wno-unused -Wno-switch -Wno-char-subscripts"
+-Wno-unused -Wunused-value -Wno-switch -Wno-char-subscripts"
 
 # Enable -Wno-format by default when using gcc on mingw since many
 # GCC versions complain about %I64.
Index: src/gdb/configure
===================================================================
--- src.orig/gdb/configure	2009-10-11 15:53:33.000000000 +0100
+++ src/gdb/configure	2009-10-11 15:54:10.000000000 +0100
@@ -13117,7 +13117,7 @@ fi
 # gdb/doc/gdbint.texinfo.
 build_warnings="-Wall -Wdeclaration-after-statement -Wpointer-arith \
 -Wformat-nonliteral -Wno-pointer-sign \
--Wno-unused -Wno-switch -Wno-char-subscripts"
+-Wno-unused -Wunused-value -Wno-switch -Wno-char-subscripts"
 
 # Enable -Wno-format by default when using gcc on mingw since many
 # GCC versions complain about %I64.

RE: A strange gcc behavior, and an argument against -Wno-unused

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-owner@sourceware.org [mailto:gdb-owner@sourceware.org] De la
> part de Pedro Alves
> Envoyé : Tuesday, October 13, 2009 4:10 PM
> À : gdb@sourceware.org; Eli Zaretskii
> Cc : tromey@redhat.com; dave.korn.cygwin@googlemail.com;
> msnyder@vmware.com
> Objet : Re: A strange gcc behavior, and an argument against -Wno-unused
> 
> Since Michael has committed his patch meanwhile, in the
> interest of moving a bit forward, how 'bout we give this a
> try, then?  No new warnings caught on an
> 'x86_64-linux --enable-targets=all' at -O2 build.


  Isn't this going to make troubles if
a standard function is defined as a pascal procedure
i. e. with return type (void) in most implementation
but returning something else on some "exotic" system?

  I thought there were some examples
but can't remember any exact ones :(
  Maybe related to signals?


Pierre Muller
Pascal language support maintainer for GDB


  

Re: A strange gcc behavior, and an argument against -Wno-unused

[Tom Tromey]

>>>>> "Pierre" == Pierre Muller <muller@ics.u-strasbg.fr> writes:

Pierre>   Isn't this going to make troubles if
Pierre> a standard function is defined as a pascal procedure
Pierre> i. e. with return type (void) in most implementation
Pierre> but returning something else on some "exotic" system?

From what I can tell, the warning only applies to statements with no
side effects, and function calls are considered to have side effects.
The GCC documentation is not extremely clear on this point, I wrote a
test program to try it out.

Tom

Re: A strange gcc behavior, and an argument against -Wno-unused

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> Since Michael has committed his patch meanwhile, in the
Pedro> interest of moving a bit forward, how 'bout we give this a
Pedro> try, then?  No new warnings caught on an
Pedro> 'x86_64-linux --enable-targets=all' at -O2 build.

Do it.

Tom

Re: A strange gcc behavior, and an argument against -Wno-unused

[Pedro Alves]

On Tuesday 13 October 2009 16:47:38, Tom Tromey wrote:
> >>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:
> 
> Pedro> Since Michael has committed his patch meanwhile, in the
> Pedro> interest of moving a bit forward, how 'bout we give this a
> Pedro> try, then?  No new warnings caught on an
> Pedro> 'x86_64-linux --enable-targets=all' at -O2 build.
> 
> Do it.

Done it.

-- 
Pedro Alves

Re: A strange gcc behavior, and an argument against -Wno-unused

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> Yeah.  I think Aleksandar's "-Wall patches" patch
Pedro> series must have fixed most of these already.  Unfortunately,
Pedro> the patch was so large, that it ended up dropped on
Pedro> the floor.

I thought he didn't want to write a ChangeLog entry for it.  I'm happy
to review it, regardless of size, if I don't have to write the
ChangeLog.

Tom