suggested compile warnings

suggested compile warnings

[Theodore A. Roth]

Hi,

I've always been configuring gdb with --enable-gdb-build-warnings=-Werror
and thought that there where a bunch of gcc compile warnings issued.
Looking more closely at my builds, that doesn't seem to be true.

For my own apps, I like to use '-Wall -Werror' as it lets the compiler
catch a lot of my stupid mistakes. I tried '-Wall -Werror' for gdb,
but that seems to be too restrictive for gdb source.

Does anyone else compile gdb with any of the -W<foo> gcc options?
Is there a recommended list of these which should be used?

For example, adding -Wunused (without -Werror), turns up 149 wanrings.
Most are unused variables, some are static decl's for functions that
aren't defined. Most if not all of these should be trivial to fix.

Ted Roth

Re: suggested compile warnings

[Andrew Cagney]

> Hi,
> 
> I've always been configuring gdb with --enable-gdb-build-warnings=-Werror
> and thought that there where a bunch of gcc compile warnings issued.
> Looking more closely at my builds, that doesn't seem to be true.
> 
> For my own apps, I like to use '-Wall -Werror' as it lets the compiler
> catch a lot of my stupid mistakes. I tried '-Wall -Werror' for gdb,
> but that seems to be too restrictive for gdb source.
> 
> Does anyone else compile gdb with any of the -W<foo> gcc options?
> Is there a recommended list of these which should be used?
> 
> For example, adding -Wunused (without -Werror), turns up 149 wanrings.
> Most are unused variables, some are static decl's for functions that
> aren't defined. Most if not all of these should be trivial to fix.

See: 13.4.3 Compiler Warnings
http://sources.redhat.com/gdb/current/onlinedocs/gdbint_13.html#SEC118

Instead of -Wunused, look at the individual -Wunused-function, 
-Wunused-variable, ...

Off hand, other intersting ones are:

-Wwrite-strings
It's just hard and really messy.   People occasionally chip away at the 
edges.  Last time I tried, I came across what appeared to be an 
effective xfree("string"), outch!

-W
More for the sign VS unsigned checks it throws up, but it throws up some 
non problems :-(

-Wswitch-default
-Wswitch-enum
-Wswitch-fallthrough (doesn't actually exist)
But that could be religious :-)

-Wstrict-prototypes
-Wmissing-prototypes
-Wmissing-declarations
But tread carefully, it tends to run a foul of head files.

The trick is to first identify warnings that add real value (-Wformat 
for instance is good++) and then ensure that the fix is not worse than 
the problem being identified (using a cast to hide a problem, for 
instance, isn't a good idea vis: xffree ((char *) "a string")).

I've appended an old list of the error counts for various flags that was 
generated by compiling all the cross targets on a Red Hat GNU/Linux 7.2 
system and then deleting duplicates (which GCC? I don't remember).

Have fun.
Andrew

     590 W
     217 Waggregate-return
     335 Wall
       8 Wbad-function-cast
       4 Wcast-align
     106 Wcast-qual
       4 Wchar-subscripts
       4 Wcomment
    2913 Wconversion
       4 Wdeprecated-declarations
       4 Werror
      11 Wfloat-equal
       4 Wformat-extra-args
       4 Wformat-nonliteral
       4 Wformat-security
       4 Wformat-y2k
       4 Wformat
     109 Wformat=2
       7 Wimplicit-function-declaration
       4 Wimplicit-int
       7 Wimplicit
       4 Wimport
       4 Winline
       4 Wlong-long
       6 Wmain
      20 Wmissing-braces
     314 Wmissing-declarations
      57 Wmissing-noreturn
     341 Wmissing-prototypes
       4 Wmultichar
      51 Wnested-externs
       5 Wpacked
     101 Wpadded
       4 Wparentheses
       4 Wpointer-arith
     356 Wredundant-decls
       4 Wreturn-type
     840 Wshadow
     340 Wsign-compare
      74 Wstrict-prototypes
      44 Wswitch
     488 Wtraditional
       4 Wtrigraphs
      26 Wundef
       4 Wuninitialized
       4 Wunknown-pragmas
     213 Wunreachable-code
      66 Wunused-function
       8 Wunused-label
    1420 Wunused-parameter
      12 Wunused-value
     200 Wunused-variable
     274 Wunused
    4421 Wwrite-strings

Re: suggested compile warnings

[Theodore A. Roth]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3851 bytes --]



On Fri, 23 May 2003, Andrew Cagney wrote:

:) > Hi,
:) >
:) > I've always been configuring gdb with --enable-gdb-build-warnings=-Werror
:) > and thought that there where a bunch of gcc compile warnings issued.
:) > Looking more closely at my builds, that doesn't seem to be true.
:) >
:) > For my own apps, I like to use '-Wall -Werror' as it lets the compiler
:) > catch a lot of my stupid mistakes. I tried '-Wall -Werror' for gdb,
:) > but that seems to be too restrictive for gdb source.
:) >
:) > Does anyone else compile gdb with any of the -W<foo> gcc options?
:) > Is there a recommended list of these which should be used?
:) >
:) > For example, adding -Wunused (without -Werror), turns up 149 wanrings.
:) > Most are unused variables, some are static decl's for functions that
:) > aren't defined. Most if not all of these should be trivial to fix.
:)
:) See: 13.4.3 Compiler Warnings
:) http://sources.redhat.com/gdb/current/onlinedocs/gdbint_13.html#SEC118

Thats what I was looking for. Guess I should have looked in the
index...

I noticed that I was using '-Werror' instead of ',-Werror'. That seems
to quietly disable all the hand picked warnings. D'oh! Now my build is
catching some problems again in my code.

:)
:) Instead of -Wunused, look at the individual -Wunused-function,
:) -Wunused-variable, ...
:)
:) Off hand, other intersting ones are:
:)
:) -Wwrite-strings
:) It's just hard and really messy.   People occasionally chip away at the
:) edges.  Last time I tried, I came across what appeared to be an
:) effective xfree("string"), outch!
:)
:) -W
:) More for the sign VS unsigned checks it throws up, but it throws up some
:) non problems :-(
:)
:) -Wswitch-default
:) -Wswitch-enum
:) -Wswitch-fallthrough (doesn't actually exist)
:) But that could be religious :-)
:)
:) -Wstrict-prototypes
:) -Wmissing-prototypes
:) -Wmissing-declarations
:) But tread carefully, it tends to run a foul of head files.
:)
:) The trick is to first identify warnings that add real value (-Wformat
:) for instance is good++) and then ensure that the fix is not worse than
:) the problem being identified (using a cast to hide a problem, for
:) instance, isn't a good idea vis: xffree ((char *) "a string")).
:)
:) I've appended an old list of the error counts for various flags that was
:) generated by compiling all the cross targets on a Red Hat GNU/Linux 7.2
:) system and then deleting duplicates (which GCC? I don't remember).
:)
:) Have fun.
:) Andrew
:)

Ok, that's a lot of warnings. 8-) Thanks for the info.

Is it worth while cleaning up the simple stuff? I am willing to do
janitorial work since it at least forces me to dig into the code more.

Before I go of half cocked, here's the warnings I get in breakpoint.c:

gcc -c -g -O2    -I. -I../../gdb -I../../gdb/config -DLOCALEDIR="\"/home/roth/local/avr/share/locale\"" -DHAVE_CONFIG_H -I../../gdb/../include/opcode -I../../gdb/../readline/.. -I../bfd -I../../gdb/../bfd  -I../../gdb/../include -I../intl -I../../gdb/../intl  -DMI_OUT=1 -Wall  ../../gdb/breakpoint.c
../../gdb/breakpoint.c: In function `insert_breakpoints':
../../gdb/breakpoint.c:971: warning: value computed is not used
../../gdb/breakpoint.c:717: warning: unused variable `disabled_breaks'
../../gdb/breakpoint.c: In function `can_use_hardware_watchpoint':
../../gdb/breakpoint.c:5526: warning: unused variable `vaddr'
../../gdb/breakpoint.c: In function `do_enable_breakpoint':
../../gdb/breakpoint.c:7207: warning: statement with no effect
../../gdb/breakpoint.c: At top level:
../../gdb/breakpoint.c:5798: warning: `ep_parse_optional_filename' defined but not used

Attached are my fixes. Are these appropriate and obvious fixes? I'm
not excited about the ',' usage in a macro, but it seemed to be used
in a lot of other macros already. [I know the ChangeLog entry is
unacceptable though...]

Ted Roth

[-- Attachment #2: Type: TEXT/PLAIN, Size: 3749 bytes --]

2003-05-23  Theodore A. Roth  <troth@openavr.org>

	* breakpoint.c: Compiler warning cleanups.

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.120
diff -u -r1.120 breakpoint.c
--- breakpoint.c	5 May 2003 17:56:54 -0000	1.120
+++ breakpoint.c	23 May 2003 20:44:35 -0000
@@ -187,8 +187,6 @@
 
 static char *ep_parse_optional_if_clause (char **arg);
 
-static char *ep_parse_optional_filename (char **arg);
-
 #if defined(CHILD_INSERT_EXEC_CATCHPOINT)
 static void catch_exec_command_1 (char *arg, int tempflag, int from_tty);
 #endif
@@ -703,6 +701,11 @@
 }
 \f
 
+#if !defined (DISABLE_UNSETTABLE_BREAK)
+#define DISABLE_UNSETTABLE_BREAK(addr) \
+    (0)
+#endif
+
 /* insert_breakpoints is used when starting or continuing the program.
    remove_breakpoints is used when the program stops.
    Both return zero if successful,
@@ -824,7 +827,6 @@
 	if (val)
 	  {
 	    /* Can't set the breakpoint.  */
-#if defined (DISABLE_UNSETTABLE_BREAK)
 	    if (DISABLE_UNSETTABLE_BREAK (b->address))
 	      {
 		/* See also: disable_breakpoints_in_shlibs. */
@@ -843,7 +845,6 @@
 				    "breakpoint #%d\n", b->number);
 	      }
 	    else
-#endif
 	      {
 #ifdef ONE_PROCESS_WRITETEXT
 		process_warning = 1;
@@ -961,6 +962,8 @@
 
 	if (within_current_scope)
 	  {
+            char *contents;
+
 	    /* Evaluate the expression and cut the chain of values
 	       produced off from the value chain.
 
@@ -968,7 +971,7 @@
 	       laziness to determine what memory GDB actually needed
 	       in order to compute the value of the expression.  */
 	    v = evaluate_expression (b->exp);
-	    VALUE_CONTENTS(v);
+	    contents = VALUE_CONTENTS(v);
 	    value_release_to_mark (mark);
 
 	    b->val_chain = v;
@@ -5469,7 +5472,8 @@
 
 #if !defined(TARGET_REGION_OK_FOR_HW_WATCHPOINT)
 #define TARGET_REGION_OK_FOR_HW_WATCHPOINT(ADDR,LEN) \
-     (TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(LEN))
+     ((void)(ADDR), /* not used, quells warning */\
+      TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(LEN))
 #endif
 
 static int
@@ -5783,42 +5787,6 @@
   return cond_string;
 }
 
-/* This function attempts to parse an optional filename from the arg
-   string.  If one is not found, it returns NULL.
-
-   Else, it returns a pointer to the parsed filename.  (This function
-   makes no attempt to verify that a file of that name exists, or is
-   accessible.)  And, it updates arg to point to the first character
-   following the parsed filename in the arg string.
-
-   Note that clients needing to preserve the returned filename for
-   future access should copy it to their own buffers. */
-static char *
-ep_parse_optional_filename (char **arg)
-{
-  static char filename[1024];
-  char *arg_p = *arg;
-  int i;
-  char c;
-
-  if ((*arg_p == '\0') || isspace (*arg_p))
-    return NULL;
-
-  for (i = 0;; i++)
-    {
-      c = *arg_p;
-      if (isspace (c))
-	c = '\0';
-      filename[i] = c;
-      if (c == '\0')
-	break;
-      arg_p++;
-    }
-  *arg = arg_p;
-
-  return filename;
-}
-
 /* Commands to deal with catching events, such as signals, exceptions,
    process start/exit, etc.  */
 
@@ -7203,8 +7171,6 @@
 	  int i = hw_watchpoint_used_count (bpt->type, &other_type_used);
 	  int mem_cnt = can_use_hardware_watchpoint (bpt->val);
 
-	  /* Hack around 'unused var' error for some targets here */
-	  (void) mem_cnt, i;
 	  target_resources_ok = TARGET_CAN_USE_HARDWARE_WATCHPOINT (
 				   bpt->type, i + mem_cnt, other_type_used);
 	  /* we can consider of type is bp_hardware_watchpoint, convert to 

Re: suggested compile warnings

[David Carlton]

On Fri, 23 May 2003 16:15:56 -0400, Andrew Cagney <ac131313@redhat.com> said:

> -Wwrite-strings
> It's just hard and really messy.  People occasionally chip away at the
> edges.  Last time I tried, I came across what appeared to be an
> effective xfree("string"), outch!

I've looked at this one fairly seriously.  From my point of view, the
main root issue here is that decode_line_1 takes a char ** instead of
a const char **.  I plan/hope to fix this eventually; I won't get
around to it for a while.

The xfree issue is an interesting one, and I agree that it's another
root problem.  My attitude is actually that xfree should take a const
void * instead of just a void *.  Basically, it seems to me that being
writeable and beeing freeable are orthogonal; examples are:

* Not writeable or freeable: literal

* Writeable and freeable: memory allocated via xmalloc.

* Writeable but not freeable: memory allocated on an obstack.

* Not writeable but freeable: memory that was originally allocated via
  xmalloc but whose contents you don't want to change.  (E.g. a name
  shared among many users, or used as a search key, or something.)

You can argue about examples in the fourth category (maybe their
existence is less clear to C programmers than to C++ programmers,
because of the different idioms for using const in the two languages)
but the third category is real.

Anyways, we can argue about this more once I've fixed decode_line_1.
If anybody else wants to fix it, the easiest thing to do is to write a
wrapper for decode_line_1 whose first argument is "const char
**argptr"; then make a copy of *argptr via alloca, and pass its
address to decode_line_1; then, before returning whatever
decode_line_1 returns, make sure that you modify *argptr in the same
way that decode_line_1 modified *copy_of_argptr.  And then go and fix
all the callers of decode_line_1 to call this new wrapper instead,
passing it a const char **.  (You might get annoyed by the fact that C
doesn't let you automatically convert a char ** into a const char **.)

Once that's happened, then we can argue about xfree; and once _that's_
been changed, then just pick a random char * somewhere (my favorite
would be the 'name' member of struct generic_symbol_info), change it
to a const char *, and follow through a huge chain of updates.

David Carlton
carlton@math.stanford.edu

Re: suggested compile warnings

[Andrew Cagney]

> On Fri, 23 May 2003 16:15:56 -0400, Andrew Cagney <ac131313@redhat.com> said:
> 
> 
>> -Wwrite-strings
>> It's just hard and really messy.  People occasionally chip away at the
>> edges.  Last time I tried, I came across what appeared to be an
>> effective xfree("string"), outch!

Just to clarify something.  The code I came across appeared to be 
xfreeing a string that had never been allocated - that's seriously broken!

> The xfree issue is an interesting one, and I agree that it's another
> root problem.  My attitude is actually that xfree should take a const
> void * instead of just a void *.  Basically, it seems to me that being
> writeable and beeing freeable are orthogonal; examples are:

Many fixes are possible without fixing decode_line_1.  The trick is 
knowing when a change is turning into a rat hole.

Andrew