[RFC] Alternate approach to keeping convenience variables

[Daniel Jacobowitz <drow at false dot org>]

On Tue, Nov 22, 2005 at 05:10:44PM +0000, Andrew STUBBS wrote:
> Well, we could recursively copy the entire type list for type and 
> enclosing_type. This would certainly be enough to print the value and to 
> use it in some expressions. I can imagine that there might be some 
> issues because these types would exist outside of the normal type table 
> and/or might clash with types in that table.
> 
> However, this would require intimate knowledge of the value and type 
> structures and everything they reference. Also, it would be impossible 
> to change the value, even though the existing contents could be read, 
> because parse_expression() wouldn't see the types.

Sure it would - you could still only assign to internal variables
things that you could write in the expression evaluator, so if it's
been reloaded from an objfile, then the name will be in scope again.

> It would need a flag in the internalvar to say it would need freeing if 
> the value changes to something else.

Nah, we can leak this memory.  It's part of the value history, more or
less.

I believe I'd earlier suggested this approach.  Here's an
implementation of it; all comments welcome.  It appears to work. I can
now print "object_files", discard GDB's symbol table, and still print
out its type - which recursively descends as far as the bfd xvec. As a
free bonus it should cut down on the watchpoint segfaults I mentioned
on gdb@ this afternoon, though they still need a separate resolution.

I needed to fix a bug in dwarf2read where some unused pointers would
contain garbage.

The tracepoint.c change is necessary for default.exp.  To see why, try
this:
  $ gdb
  (gdb) show convenience
  $trace_frame = -1
  $tpnum = 0

vs.
  $ gdb /bin/ls
  (gdb) show convenience
  No debugger convenience variables now defined.
  Convenience variables have names starting with "$";
  use "set" as in "set $foo = 5" to define them.

If you want the inconsistency resolve the other way round, please
holler.

How does this look?

-- 
Daniel Jacobowitz
CodeSourcery, LLC

2005-12-09  Daniel Jacobowitz  <dan@codesourcery.com>

	* Makefile.in (gdbtypes_h, gdbtypes.o, utils.o): Update.
	* defs.h (hashtab_obstack_allocate, dummy_obstack_deallocate): Add
	prototypes.
	* dwarf2read.c (read_subroutine_type): Use TYPE_ZALLOC.
	(hashtab_obstack_allocate, dummy_obstack_deallocate): Moved to...
	* utils.c (hashtab_obstack_allocate, dummy_obstack_deallocate):
	...here.
	* gdbtypes.c: Include "hashtab.h".
	(build_gdbtypes): Remove extra prototype.
	(struct type_pair, type_pair_hash, type_pair_eq)
	(create_copied_types_hash, copy_type_recursive): New.
	* gdbtypes.h: Include "hashtab.h".
	(TYPE_ZALLOC): New.
	(create_copied_types_hash, copy_type_recursive): New prototypes.
	* objfiles.c (free_objfile): Call preserve_values.
	* symfile.c (reread_symbols): Likewise.
	(clear_symtab_users): Remove calls to clear_value_history and
	clear_internalvars.
	* value.c (clear_value_history, clear_internalvars): Removed.
	(preserve_one_value, preserve_values): New functions.
	* value.h (clear_value_history, clear_internalvars): Removed.
	(preserve_values): New prototype.

	* tracepoint.c (_initialize_tracepoint): Do not initialize convenience
	variables here.

Index: src/gdb/Makefile.in
===================================================================
--- src.orig/gdb/Makefile.in	2005-12-09 14:48:06.000000000 -0500
+++ src/gdb/Makefile.in	2005-12-09 15:19:49.000000000 -0500
@@ -696,7 +696,7 @@ gdb_stat_h = gdb_stat.h
 gdb_string_h = gdb_string.h
 gdb_thread_db_h = gdb_thread_db.h
 gdbthread_h = gdbthread.h $(breakpoint_h) $(frame_h)
-gdbtypes_h = gdbtypes.h
+gdbtypes_h = gdbtypes.h $(hashtab_h)
 gdb_vfork_h = gdb_vfork.h
 gdb_wait_h = gdb_wait.h
 glibc_tdep_h = glibc-tdep.h
@@ -1979,7 +1979,7 @@ gdb-events.o: gdb-events.c $(defs_h) $(g
 gdbtypes.o: gdbtypes.c $(defs_h) $(gdb_string_h) $(bfd_h) $(symtab_h) \
 	$(symfile_h) $(objfiles_h) $(gdbtypes_h) $(expression_h) \
 	$(language_h) $(target_h) $(value_h) $(demangle_h) $(complaints_h) \
-	$(gdbcmd_h) $(wrapper_h) $(cp_abi_h) $(gdb_assert_h)
+	$(gdbcmd_h) $(wrapper_h) $(cp_abi_h) $(gdb_assert_h) $(hashtab_h)
 glibc-tdep.o: glibc-tdep.c $(defs_h) $(frame_h) $(symtab_h) $(symfile_h) \
 	$(objfiles_h) $(glibc_tdep_h)
 gnu-nat.o: gnu-nat.c $(gdb_string_h) $(defs_h) $(inferior_h) $(symtab_h) \
@@ -2714,7 +2714,7 @@ utils.o: utils.c $(defs_h) $(gdb_assert_
 	$(exceptions_h) $(tui_h) $(gdbcmd_h) $(serial_h) $(bfd_h) \
 	$(target_h) $(demangle_h) $(expression_h) $(language_h) $(charset_h) \
 	$(annotate_h) $(filenames_h) $(symfile_h) $(inferior_h) \
-	$(gdb_curses_h) $(readline_h)
+	$(gdb_curses_h) $(readline_h) $(gdb_obstack_h)
 uw-thread.o: uw-thread.c $(defs_h) $(gdbthread_h) $(target_h) $(inferior_h) \
 	$(regcache_h) $(gregset_h)
 v850-tdep.o: v850-tdep.c $(defs_h) $(frame_h) $(frame_base_h) $(trad_frame_h) \
Index: src/gdb/defs.h
===================================================================
--- src.orig/gdb/defs.h	2005-12-09 14:48:06.000000000 -0500
+++ src/gdb/defs.h	2005-12-09 15:19:49.000000000 -0500
@@ -1217,4 +1217,9 @@ extern int use_windows;
 extern ULONGEST align_up (ULONGEST v, int n);
 extern ULONGEST align_down (ULONGEST v, int n);
 
+/* Allocation and deallocation functions for the libiberty hash table
+   which use obstacks.  */
+void *hashtab_obstack_allocate (void *data, size_t size, size_t count);
+void dummy_obstack_deallocate (void *object, void *data);
+
 #endif /* #ifndef DEFS_H */
Index: src/gdb/dwarf2read.c
===================================================================
--- src.orig/gdb/dwarf2read.c	2005-12-09 14:48:06.000000000 -0500
+++ src/gdb/dwarf2read.c	2005-12-09 15:19:49.000000000 -0500
@@ -1026,10 +1026,6 @@ static char *skip_one_die (char *info_pt
 
 static void free_stack_comp_unit (void *);
 
-static void *hashtab_obstack_allocate (void *data, size_t size, size_t count);
-
-static void dummy_obstack_deallocate (void *object, void *data);
-
 static hashval_t partial_die_hash (const void *item);
 
 static int partial_die_eq (const void *item_lhs, const void *item_rhs);
@@ -4622,7 +4618,7 @@ read_subroutine_type (struct die_info *d
       /* Allocate storage for parameters and fill them in.  */
       TYPE_NFIELDS (ftype) = nparams;
       TYPE_FIELDS (ftype) = (struct field *)
-	TYPE_ALLOC (ftype, nparams * sizeof (struct field));
+	TYPE_ZALLOC (ftype, nparams * sizeof (struct field));
 
       child_die = die->child;
       while (child_die && child_die->tag)
@@ -9617,28 +9613,6 @@ dwarf2_clear_marks (struct dwarf2_per_cu
     }
 }
 
-/* Allocation function for the libiberty hash table which uses an
-   obstack.  */
-
-static void *
-hashtab_obstack_allocate (void *data, size_t size, size_t count)
-{
-  unsigned int total = size * count;
-  void *ptr = obstack_alloc ((struct obstack *) data, total);
-  memset (ptr, 0, total);
-  return ptr;
-}
-
-/* Trivial deallocation function for the libiberty splay tree and hash
-   table - don't deallocate anything.  Rely on later deletion of the
-   obstack.  */
-
-static void
-dummy_obstack_deallocate (void *object, void *data)
-{
-  return;
-}
-
 /* Trivial hash function for partial_die_info: the hash value of a DIE
    is its offset in .debug_info for this objfile.  */
 
Index: src/gdb/gdbtypes.c
===================================================================
--- src.orig/gdb/gdbtypes.c	2005-12-09 14:48:06.000000000 -0500
+++ src/gdb/gdbtypes.c	2005-12-09 15:19:49.000000000 -0500
@@ -1,6 +1,6 @@
 /* Support routines for manipulating internal types for GDB.
    Copyright 1992, 1993, 1994, 1995, 1996, 1998, 1999, 2000, 2001, 2002, 2003,
-   2004 Free Software Foundation, Inc.
+   2004, 2005 Free Software Foundation, Inc.
    Contributed by Cygnus Support, using pieces from other GDB modules.
 
    This file is part of GDB.
@@ -37,6 +37,7 @@
 #include "wrapper.h"
 #include "cp-abi.h"
 #include "gdb_assert.h"
+#include "hashtab.h"
 
 /* These variables point to the objects
    representing the predefined C data types.  */
@@ -3111,7 +3112,132 @@ recursive_dump_type (struct type *type, 
     obstack_free (&dont_print_type_obstack, NULL);
 }
 
-static void build_gdbtypes (void);
+struct type_pair
+{
+  struct type *old, *new;
+};
+
+static hashval_t
+type_pair_hash (const void *item)
+{
+  const struct type_pair *pair = item;
+  return htab_hash_pointer (pair->old);
+}
+
+static int
+type_pair_eq (const void *item_lhs, const void *item_rhs)
+{
+  const struct type_pair *lhs = item_lhs, *rhs = item_rhs;
+  return lhs->old == rhs->old;
+}
+
+htab_t
+create_copied_types_hash (struct objfile *objfile)
+{
+  return htab_create_alloc_ex (1, type_pair_hash, type_pair_eq,
+			       NULL, &objfile->objfile_obstack,
+			       hashtab_obstack_allocate,
+			       dummy_obstack_deallocate);
+}
+
+struct type *
+copy_type_recursive (struct objfile *objfile, struct type *type,
+		     htab_t copied_types)
+{
+  struct type_pair *stored, pair;
+  void **slot;
+  struct type *new_type;
+
+  if (TYPE_OBJFILE (type) == NULL)
+    return type;
+
+  gdb_assert (TYPE_OBJFILE (type) == objfile);
+
+  pair.old = type;
+  slot = htab_find_slot (copied_types, &pair, INSERT);
+  if (*slot != NULL)
+    return ((struct type_pair *) *slot)->new;
+
+  new_type = alloc_type (NULL);
+
+  /* Add the new type to the hash table immediately.  */
+  stored = xmalloc (sizeof (struct type_pair));
+  stored->old = type;
+  stored->new = new_type;
+  *slot = stored;
+
+  /* Copy the common fields of types.  */
+  TYPE_CODE (new_type) = TYPE_CODE (type);
+  TYPE_ARRAY_UPPER_BOUND_TYPE (new_type) = TYPE_ARRAY_UPPER_BOUND_TYPE (type);
+  TYPE_ARRAY_LOWER_BOUND_TYPE (new_type) = TYPE_ARRAY_LOWER_BOUND_TYPE (type);
+  if (TYPE_NAME (type))
+    TYPE_NAME (new_type) = xstrdup (TYPE_NAME (type));
+  if (TYPE_TAG_NAME (type))
+    TYPE_TAG_NAME (new_type) = xstrdup (TYPE_TAG_NAME (type));
+  TYPE_FLAGS (new_type) = TYPE_FLAGS (type);
+  TYPE_VPTR_FIELDNO (new_type) = TYPE_VPTR_FIELDNO (type);
+
+  TYPE_INSTANCE_FLAGS (new_type) = TYPE_INSTANCE_FLAGS (type);
+  TYPE_LENGTH (new_type) = TYPE_LENGTH (type);
+
+  /* Copy the fields.  */
+  TYPE_NFIELDS (new_type) = TYPE_NFIELDS (type);
+  if (TYPE_NFIELDS (type))
+    {
+      int i, nfields;
+
+      nfields = TYPE_NFIELDS (type);
+      TYPE_FIELDS (new_type) = xmalloc (sizeof (struct field) * nfields);
+      for (i = 0; i < nfields; i++)
+	{
+	  TYPE_FIELD_ARTIFICIAL (new_type, i) = TYPE_FIELD_ARTIFICIAL (type, i);
+	  TYPE_FIELD_BITSIZE (new_type, i) = TYPE_FIELD_BITSIZE (type, i);
+	  if (TYPE_FIELD_TYPE (type, i))
+	    TYPE_FIELD_TYPE (new_type, i)
+	      = copy_type_recursive (objfile, TYPE_FIELD_TYPE (type, i),
+				     copied_types);
+	  if (TYPE_FIELD_NAME (type, i))
+	    TYPE_FIELD_NAME (new_type, i) = xstrdup (TYPE_FIELD_NAME (type, i));
+	  if (TYPE_FIELD_STATIC_HAS_ADDR (type, i))
+	    SET_FIELD_PHYSADDR (TYPE_FIELD (new_type, i),
+				TYPE_FIELD_STATIC_PHYSADDR (type, i));
+	  else if (TYPE_FIELD_STATIC (type, i))
+	    SET_FIELD_PHYSNAME (TYPE_FIELD (new_type, i),
+				xstrdup (TYPE_FIELD_STATIC_PHYSNAME (type, i)));
+	  else
+	    {
+	      TYPE_FIELD_BITPOS (new_type, i) = TYPE_FIELD_BITPOS (type, i);
+	      TYPE_FIELD_STATIC_KIND (new_type, i) = 0;
+	    }
+	}
+    }
+
+  /* Copy pointers to other types.  */
+  if (TYPE_TARGET_TYPE (type))
+    TYPE_TARGET_TYPE (new_type) = copy_type_recursive (objfile,
+						       TYPE_TARGET_TYPE (type),
+						       copied_types);
+  if (TYPE_VPTR_BASETYPE (type))
+    TYPE_VPTR_BASETYPE (new_type) = copy_type_recursive (objfile,
+							 TYPE_VPTR_BASETYPE (type),
+							 copied_types);
+  /* Maybe copy the type_specific bits.
+
+     NOTE drow/2005-12-09: We do not copy the C++-specific bits like
+     base classes and methods.  There's no fundamental reason why we
+     can't, but at the moment it is not needed.  */
+
+  if (TYPE_CODE (type) == TYPE_CODE_FLT)
+    TYPE_FLOATFORMAT (new_type) == TYPE_FLOATFORMAT (type);
+  else if (TYPE_CODE (type) == TYPE_CODE_STRUCT
+	   || TYPE_CODE (type) == TYPE_CODE_UNION
+	   || TYPE_CODE (type) == TYPE_CODE_TEMPLATE
+	   || TYPE_CODE (type) == TYPE_CODE_NAMESPACE)
+    INIT_CPLUS_SPECIFIC (new_type);
+
+  return new_type;
+}
+
 static void
 build_gdbtypes (void)
 {
Index: src/gdb/gdbtypes.h
===================================================================
--- src.orig/gdb/gdbtypes.h	2005-12-09 14:48:06.000000000 -0500
+++ src/gdb/gdbtypes.h	2005-12-09 15:19:49.000000000 -0500
@@ -25,6 +25,8 @@
 #if !defined (GDBTYPES_H)
 #define GDBTYPES_H 1
 
+#include "hashtab.h"
+
 /* Forward declarations for prototypes.  */
 struct field;
 struct block;
@@ -1175,6 +1177,12 @@ extern struct type *builtin_type_f_void;
     ? obstack_alloc (&TYPE_OBJFILE (t) -> objfile_obstack, size) \
     : xmalloc (size))
 
+#define TYPE_ZALLOC(t,size)  \
+   (TYPE_OBJFILE (t) != NULL  \
+    ? memset (obstack_alloc (&TYPE_OBJFILE (t)->objfile_obstack, size),  \
+	      0, size)  \
+    : xzalloc (size))
+
 extern struct type *alloc_type (struct objfile *);
 
 extern struct type *init_type (enum type_code, int, int, char *,
@@ -1364,4 +1372,10 @@ extern int is_integral_type (struct type
 
 extern void maintenance_print_type (char *, int);
 
+extern htab_t create_copied_types_hash (struct objfile *objfile);
+
+extern struct type *copy_type_recursive (struct objfile *objfile,
+					 struct type *type,
+					 htab_t copied_types);
+
 #endif /* GDBTYPES_H */
Index: src/gdb/objfiles.c
===================================================================
--- src.orig/gdb/objfiles.c	2005-12-09 14:48:06.000000000 -0500
+++ src/gdb/objfiles.c	2005-12-09 15:19:49.000000000 -0500
@@ -392,6 +392,10 @@ free_objfile (struct objfile *objfile)
       objfile->separate_debug_objfile_backlink->separate_debug_objfile = NULL;
     }
   
+  /* Remove any references to this objfile in the global value
+     lists.  */
+  preserve_values (objfile);
+
   /* First do any symbol file specific actions required when we are
      finished with a particular symbol file.  Note that if the objfile
      is using reusable symbol information (via mmalloc) then each of
Index: src/gdb/symfile.c
===================================================================
--- src.orig/gdb/symfile.c	2005-12-09 14:48:06.000000000 -0500
+++ src/gdb/symfile.c	2005-12-09 15:19:49.000000000 -0500
@@ -2012,6 +2012,10 @@ reread_symbols (void)
 	      memcpy (offsets, objfile->section_offsets,
 		      SIZEOF_N_SECTION_OFFSETS (num_offsets));
 
+	      /* Remove any references to this objfile in the global
+		 value lists.  */
+	      preserve_values (objfile);
+
 	      /* Nuke all the state that we will re-read.  Much of the following
 	         code which sets things to NULL really is necessary to tell
 	         other parts of GDB that there is nothing currently there.  */
@@ -2484,9 +2488,7 @@ clear_symtab_users (void)
      breakpoint_re_set may try to access the current symtab.  */
   clear_current_source_symtab_and_line ();
 
-  clear_value_history ();
   clear_displays ();
-  clear_internalvars ();
   breakpoint_re_set ();
   set_default_breakpoint (0, 0, 0, 0);
   clear_pc_function_cache ();
Index: src/gdb/tracepoint.c
===================================================================
--- src.orig/gdb/tracepoint.c	2005-11-20 19:30:22.000000000 -0500
+++ src/gdb/tracepoint.c	2005-12-09 15:45:30.000000000 -0500
@@ -2694,11 +2694,6 @@ _initialize_tracepoint (void)
   traceframe_number = -1;
   tracepoint_number = -1;
 
-  set_internalvar (lookup_internalvar ("tpnum"),
-		   value_from_longest (builtin_type_int, (LONGEST) 0));
-  set_internalvar (lookup_internalvar ("trace_frame"),
-		   value_from_longest (builtin_type_int, (LONGEST) - 1));
-
   if (tracepoint_list.list == NULL)
     {
       tracepoint_list.listsize = 128;
Index: src/gdb/utils.c
===================================================================
--- src.orig/gdb/utils.c	2005-12-09 14:48:06.000000000 -0500
+++ src/gdb/utils.c	2005-12-09 15:19:49.000000000 -0500
@@ -53,6 +53,7 @@
 #include "annotate.h"
 #include "filenames.h"
 #include "symfile.h"
+#include "gdb_obstack.h"
 
 #include "inferior.h"		/* for signed_pointer_to_address */
 
@@ -3124,3 +3125,25 @@ align_down (ULONGEST v, int n)
   gdb_assert (n && (n & (n-1)) == 0);
   return (v & -n);
 }
+
+/* Allocation function for the libiberty hash table which uses an
+   obstack.  */
+
+void *
+hashtab_obstack_allocate (void *data, size_t size, size_t count)
+{
+  unsigned int total = size * count;
+  void *ptr = obstack_alloc ((struct obstack *) data, total);
+  memset (ptr, 0, total);
+  return ptr;
+}
+
+/* Trivial deallocation function for the libiberty splay tree and hash
+   table - don't deallocate anything.  Rely on later deletion of the
+   obstack.  */
+
+void
+dummy_obstack_deallocate (void *object, void *data)
+{
+  return;
+}
Index: src/gdb/value.c
===================================================================
--- src.orig/gdb/value.c	2005-12-09 14:48:06.000000000 -0500
+++ src/gdb/value.c	2005-12-09 15:19:49.000000000 -0500
@@ -653,29 +653,6 @@ access_value_history (int num)
   return value_copy (chunk->values[absnum % VALUE_HISTORY_CHUNK]);
 }
 
-/* Clear the value history entirely.
-   Must be done when new symbol tables are loaded,
-   because the type pointers become invalid.  */
-
-void
-clear_value_history (void)
-{
-  struct value_history_chunk *next;
-  int i;
-  struct value *val;
-
-  while (value_history_chain)
-    {
-      for (i = 0; i < VALUE_HISTORY_CHUNK; i++)
-	if ((val = value_history_chain->values[i]) != NULL)
-	  xfree (val);
-      next = value_history_chain->next;
-      xfree (value_history_chain);
-      value_history_chain = next;
-    }
-  value_history_count = 0;
-}
-
 static void
 show_values (char *num_exp, int from_tty)
 {
@@ -842,22 +819,46 @@ internalvar_name (struct internalvar *va
   return var->name;
 }
 
-/* Free all internalvars.  Done when new symtabs are loaded,
-   because that makes the values invalid.  */
+/* Update VALUE before discarding OBJFILE.  COPIED_TYPES is used to
+   prevent cycles / duplicates.  */
+
+static void
+preserve_one_value (struct value *value, struct objfile *objfile,
+		    htab_t copied_types)
+{
+  if (TYPE_OBJFILE (value->type) == objfile)
+    value->type = copy_type_recursive (objfile, value->type, copied_types);
+
+  if (TYPE_OBJFILE (value->enclosing_type) == objfile)
+    value->enclosing_type = copy_type_recursive (objfile,
+						 value->enclosing_type,
+						 copied_types);
+}
+
+/* Update the internal variables and value history when OBJFILE is
+   discarded; we must copy the types out of the objfile.  */
 
 void
-clear_internalvars (void)
+preserve_values (struct objfile *objfile)
 {
+  htab_t copied_types;
+  struct value_history_chunk *cur;
   struct internalvar *var;
+  int i;
 
-  while (internalvars)
-    {
-      var = internalvars;
-      internalvars = var->next;
-      xfree (var->name);
-      xfree (var->value);
-      xfree (var);
-    }
+  /* Create the hash table.  We allocate on the objfile's obstack, since
+     it is soon to be deleted.  */
+  copied_types = create_copied_types_hash (objfile);
+
+  for (cur = value_history_chain; cur; cur = cur->next)
+    for (i = 0; i < VALUE_HISTORY_CHUNK; i++)
+      if (cur->values[i])
+	preserve_one_value (cur->values[i], objfile, copied_types);
+
+  for (var = internalvars; var; var = var->next)
+    preserve_one_value (var->value, objfile, copied_types);
+
+  htab_delete (copied_types);
 }
 
 static void
Index: src/gdb/value.h
===================================================================
--- src.orig/gdb/value.h	2005-12-09 14:48:06.000000000 -0500
+++ src/gdb/value.h	2005-12-09 15:19:49.000000000 -0500
@@ -502,9 +502,7 @@ extern void typedef_print (struct type *
 
 extern char *internalvar_name (struct internalvar *var);
 
-extern void clear_value_history (void);
-
-extern void clear_internalvars (void);
+extern void preserve_values (struct objfile *);
 
 /* From values.c */