Re: [patch/rfc] complaint() -> complaints() rewrite #2

[Andrew Cagney <ac131313@ges.redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2492 bytes --]

> Hello,
> 
> This follows up an earler e-mail where I wrote:
> 
> 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?
> 
> I've implemented this.  In the process I've tried to make the code a little bit more i18n friendly (it was assuming that things read left to right).
> 
> Anyway, this is my original description:
> 
> 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().
> 
> -- 
> 
> thoughts?
> Andrew 

I've checked the attached in.  It's slightly different to the original 
in that:

- it includes internal_complaint() to match intenal_error() and 
internal_warning()

- complain() simply re-calls complaint() with the message

Thanks to Kevin for giving it the once over.

Attached is the final patch,
Andrew


[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 19535 bytes --]

2002-09-18  Andrew Cagney  <ac131313@redhat.com>

	* complaints.h: Update copyright.
	(struct complaints): Declare.
	(struct complaint): Make `message' constant.
	(internal_complaint): Declare.
	(complaint): Declare.
	(complaint_root): Delete declaration.
	(symfile_complaints): Delete declaration.
	(struct complaints): Add opaque declaration.
	(clear_complaints): Add a complaints parameter.
	* complaints.c: Update copyright.
	(enum complaint_series): Define.
	(complaint_root): Delete.
	(struct complaints): Define.
	(complaint_sentinel, symfile_complaint_book): New variables.
	(symfile_explanations, symfile_complaints): New variables.
	New variables.
	(get_complaints): New function.
	(vcomplaint): New function.
	(complaint): New function.
	(internal_complaint): New function.
	(complain): Call vcomplain with symfile_complaint.
	(clear_complaints): Rewrite.
	(_initialize_complaints): Use add_setshow_command.
	* Makefile.in (complaints.o): Update dependencies.
	* symfile.c (syms_from_objfile): Add symfile_complaints parameter
	to call to clear_complaints.
	(new_symfile_objfile, reread_symbols): Ditto.
	(oldsyms_complaint): Delete.
	(empty_symtab_complaint, unknown_option_complaint): Delete.
	(free_named_symtabs): Use complaint instead of complain.

Index: Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/Makefile.in,v
retrieving revision 1.259
diff -u -r1.259 Makefile.in
--- Makefile.in	17 Sep 2002 04:44:21 -0000	1.259
+++ Makefile.in	19 Sep 2002 00:38:57 -0000
@@ -1572,7 +1572,8 @@
 	$(coff_internal_h) $(libcoff_h) $(symfile_h) $(objfiles_h) \
 	$(buildsym_h) $(gdb_stabs_h) $(stabsread_h) $(complaints_h) \
 	$(target_h) $(gdb_assert_h)
-complaints.o: complaints.c $(defs_h) $(complaints_h) $(gdbcmd_h)
+complaints.o: complaints.c $(defs_h) $(complaints_h) $(gdb_assert_h) \
+	$(command_h) $(gdbcmd_h)
 completer.o: completer.c $(defs_h) $(symtab_h) $(gdbtypes_h) $(expression_h) \
 	$(filenames_h) $(cli_decode_h) $(gdbcmd_h) $(completer_h)
 copying.o: copying.c $(defs_h) $(command_h) $(gdbcmd_h)
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	19 Sep 2002 00:38:57 -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.
 
@@ -21,148 +22,273 @@
 
 #include "defs.h"
 #include "complaints.h"
+#include "gdb_assert.h"
+#include "command.h"
 #include "gdbcmd.h"
 
 extern void _initialize_complaints (void);
 
+/* Should each complaint message be self explanatory, or should we assume that
+   a series of complaints is being produced?  */
+
+/* case 1: First message of a series that must
+   start off with explanation.  case 2: Subsequent message of a series
+   that needs no explanation (the user already knows we have a problem
+   so we can just state our piece).  */
+enum complaint_series {
+  /* Isolated self explanatory message.  */
+  ISOLATED_MESSAGE,
+  /* First message of a series, includes an explanation.  */
+  FIRST_MESSAGE,
+  /* First message of a series, but does not need to include any sort
+     of explanation.  */
+  SHORT_FIRST_MESSAGE,
+  /* Subsequent message of a series that needs no explanation (the
+     user already knows we have a problem so we can just state our
+     piece).  */
+  SUBSEQUENT_MESSAGE
+};
+
 /* Structure to manage complaints about symbol file contents.  */
 
-struct complaint complaint_root[1] =
+struct complain
 {
-  {
-    (char *) NULL,		/* Complaint message */
-    0,				/* Complaint counter */
-    complaint_root		/* Next complaint. */
-  }
+  const char *file;
+  int line;
+  const char *fmt;
+  int counter;
+  struct complain *next;
 };
 
-/* How many complaints about a particular thing should be printed before
-   we stop whining about it?  Default is no whining at all, since so many
-   systems have ill-constructed symbol files.  */
+struct complaints
+{
+  struct complain *root;
 
-static unsigned int stop_whining = 0;
+  /* Should each complaint be self explanatory, or should we assume
+     that a series of complaints is being produced?  case 0: Isolated
+     self explanatory message.  case 1: First message of a series that
+     must start off with explanation.  case 2: Subsequent message of a
+     series that needs no explanation (the user already knows we have
+     a problem so we can just state our piece).  */
+  int series;
+
+  /* The explanatory messages that should accompany the complaint.
+     NOTE: cagney/2002-08-14: In a desperate attempt at being vaguely
+     i18n friendly, this is an array of two messages.  When present,
+     EXPLANATION[SERIES] is used to wrap the message.  */
+  const char **explanation;
+};
 
-/* Should each complaint be self explanatory, or should we assume that
-   a series of complaints is being produced? 
-   case 0:  self explanatory message.
-   case 1:  First message of a series that must start off with explanation.
-   case 2:  Subsequent message, when user already knows we are reading
-   symbols and we can just state our piece.  */
+static struct complain complaint_sentinel;
 
-static int complaint_series = 0;
+/* The symbol table complaint table.  */
 
-\f
+static const char *symfile_explanations[] = {
+  "During symbol reading, %s.\n",
+  "During symbol reading...%s...",
+  "%s...",
+  "%s...",
+  NULL
+};
 
-/* Functions to handle complaints during symbol reading.  */
+static struct complaints symfile_complaint_book = {
+  &complaint_sentinel,
+  0,
+  symfile_explanations
+};
+struct complaints *symfile_complaints = &symfile_complaint_book;
 
-/* Print a complaint about the input symbols, and link the complaint block
-   into a chain for later handling.  */
+/* Wrapper function to, on-demand, fill in a complaints object.  */
 
-void
-complain (struct complaint *complaint,...)
+static struct complaints *
+get_complaints (struct complaints **c)
 {
-  va_list args;
-  va_start (args, complaint);
+  if ((*c) != NULL)
+    return (*c);
+  (*c) = XMALLOC (struct complaints);
+  (*c)->root = &complaint_sentinel;
+  (*c)->series = ISOLATED_MESSAGE;
+  (*c)->explanation = NULL;
+  return (*c);
+}
 
-  complaint->counter++;
-  if (complaint->next == NULL)
+static struct complain *
+find_complaint (struct complaints *complaints, const char *file,
+		int line, const char *fmt)
+{
+  struct complain *complaint;
+
+  /* Find the complaint in the table.  A more efficient search
+     algorithm (based on hash table or something) could be used.  But
+     that can wait until someone shows evidence that this lookup is
+     a real bottle neck.  */
+  for (complaint = complaints->root;
+       complaint != NULL;
+       complaint = complaint->next)
     {
-      complaint->next = complaint_root->next;
-      complaint_root->next = complaint;
+      if (complaint->fmt == fmt
+	  && complaint->file == file
+	  && complaint->line == line)
+	return complaint;
     }
+
+  /* Oops not seen before, fill in a new complaint.  */
+  complaint = XMALLOC (struct complain);
+  complaint->fmt = fmt;
+  complaint->file = file;
+  complaint->line = line;
+  complaint->counter = 0;
+  complaint->next = NULL;
+
+  /* File it, return it.  */
+  complaint->next = complaints->root;
+  complaints->root = complaint;
+  return complaint;
+}
+
+
+/* How many complaints about a particular thing should be printed
+   before we stop whining about it?  Default is no whining at all,
+   since so many systems have ill-constructed symbol files.  */
+
+static unsigned int stop_whining = 0;
+
+/* Print a complaint, and link the complaint block into a chain for
+   later handling.  */
+
+static void
+vcomplaint (struct complaints **c, const char *file, int line, const char *fmt,
+	    va_list args)
+{
+  struct complaints *complaints = get_complaints (c);
+  struct complain *complaint = find_complaint (complaints, file, line, fmt);
+  enum complaint_series series;
+  gdb_assert (complaints != NULL);
+
+  complaint->counter++;
   if (complaint->counter > stop_whining)
+    return;
+
+  if (info_verbose)
+    series = SUBSEQUENT_MESSAGE;
+  else
+    series = complaints->series;
+
+  if (complaint->file != NULL)
+    internal_vwarning (complaint->file, complaint->line, complaint->fmt, args);
+  else if (warning_hook)
+    (*warning_hook) (complaint->fmt, args);
+  else
     {
-      return;
+      if (complaints->explanation == NULL)
+	vwarning (complaint->fmt, args);
+      else
+	{
+	  char *msg;
+	  struct cleanup *cleanups;
+	  xvasprintf (&msg, complaint->fmt, args);
+	  cleanups = make_cleanup (xfree, msg);
+	  wrap_here ("");
+	  if (series != SUBSEQUENT_MESSAGE)
+	    begin_line ();
+	  fprintf_filtered (gdb_stderr,
+			    complaints->explanation[series],
+			    msg);
+	  wrap_here ("");
+	  do_cleanups (cleanups);
+	}
     }
-  wrap_here ("");
 
-  switch (complaint_series + (info_verbose << 1))
+  switch (series)
     {
-
-      /* Isolated messages, must be self-explanatory.  */
-    case 0:
-      if (warning_hook)
-        (*warning_hook) (complaint->message, args);
-      else
-        {
-          begin_line ();
-          fputs_filtered ("During symbol reading, ", gdb_stderr);
-          wrap_here ("");
-          vfprintf_filtered (gdb_stderr, complaint->message, args);
-          fputs_filtered (".\n", gdb_stderr);
-        }
+    case ISOLATED_MESSAGE:
       break;
-
-      /* First of a series, without `set verbose'.  */
-    case 1:
-      if (warning_hook)
-        (*warning_hook) (complaint->message, args);
-      else
-        {
-          begin_line ();
-          fputs_filtered ("During symbol reading...", gdb_stderr);
-          vfprintf_filtered (gdb_stderr, complaint->message, args);
-          fputs_filtered ("...", gdb_stderr);
-          wrap_here ("");
-          complaint_series++;
-        }
+    case FIRST_MESSAGE:
+      complaints->series = SUBSEQUENT_MESSAGE;
+      break;
+    case SUBSEQUENT_MESSAGE:
+    case SHORT_FIRST_MESSAGE:
+      complaints->series = SUBSEQUENT_MESSAGE;
       break;
-
-      /* Subsequent messages of a series, or messages under `set verbose'.
-         (We'll already have produced a "Reading in symbols for XXX..."
-         message and will clean up at the end with a newline.)  */
-    default:
-      if (warning_hook)
-        (*warning_hook) (complaint->message, args);
-      else
-        {
-          vfprintf_filtered (gdb_stderr, complaint->message, args);
-          fputs_filtered ("...", gdb_stderr);
-          wrap_here ("");
-        }
     }
-  /* If GDB dumps core, we'd like to see the complaints first.  Presumably
-     GDB will not be sending so many complaints that this becomes a
-     performance hog.  */
+
+  /* If GDB dumps core, we'd like to see the complaints first.
+     Presumably GDB will not be sending so many complaints that this
+     becomes a performance hog.  */
+
   gdb_flush (gdb_stderr);
+}
+
+void
+complaint (struct complaints **complaints, const char *fmt, ...)
+{
+  va_list args;
+  va_start (args, fmt);
+  vcomplaint (complaints, NULL/*file*/, 0/*line*/, fmt, args);
+  va_end (args);
+}
+
+void
+internal_complaint (struct complaints **complaints, const char *file,
+		    int line, const char *fmt, ...)
+{
+  va_list args;
+  va_start (args, fmt);
+  vcomplaint (complaints, file, line, fmt, args);
+  va_end (args);
+}
+
+void
+complain (struct complaint *complaint, ...)
+{
+  va_list args;
+  va_start (args, complaint);
+  vcomplaint (&symfile_complaints, NULL/*file*/, 0/*line*/,
+	      complaint->message, args);
   va_end (args);
 }
 
-/* Clear out all complaint counters that have ever been incremented.
-   If sym_reading is 1, be less verbose about successive complaints,
-   since the messages are appearing all together during a command that
-   reads symbols (rather than scattered around as psymtabs get fleshed
-   out into symtabs at random times).  If noisy is 1, we are in a
-   noisy symbol reading command, and our caller will print enough
-   context for the user to figure it out.  */
+/* Clear out / initialize all complaint counters that have ever been
+   incremented.  If LESS_VERBOSE is 1, be less verbose about
+   successive complaints, since the messages are appearing all
+   together during a command that is reporting a contiguous block of
+   complaints (rather than being interleaved with other messages).  If
+   noisy is 1, we are in a noisy command, and our caller will print
+   enough context for the user to figure it out.  */
 
 void
-clear_complaints (int sym_reading, int noisy)
+clear_complaints (struct complaints **c, int less_verbose, int noisy)
 {
-  struct complaint *p;
+  struct complaints *complaints = get_complaints (c);
+  struct complain *p;
 
-  for (p = complaint_root->next; p != complaint_root; p = p->next)
+  for (p = complaints->root; p != NULL; p = p->next)
     {
       p->counter = 0;
     }
 
-  if (!sym_reading && !noisy && complaint_series > 1 && !warning_hook)
+  if (complaints->series > 1 && !warning_hook)
     {
       /* Terminate previous series, since caller won't.  */
       puts_filtered ("\n");
     }
 
-  complaint_series = sym_reading ? 1 + noisy : 0;
+  if (!less_verbose)
+    complaints->series = ISOLATED_MESSAGE;
+  else if (!noisy)
+    complaints->series = FIRST_MESSAGE;
+  else
+    complaints->series = SHORT_FIRST_MESSAGE;
 }
 
 void
 _initialize_complaints (void)
 {
-  add_show_from_set
-    (add_set_cmd ("complaints", class_support, var_zinteger,
-		  (char *) &stop_whining,
-		  "Set max number of complaints about incorrect symbols.",
-		  &setlist),
-     &showlist);
+  add_setshow_cmd ("complaints", class_support, var_zinteger,
+		   &stop_whining,
+		   "Set max number of complaints about incorrect symbols.",
+		   "Show max number of complaints about incorrect symbols.",
+		   NULL, NULL,
+		   &setlist, &showlist);
 
 }
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	19 Sep 2002 00:38:57 -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.
 
@@ -23,8 +24,37 @@
 #if !defined (COMPLAINTS_H)
 #define COMPLAINTS_H
 
+/* Opaque object used to track the number of complaints of a
+   particular category.  */
+struct complaints;
+
+/* Predefined categories.  */
+extern struct complaints *symfile_complaints;
+
+/* Register a complaint.  */
+extern void complaint (struct complaints **complaints, const char *fmt,
+		       ...) ATTR_FORMAT (printf, 2, 3);
+extern void internal_complaint (struct complaints **complaints,
+				const char *file, int line, const char *fmt,
+				...) ATTR_FORMAT (printf, 4, 5);
+
+/* Clear out / initialize all complaint counters that have ever been
+   incremented.  If LESS_VERBOSE is 1, be less verbose about
+   successive complaints, since the messages are appearing all
+   together during a command that is reporting a contiguous block of
+   complaints (rather than being interleaved with other messages).  If
+   noisy is 1, we are in a noisy command, and our caller will print
+   enough context for the user to figure it out.  */
+
+extern void clear_complaints (struct complaints **complaints,
+			      int less_verbose, int noisy);
+
+
+/* Legacy interfaces to keep the old code working (until it is all
+   converted to the above).  While the structure below contains a
+   number of fields, all but .message are ignored.
 
-/* Support for complaining about things in the symbol file that aren't
+   Support for complaining about things in the symbol file that aren't
    catastrophic.
 
    Each such thing gets a counter.  The first time we have the problem,
@@ -32,22 +62,12 @@
    if verbose, we report how many of each problem we had.  */
 
 struct complaint
-  {
-    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.  */
-
-extern struct complaint complaint_root[1];
-
-/* Functions that handle complaints.  (in complaints.c)  */
+{
+  const char *message;
+  unsigned counter_ignored;
+  struct complaint *next_ignored;
+};
 
 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	19 Sep 2002 00:38:57 -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);
@@ -706,7 +691,7 @@
      initial symbol reading for this file. */
 
   (*objfile->sf->sym_init) (objfile);
-  clear_complaints (1, verbo);
+  clear_complaints (&symfile_complaints, 1, verbo);
 
   (*objfile->sf->sym_offsets) (objfile, addrs);
 
@@ -818,7 +803,7 @@
     }
 
   /* We're done reading the symbol file; finish off complaints.  */
-  clear_complaints (0, verbo);
+  clear_complaints (&symfile_complaints, 0, verbo);
 }
 
 /* Process a symbol file, as either the main file or as a dynamically
@@ -1801,7 +1786,7 @@
 		}
 
 	      (*objfile->sf->sym_init) (objfile);
-	      clear_complaints (1, 1);
+	      clear_complaints (&symfile_complaints, 1, 1);
 	      /* The "mainline" parameter is a hideous hack; I think leaving it
 	         zero is OK since dbxread.c also does what it needs to do if
 	         objfile->global_psymbols.size is 0.  */
@@ -1815,7 +1800,7 @@
 	      objfile->flags |= OBJF_SYMS;
 
 	      /* We're done reading the symbol file; finish off complaints.  */
-	      clear_complaints (0, 1);
+	      clear_complaints (&symfile_complaints, 0, 1);
 
 	      /* Getting new symbols may change our opinion about what is
 	         frameless.  */
@@ -2305,15 +2290,16 @@
 	  || BLOCK_NSYMS (BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK))
 	  || BLOCK_NSYMS (BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK)))
 	{
-	  complain (&oldsyms_complaint, name);
-
+	  complaint (&symfile_complaints, "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 (&symfile_complaints, "Empty symbol table found for `%s'",
+		     name);
 	}
 
       free_symtab (s);