Re: [3/3] unconditionally call via SYMBOL_COMPUTED_OPS

[Jan Kratochvil <jan.kratochvil@redhat.com>]

On Fri, 08 Feb 2013 12:52:10 +0100, Tom Tromey wrote:
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> 
> Jan> Without the full virtualization / unconditional calls via
> Jan> SYMBOL_COMPUTED_OPS I do not understand this patch and I am against
> Jan> it.
> 
> This was troubling me yesterday and I finally remembered that I did
> implement the needed change, but in a non-obvious way.  I now think
> either your analysis is incorrect or I didn't understand it.

I was wrong as I see.

IIUC one of the goals of this patchset is that now one can incrementally start
to virtualize each LOC_* handling by filling in separate SYMBOL_COMPUTED_OPS
vector for each LOC_* value.


> Ah, I see.  Your follow-on patch restored the vtable in all cases.
> This isn't correct according to the approach of patch #3 and the design
> implied by the comments and the PR.

Yes, my previous patch broke what your patches have achieved.


I have filled in the missing frame base cleanup, now is the frame base vs.
location separation better understandable IMO (or maybe just to me, not sure).

No regressions on {x86_64,x86_64-m32,i686}-fedora19pre-linux-gnu.


Thanks,
Jan


gdb/
2013-02-08  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Code cleanup.
	* dwarf2loc.c (locexpr_find_frame_base_location)
	(dwarf2_block_frame_base_locexpr_funcs)
	(loclist_find_frame_base_location)
	(dwarf2_block_frame_base_loclist_funcs): New.
	(dwarf_expr_frame_base_1): Call SYMBOL_BLOCK_OPS, remove internal_error.
	(dwarf2_locexpr_funcs, dwarf2_loclist_funcs): Add location_has_loclist.
	* dwarf2loc.h (dwarf2_locexpr_block_index, dwarf2_loclist_block_index):
	Remove.
	(dwarf2_block_frame_base_locexpr_funcs)
	(dwarf2_block_frame_base_loclist_funcs): New.
	* dwarf2read.c (dwarf2_locexpr_block_index, dwarf2_loclist_block_index):
	Make them static.
	(var_decode_location): Use location_has_loclist.
	(_initialize_dwarf2_read): Replace register_symbol_alias_impl by
	register_symbol_block_impl.
	* symtab.c (register_symbol_alias_impl): Remove.
	(register_symbol_block_impl): New.
	* symtab.h (struct symbol_computed_ops): Add location_has_loclist.
	(struct symbol_block_ops): New.
	(struct symbol_impl): Add ops_block.
	(SYMBOL_BLOCK_OPS): New.
	(register_symbol_alias_impl): Remove declaration.
	(register_symbol_block_impl): New declaration.

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 6047f09..206a318 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -357,34 +357,59 @@ dwarf_expr_frame_base (void *baton, const gdb_byte **start, size_t * length)
 			   start, length);
 }
 
+/* Implement find_frame_base_location method for LOC_BLOCK functions using
+   DWARF expression for its DW_AT_frame_base.  */
+
+static void
+locexpr_find_frame_base_location (struct symbol *framefunc, CORE_ADDR pc,
+				  const gdb_byte **start, size_t *length)
+{
+  struct dwarf2_locexpr_baton *symbaton = SYMBOL_LOCATION_BATON (framefunc);
+
+  *length = symbaton->size;
+  *start = symbaton->data;
+}
+
+/* Vector for inferior functions as represented by LOC_BLOCK, if the inferior
+   function uses DWARF expression for its DW_AT_frame_base.  */
+
+const struct symbol_block_ops dwarf2_block_frame_base_locexpr_funcs =
+{
+  locexpr_find_frame_base_location
+};
+
+/* Implement find_frame_base_location method for LOC_BLOCK functions using
+   DWARF location list for its DW_AT_frame_base.  */
+
+static void
+loclist_find_frame_base_location (struct symbol *framefunc, CORE_ADDR pc,
+				  const gdb_byte **start, size_t *length)
+{
+  struct dwarf2_loclist_baton *symbaton = SYMBOL_LOCATION_BATON (framefunc);
+
+  *start = dwarf2_find_location_expression (symbaton, length, pc);
+}
+
+/* Vector for inferior functions as represented by LOC_BLOCK, if the inferior
+   function uses DWARF location list for its DW_AT_frame_base.  */
+
+const struct symbol_block_ops dwarf2_block_frame_base_loclist_funcs =
+{
+  loclist_find_frame_base_location
+};
+
 static void
 dwarf_expr_frame_base_1 (struct symbol *framefunc, CORE_ADDR pc,
 			 const gdb_byte **start, size_t *length)
 {
-  if (SYMBOL_LOCATION_BATON (framefunc) == NULL)
-    *length = 0;
-  else if (SYMBOL_ACLASS_INDEX (framefunc) == dwarf2_loclist_block_index)
+  if (SYMBOL_BLOCK_OPS (framefunc) != NULL)
     {
-      struct dwarf2_loclist_baton *symbaton;
+      const struct symbol_block_ops *ops_block = SYMBOL_BLOCK_OPS (framefunc);
 
-      symbaton = SYMBOL_LOCATION_BATON (framefunc);
-      *start = dwarf2_find_location_expression (symbaton, length, pc);
-    }
-  else if (SYMBOL_ACLASS_INDEX (framefunc) == dwarf2_locexpr_block_index)
-    {
-      struct dwarf2_locexpr_baton *symbaton;
-
-      symbaton = SYMBOL_LOCATION_BATON (framefunc);
-      if (symbaton != NULL)
-	{
-	  *length = symbaton->size;
-	  *start = symbaton->data;
-	}
-      else
-	*length = 0;
+      ops_block->find_frame_base_location (framefunc, pc, start, length);
     }
   else
-    internal_error (__FILE__, __LINE__, _("invalid function aclass index"));
+    *length = 0;
 
   if (*length == 0)
     error (_("Could not find the frame base for \"%s\"."),
@@ -3978,6 +4003,7 @@ const struct symbol_computed_ops dwarf2_locexpr_funcs = {
   locexpr_read_variable_at_entry,
   locexpr_read_needs_frame,
   locexpr_describe_location,
+  0,	/* location_has_loclist */
   locexpr_tracepoint_var_ref
 };
 
@@ -4156,6 +4182,7 @@ const struct symbol_computed_ops dwarf2_loclist_funcs = {
   loclist_read_variable_at_entry,
   loclist_read_needs_frame,
   loclist_describe_location,
+  1,	/* location_has_loclist */
   loclist_tracepoint_var_ref
 };
 
diff --git a/gdb/dwarf2loc.h b/gdb/dwarf2loc.h
index 9060a65..78448e6 100644
--- a/gdb/dwarf2loc.h
+++ b/gdb/dwarf2loc.h
@@ -133,9 +133,8 @@ struct dwarf2_loclist_baton
 extern const struct symbol_computed_ops dwarf2_locexpr_funcs;
 extern const struct symbol_computed_ops dwarf2_loclist_funcs;
 
-/* Two variables from dwarf2read.c.  */
-extern int dwarf2_locexpr_block_index;
-extern int dwarf2_loclist_block_index;
+extern const struct symbol_block_ops dwarf2_block_frame_base_locexpr_funcs;
+extern const struct symbol_block_ops dwarf2_block_frame_base_loclist_funcs;
 
 /* Compile a DWARF location expression to an agent expression.
    
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 6086051..43b92e5 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -96,8 +96,8 @@ static const struct objfile_data *dwarf2_objfile_data_key;
 
 static int dwarf2_locexpr_index;
 static int dwarf2_loclist_index;
-int dwarf2_locexpr_block_index;
-int dwarf2_loclist_block_index;
+static int dwarf2_locexpr_block_index;
+static int dwarf2_loclist_block_index;
 
 struct dwarf2_section_info
 {
@@ -15748,7 +15748,7 @@ var_decode_location (struct attribute *attr, struct symbol *sym,
 
   dwarf2_symbol_mark_computed (attr, sym, cu, 0);
 
-  if (SYMBOL_COMPUTED_OPS (sym) == &dwarf2_loclist_funcs)
+  if (SYMBOL_COMPUTED_OPS (sym)->location_has_loclist)
     cu->has_loclist = 1;
 }
 
@@ -20709,6 +20709,8 @@ Usage: save gdb-index DIRECTORY"),
   dwarf2_loclist_index = register_symbol_computed_impl (LOC_COMPUTED,
 							&dwarf2_loclist_funcs);
 
-  dwarf2_locexpr_block_index = register_symbol_alias_impl (LOC_BLOCK);
-  dwarf2_loclist_block_index = register_symbol_alias_impl (LOC_BLOCK);
+  dwarf2_locexpr_block_index = register_symbol_block_impl (LOC_BLOCK,
+					&dwarf2_block_frame_base_locexpr_funcs);
+  dwarf2_loclist_block_index = register_symbol_block_impl (LOC_BLOCK,
+					&dwarf2_block_frame_base_loclist_funcs);
 }
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 11ee5b0..685f1ff 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -5089,17 +5089,25 @@ register_symbol_computed_impl (enum address_class aclass,
   return result;
 }
 
-/* Register a symbol type to be recognized by its index.  ACLASS can be
-   anything.  This returns the new index, which should be used as the
-   aclass_index field for symbols of this type.  */
+/* Register a function with frame base type.  ACLASS must be LOC_BLOCK.
+   OPS is the ops vector associated with this index.  This returns the
+   new index, which should be used as the aclass_index field for symbols
+   of this type.  */
 
 int
-register_symbol_alias_impl (enum address_class aclass)
+register_symbol_block_impl (enum address_class aclass,
+			    const struct symbol_block_ops *ops)
 {
   int result = next_aclass_value++;
 
+  gdb_assert (aclass == LOC_BLOCK);
   gdb_assert (result < MAX_SYMBOL_IMPLS);
   symbol_impl[result].aclass = aclass;
+  symbol_impl[result].ops_block = ops;
+
+  /* Sanity check OPS.  */
+  gdb_assert (ops != NULL);
+  gdb_assert (ops->find_frame_base_location != NULL);
 
   return result;
 }
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 293c443..c69f39e 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -575,6 +575,9 @@ struct symbol_computed_ops
   void (*describe_location) (struct symbol * symbol, CORE_ADDR addr,
 			     struct ui_file * stream);
 
+  /* Non-zero if this symbol's address computation is dependent on PC.  */
+  unsigned char location_has_loclist;
+
   /* Tracepoint support.  Append bytecodes to the tracepoint agent
      expression AX that push the address of the object SYMBOL.  Set
      VALUE appropriately.  Note --- for objects in registers, this
@@ -586,6 +589,20 @@ struct symbol_computed_ops
 			      struct agent_expr *ax, struct axs_value *value);
 };
 
+/* The methods needed to implement LOC_BLOCK for inferior functions.
+   These methods can use the symbol's .aux_value for additional
+   per-symbol information.  */
+
+struct symbol_block_ops
+{
+  /* Fill in *START and *LENGTH with DWARF block data of function
+     FRAMEFUNC valid for inferior context address PC.  Set *LENGTH to
+     zero if such location is not valid for PC; *START is left
+     uninitialized in such case.  */
+  void (*find_frame_base_location) (struct symbol *framefunc, CORE_ADDR pc,
+				    const gdb_byte **start, size_t *length);
+};
+
 /* Functions used with LOC_REGISTER and LOC_REGPARM_ADDR.  */
 
 struct symbol_register_ops
@@ -603,6 +620,9 @@ struct symbol_impl
   /* Used with LOC_COMPUTED.  */
   const struct symbol_computed_ops *ops_computed;
 
+  /* Used with LOC_BLOCK.  */
+  const struct symbol_block_ops *ops_block;
+
   /* Used with LOC_REGISTER and LOC_REGPARM_ADDR.  */
   const struct symbol_register_ops *ops_register;
 };
@@ -697,13 +717,15 @@ extern const struct symbol_impl *symbol_impls;
 #define SYMBOL_LINE(symbol)		(symbol)->line
 #define SYMBOL_SYMTAB(symbol)		(symbol)->symtab
 #define SYMBOL_COMPUTED_OPS(symbol)	(SYMBOL_IMPL (symbol).ops_computed)
+#define SYMBOL_BLOCK_OPS(symbol)	(SYMBOL_IMPL (symbol).ops_block)
 #define SYMBOL_REGISTER_OPS(symbol)	(SYMBOL_IMPL (symbol).ops_register)
 #define SYMBOL_LOCATION_BATON(symbol)   (symbol)->aux_value
 
 extern int register_symbol_computed_impl (enum address_class,
 					  const struct symbol_computed_ops *);
 
-extern int register_symbol_alias_impl (enum address_class);
+extern int register_symbol_block_impl (enum address_class aclass,
+				       const struct symbol_block_ops *ops);
 
 extern int register_symbol_register_impl (enum address_class,
 					  const struct symbol_register_ops *);