Re: suggested compile warnings

["Theodore A. Roth" <troth@openavr.org>]

[-- 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