* Re: suggested compile warnings
2003-05-23 20:17 ` Andrew Cagney
@ 2003-05-23 21:02 ` Theodore A. Roth
2003-05-23 21:17 ` David Carlton
1 sibling, 0 replies; 5+ messages in thread
From: Theodore A. Roth @ 2003-05-23 21:02 UTC (permalink / raw)
To: Andrew Cagney; +Cc: gdb
[-- 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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: suggested compile warnings
2003-05-23 20:17 ` Andrew Cagney
2003-05-23 21:02 ` Theodore A. Roth
@ 2003-05-23 21:17 ` David Carlton
2003-05-23 21:46 ` Andrew Cagney
1 sibling, 1 reply; 5+ messages in thread
From: David Carlton @ 2003-05-23 21:17 UTC (permalink / raw)
To: Andrew Cagney; +Cc: Theodore A. Roth, gdb
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
^ permalink raw reply [flat|nested] 5+ messages in thread