[patch/rfc] complain() -> complaint()

[Andrew Cagney <ac131313 at ges dot redhat dot com>]

Hello,

The files complaints.[hc] implement a mechanism for supressing warning 
messages that occure more than a small number of times.  GDB's debug 
readers use this mechanism to supress all but the first few warnings 
generated when reading an object file.

The current implementation stores the warning message string in a 
structure vis:

	struct complaint argument_complaint = { "Argument '%d'", };

	DOUBLEST argument;
	complain (&argument_complaint, "argument");
	complain (&argument_complaint, argument);
	complain (&argument_complaint, &argument);

The problem I see with this is that there is nothing (other than the 
human eye) checking for consistency between the format string and the call.

I'd like to propose a new complaints interface:

	void complaint (const char *fmt, ...);

(with a format printf attribute) so that the compiler (GCC with 
-Wformat) can check, at build time, the consistency of the format string 
and its parameter list.  To issue the same complaint from multiple 
points in the code, a wrapper function can be used:

	argument_complaint (int argument)
	{
	  complaint ("Argument '%d'", argument);
	}

The process of updating the above should flush out a few bugs :-)

This change would help eliminate problems such as:
http://sources.redhat.com/cgi-bin/gnatsweb.pl?cmd=view%20audit-trail&database=gdb&pr=211

--

I don't use the most efficient of algorithms when detecting duplicate 
complaints.  But then, I don't know how often complaints are occuring 
and my objective is to fix the format miss-match.

The complaint() interface should probably be documented along with 
error() and warning().

Should this be called symtab_complaint(), or perhaphs, should 
complaint() take an extra parameter (the complaint class) so that other 
code can use this mechanism?

--

thoughts?
Andrew
2002-08-10  Andrew Cagney  <ac131313@redhat.com>

	* symfile.c (oldsyms_complaint): Delete.
	(empty_symtab_complaint, unknown_option_complaint): Delete.
	(free_named_symtabs): Use complaint instead of complain.

	* complaints.h: Update copyright.
	(struct complaint): Make `message' constant.
	(complaint): Declare.
	(complaint_root): Delete declaration.

	* complaints.c: Update copyright.
	(vcomplain): New function.
	(complain): Move function body to vcomplain.
	(complaint): New function.
	(complaint_root): Make static.

Index: testsuite/ChangeLog
2002-08-10  Andrew Cagney  <ac131313@redhat.com>

	* gdb.gdb/complaints.exp: New file.

Index: complaints.c
===================================================================
RCS file: /cvs/src/src/gdb/complaints.c,v
retrieving revision 1.6
diff -u -r1.6 complaints.c
--- complaints.c	6 Nov 2001 23:38:14 -0000	1.6
+++ complaints.c	11 Aug 2002 18:59:28 -0000
@@ -1,6 +1,7 @@
 /* Support for complaint handling during symbol reading in GDB.
-   Copyright 1990, 1991, 1992, 1993, 1995, 1998, 1999, 2000
-   Free Software Foundation, Inc.
+
+   Copyright 1990, 1991, 1992, 1993, 1995, 1998, 1999, 2000, 2002 Free
+   Software Foundation, Inc.
 
    This file is part of GDB.
 
@@ -27,7 +28,7 @@
 
 /* Structure to manage complaints about symbol file contents.  */
 
-struct complaint complaint_root[1] =
+static struct complaint complaint_root[1] =
 {
   {
     (char *) NULL,		/* Complaint message */
@@ -59,11 +60,8 @@
    into a chain for later handling.  */
 
 void
-complain (struct complaint *complaint,...)
+vcomplain (struct complaint *complaint, va_list args)
 {
-  va_list args;
-  va_start (args, complaint);
-
   complaint->counter++;
   if (complaint->next == NULL)
     {
@@ -125,6 +123,38 @@
      GDB will not be sending so many complaints that this becomes a
      performance hog.  */
   gdb_flush (gdb_stderr);
+}
+
+void
+complain (struct complaint *complaint, ...)
+{
+  va_list args;
+  va_start (args, complaint);
+  vcomplain (complaint, args);
+  va_end (args);
+}
+
+void
+complaint (const char *fmt, ...)
+{
+  struct complaint *complaint = complaint_root;
+  va_list args;
+  for (complaint = complaint_root->next;
+       complaint != complaint_root;
+       complaint = complaint->next)
+    {
+      if (complaint->message == fmt)
+	break;
+    }
+  if (complaint == complaint_root)
+    {
+      complaint = XMALLOC (struct complaint);
+      complaint->message = fmt;
+      complaint->counter = 0;
+      complaint->next = NULL;
+    }
+  va_start (args, fmt);
+  vcomplain (complaint, args);
   va_end (args);
 }
 
Index: complaints.h
===================================================================
RCS file: /cvs/src/src/gdb/complaints.h,v
retrieving revision 1.3
diff -u -r1.3 complaints.h
--- complaints.h	6 Mar 2001 08:21:06 -0000	1.3
+++ complaints.h	11 Aug 2002 18:59:28 -0000
@@ -1,6 +1,7 @@
 /* Definitions for complaint handling during symbol reading in GDB.
-   Copyright 1990, 1991, 1992, 1995, 1998, 2000
-   Free Software Foundation, Inc.
+
+   Copyright 1990, 1991, 1992, 1995, 1998, 2000, 2002 Free Software
+   Foundation, Inc.
 
    This file is part of GDB.
 
@@ -33,21 +34,20 @@
 
 struct complaint
   {
-    char *message;
+    const char *message;
     unsigned counter;
     struct complaint *next;
   };
 
-/* Root of the chain of complaints that have at some point been issued. 
-   This is used to reset the counters, and/or report the total counts.  */
+/* Functions that handle complaints.  */
 
-extern struct complaint complaint_root[1];
+extern void complaint (const char *fmt, ...) ATTR_FORMAT (printf, 1, 2);
 
-/* Functions that handle complaints.  (in complaints.c)  */
+/* Old version of function that handled complaints.  Takes a
+   pre-initialized complaints struct.  */
 
 extern void complain (struct complaint *, ...);
 
 extern void clear_complaints (int, int);
-
 
 #endif /* !defined (COMPLAINTS_H) */
Index: symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.65
diff -u -r1.65 symfile.c
--- symfile.c	1 Aug 2002 17:18:32 -0000	1.65
+++ symfile.c	11 Aug 2002 19:01:53 -0000
@@ -81,21 +81,6 @@
 /* Global variables owned by this file */
 int readnow_symbol_files;	/* Read full symbols immediately */
 
-struct complaint oldsyms_complaint =
-{
-  "Replacing old symbols for `%s'", 0, 0
-};
-
-struct complaint empty_symtab_complaint =
-{
-  "Empty symbol table found for `%s'", 0, 0
-};
-
-struct complaint unknown_option_complaint =
-{
-  "Unknown option `%s' ignored", 0, 0
-};
-
 /* External variables and functions referenced. */
 
 extern void report_transfer_performance (unsigned long, time_t, time_t);
@@ -2305,15 +2290,14 @@
 	  || BLOCK_NSYMS (BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK))
 	  || BLOCK_NSYMS (BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK)))
 	{
-	  complain (&oldsyms_complaint, name);
-
+	  complaint ("Replacing old symbols for `%s'", name);
 	  clear_symtab_users_queued++;
 	  make_cleanup (clear_symtab_users_once, 0);
 	  blewit = 1;
 	}
       else
 	{
-	  complain (&empty_symtab_complaint, name);
+	  complaint ("Empty symbol table found for `%s'", name);
 	}
 
       free_symtab (s);
Index: testsuite/gdb.gdb/complaints.exp
===================================================================
RCS file: testsuite/gdb.gdb/complaints.exp
diff -N testsuite/gdb.gdb/complaints.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.gdb/complaints.exp	11 Aug 2002 19:02:33 -0000
@@ -0,0 +1,182 @@
+#   Copyright 2002
+#   Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+# 
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+# 
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.  
+
+# Please email any bugs, comments, and/or additions to this file to:
+# bug-gdb@prep.ai.mit.edu
+
+# This file was written by Andrew Cagney (cagney at redhat dot com),
+# derived from xfullpath.exp (written by Joel Brobecker), derived from
+# selftest.exp (written by Rob Savoye).
+
+if $tracelevel then {
+    strace $tracelevel
+}
+
+set prms_id 0
+set bug_id 0
+
+# are we on a target board
+if [is_remote target] {
+    return
+}
+
+proc setup_test { executable } {
+    global gdb_prompt
+    global timeout
+
+    # load yourself into the debugger
+    # This can take a relatively long time, particularly for testing where
+    # the executable is being accessed over a network, or where gdb does not
+    # support partial symbols for a particular target and has to load the
+    # entire symbol table.  Set the timeout to 10 minutes, which should be
+    # adequate for most environments (it *has* timed out with 5 min on a
+    # SPARCstation SLC under moderate load, so this isn't unreasonable).
+    # After gdb is started, set the timeout to 30 seconds for the duration
+    # of this test, and then back to the original value.
+
+    set oldtimeout $timeout
+    set timeout 600
+    verbose "Timeout is now $timeout seconds" 2
+    if {[gdb_load $executable] <0} then {
+	set timeout $oldtimeout
+	verbose "Timeout is now $timeout seconds" 2
+	return -1
+    }
+    set timeout $oldtimeout
+    verbose "Timeout is now $timeout seconds" 2
+
+    # Set a breakpoint at main
+    gdb_test "break captured_command_loop" \
+            "Breakpoint.*at.* file.*, line.*" \
+            "breakpoint in captured_command_loop"
+
+    # run yourself
+    # It may take a very long time for the inferior gdb to start (lynx),
+    # so we bump it back up for the duration of this command.
+    set timeout 600
+
+    set description "run until breakpoint at captured_command_loop"
+    send_gdb "run -nw\n"
+    gdb_expect {
+        -re "Starting program.*Breakpoint \[0-9\]+,.*captured_command_loop .data.* at .*main.c:.*$gdb_prompt $" {
+            pass "$description"
+        }
+        -re "Starting program.*Breakpoint \[0-9\]+,.*captured_command_loop .data.*$gdb_prompt $" {
+            xfail "$description (line numbers scrambled?)"
+        }
+        -re "vfork: No more processes.*$gdb_prompt $" {
+            fail "$description (out of virtual memory)"
+            set timeout $oldtimeout
+            verbose "Timeout is now $timeout seconds" 2
+            return -1
+        }
+        -re ".*$gdb_prompt $" {
+            fail "$description"
+            set timeout $oldtimeout
+            verbose "Timeout is now $timeout seconds" 2
+            return -1
+        }
+        timeout {
+            fail "$description (timeout)"
+        }
+    }
+
+    set timeout $oldtimeout
+    verbose "Timeout is now $timeout seconds" 2
+
+    return 0
+}
+
+proc test_with_self { executable } {
+
+    global gdb_prompt
+
+    set setup_result [setup_test $executable]
+    if {$setup_result <0} then {
+        return -1
+    }
+
+    # Unsupress complaints
+    gdb_test "set stop_whining = 2"
+
+    # Prime the system
+    gdb_test "call complaint (\"Register a complaint\")" \
+	    "During symbol reading, Register a complaint."
+
+    # Check that the complaint was inserted and where
+    gdb_test "print complaint_root->next->message" \
+	    ".\[0-9\]+ =.*\"Register a complaint\""
+
+    # Re-issue the first message #1
+    gdb_test "call complaint (complaint_root->next->message)" \
+	    "During symbol reading, Register a complaint."
+
+    # Check that there is only one thing in the list
+    gdb_test "print complaint_root == complaint_root->next" \
+	    ".\[0-9\]+ = 0" "list is not empty"
+    gdb_test "print complaint_root == complaint_root->next->next" \
+	    ".\[0-9\]+ = 1" "list has one entry"
+
+    # Add a second complaint, expect it
+    gdb_test "call complaint (\"Testing! Testing! Testing!\")" \
+	    "During symbol reading, Testing. Testing. Testing.."
+
+    return 0
+}
+
+# Find a pathname to a file that we would execute if the shell was asked
+# to run $arg using the current PATH.
+
+proc find_gdb { arg } {
+
+    # If the arg directly specifies an existing executable file, then
+    # simply use it.
+
+    if [file executable $arg] then {
+	return $arg
+    }
+
+    set result [which $arg]
+    if [string match "/" [ string range $result 0 0 ]] then {
+	return $result
+    }
+
+    # If everything fails, just return the unqualified pathname as default
+    # and hope for best.
+
+    return $arg
+}
+
+# Run the test with self.
+# Copy the file executable file in case this OS doesn't like to edit its own
+# text space.
+
+set GDB_FULLPATH [find_gdb $GDB]
+
+# Remove any old copy lying around.
+remote_file host delete x$tool
+
+gdb_start
+set file [remote_download host $GDB_FULLPATH x$tool]
+set result [test_with_self $file];
+gdb_exit;
+catch "remote_file host delete $file";
+
+if {$result <0} then {
+    warning "Couldn't test self"
+    return -1
+}