[3/3] unconditionally call via SYMBOL_COMPUTED_OPS

[3/3] unconditionally call via SYMBOL_COMPUTED_OPS

[Tom Tromey]

There are some FIXME comments scattered about the code concerning the
unconditional use of SYMBOL_COMPUTED_OPS.

I think the idea here was that we could remove LOC_COMPUTED entirely,
and just have a computed ops vector alongside some other LOC_* constant.

I didn't go quite that far, but I made it so that if the symbol has
computed ops, they are called unconditionally.  Then I removed the
FIXMEs.

Patch #2 fixed the DWARF frame base issue mentioned in these comments by
adding the "reader datum" form of synthetic address classes.  I did this
in patch #2, and not here, because splitting this differently made the
patches uglier -- introducing new code only to delete it again.

Built and regtested on x86-64 Fedora 16.

Tom

	* ax-gdb.c (gen_var_ref): Unconditionally call via computed ops,
	if possible.
	* dwarf2read.c (read_func_scope): Remove old FIXME.
	* eval.c (evaluate_subexp_standard): Check SYMBOL_COMPUTED_OPS,
	not LOC_COMPUTED.
	* findvar.c (symbol_read_needs_frame, default_read_var_value):
	Unconditionally call via computed ops, if possible.
	* printcmd.c (address_info): Unconditionally call via computed ops,
	if possible.
	* stack.c (read_frame_arg): Unconditionally call via computed ops,
	if possible.
	* symtab.c (register_symbol_computed_impl): Sanity check 'ops'.
	* tracepoint.c (scope_info): Unconditionally call via computed ops,
	if possible.
---
 gdb/ax-gdb.c     |   14 ++--
 gdb/dwarf2read.c |    9 ---
 gdb/eval.c       |    2 +-
 gdb/findvar.c    |   20 ++----
 gdb/printcmd.c   |   17 +++--
 gdb/stack.c      |    3 +-
 gdb/symtab.c     |    7 ++
 gdb/tracepoint.c |  188 ++++++++++++++++++++++++++++--------------------------
 8 files changed, 131 insertions(+), 129 deletions(-)

diff --git a/gdb/ax-gdb.c b/gdb/ax-gdb.c
index f0e33cb..a83fb00 100644
--- a/gdb/ax-gdb.c
+++ b/gdb/ax-gdb.c
@@ -662,6 +662,12 @@ gen_var_ref (struct gdbarch *gdbarch, struct agent_expr *ax,
   value->type = check_typedef (SYMBOL_TYPE (var));
   value->optimized_out = 0;
 
+  if (SYMBOL_COMPUTED_OPS (var) != NULL)
+    {
+      SYMBOL_COMPUTED_OPS (var)->tracepoint_var_ref (var, gdbarch, ax, value);
+      return;
+    }
+
   /* I'm imitating the code in read_var_value.  */
   switch (SYMBOL_CLASS (var))
     {
@@ -750,13 +756,7 @@ gen_var_ref (struct gdbarch *gdbarch, struct agent_expr *ax,
       break;
 
     case LOC_COMPUTED:
-      /* FIXME: cagney/2004-01-26: It should be possible to
-	 unconditionally call the SYMBOL_COMPUTED_OPS method when available.
-	 Unfortunately DWARF 2 stores the frame-base (instead of the
-	 function) location in a function's symbol.  Oops!  For the
-	 moment enable this when/where applicable.  */
-      SYMBOL_COMPUTED_OPS (var)->tracepoint_var_ref (var, gdbarch, ax, value);
-      break;
+      gdb_assert_not_reached (_("LOC_COMPUTED variable missing a method"));
 
     case LOC_OPTIMIZED_OUT:
       /* Flag this, but don't say anything; leave it up to callers to
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index a2819ca..e1a20ef 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -9528,15 +9528,6 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
      it.  */
   attr = dwarf2_attr (die, DW_AT_frame_base, cu);
   if (attr)
-    /* FIXME: cagney/2004-01-26: The DW_AT_frame_base's location
-       expression is being recorded directly in the function's symbol
-       and not in a separate frame-base object.  I guess this hack is
-       to avoid adding some sort of frame-base adjunct/annex to the
-       function's symbol :-(.  The problem with doing this is that it
-       results in a function symbol with a location expression that
-       has nothing to do with the location of the function, ouch!  The
-       relationship should be: a function's symbol has-a frame base; a
-       frame-base has-a location expression.  */
     dwarf2_symbol_mark_computed (attr, new->name, cu, 1);
 
   cu->list_in_scope = &local_symbols;
diff --git a/gdb/eval.c b/gdb/eval.c
index c9630df..74e33a0 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -792,7 +792,7 @@ evaluate_subexp_standard (struct type *expect_type,
 	if (noside == EVAL_AVOID_SIDE_EFFECTS)
 	  return value_zero (SYMBOL_TYPE (sym), not_lval);
 
-	if (SYMBOL_CLASS (sym) != LOC_COMPUTED
+	if (SYMBOL_COMPUTED_OPS (sym) == NULL
 	    || SYMBOL_COMPUTED_OPS (sym)->read_variable_at_entry == NULL)
 	  error (_("Symbol \"%s\" does not have any specific entry value"),
 		 SYMBOL_PRINT_NAME (sym));
diff --git a/gdb/findvar.c b/gdb/findvar.c
index fb66e0f..22be47a 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -367,17 +367,15 @@ address_to_signed_pointer (struct gdbarch *gdbarch, struct type *type,
 int
 symbol_read_needs_frame (struct symbol *sym)
 {
+  if (SYMBOL_COMPUTED_OPS (sym) != NULL)
+    return SYMBOL_COMPUTED_OPS (sym)->read_needs_frame (sym);
+
   switch (SYMBOL_CLASS (sym))
     {
       /* All cases listed explicitly so that gcc -Wall will detect it if
          we failed to consider one.  */
     case LOC_COMPUTED:
-      /* FIXME: cagney/2004-01-26: It should be possible to
-	 unconditionally call the SYMBOL_COMPUTED_OPS method when available.
-	 Unfortunately DWARF 2 stores the frame-base (instead of the
-	 function) location in a function's symbol.  Oops!  For the
-	 moment enable this when/where applicable.  */
-      return SYMBOL_COMPUTED_OPS (sym)->read_needs_frame (sym);
+      gdb_assert_not_reached (_("LOC_COMPUTED variable missing a method"));
 
     case LOC_REGISTER:
     case LOC_ARG:
@@ -456,6 +454,9 @@ default_read_var_value (struct symbol *var, struct frame_info *frame)
   if (symbol_read_needs_frame (var))
     gdb_assert (frame);
 
+  if (SYMBOL_COMPUTED_OPS (var) != NULL)
+    return SYMBOL_COMPUTED_OPS (var)->read_variable (var, frame);
+
   switch (SYMBOL_CLASS (var))
     {
     case LOC_CONST:
@@ -578,12 +579,7 @@ default_read_var_value (struct symbol *var, struct frame_info *frame)
       break;
 
     case LOC_COMPUTED:
-      /* FIXME: cagney/2004-01-26: It should be possible to
-	 unconditionally call the SYMBOL_COMPUTED_OPS method when available.
-	 Unfortunately DWARF 2 stores the frame-base (instead of the
-	 function) location in a function's symbol.  Oops!  For the
-	 moment enable this when/where applicable.  */
-      return SYMBOL_COMPUTED_OPS (var)->read_variable (var, frame);
+      gdb_assert_not_reached (_("LOC_COMPUTED variable missing a method"));
 
     case LOC_UNRESOLVED:
       {
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 0c7eb1e..82d1c00 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -1240,6 +1240,14 @@ address_info (char *exp, int from_tty)
   section = SYMBOL_OBJ_SECTION (sym);
   gdbarch = get_objfile_arch (SYMBOL_SYMTAB (sym)->objfile);
 
+  if (SYMBOL_COMPUTED_OPS (sym) != NULL)
+    {
+      SYMBOL_COMPUTED_OPS (sym)->describe_location (sym, context_pc,
+						    gdb_stdout);
+      printf_filtered (".\n");
+      return;
+    }
+
   switch (SYMBOL_CLASS (sym))
     {
     case LOC_CONST:
@@ -1262,14 +1270,7 @@ address_info (char *exp, int from_tty)
       break;
 
     case LOC_COMPUTED:
-      /* FIXME: cagney/2004-01-26: It should be possible to
-	 unconditionally call the SYMBOL_COMPUTED_OPS method when available.
-	 Unfortunately DWARF 2 stores the frame-base (instead of the
-	 function) location in a function's symbol.  Oops!  For the
-	 moment enable this when/where applicable.  */
-      SYMBOL_COMPUTED_OPS (sym)->describe_location (sym, context_pc,
-						    gdb_stdout);
-      break;
+      gdb_assert_not_reached (_("LOC_COMPUTED variable missing a method"));
 
     case LOC_REGISTER:
       /* GDBARCH is the architecture associated with the objfile the symbol
diff --git a/gdb/stack.c b/gdb/stack.c
index a9dabb5..5f1d4af 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -322,7 +322,8 @@ read_frame_arg (struct symbol *sym, struct frame_info *frame,
 	}
     }
 
-  if (SYMBOL_CLASS (sym) == LOC_COMPUTED
+  if (SYMBOL_COMPUTED_OPS (sym) != NULL
+      && SYMBOL_COMPUTED_OPS (sym)->read_variable_at_entry != NULL
       && print_entry_values != print_entry_values_no
       && (print_entry_values != print_entry_values_if_needed
 	  || !val || value_optimized_out (val)))
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 6256d9d..e22b342 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -5084,6 +5084,13 @@ register_symbol_computed_impl (enum address_class aclass,
   symbol_impl[result].aclass = aclass;
   symbol_impl[result].ops_computed = ops;
 
+  /* Sanity check OPS.  */
+  gdb_assert (ops != NULL);
+  gdb_assert (ops->tracepoint_var_ref != NULL);
+  gdb_assert (ops->describe_location != NULL);
+  gdb_assert (ops->read_needs_frame != NULL);
+  gdb_assert (ops->read_variable != NULL);
+
   return result;
 }
 
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index be45cb4..5b67be2 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -2653,101 +2653,107 @@ scope_info (char *args, int from_tty)
 	  gdbarch = get_objfile_arch (SYMBOL_SYMTAB (sym)->objfile);
 
 	  printf_filtered ("Symbol %s is ", symname);
-	  switch (SYMBOL_CLASS (sym))
+
+	  if (SYMBOL_COMPUTED_OPS (sym) != NULL)
+	    SYMBOL_COMPUTED_OPS (sym)->describe_location (sym,
+							  BLOCK_START (block),
+							  gdb_stdout);
+	  else
 	    {
-	    default:
-	    case LOC_UNDEF:	/* Messed up symbol?  */
-	      printf_filtered ("a bogus symbol, class %d.\n",
-			       SYMBOL_CLASS (sym));
-	      count--;		/* Don't count this one.  */
-	      continue;
-	    case LOC_CONST:
-	      printf_filtered ("a constant with value %s (%s)",
-			       plongest (SYMBOL_VALUE (sym)),
-			       hex_string (SYMBOL_VALUE (sym)));
-	      break;
-	    case LOC_CONST_BYTES:
-	      printf_filtered ("constant bytes: ");
-	      if (SYMBOL_TYPE (sym))
-		for (j = 0; j < TYPE_LENGTH (SYMBOL_TYPE (sym)); j++)
-		  fprintf_filtered (gdb_stdout, " %02x",
-				    (unsigned) SYMBOL_VALUE_BYTES (sym)[j]);
-	      break;
-	    case LOC_STATIC:
-	      printf_filtered ("in static storage at address ");
-	      printf_filtered ("%s", paddress (gdbarch,
-					       SYMBOL_VALUE_ADDRESS (sym)));
-	      break;
-	    case LOC_REGISTER:
-	      /* GDBARCH is the architecture associated with the objfile
-		 the symbol is defined in; the target architecture may be
-		 different, and may provide additional registers.  However,
-		 we do not know the target architecture at this point.
-		 We assume the objfile architecture will contain all the
-		 standard registers that occur in debug info in that
-		 objfile.  */
-	      regno = SYMBOL_REGISTER_OPS (sym)->register_number (sym,
-								  gdbarch);
-
-	      if (SYMBOL_IS_ARGUMENT (sym))
-		printf_filtered ("an argument in register $%s",
-				 gdbarch_register_name (gdbarch, regno));
-	      else
-		printf_filtered ("a local variable in register $%s",
-				 gdbarch_register_name (gdbarch, regno));
-	      break;
-	    case LOC_ARG:
-	      printf_filtered ("an argument at stack/frame offset %s",
-			       plongest (SYMBOL_VALUE (sym)));
-	      break;
-	    case LOC_LOCAL:
-	      printf_filtered ("a local variable at frame offset %s",
-			       plongest (SYMBOL_VALUE (sym)));
-	      break;
-	    case LOC_REF_ARG:
-	      printf_filtered ("a reference argument at offset %s",
-			       plongest (SYMBOL_VALUE (sym)));
-	      break;
-	    case LOC_REGPARM_ADDR:
-	      /* Note comment at LOC_REGISTER.  */
-	      regno = SYMBOL_REGISTER_OPS (sym)->register_number (sym,
-								  gdbarch);
-	      printf_filtered ("the address of an argument, in register $%s",
-			       gdbarch_register_name (gdbarch, regno));
-	      break;
-	    case LOC_TYPEDEF:
-	      printf_filtered ("a typedef.\n");
-	      continue;
-	    case LOC_LABEL:
-	      printf_filtered ("a label at address ");
-	      printf_filtered ("%s", paddress (gdbarch,
-					       SYMBOL_VALUE_ADDRESS (sym)));
-	      break;
-	    case LOC_BLOCK:
-	      printf_filtered ("a function at address ");
-	      printf_filtered ("%s",
-		paddress (gdbarch, BLOCK_START (SYMBOL_BLOCK_VALUE (sym))));
-	      break;
-	    case LOC_UNRESOLVED:
-	      msym = lookup_minimal_symbol (SYMBOL_LINKAGE_NAME (sym),
-					    NULL, NULL);
-	      if (msym == NULL)
-		printf_filtered ("Unresolved Static");
-	      else
+	      switch (SYMBOL_CLASS (sym))
 		{
-		  printf_filtered ("static storage at address ");
+		default:
+		case LOC_UNDEF:	/* Messed up symbol?  */
+		  printf_filtered ("a bogus symbol, class %d.\n",
+				   SYMBOL_CLASS (sym));
+		  count--;		/* Don't count this one.  */
+		  continue;
+		case LOC_CONST:
+		  printf_filtered ("a constant with value %s (%s)",
+				   plongest (SYMBOL_VALUE (sym)),
+				   hex_string (SYMBOL_VALUE (sym)));
+		  break;
+		case LOC_CONST_BYTES:
+		  printf_filtered ("constant bytes: ");
+		  if (SYMBOL_TYPE (sym))
+		    for (j = 0; j < TYPE_LENGTH (SYMBOL_TYPE (sym)); j++)
+		      fprintf_filtered (gdb_stdout, " %02x",
+					(unsigned) SYMBOL_VALUE_BYTES (sym)[j]);
+		  break;
+		case LOC_STATIC:
+		  printf_filtered ("in static storage at address ");
+		  printf_filtered ("%s", paddress (gdbarch,
+						   SYMBOL_VALUE_ADDRESS (sym)));
+		  break;
+		case LOC_REGISTER:
+		  /* GDBARCH is the architecture associated with the objfile
+		     the symbol is defined in; the target architecture may be
+		     different, and may provide additional registers.  However,
+		     we do not know the target architecture at this point.
+		     We assume the objfile architecture will contain all the
+		     standard registers that occur in debug info in that
+		     objfile.  */
+		  regno = SYMBOL_REGISTER_OPS (sym)->register_number (sym,
+								      gdbarch);
+
+		  if (SYMBOL_IS_ARGUMENT (sym))
+		    printf_filtered ("an argument in register $%s",
+				     gdbarch_register_name (gdbarch, regno));
+		  else
+		    printf_filtered ("a local variable in register $%s",
+				     gdbarch_register_name (gdbarch, regno));
+		  break;
+		case LOC_ARG:
+		  printf_filtered ("an argument at stack/frame offset %s",
+				   plongest (SYMBOL_VALUE (sym)));
+		  break;
+		case LOC_LOCAL:
+		  printf_filtered ("a local variable at frame offset %s",
+				   plongest (SYMBOL_VALUE (sym)));
+		  break;
+		case LOC_REF_ARG:
+		  printf_filtered ("a reference argument at offset %s",
+				   plongest (SYMBOL_VALUE (sym)));
+		  break;
+		case LOC_REGPARM_ADDR:
+		  /* Note comment at LOC_REGISTER.  */
+		  regno = SYMBOL_REGISTER_OPS (sym)->register_number (sym,
+								      gdbarch);
+		  printf_filtered ("the address of an argument, in register $%s",
+				   gdbarch_register_name (gdbarch, regno));
+		  break;
+		case LOC_TYPEDEF:
+		  printf_filtered ("a typedef.\n");
+		  continue;
+		case LOC_LABEL:
+		  printf_filtered ("a label at address ");
+		  printf_filtered ("%s", paddress (gdbarch,
+						   SYMBOL_VALUE_ADDRESS (sym)));
+		  break;
+		case LOC_BLOCK:
+		  printf_filtered ("a function at address ");
 		  printf_filtered ("%s",
-		    paddress (gdbarch, SYMBOL_VALUE_ADDRESS (msym)));
+				   paddress (gdbarch, BLOCK_START (SYMBOL_BLOCK_VALUE (sym))));
+		  break;
+		case LOC_UNRESOLVED:
+		  msym = lookup_minimal_symbol (SYMBOL_LINKAGE_NAME (sym),
+						NULL, NULL);
+		  if (msym == NULL)
+		    printf_filtered ("Unresolved Static");
+		  else
+		    {
+		      printf_filtered ("static storage at address ");
+		      printf_filtered ("%s",
+				       paddress (gdbarch,
+						 SYMBOL_VALUE_ADDRESS (msym)));
+		    }
+		  break;
+		case LOC_OPTIMIZED_OUT:
+		  printf_filtered ("optimized out.\n");
+		  continue;
+		case LOC_COMPUTED:
+		  gdb_assert_not_reached (_("LOC_COMPUTED variable missing a method"));
 		}
-	      break;
-	    case LOC_OPTIMIZED_OUT:
-	      printf_filtered ("optimized out.\n");
-	      continue;
-	    case LOC_COMPUTED:
-	      SYMBOL_COMPUTED_OPS (sym)->describe_location (sym,
-							    BLOCK_START (block),
-							    gdb_stdout);
-	      break;
 	    }
 	  if (SYMBOL_TYPE (sym))
 	    printf_filtered (", length %d.\n",
-- 
1.7.7.6

Re: [3/3] unconditionally call via SYMBOL_COMPUTED_OPS

[Jan Kratochvil]

On Wed, 16 Jan 2013 17:08:56 +0100, Tom Tromey wrote:
> There are some FIXME comments scattered about the code concerning the
> unconditional use of SYMBOL_COMPUTED_OPS.
> 
> I think the idea here was that we could remove LOC_COMPUTED entirely,
> and just have a computed ops vector alongside some other LOC_* constant.
> 
> I didn't go quite that far, but I made it so that if the symbol has
> computed ops, they are called unconditionally.  Then I removed the
> FIXMEs.
> 
> Patch #2 fixed the DWARF frame base issue mentioned in these comments by
> adding the "reader datum" form of synthetic address classes.  I did this
> in patch #2, and not here, because splitting this differently made the
> patches uglier -- introducing new code only to delete it again.

This patch removes comments which are still valid even after this patch.  And
this patch 3/3 IMO makes it worse by using SYMBOL_COMPUTED_OPS without checking
SYMBOL_CLASS first, it is rather a "reverse-cleanup" patch.  I had problems
cleaning up patch 2/2 now because issues brought in by this patch 3/3 and I
find this patch 3/3 is the inappropriate one.

The problem is that functions have no real DW_AT_location.  They have only
DW_AT_low_pc + DW_AT_high_pc (or other variants we know), these are stored in
SYMBOL_BLOCK_VALUE thanks to LOC_BLOCK.  That would be complete and fine.  But
there is additionally SYMBOL_COMPUTED_OPS and SYMBOL_LOCATION_BATON present
there which make no sense there, the location is already complete by
SYMBOL_BLOCK_VALUE+LOC_BLOCK.  SYMBOL_COMPUTED_OPS+SYMBOL_LOCATION_BATON there
describe DW_AT_frame_base but that is just one of the many additional
attributes.
(gdb) p *((struct symtab *) 0x22d0f80).blockvector.block[2].function
$1 = {ginfo = {name = 0x22b4dbd "main", value = {ivalue = 36507008, block = 0x22d0d80, bytes = 0x22d0d80 "\360\004@", address = 36507008, common_block = 0x22d0d80, chain = 0x22d0d80}, language_specific = {mangled_lang = {demangled_name = 0x0}, cplus_specific = 0x0}, language = language_c, section = 0, obj_section = 0x0}, type = 0x22c7a70, symtab = 0x22d0f80, domain = VAR_DOMAIN, aclass = LOC_BLOCK, is_argument = 0, is_inlined = 0, is_cplus_template_function = 0, line = 1, ops = {ops_computed = 0xfd7a20 <dwarf2_locexpr_funcs>, ops_register = 0xfd7a20 <dwarf2_locexpr_funcs>}, aux_value = 0x22d0cd0, hash_next = 0x0}
 <1><2d>: Abbrev Number: 2 (DW_TAG_subprogram)
    <2e>   DW_AT_name        : (indirect string, offset: 0x7c): main
    <34>   DW_AT_type        : <0x5b>
    <38>   DW_AT_low_pc      : 0x4004f0
    <40>   DW_AT_high_pc     : 0x10
    <48>   DW_AT_frame_base  : 1 byte block: 9c         (DW_OP_call_frame_cfa)

When you compare it with an autovariable it uses LOC_COMPUTED, therefore
symbol.ginfo.value is unused.  And there is correctly (because it is
LOC_COMPUTED) used SYMBOL_COMPUTED_OPS+SYMBOL_LOCATION_BATON which really
describe location of the autovariable itself.
(gdb) p *((struct symtab *) 0x22d0f80).blockvector.block[2].function.ginfo.value.block.dict.data.linear.syms[0]
$6 = {ginfo = {name = 0x22c050f "i", value = {ivalue = 0, block = 0x0, bytes = 0x0, address = 0, common_block = 0x0, chain = 0x0}, language_specific = { mangled_lang = {demangled_name = 0x0}, cplus_specific = 0x0}, language = language_c, section = 0, obj_section = 0x0}, type = 0x22ccb00, symtab = 0x22d0f80, domain = VAR_DOMAIN, aclass = LOC_COMPUTED, is_argument = 0, is_inlined = 0, is_cplus_template_function = 0, line = 1, ops = { ops_computed = 0xfd7a20 <dwarf2_locexpr_funcs>, ops_register = 0xfd7a20 <dwarf2_locexpr_funcs>}, aux_value = 0x22d0d60, hash_next = 0x0}
 <2><4e>: Abbrev Number: 3 (DW_TAG_variable)
    <4f>   DW_AT_name        : i
    <53>   DW_AT_type        : <0x5b>
    <57>   DW_AT_location    : 2 byte block: 91 6c      (DW_OP_fbreg: -20)

We could talk differently if SYMBOL_COMPUTED_OPS would apply for all LOC_*, the
full virtualization.  But that would require resolving first the inappropriate
reuse of SYMBOL_COMPUTED_OPS for DW_AT_frame_base in function symbols.

Without the full virtualization / unconditional calls via SYMBOL_COMPUTED_OPS I
do not understand this patch and I am against it.


Thanks,
Jan

Re: [3/3] unconditionally call via SYMBOL_COMPUTED_OPS

[Tom Tromey]

Jan> We could talk differently if SYMBOL_COMPUTED_OPS would apply for
Jan> all LOC_*, the full virtualization.  But that would require
Jan> resolving first the inappropriate reuse of SYMBOL_COMPUTED_OPS for
Jan> DW_AT_frame_base in function symbols.

Jan> Without the full virtualization / unconditional calls via
Jan> SYMBOL_COMPUTED_OPS I do not understand this patch and I am against
Jan> it.

I thought I had made this "has a frame-base" change, but I don't see it.
WTF?  You are right to oppose this now.
I'll write the needed patch.

If you're against the full virtualization plan, it would be nice to know now.
IIUC this was the original point of all those FIXME comments.  But if
nobody wants it, we can just drop them directly.

Tom

Re: [3/3] unconditionally call via SYMBOL_COMPUTED_OPS

[Jan Kratochvil]

On Thu, 07 Feb 2013 17:52:24 +0100, Tom Tromey wrote:
> If you're against the full virtualization plan, it would be nice to know now.

I am not but I cannot imagine how huge such patch would be.


> IIUC this was the original point of all those FIXME comments.  But if
> nobody wants it, we can just drop them directly.

Unless you want to do the full virtualization for the virtualization itself
I am OK with just fully documenting the issue at struct symbol instead of the
scattered FIXME comments.  I find such current fields override fits fine into
the "code clarity" level GDB currently has.


Thanks,
Jan

Re: [3/3] unconditionally call via SYMBOL_COMPUTED_OPS

[Tom Tromey]

>>>>> "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.

Basically, it is register_symbol_alias_impl that does the work.
For a function, dwarf2_symbol_mark_computed uses the _block indices,
e.g.:

+      SYMBOL_ACLASS_INDEX (sym) = (is_block
+				   ? dwarf2_loclist_block_index
+				   : dwarf2_loclist_index);

These are registered as aliases of LOC_BLOCK:

+  dwarf2_loclist_block_index = register_symbol_alias_impl (LOC_BLOCK);

What this means is that the resulting symbol will have LOC_BLOCK and a
SYMBOL_LOCATION_BATON, but SYMBOL_COMPUTED_OPS will be NULL.  So, the
various calls via the vtable will never be taken.

Jan> I had problems cleaning up patch 2/2 now because issues brought in
Jan> by this patch 3/3 and I find this patch 3/3 is the inappropriate
Jan> one.

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.

I tend to like the approach taken in the patch because it is more
flexible.  If you disagree I would like to understand why.

Tom

Re: [3/3] unconditionally call via SYMBOL_COMPUTED_OPS

[Jan Kratochvil]

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 *);

Re: [3/3] unconditionally call via SYMBOL_COMPUTED_OPS

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

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

Sorry about the delay on this.

I like this patch too.  I will roll it into my patch #2 and commit it
sometime after the release branch is made.

thanks,
Tom

Re: [3/3] unconditionally call via SYMBOL_COMPUTED_OPS

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Tom> There are some FIXME comments scattered about the code concerning the
Tom> unconditional use of SYMBOL_COMPUTED_OPS.

Tom> I think the idea here was that we could remove LOC_COMPUTED entirely,
Tom> and just have a computed ops vector alongside some other LOC_* constant.

I'm checking this in now.

Regtested again on x86-64 Fedora 18.

Tom