Re: [RFC] add 'save-breakpoints' command

[Klee Dienes <klee at apple dot com>]

The following patch overlaps a bit with the previous one (it requires
the symnbol_generation support), but otherwise it should be
standalone.  I'm mainly submitting it now because it is required by
some of the Objective-C patches, but is significant enough in it's own
right that I wanted to address any issues with it separately rather
than as part of the Objective-C submission.

There are two primary things changed by the patch:

1) is that functions in the target used by GDB ("malloc",
"scm_lookup_cstr", and later a ton of Objective-C functions) now have
their values cached and re-used unless the symbol table has changed
in-between calls.  This is a performance win overall, and a particular
win when dispatching Objective-C method calls and looking up
Objective-C type information from the runtime.

2) is potentially a bit more controversial.  It adds "expected return
type" support to the target function call interface.  The "expected
return type" is specified using cast syntax; it is ignored if actual
type information for the function is available.  If no type
information is provided for a function, the user is now required to
provide an expected return type using the cast syntax, or get an
error.

Some examples:

(gdb) print fabs (3.0)			(no type information present)
Unable to call function at 0x%lx: no return type information available.
To call this function anyway, you can cast the return type explicitly
(e.g. 'print (float) fabs (3.0)').

(gdb) print (float) fabs (-3.12)	(no type information present)
$1 = 3.11999989

(gdb) print (int) fabs (-3.12)		(no type information present)
$2 = 1074329026
(here, the return value of `fabs' has been interpreted as an int
... not cast to one)

(gdb) print (int) fabs (-3.12)		(type information present)
$3 = -3
(since type information was available, the return value was
interpreted as a float, then cast to an int).

(gdb) print (struct stat) fabs (-3.12)	(no type information present)
Cannot access memory at address 0xf5c28f6
(this causes an error, since the struct return convention is being
misused)

  struct s { int i; int j; };
  struct s f (void) { struct s ss = { 3, 1 }; return ss; }

(gdb) print f ()			(no type information present)
Unable to call function at 0x%lx: no return type information available.
To call this function anyway, you can cast the return type explicitly
(e.g. 'print (float) fabs (3.0)').

(gdb) print (struct s) f (3.0)		(no type information present)
$4 = {i = 3, j = 1}

(gdb) print (float) f (3.0)		(no type information present)
Program received signal SIGSEGV, Segmentation fault.
0x0804841d in f ()
(here we lied to GDB about the return type, causing it to generate
incorrect struct return code).

Compare this to the old behavior:

(gdb) print fabs (3.0)			(no type information present)
$1 = 1074266112
(GDB has no type information available, so it assumes 'int').

(gdb) print (float) fabs (-3.12)	(no type information present)
$2 = 1.07426611e+09
(GDB still assumes the function returns 'int', then casts the result
to a float).

  struct s { int i; int j; };
  struct s f (void) { struct s ss = { 3, 1 }; return ss; }

(gdb) print f ()
Program received signal SIGSEGV, Segmentation fault.
0x0804841d in f ()
(here there is no way out ... no matter what we pass in as the return
value to f(), GDB will generate the calling convention for a function
returning int)

The reason I suggest 2) might be controversial is that the old
behavior can be convenient in the general case.  A lot of functions
*do* return int, and when debugging programs without symbols, it can
be annoying to have to declare the return type, just to make the rare
case when functions return floats/structs behave correctly.  But I'd
argue that it's not *that* big a deal to cast a return value, and
correctness must always take priority.

We've been using a variant of this patch at Apple for a little over a
year; it seems to be working.  The test suite shows four additional
unexpected successes when I run it with this patch, but I figure
that's probably OK.

I haven't included documentation patches ... where (if anywhere) would
you suggest I include them?  I wasn't able to find a compelling spot.
Or perhaps one could argue that the new behavior is simply "correct,"
and the new message you get from print fabs (3.0) is enough.


2001-12-12  Klee Dienes <kdienes@apple.com>

	* breakpoint.c (breakpoint_re_set, breakpoint_re_set_all,
	breakpoint_update): Instead of re-parsing all deferred breakpoints
	every time breakpoint_re_set is called, increment a generation
	number.  When breakpoints need to be up-to-date, call
	breakpoint_update.  This prevents unnecessary re-parsing of
	breakpoint information (and massive future-break spam) when
	multiple shared libraries are loaded at the same time.

	* breakpoint.h: export symbol_generation, breakpoint_update.

	* eval.c (evaluate_subexp_standard): when called with
	EVAL_AVOID_SIDE_EFFECTS, return the passed-in `expect_type' if the
	type of the evaluated expression is unknown.  Also update to use
	the new `call_function_by_hand_expecting_type' interface.

	* gdbtypes.c, gdbtypes.h: add new type
	builtin_type_voidptrfuncptr.

	* parse.c, parse.h: export msym_{text,data,unknown}_symbol_type.
	Make them of type TYPE_CODE_ERROR instead of TYPE_CODE_INT.

	* scm-lang.c (scm_lookup_name): update to use new interface to
	find_function_in_inferior.

	* valops.c (create_cached_function, lookup_cached_function): add.
	These functions create a new data type (a `cached_value'), whose
	purpose is to store the lookup of commonly used symbols GDB needs
	from the inferior.  For example, evaluating the expression 'print
	"hello"' causes GDB to call `malloc' in the target.  Looking up
	the symbol for `malloc' takes a non-trivial amount of time, and
	has no need to be done if the symbols have not changed.
	create/lookup_cached_function allow GDB to cache the results of
	these lookups; re-looking them up only when the symbols have
	changed.
	(find_function_in_inferior): add a default type as second
	argument.  This type will be used for the returned value if no
	type information is available for the function (previously, the
	type was assumed to be (char *) (*).
	(hand_function_call): add new `expect_type' parameter.  This type
	will be used as the return type for the function if no type
	information is available.
	(call_function_by_hand_expecting_type): like
	call_function_by_hand, but uses the new `expect_type' interface to
	hand_function_call.
	(call_function_by_hand): implement as a wrapper to
	call_function_by_hand_expecting_type.
	(value_allocate_space_in_inferior): use new cached_function
	interface.
	(find_function_addr): if asked the return type of a function with
	type TYPE_CODE_ERROR, return TYPE_CODE_ERROR.
	
	* value.h (call_function_by_hand_expecting_type): add.
	(cached_value) add.
	(create_cached_function) add.
	(lookup_cached_function) add.
	(find_function_in_inferior) update to new signature.


Index: gdb/breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.57
diff -u -r1.57 breakpoint.c
--- breakpoint.c	2001/11/11 16:39:59	1.57
+++ breakpoint.c	2001/12/14 19:42:15
@@ -70,8 +70,12 @@
 
 static void ignore_command (char *, int);
 
+void breakpoint_update (void);
+
 static int breakpoint_re_set_one (PTR);
 
+static void breakpoint_re_set_all (void);
+
 static void clear_command (char *, int);
 
 static void catch_command (char *, int);
@@ -720,6 +724,7 @@
   static char message1[] = "Error inserting catchpoint %d:\n";
   static char message[sizeof (message1) + 30];
 
+  breakpoint_update ();
 
   ALL_BREAKPOINTS_SAFE (b, temp)
   {
@@ -7187,9 +7192,26 @@
   return 0;
 }
 
-/* Re-set all breakpoints after symbols have been re-loaded.  */
+unsigned int symbol_generation = 1;
+static unsigned int breakpoint_generation = 0;
+
+void breakpoint_update (void)
+{
+  if (breakpoint_generation != symbol_generation) {
+    breakpoint_re_set_all ();
+    breakpoint_generation = symbol_generation;
+  }
+}
+
 void
 breakpoint_re_set (void)
+{
+  symbol_generation++;
+}
+
+/* Re-set all breakpoints after symbols have been re-loaded.  */
+static void
+breakpoint_re_set_all (void)
 {
   struct breakpoint *b, *temp;
   enum language save_language;
Index: gdb/breakpoint.h
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.h,v
retrieving revision 1.10
diff -u -r1.10 breakpoint.h
--- breakpoint.h	2001/10/20 23:54:29	1.10
+++ breakpoint.h	2001/12/14 19:42:16
@@ -305,6 +305,8 @@
 
 typedef struct bpstats *bpstat;
 
+extern unsigned int symbol_generation;
+
 /* Interface:  */
 /* Clear a bpstat so that it says we are not at any breakpoint.
    Also free any storage that is part of a bpstat.  */
@@ -525,6 +527,8 @@
 extern int breakpoint_thread_match (CORE_ADDR, ptid_t);
 
 extern void until_break_command (char *, int);
+
+extern void breakpoint_update (void);
 
 extern void breakpoint_re_set (void);
 
Index: gdb/eval.c
===================================================================
RCS file: /cvs/src/src/gdb/eval.c,v
retrieving revision 1.17
diff -u -r1.17 eval.c
--- eval.c	2001/12/10 23:05:00	1.17
+++ eval.c	2001/12/14 19:42:18
@@ -928,15 +928,20 @@
 	     gdb isn't asked for it's opinion (ie. through "whatis"),
 	     it won't offer it. */
 
-	  struct type *ftype =
+	  struct type *type =
 	  TYPE_TARGET_TYPE (VALUE_TYPE (argvec[0]));
 
-	  if (ftype)
-	    return allocate_value (TYPE_TARGET_TYPE (VALUE_TYPE (argvec[0])));
+	  if (type)
+	    {
+	      if ((TYPE_CODE (type) == TYPE_CODE_ERROR) && expect_type)
+		return allocate_value (expect_type);
+	      else
+		return allocate_value (type);
+	    }
 	  else
 	    error ("Expression of type other than \"Function returning ...\" used as function");
 	}
-      return call_function_by_hand (argvec[0], nargs, argvec + 1);
+      return call_function_by_hand_expecting_type (argvec[0], expect_type, nargs, argvec + 1);
       /* pai: FIXME save value from call_function_by_hand, then adjust pc by adjust_fn_pc if +ve  */
 
     case OP_F77_UNDETERMINED_ARGLIST:
Index: gdb/gdbtypes.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbtypes.c,v
retrieving revision 1.34
diff -u -r1.34 gdbtypes.c
--- gdbtypes.c	2001/12/12 02:11:51	1.34
+++ gdbtypes.c	2001/12/14 19:42:22
@@ -96,6 +96,7 @@
 struct type *builtin_type_void_func_ptr;
 struct type *builtin_type_CORE_ADDR;
 struct type *builtin_type_bfd_vma;
+struct type *builtin_type_voidptrfuncptr;
 
 int opaque_type_resolution = 1;
 int overload_debug = 0;
@@ -3208,6 +3209,8 @@
     init_type (TYPE_CODE_INT, TARGET_BFD_VMA_BIT / 8,
 	       TYPE_FLAG_UNSIGNED,
 	       "__bfd_vma", (struct objfile *) NULL);
+  builtin_type_voidptrfuncptr =
+    lookup_pointer_type (lookup_function_type (lookup_pointer_type (builtin_type_void)));
 }
 
 
Index: gdb/gdbtypes.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbtypes.h,v
retrieving revision 1.21
diff -u -r1.21 gdbtypes.h
--- gdbtypes.h	2001/12/10 06:17:01	1.21
+++ gdbtypes.h	2001/12/14 19:42:24
@@ -930,6 +930,7 @@
    bit address type even though the TARGET has a 64 bit pointer type
    (cf MIPS). */
 extern struct type *builtin_type_bfd_vma;
+extern struct type *builtin_type_voidptrfuncptr;
 
 /* Explicit sizes - see C9X <intypes.h> for naming scheme */
 extern struct type *builtin_type_int8;
Index: gdb/parse.c
===================================================================
RCS file: /cvs/src/src/gdb/parse.c,v
retrieving revision 1.17
diff -u -r1.17 parse.c
--- parse.c	2001/11/15 01:55:59	1.17
+++ parse.c	2001/12/14 19:42:25
@@ -399,9 +399,9 @@
    based on the language, but they no longer have names like "int", so
    the initial rationale is gone.  */
 
-static struct type *msym_text_symbol_type;
-static struct type *msym_data_symbol_type;
-static struct type *msym_unknown_symbol_type;
+struct type *msym_text_symbol_type;
+struct type *msym_data_symbol_type;
+struct type *msym_unknown_symbol_type;
 
 void
 write_exp_msymbol (struct minimal_symbol *msymbol, 
@@ -1360,12 +1360,12 @@
 
   msym_text_symbol_type =
     init_type (TYPE_CODE_FUNC, 1, 0, "<text variable, no debug info>", NULL);
-  TYPE_TARGET_TYPE (msym_text_symbol_type) = builtin_type_int;
+  TYPE_TARGET_TYPE (msym_text_symbol_type) = builtin_type_error;
   msym_data_symbol_type =
-    init_type (TYPE_CODE_INT, TARGET_INT_BIT / HOST_CHAR_BIT, 0,
+    init_type (TYPE_CODE_ERROR, 0, 0,
 	       "<data variable, no debug info>", NULL);
   msym_unknown_symbol_type =
-    init_type (TYPE_CODE_INT, 1, 0,
+    init_type (TYPE_CODE_ERROR, 0, 0,
 	       "<variable (not text or data), no debug info>",
 	       NULL);
 
Index: gdb/parser-defs.h
===================================================================
RCS file: /cvs/src/src/gdb/parser-defs.h,v
retrieving revision 1.6
diff -u -r1.6 parser-defs.h
--- parser-defs.h	2001/11/15 01:55:59	1.6
+++ parser-defs.h	2001/12/14 19:42:25
@@ -39,6 +39,10 @@
 extern int expout_size;
 extern int expout_ptr;
 
+extern struct type *msym_text_symbol_type;
+extern struct type *msym_data_symbol_type;
+extern struct type *msym_unknown_symbol_type;
+
 /* If this is nonzero, this block is used as the lexical context
    for symbol names.  */
 
Index: gdb/scm-lang.c
===================================================================
RCS file: /cvs/src/src/gdb/scm-lang.c,v
retrieving revision 1.6
diff -u -r1.6 scm-lang.c
--- scm-lang.c	2001/11/10 20:44:38	1.6
+++ scm-lang.c	2001/12/14 19:42:26
@@ -169,7 +169,7 @@
     /* FIXME in this case, we should try lookup_symbol first */
     args[2] = value_from_longest (builtin_type_scm, SCM_EOL);
 
-  func = find_function_in_inferior ("scm_lookup_cstr");
+  func = find_function_in_inferior ("scm_lookup_cstr", builtin_type_voidptrfuncptr);
   val = call_function_by_hand (func, 3, args);
   if (!value_logical_not (val))
     return value_ind (val);
@@ -192,7 +192,7 @@
   write_memory (iaddr, str, len);
   /* FIXME - should find and pass env */
   write_memory (iaddr + len, "", 1);
-  func = find_function_in_inferior ("scm_evstr");
+  func = find_function_in_inferior ("scm_evstr", builtin_type_voidptrfuncptr);
   return call_function_by_hand (func, 1, &addr);
 }
 
Index: gdb/valops.c
===================================================================
RCS file: /cvs/src/src/gdb/valops.c,v
retrieving revision 1.44
diff -u -r1.44 valops.c
--- valops.c	2001/12/12 02:11:52	1.44
+++ valops.c	2001/12/14 19:42:30
@@ -32,6 +32,7 @@
 #include "gdbcmd.h"
 #include "regcache.h"
 #include "cp-abi.h"
+#include "parser-defs.h"
 
 #include <errno.h>
 #include "gdb_string.h"
@@ -88,11 +89,49 @@
 int unwind_on_signal_p = 0;
 
 
+cached_value_ptr
+create_cached_function (char *name, struct type *type)
+{
+  cached_value_ptr ptr;
+
+  ptr = (struct cached_value *) xmalloc (sizeof (struct cached_value));
+  ptr->name = xstrdup (name);
+  ptr->type = type;
+  memset (&ptr->val, 0, sizeof (struct value));
+  ptr->generation = (unsigned int) -1;
+
+  return ptr;
+}
+
+value_ptr
+lookup_cached_function (cached_value_ptr cval)
+{
+  value_ptr val = NULL;
+  value_ptr next = NULL;
+
+  if (cval->generation != symbol_generation)
+    {
+      val = find_function_in_inferior (cval->name, cval->type);
+      cval->val = *val;
+      cval->val.next = NULL;
+      cval->generation = symbol_generation;
+    }
+
+  val = allocate_value (cval->val.type);
+  next = val->next;
+  *val = cval->val;
+  val->next = next;
+
+  return val;
+}
 
-/* Find the address of function name NAME in the inferior.  */
+/* Find the address of function name NAME in the inferior.  If no type
+   information is available for NAME, use `type' as the type for the
+   resulting value.
+*/
 
 value_ptr
-find_function_in_inferior (char *name)
+find_function_in_inferior (char *name, struct type *type)
 {
   register struct symbol *sym;
   sym = lookup_symbol (name, 0, VAR_NAMESPACE, 0, NULL);
@@ -110,20 +149,18 @@
       struct minimal_symbol *msymbol = lookup_minimal_symbol (name, NULL, NULL);
       if (msymbol != NULL)
 	{
-	  struct type *type;
-	  CORE_ADDR maddr;
-	  type = lookup_pointer_type (builtin_type_char);
-	  type = lookup_function_type (type);
-	  type = lookup_pointer_type (type);
-	  maddr = SYMBOL_VALUE_ADDRESS (msymbol);
-	  return value_from_pointer (type, maddr);
+	  if (type != NULL)
+	    return value_from_longest (type, (LONGEST) SYMBOL_VALUE_ADDRESS (msymbol));
+	  else
+	    return value_from_longest (lookup_pointer_type (msym_text_symbol_type),
+				       (LONGEST) SYMBOL_VALUE_ADDRESS (msymbol));
 	}
       else
 	{
 	  if (!target_has_execution)
 	    error ("evaluation of this expression requires the target program to be active");
 	  else
-	    error ("evaluation of this expression requires the program to have a function \"%s\".", name);
+	    error ("evaluation of this expression requires the program to have a function named \"%s\".", name);
 	}
     }
 }
@@ -135,10 +172,14 @@
 value_allocate_space_in_inferior (int len)
 {
   value_ptr blocklen;
-  register value_ptr val = find_function_in_inferior ("malloc");
+  value_ptr val;
+  static cached_value_ptr fval = NULL;
+  
+  if (fval == NULL) 
+    fval = create_cached_function ("malloc", builtin_type_voidptrfuncptr); 
 
   blocklen = value_from_longest (builtin_type_int, (LONGEST) len);
-  val = call_function_by_hand (val, 1, &blocklen);
+  val = call_function_by_hand (lookup_cached_function (fval), 1, &blocklen);
   if (value_logical_not (val))
     {
       if (!target_has_execution)
@@ -1258,6 +1299,11 @@
 
       value_type = builtin_type_int;
     }
+  else if (code == TYPE_CODE_ERROR)
+    {
+      value_type = builtin_type_error;
+      funaddr = (CORE_ADDR) -1;
+    }
   else
     error ("Invalid data type for function to be called.");
 
@@ -1283,10 +1329,9 @@
 
    ARGS is modified to contain coerced values. */
 
-static value_ptr hand_function_call (value_ptr function, int nargs,
-				     value_ptr * args);
 static value_ptr
-hand_function_call (value_ptr function, int nargs, value_ptr *args)
+hand_function_call (value_ptr function,
+		    struct type *expect_type, int nargs, value_ptr *args)
 {
   register CORE_ADDR sp;
   register int i;
@@ -1333,6 +1378,32 @@
   inf_status = save_inferior_status (1);
   old_chain = make_cleanup_restore_inferior_status (inf_status);
 
+  funaddr = find_function_addr (function, &value_type);
+  CHECK_TYPEDEF (value_type);
+  
+  if ((value_type == NULL) || (value_type->code == TYPE_CODE_ERROR))
+    value_type = expect_type;
+  
+  if ((value_type == NULL) || (value_type->code == TYPE_CODE_ERROR))
+    error ("Unable to call function at 0x%lx: no return type information available.\n"
+           "To call this function anyway, you can cast the return type explicitly\n"
+	   "(e.g. 'print (float) fabs (3.0)')",
+           (unsigned long) funaddr);
+
+  CHECK_TYPEDEF (value_type);
+
+  {
+    struct block *b = block_for_pc (funaddr);
+    /* If compiled without -g, assume GCC 2.  */
+    using_gcc = (b == NULL ? 2 : BLOCK_GCC_COMPILED (b));
+  }
+
+  /* Are we returning a value using a structure return or a normal
+     value return? */
+
+  struct_return = using_struct_return (function, funaddr, value_type,
+				       using_gcc);
+
   /* PUSH_DUMMY_FRAME is responsible for saving the inferior registers
      (and POP_FRAME for restoring them).  (At least on most machines)
      they are saved on the stack in the inferior.  */
@@ -1353,21 +1424,6 @@
       sp += sizeof_dummy1;
     }
 
-  funaddr = find_function_addr (function, &value_type);
-  CHECK_TYPEDEF (value_type);
-
-  {
-    struct block *b = block_for_pc (funaddr);
-    /* If compiled without -g, assume GCC 2.  */
-    using_gcc = (b == NULL ? 2 : BLOCK_GCC_COMPILED (b));
-  }
-
-  /* Are we returning a value using a structure return or a normal
-     value return? */
-
-  struct_return = using_struct_return (function, funaddr, value_type,
-				       using_gcc);
-
   /* Create a call sequence customized for this function
      and the number of arguments for it.  */
   for (i = 0; i < (int) (SIZEOF_CALL_DUMMY_WORDS / sizeof (dummy[0])); i++)
@@ -1785,16 +1841,24 @@
 }
 
 value_ptr
-call_function_by_hand (value_ptr function, int nargs, value_ptr *args)
+call_function_by_hand_expecting_type (value_ptr function, 
+				      struct type *expect_type,
+				      int nargs, value_ptr *args)
 {
   if (CALL_DUMMY_P)
     {
-      return hand_function_call (function, nargs, args);
+      return hand_function_call (function, expect_type, nargs, args);
     }
   else
     {
       error ("Cannot invoke functions on this machine.");
     }
+}
+
+value_ptr
+call_function_by_hand (value_ptr function, int nargs, value_ptr *args)
+{
+  call_function_by_hand_expecting_type (function, NULL, nargs, args);
 }
 
 
Index: gdb/value.h
===================================================================
RCS file: /cvs/src/src/gdb/value.h,v
retrieving revision 1.22
diff -u -r1.22 value.h
--- value.h	2001/10/16 01:58:07	1.22
+++ value.h	2001/12/14 19:42:31
@@ -551,6 +551,10 @@
 
 extern value_ptr call_function_by_hand (value_ptr, int, value_ptr *);
 
+extern value_ptr call_function_by_hand_expecting_type (value_ptr,
+						       struct type *, int,
+						       value_ptr *);
+
 extern int default_coerce_float_to_double (struct type *, struct type *);
 
 extern int standard_coerce_float_to_double (struct type *, struct type *);
@@ -559,8 +563,21 @@
 
 extern void find_rt_vbase_offset (struct type *, struct type *, char *, int,
 				  int *, int *);
+
+struct cached_value
+{
+  char *name;
+  struct type *type;
+  struct value val;
+  unsigned int generation;
+};
+typedef struct cached_value *cached_value_ptr;
+
+extern cached_value_ptr create_cached_function (char *, struct type *);
+
+extern value_ptr lookup_cached_function (cached_value_ptr cval);
 
-extern value_ptr find_function_in_inferior (char *);
+extern value_ptr find_function_in_inferior (char *, struct type *);
 
 extern value_ptr value_allocate_space_in_inferior (int);