[RFA]: LOC_COMPUTED + abstracted dwarf2 evaluator, again

[RFA]: LOC_COMPUTED + abstracted dwarf2 evaluator, again

[Daniel Berlin]

For yer convenience,  I rediffed it against the current gdb sources.
I'll update the changelog dates if it ever gets reviewed or approved.
Slightly updated accompanying text (from the april submission):
"
The only real things left to do is give some *real* description in 
locexpr_describe_location, add tracepoint support, and remove the 
dwarf2cfi.c evaluator .  
None of these should be blockers to committing it or reviewing it.

It would also be nice to use location expressions in *all* cases 
(currently, it only does it for variables) so you can get rid of 
decode_loc_desc, but i'll leave that for the next patch.
 
There are a few lines that are too long (<10, i *think*), sorry about 
that, i couldn't see an easy way to break them up and have them make 
sense.

Splitting this patch into loc_computed addition, and dwarf2 implementation 
of loc_computed, doesn't make it much smaller (loc_computed_addition is 
maybe 5% of the patch), so if size is a concern, sorry, but it's pretty 
much unsplittable beyond that (the biggest portion is the addition of the 
abstracted evaluator, but the dwarf2 implementation doesn't work without 
it).

This patch has been tested on both powerpc-linux and 686-linux.
"

2002-04-11  Daniel Berlin  <dberlin@dberlin.org>

	* dwarf2read.c: Add value.h, frame.h, gdbcore.h, dwarf2expr.h,
	needed for the new location expression functions. 
	(read_func_scope): Set location expression and location funcs on
	symbol.
	(new_symbol): Ditto.
	(locexpr_read_needs_frame): New function, used in struct
	location for dwarf2 location expressions.
	(locexpr_read_variable): Ditto.
	(locexpr_describe_location): Ditto.
	(dwarf_expr_read_reg): New function, used to read register for
	dwarf2 expression eval.
	(dwarf_expr_read_mem): Ditto for memory.
	(dwarf_expr_frame_base): Ditto for returning the frame base.
	(evaluate_loc_desc): New function, helper for
	locexpr_read_variable.
	

2002-04-11  Daniel Berlin  <dan@dberlin.org>
	
	* stack.c (print_block_frame_locals): LOC_COMPUTED symbols are
	locals, too.

	* dwarf2expr.c: New file, abstracted dwarf2 expression evaluator.

	* dwarf2expr.h: New file, header for dwarf2 expression evaluator.
	
	(Simple cases of *.c containing LOC_): Handle LOC_COMPUTED and
	LOC_COMPUTED_ARG. 

2002-04-11  Daniel Berlin  <dan@dberlin.org>

	* buildsym.c (finish_block): Handle LOC_COMPUTED and
	LOC_COMPUTED_ARG. 

	* findvar.c (symbol_read_needs_frame): Handle LOC_COMPUTED and
	LOC_COMPUTED_ARG. 
	(read_var_value): Ditto.

	* printcmd.c (address_info): Ditto.

	* symmisc.c (print_partial_symbols): Ditto.

	* symtab.h: Add struct location, LOC_COMPUTED, and LOC_COMPUTED_ARG.
	

Index: Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/Makefile.in,v
retrieving revision 1.218
diff -c -3 -p -w -B -b -r1.218 Makefile.in
*** Makefile.in	3 Jul 2002 20:36:54 -0000	1.218
--- Makefile.in	8 Jul 2002 16:52:38 -0000
*************** SFILES = ax-general.c ax-gdb.c bcache.c 
*** 527,533 ****
  	buildsym.c c-exp.y c-lang.c c-typeprint.c c-valprint.c \
  	ch-exp.c ch-lang.c ch-typeprint.c ch-valprint.c coffread.c \
  	complaints.c completer.c corefile.c cp-valprint.c dbxread.c \
! 	demangle.c dwarfread.c dwarf2read.c elfread.c environ.c eval.c \
  	event-loop.c event-top.c \
  	expprint.c f-exp.y f-lang.c f-typeprint.c f-valprint.c \
  	findvar.c regcache.c gdbarch.c arch-utils.c gdbtypes.c osabi.c \
--- 527,534 ----
  	buildsym.c c-exp.y c-lang.c c-typeprint.c c-valprint.c \
  	ch-exp.c ch-lang.c ch-typeprint.c ch-valprint.c coffread.c \
  	complaints.c completer.c corefile.c cp-valprint.c dbxread.c \
! 	demangle.c dwarfread.c dwarf2cfi.c dwarf2expr.c dwarf2read.c \
!       elfread.c environ.c eval.c \
  	event-loop.c event-top.c \
  	expprint.c f-exp.y f-lang.c f-typeprint.c f-valprint.c \
  	findvar.c regcache.c gdbarch.c arch-utils.c gdbtypes.c osabi.c \
*************** COMMON_OBS = version.o blockframe.o brea
*** 734,740 ****
  	gdb-events.o \
  	exec.o bcache.o objfiles.o minsyms.o maint.o demangle.o \
  	dbxread.o coffread.o elfread.o \
! 	dwarfread.o dwarf2read.o mipsread.o stabsread.o corefile.o \
  	c-lang.o ch-exp.o ch-lang.o f-lang.o \
  	ui-out.o cli-out.o \
  	varobj.o wrapper.o \
--- 735,742 ----
  	gdb-events.o \
  	exec.o bcache.o objfiles.o minsyms.o maint.o demangle.o \
  	dbxread.o coffread.o elfread.o \
! 	dwarfread.o dwarf2cfi.o dwarf2expr.o \
!       dwarf2read.o mipsread.o stabsread.o corefile.o \
  	c-lang.o ch-exp.o ch-lang.o f-lang.o \
  	ui-out.o cli-out.o \
  	varobj.o wrapper.o \
*************** dwarfread.o: dwarfread.c $(bfd_h) $(buil
*** 1435,1440 ****
--- 1437,1446 ----
  	$(symfile_h) $(symtab_h) $(gdb_string_h)
  
  dwarf2read.o: dwarf2read.c $(bfd_h) $(buildsym_h) $(defs_h) \
+ 	$(expression_h) $(gdbtypes_h) $(language_h) $(objfiles_h) \
+ 	$(symfile_h) $(symtab_h) $(gdb_string_h)
+ 
+ dwarf2expr.o: dwarf2expr.c $(bfd_h) $(buildsym_h) $(defs_h) \
  	$(expression_h) $(gdbtypes_h) $(language_h) $(objfiles_h) \
  	$(symfile_h) $(symtab_h) $(gdb_string_h) $(macrotab_h)
  
Index: buildsym.c
===================================================================
RCS file: /cvs/src/src/gdb/buildsym.c,v
retrieving revision 1.16
diff -c -3 -p -w -B -b -r1.16 buildsym.c
*** buildsym.c	15 May 2002 21:19:18 -0000	1.16
--- buildsym.c	8 Jul 2002 16:52:38 -0000
*************** finish_block (struct symbol *symbol, str
*** 289,294 ****
--- 289,295 ----
  		case LOC_REGPARM_ADDR:
  		case LOC_BASEREG_ARG:
  		case LOC_LOCAL_ARG:
+ 		case LOC_COMPUTED_ARG:
  		  nparams++;
  		  break;
  		case LOC_UNDEF:
*************** finish_block (struct symbol *symbol, str
*** 304,309 ****
--- 305,311 ----
  		case LOC_BASEREG:
  		case LOC_UNRESOLVED:
  		case LOC_OPTIMIZED_OUT:
+ 		case LOC_COMPUTED:
  		default:
  		  break;
  		}
*************** finish_block (struct symbol *symbol, str
*** 325,330 ****
--- 327,333 ----
  		    case LOC_REGPARM_ADDR:
  		    case LOC_BASEREG_ARG:
  		    case LOC_LOCAL_ARG:
+ 		    case LOC_COMPUTED_ARG:
  		      TYPE_FIELD_TYPE (ftype, iparams) = SYMBOL_TYPE (sym);
  		      TYPE_FIELD_ARTIFICIAL (ftype, iparams) = 0;
  		      iparams++;
*************** finish_block (struct symbol *symbol, str
*** 342,347 ****
--- 345,351 ----
  		    case LOC_BASEREG:
  		    case LOC_UNRESOLVED:
  		    case LOC_OPTIMIZED_OUT:
+ 		    case LOC_COMPUTED:
  		    default:
  		      break;
  		    }
Index: dwarf2read.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2read.c,v
retrieving revision 1.60
diff -c -3 -p -w -B -b -r1.60 dwarf2read.c
*** dwarf2read.c	22 Jun 2002 00:05:59 -0000	1.60
--- dwarf2read.c	8 Jul 2002 16:52:39 -0000
***************
*** 46,51 ****
--- 46,56 ----
  #include "gdb_string.h"
  #include "gdb_assert.h"
  #include <sys/types.h>
+ #include "ui-out.h"
+ #include "value.h"
+ #include "frame.h"
+ #include "gdbcore.h"
+ #include "dwarf2expr.h"
  
  #ifndef DWARF2_REG_TO_REGNUM
  #define DWARF2_REG_TO_REGNUM(REG) (REG)
*************** static struct abbrev_info *dwarf_alloc_a
*** 898,903 ****
--- 903,929 ----
  
  static struct die_info *dwarf_alloc_die (void);
  
+ 
+ static struct value * locexpr_read_variable (void *baton, struct frame_info *frame);
+ 
+ static int locexpr_read_needs_frame (void *baton);
+ 
+ static int locexpr_describe_location (void *baton, struct ui_file *stream);
+ 
+ static struct location_funcs locexpr_funcs = { 
+   locexpr_read_variable,
+   locexpr_read_needs_frame,
+   locexpr_describe_location,
+   NULL
+ };
+ 
+ struct locexpr_baton
+ {
+   struct symbol * sym;
+   struct dwarf_block locexpr;
+   struct objfile *obj;
+ };
+ 
  static void initialize_cu_func_list (void);
  
  static void add_to_cu_func_list (const char *, CORE_ADDR, CORE_ADDR);
*************** read_func_scope (struct die_info *die, s
*** 1842,1848 ****
    struct die_info *child_die;
    struct attribute *attr;
    char *name;
! 
    name = dwarf2_linkage_name (die);
  
    /* Ignore functions with missing or empty names and functions with
--- 1868,1874 ----
    struct die_info *child_die;
    struct attribute *attr;
    char *name;
!   struct locexpr_baton *baton;
    name = dwarf2_linkage_name (die);
  
    /* Ignore functions with missing or empty names and functions with
*************** read_func_scope (struct die_info *die, s
*** 1886,1891 ****
--- 1912,1931 ----
  
    new = push_context (0, lowpc);
    new->name = new_symbol (die, die->type, objfile, cu_header);
+ 
+   /* If it has a location expression for fbreg, record it.  
+      Since not all symbols use location expressions exclusively yet,
+      we still need to decode it above. */
+   if (attr)
+     {
+       baton = obstack_alloc (&objfile->symbol_obstack, sizeof (struct locexpr_baton));
+       baton->sym = new->name;
+       baton->obj = objfile;
+       memcpy (&baton->locexpr, DW_BLOCK (attr), sizeof (struct dwarf_block));
+       SYMBOL_LOCATION_FUNCS (new->name) = &locexpr_funcs;
+       SYMBOL_LOCATION_BATON (new->name) = baton;
+     }
+ 
    list_in_scope = &local_symbols;
  
    if (die->has_children)
*************** new_symbol (struct die_info *die, struct
*** 4603,4666 ****
  	  attr = dwarf_attr (die, DW_AT_location);
  	  if (attr)
  	    {
  	      attr2 = dwarf_attr (die, DW_AT_external);
  	      if (attr2 && (DW_UNSND (attr2) != 0))
- 		{
- 		  SYMBOL_VALUE_ADDRESS (sym) =
- 		    decode_locdesc (DW_BLOCK (attr), objfile, cu_header);
  		  add_symbol_to_list (sym, &global_symbols);
- 
- 		  /* In shared libraries the address of the variable
- 		     in the location descriptor might still be relocatable,
- 		     so its value could be zero.
- 		     Enter the symbol as a LOC_UNRESOLVED symbol, if its
- 		     value is zero, the address of the variable will then
- 		     be determined from the minimal symbol table whenever
- 		     the variable is referenced.  */
- 		  if (SYMBOL_VALUE_ADDRESS (sym))
- 		    {
- 		      fixup_symbol_section (sym, objfile);
- 		      SYMBOL_VALUE_ADDRESS (sym) +=
- 			ANOFFSET (objfile->section_offsets,
- 			          SYMBOL_SECTION (sym));
- 		      SYMBOL_CLASS (sym) = LOC_STATIC;
- 		    }
- 		  else
- 		    SYMBOL_CLASS (sym) = LOC_UNRESOLVED;
- 		}
  	      else
- 		{
- 		  SYMBOL_VALUE (sym) = addr =
- 		    decode_locdesc (DW_BLOCK (attr), objfile, cu_header);
  		  add_symbol_to_list (sym, list_in_scope);
- 		  if (optimized_out)
- 		    {
- 		      SYMBOL_CLASS (sym) = LOC_OPTIMIZED_OUT;
- 		    }
- 		  else if (isreg)
- 		    {
- 		      SYMBOL_CLASS (sym) = LOC_REGISTER;
- 		      SYMBOL_VALUE (sym) = 
- 			DWARF2_REG_TO_REGNUM (SYMBOL_VALUE (sym));
- 		    }
- 		  else if (offreg)
- 		    {
- 		      SYMBOL_CLASS (sym) = LOC_BASEREG;
- 		      SYMBOL_BASEREG (sym) = DWARF2_REG_TO_REGNUM (basereg);
- 		    }
- 		  else if (islocal)
- 		    {
- 		      SYMBOL_CLASS (sym) = LOC_LOCAL;
- 		    }
- 		  else
- 		    {
- 		      fixup_symbol_section (sym, objfile);
- 		      SYMBOL_VALUE_ADDRESS (sym) =
- 		        addr + ANOFFSET (objfile->section_offsets,
- 			                 SYMBOL_SECTION (sym));
- 		      SYMBOL_CLASS (sym) = LOC_STATIC;
- 		    }
- 		}
  	    }
  	  else
  	    {
--- 4643,4662 ----
  	  attr = dwarf_attr (die, DW_AT_location);
  	  if (attr)
  	    {
+ 	      struct locexpr_baton *baton = obstack_alloc (&objfile->symbol_obstack, 
+ 							   sizeof (struct locexpr_baton));
+ 	      baton->sym = sym;
+ 	      memcpy (&baton->locexpr, DW_BLOCK (attr), sizeof (struct dwarf_block));
+ 	      baton->obj = objfile;
+ 	      SYMBOL_CLASS (sym) = LOC_COMPUTED;
+ 	      SYMBOL_LOCATION_FUNCS (sym) = &locexpr_funcs;
+ 	      SYMBOL_LOCATION_BATON (sym) = baton;
+ 	      
  	      attr2 = dwarf_attr (die, DW_AT_external);
  	      if (attr2 && (DW_UNSND (attr2) != 0))
  		  add_symbol_to_list (sym, &global_symbols);
  	      else
  		  add_symbol_to_list (sym, list_in_scope);
  	    }
  	  else
  	    {
*************** dwarf_alloc_die (void)
*** 6420,6425 ****
--- 6416,6541 ----
    die = (struct die_info *) xmalloc (sizeof (struct die_info));
    memset (die, 0, sizeof (struct die_info));
    return (die);
+ }
+ 
+ static struct value * evaluate_loc_desc (struct symbol *, struct frame_info *, 
+ 					 struct dwarf_block *, struct type *);
+ 
+ /* This is one of the struct location functions. It returns the value
+    of a variable, given our baton and a frame.  */
+ static struct value *
+ locexpr_read_variable (void *baton, struct frame_info *frame)
+ {
+   struct locexpr_baton *dlbaton = (struct locexpr_baton *) baton;
+   struct value * val;
+   val = evaluate_loc_desc (dlbaton->sym, frame, 
+ 			   &dlbaton->locexpr, SYMBOL_TYPE (dlbaton->sym));
+   return val;
+ }
+ 
+ /* This is a struct location function, this one returns 1 if we need a
+    frame to read the value of the variable. */
+ static int 
+ locexpr_read_needs_frame (void *baton)
+ {
+   return 1;
+ }
+ 
+ /* This is a struct location function, it describes the location in
+    english. */
+ static int 
+ locexpr_describe_location (void *baton, struct ui_file *stream)
+ {
+   fprintf_filtered (stream, "a DWARF2 location expression evaluated at runtime");
+   return 1;
+ }
+ 
+ /* This is the baton used when performing dwarf2 expression
+    evaluation.  */
+ struct dwarf_expr_baton
+ {
+   struct symbol *var;
+   struct frame_info *frame;
+ };
+ 
+ /* Read a register for the dwarf2 expression evaluator.  */
+ CORE_ADDR dwarf_expr_read_reg (void *baton, int regnum)
+ {
+   CORE_ADDR result;
+   struct dwarf_expr_baton  *debaton = (struct dwarf_expr_baton *)baton;
+   char * buf = (char *) alloca (MAX_REGISTER_RAW_SIZE);
+   get_saved_register (buf, NULL, NULL, debaton->frame, regnum, NULL);
+   result = extract_address  (buf, REGISTER_RAW_SIZE (regnum));
+   return result;
+ }
+ 
+ /* Read memory for the dwarf2 expression evaluator.  */
+ CORE_ADDR dwarf_expr_read_mem (void *baton, CORE_ADDR addr, size_t len)
+ {
+   return (CORE_ADDR) read_memory_unsigned_integer (addr, len);
+ }
+ 
+ /* Get the frame base location expression for the dwarf2 expression
+    evaluator.  */
+ void dwarf_expr_frame_base (void *baton, unsigned char **begin, unsigned char **end)
+ {
+   struct symbol *framefunc;
+   struct locexpr_baton *symbaton; 
+   struct dwarf_block *theblock;
+   struct dwarf_expr_baton *debaton = (struct dwarf_expr_baton *)baton;
+   framefunc = get_frame_function (debaton->frame);
+   symbaton = SYMBOL_LOCATION_BATON (framefunc);
+   theblock = &symbaton->locexpr;
+   *begin = theblock->data;
+   *end = theblock->data + theblock->size;
+ }
+ 
+ /* Evaluate a location description, given in THEBLOCK, in the
+    context of frame FRAME. */
+ static struct value *
+ evaluate_loc_desc (struct symbol *var, struct frame_info *frame,
+ 		   struct dwarf_block *theblock, struct type *type)
+ {
+   CORE_ADDR result;
+   struct value * retval;
+   struct dwarf_expr_baton *baton = xmalloc (sizeof (struct dwarf_expr_baton));
+   struct dwarf_expr_context *ctx;
+   
+   retval = allocate_value(type);
+   baton->var = var;
+   baton->frame = frame;
+   
+   VALUE_LVAL (retval) = lval_memory;
+   VALUE_BFD_SECTION (retval) = SYMBOL_BFD_SECTION (var);
+   
+   ctx = new_dwarf_expr_context ();
+   ctx->read_reg_baton = baton;
+   ctx->read_reg = dwarf_expr_read_reg;
+   ctx->read_mem_baton = baton;
+   ctx->read_mem = dwarf_expr_read_mem;
+   ctx->get_frame_base_baton = baton;
+   ctx->get_frame_base = dwarf_expr_frame_base;
+   ctx->error = error;
+   
+   dwarf_expr_eval (ctx, theblock->data, theblock->size);
+   
+   if (ctx->in_reg)
+     {
+       store_typed_address (VALUE_CONTENTS_RAW (retval), 
+ 			   SYMBOL_TYPE (var), dwarf_expr_fetch (ctx, 0));
+       VALUE_LVAL (retval) = not_lval;
+     }
+   else
+     {
+       result = dwarf_expr_fetch (ctx, 0);
+       VALUE_LAZY (retval) = 1;
+       VALUE_ADDRESS (retval) = result;
+     }
+   
+   free_dwarf_expr_context (ctx);
+   free (baton);
+   
+   return retval;
  }
  
  \f
Index: findvar.c
===================================================================
RCS file: /cvs/src/src/gdb/findvar.c,v
retrieving revision 1.34
diff -c -3 -p -w -B -b -r1.34 findvar.c
*** findvar.c	15 May 2002 01:01:56 -0000	1.34
--- findvar.c	8 Jul 2002 16:52:39 -0000
*************** symbol_read_needs_frame (struct symbol *
*** 382,387 ****
--- 382,391 ----
      {
        /* All cases listed explicitly so that gcc -Wall will detect it if
           we failed to consider one.  */
+     case LOC_COMPUTED:
+     case LOC_COMPUTED_ARG:
+       return (SYMBOL_LOCATION_FUNCS(sym)->read_needs_frame) (SYMBOL_LOCATION_BATON(sym));
+ 
      case LOC_REGISTER:
      case LOC_ARG:
      case LOC_REF_ARG:
*************** addresses have not been bound by the dyn
*** 589,594 ****
--- 593,610 ----
        }
        break;
  
+      case LOC_COMPUTED:
+      case LOC_COMPUTED_ARG:
+        {
+  	struct location_funcs *funcs = SYMBOL_LOCATION_FUNCS (var);
+  	void *baton = SYMBOL_LOCATION_BATON (var);
+  	
+  	if (frame == 0 && (funcs->read_needs_frame)(baton))
+  	  return 0;
+  	return (funcs->read_variable) (baton, frame);
+  	
+        }
+        break;
      case LOC_UNRESOLVED:
        {
  	struct minimal_symbol *msym;
Index: printcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/printcmd.c,v
retrieving revision 1.39
diff -c -3 -p -w -B -b -r1.39 printcmd.c
*** printcmd.c	11 May 2002 23:48:23 -0000	1.39
--- printcmd.c	8 Jul 2002 16:52:40 -0000
*************** address_info (char *exp, int from_tty)
*** 1162,1167 ****
--- 1162,1171 ----
  	}
        break;
  
+     case LOC_COMPUTED:
+     case LOC_COMPUTED_ARG:
+       (SYMBOL_LOCATION_FUNCS (sym)->describe_location)(SYMBOL_LOCATION_BATON (sym), gdb_stdout);
+       break;
      case LOC_REGISTER:
        printf_filtered ("a variable in register %s", REGISTER_NAME (val));
        break;
Index: stack.c
===================================================================
RCS file: /cvs/src/src/gdb/stack.c,v
retrieving revision 1.39
diff -c -3 -p -w -B -b -r1.39 stack.c
*** stack.c	10 Jun 2002 23:25:50 -0000	1.39
--- stack.c	8 Jul 2002 16:52:40 -0000
*************** print_block_frame_locals (struct block *
*** 1229,1234 ****
--- 1229,1235 ----
  	case LOC_REGISTER:
  	case LOC_STATIC:
  	case LOC_BASEREG:
+         case LOC_COMPUTED:
  	  values_printed = 1;
  	  for (j = 0; j < num_tabs; j++)
  	    fputs_filtered ("\t", stream);
Index: symmisc.c
===================================================================
RCS file: /cvs/src/src/gdb/symmisc.c,v
retrieving revision 1.9
diff -c -3 -p -w -B -b -r1.9 symmisc.c
*** symmisc.c	15 May 2002 21:19:21 -0000	1.9
--- symmisc.c	8 Jul 2002 16:52:40 -0000
*************** print_partial_symbols (struct partial_sy
*** 862,867 ****
--- 862,871 ----
  	case LOC_OPTIMIZED_OUT:
  	  fputs_filtered ("optimized out", outfile);
  	  break;
+ 	case LOC_COMPUTED:
+ 	case LOC_COMPUTED_ARG:
+ 	  fputs_filtered ("computed at runtime", outfile);
+ 	  break;
  	default:
  	  fputs_filtered ("<invalid location>", outfile);
  	  break;
Index: symtab.h
===================================================================
RCS file: /cvs/src/src/gdb/symtab.h,v
retrieving revision 1.33
diff -c -3 -p -w -B -b -r1.33 symtab.h
*** symtab.h	28 Jun 2002 22:09:11 -0000	1.33
--- symtab.h	8 Jul 2002 16:52:40 -0000
***************
*** 28,34 ****
  #include "obstack.h"
  #define obstack_chunk_alloc xmalloc
  #define obstack_chunk_free xfree
! 
  /* Don't do this; it means that if some .o's are compiled with GNU C
     and some are not (easy to do accidentally the way we configure
     things; also it is a pain to have to "make clean" every time you
--- 28,36 ----
  #include "obstack.h"
  #define obstack_chunk_alloc xmalloc
  #define obstack_chunk_free xfree
! #include "bcache.h"
! struct axs_value;
! struct agent_expr;
  /* Don't do this; it means that if some .o's are compiled with GNU C
     and some are not (easy to do accidentally the way we configure
     things; also it is a pain to have to "make clean" every time you
*************** enum address_class
*** 603,609 ****
       * with a level of indirection.
       */
  
!     LOC_INDIRECT
  
    };
  
--- 605,661 ----
       * with a level of indirection.
       */
  
!     LOC_INDIRECT,
!     
!     /* The variable address is computed by a set of location
!        functions.  */
!     LOC_COMPUTED,
!     
!     /* The variable is an argument, and it's address is computed by a
!        set of location functions.  */
!     LOC_COMPUTED_ARG
! 
!   };
! 
! /* A structure of function pointers describing the location of a
!    variable, structure member, or structure base class.
!    
!    These functions' BATON arguments are generic data pointers, holding
!    whatever data the functions need --- the code which provides this
!    structure also provides the actual contents of the baton, and
!    decides its form.  However, there may be other rules about where
!    the baton data must be allocated; whoever is pointing to this
!    `struct location_funcs' object will know the rules.  For example,
!    when a symbol S's location is LOC_COMPUTED, then
!    SYMBOL_LOCATION_FUNCS(S) is pointing to a location_funcs structure,
!    and SYMBOL_LOCATION_BATON(S) is the baton, which must be allocated
!    on the same obstack as the symbol itself.  */
! 
! struct location_funcs {
!   
!   /* Return the value of the variable described by BATON, relative to
!      the stack frame FRAME.  If the variable has been optimized
!      out, return zero.
! 
!      If `read_needs_frame (BATON)' is zero, then FRAME may be
!      zero.  */
!   struct value *(*read_variable) (void *baton, struct frame_info *frame);
!   
!   /* Return true if we need a frame to find the value of the object
!      described by BATON.  */
!   int (*read_needs_frame) (void *baton);
!   
!   /* Write to STREAM a natural-language description of the location of
!      the object described by BATON.  */
!   int (*describe_location) (void *baton, struct ui_file *stream);
!   
!   /* Tracepoint support.  Append bytecodes to the tracepoint agent
!      expression AX that push the address of the object described by
!      BATON.  Set VALUE appropriately.  Note --- for objects in
!      registers, this needn't emit any code; as long as it sets VALUE
!      properly, then the caller will generate the right code in the
!      process of treating this as an lvalue or rvalue.  */ void
!    (*tracepoint_var_ref) (void *baton, struct agent_expr *ax, struct axs_value *value);
  
    };
  
*************** struct symbol
*** 660,665 ****
--- 712,719 ----
        {
  	/* Used by LOC_BASEREG and LOC_BASEREG_ARG.  */
  	short basereg;
+ 	/* Location baton to pass to location functions.  */
+ 	void *locbaton;
        }
      aux_value;
  
*************** struct symbol
*** 671,676 ****
--- 725,734 ----
      /* List of ranges where this symbol is active.  This is only
         used by alias symbols at the current time.  */
      struct range_list *ranges;
+     
+     /* Location functions for LOC_COMPUTED and LOC_COMPUTED_ARGS.  */
+     struct location_funcs *locfuncs;
+     
    };
  
  
*************** struct symbol
*** 681,686 ****
--- 739,746 ----
  #define SYMBOL_BASEREG(symbol)		(symbol)->aux_value.basereg
  #define SYMBOL_ALIASES(symbol)		(symbol)->aliases
  #define SYMBOL_RANGES(symbol)		(symbol)->ranges
+ #define SYMBOL_LOCATION_BATON(symbol)   (symbol)->aux_value.locbaton
+ #define SYMBOL_LOCATION_FUNCS(symbol)   (symbol)->locfuncs
  \f
  /* A partial_symbol records the name, namespace, and address class of
     symbols whose types we have not parsed yet.  For functions, it also

Re: [RFA]: LOC_COMPUTED + abstracted dwarf2 evaluator, again

[Jim Blandy]

Daniel Berlin <dberlin@dberlin.org> writes:
> For yer convenience,  I rediffed it against the current gdb sources.

Thanks!

> I'll update the changelog dates if it ever gets reviewed or
> approved.

I do appreciate your re-posting the patch.  I have been trying to be
more prompt with reviews in the last few weeks, and not fall behind.

ChangeLog entries like this don't work:

> 	(Simple cases of *.c containing LOC_): Handle LOC_COMPUTED and
> 	LOC_COMPUTED_ARG. 

Nobody reads a ChangeLog from beginning to end; they search it.  So
even when the list of files and functions is a dozen times longer than
the actual ChangeLog entry itself, it's best to actually list all the
files explicitly, with no `*-tdep.c', `mips-{tdep,nat}.c', at all.
It takes time and it's boring, but I think it's a good rule.

> Slightly updated accompanying text (from the april submission):
> "
> The only real things left to do is give some *real* description in 
> locexpr_describe_location

Like something that prints out the expression?  Yeah, that would be
nice, but it can get involved if you've got branches.  Maybe you can
steal code from readelf; that prints location expressions.

> add tracepoint support

In general, you're looking at a Dwarf -> Agent Expression compiler.
Yahoo.  I don't know if anyone's even using the tracepoint stuff; it
seems okay to me to leave this unsupported for now.  (Although it
seems to me that tracepoints would be The Thing for debugging the
kernel.  Just write a kernel module that logs stuff to a buffer
visible via /proc, write a custom gdbserver, and away you go,
debugging page fault handlers.)

> and remove the dwarf2cfi.c evaluator .  None of these should be
> blockers to committing it or reviewing it.

No, I agree.

> It would also be nice to use location expressions in *all* cases 
> (currently, it only does it for variables) so you can get rid of 
> decode_loc_desc, but i'll leave that for the next patch.

Makes sense.

> There are a few lines that are too long (<10, i *think*), sorry about 
> that, i couldn't see an easy way to break them up and have them make 
> sense.

Well, this does need to be corrected before you commit.  Come on, the
rest of us all do it, I think you can handle the agony.  :)

> Splitting this patch into loc_computed addition, and dwarf2 implementation 
> of loc_computed, doesn't make it much smaller (loc_computed_addition is 
> maybe 5% of the patch), so if size is a concern, sorry, but it's pretty 
> much unsplittable beyond that (the biggest portion is the addition of the 
> abstracted evaluator, but the dwarf2 implementation doesn't work without 
> it).

It's not so much that a patch is too large or too small, it's the
additional brain load from having to figure out which change each hunk
belongs to.  I^HSome of us have limited resources to work with.


The patch misses some cases:

- I think ch-exp.c:ch_lex and m2-exp.y:yylex need some additional
  `case's for LOC_COMPUTED{,_ARG}.  I grant you, Chill is dead, but
  they're easy tweaks.
- print_frame_args needs a case for LOC_COMPUTED_ARG.
- print_block_frame_locals needs a case for LOC_COMPUTED.
- print_frame_arg_vars needs a case for LOC_COMPUTED_ARG.

> Index: symmisc.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/symmisc.c,v
> retrieving revision 1.9
> diff -c -3 -p -w -B -b -r1.9 symmisc.c
> *** symmisc.c	15 May 2002 21:19:21 -0000	1.9
> --- symmisc.c	8 Jul 2002 16:52:40 -0000
> *************** print_partial_symbols (struct partial_sy
> *** 862,867 ****
> --- 862,871 ----
>   	case LOC_OPTIMIZED_OUT:
>   	  fputs_filtered ("optimized out", outfile);
>   	  break;
> + 	case LOC_COMPUTED:
> + 	case LOC_COMPUTED_ARG:
> + 	  fputs_filtered ("computed at runtime", outfile);
> + 	  break;
>   	default:
>   	  fputs_filtered ("<invalid location>", outfile);
>   	  break;

I guess we can't do anything more detailed here because we only have
partial symbol table information --- the address class, but not the
function group or the baton.  That's a pity, but I don't see how to do
better.

> + /* This is a struct location function, this one returns 1 if we need a
> +    frame to read the value of the variable. */
> + static int 
> + locexpr_read_needs_frame (void *baton)
> + {
> +   return 1;
> + }

This needs to be smarter, in light of this code in find_var_value:

> +      case LOC_COMPUTED:
> +      case LOC_COMPUTED_ARG:
> +        {
> +  	struct location_funcs *funcs = SYMBOL_LOCATION_FUNCS (var);
> +  	void *baton = SYMBOL_LOCATION_BATON (var);
> +  	
> +  	if (frame == 0 && (funcs->read_needs_frame)(baton))
> +  	  return 0;
> +  	return (funcs->read_variable) (baton, frame);
> +  	
> +        }
> +        break;

So read_var_value will return zero whenever it's given a
LOC_COMPUTED{,_ARG} symbol and zero for a frame.  But read_var_value
gets zero for the frame whenever you try to print a global or static
variable when the program isn't running, even though the lack of a
frame is harmless.  (value_struct_elt_for_reference seems to pass a
frame of zero, too, but I'm not sure in what contexts that occurs.)

> !     LOC_INDIRECT,
> !     
> !     /* The variable address is computed by a set of location
> !        functions.  */
> !     LOC_COMPUTED,
> !     
> !     /* The variable is an argument, and it's address is computed by a
> !        set of location functions.  */
> !     LOC_COMPUTED_ARG
> ! 
> !   };

These comments should explicitly say how one computes the address ---
or at least refer to the comment above `struct location_funcs', just
below.

> *************** struct symbol
> *** 660,665 ****
> --- 712,719 ----
>         {
>   	/* Used by LOC_BASEREG and LOC_BASEREG_ARG.  */
>   	short basereg;
> + 	/* Location baton to pass to location functions.  */
> + 	void *locbaton;
>         }
>       aux_value;
>   
> *************** struct symbol
> *** 671,676 ****
> --- 725,734 ----
>       /* List of ranges where this symbol is active.  This is only
>          used by alias symbols at the current time.  */
>       struct range_list *ranges;
> +     
> +     /* Location functions for LOC_COMPUTED and LOC_COMPUTED_ARGS.  */
> +     struct location_funcs *locfuncs;
> +     
>     };

I think both locbaton and locfuncs should be in the union (as a
struct, of course).

> *************** read_func_scope (struct die_info *die, s
> *** 1886,1891 ****
> --- 1912,1931 ----
>   
>     new = push_context (0, lowpc);
>     new->name = new_symbol (die, die->type, objfile, cu_header);
> + 
> +   /* If it has a location expression for fbreg, record it.  
> +      Since not all symbols use location expressions exclusively yet,
> +      we still need to decode it above. */
> +   if (attr)
> +     {
> +       baton = obstack_alloc (&objfile->symbol_obstack, sizeof (struct locexpr_baton));
> +       baton->sym = new->name;
> +       baton->obj = objfile;
> +       memcpy (&baton->locexpr, DW_BLOCK (attr), sizeof (struct dwarf_block));
> +       SYMBOL_LOCATION_FUNCS (new->name) = &locexpr_funcs;
> +       SYMBOL_LOCATION_BATON (new->name) = baton;
> +     }
> + 
>     list_in_scope = &local_symbols;
>   
>     if (die->has_children)

Whoa, this is problematic.

- Big issue: you're assigning a meaning to the aux_value field of a
  function's symbol, where previously it had none.  This at the very
  least needs to be documented --- I almost didn't notice the issue,
  since it isn't mentioned in the comments for `struct symbol'.  And
  while we certainly need some way to find the fbreg expression, I'm
  sure that taking over generic fields of function symbols for a
  rather Dwarf-specific optimization is not the right way to do this.

  I think it would be better if all the batons for a function's
  locals, etc. had a pointer to a shared thingy for the fbreg
  expression.  That means the Dwarf reader has to build this new fbreg
  structure when it enters the function, and just leave a pointer to
  it lying around for all the new batons to use, which is a bit icky.
  But that's basically what we do now, with the `frame_base_reg' and
  `frame_base_offset' global variables.

- Small issue: DW_BLOCK points into the .dwarf_info buffer, right?
  Are there any other cases where the symbol table points into
  .dwarf_info?  The names get copied for partial symbols and full
  symbols.  I think we need to copy the whole location expression into
  the objfile's obstack.  Similarly for the DW_TAG_variable case.

> +   struct dwarf_expr_baton  *debaton = (struct dwarf_expr_baton *)baton;

(a `debaton' being the fundamental particle of argument...  :) )

> + /* Read a register for the dwarf2 expression evaluator.  */
> + CORE_ADDR dwarf_expr_read_reg (void *baton, int regnum)
> + {
> +   CORE_ADDR result;
> +   struct dwarf_expr_baton  *debaton = (struct dwarf_expr_baton *)baton;
> +   char * buf = (char *) alloca (MAX_REGISTER_RAW_SIZE);
> +   get_saved_register (buf, NULL, NULL, debaton->frame, regnum, NULL);
> +   result = extract_address  (buf, REGISTER_RAW_SIZE (regnum));
> +   return result;
> + }

Would `read_register' work here?

Your post doesn't include dwarf2expr.[ch].

If your evaluator is based on the one in dwarf2cfi.c, did you
incorporate Mark's recent fix?

2002-07-08  Mark Kettenis  <kettenis@gnu.org>

	* dwarf2cfi.c: Include "gcore.h".
	(execute_stack_op): Fix implementation of the
	DW_OP_deref and DW_OP_deref_size operators by letting do their
	lookup in the target.

But overall --- this is great.  I'm looking forward to the next
revision.  Thanks!

Re: [RFA]: LOC_COMPUTED + abstracted dwarf2 evaluator, again

[Daniel Berlin]

On 8 Jul 2002, Jim Blandy wrote:

> 
> Daniel Berlin <dberlin@dberlin.org> writes:
> > For yer convenience,  I rediffed it against the current gdb sources.
> 
> Thanks!
> 
> > I'll update the changelog dates if it ever gets reviewed or
> > approved.
> 
> I do appreciate your re-posting the patch.  I have been trying to be
> more prompt with reviews in the last few weeks, and not fall behind.
> 
> ChangeLog entries like this don't work:
> 
> > 	(Simple cases of *.c containing LOC_): Handle LOC_COMPUTED and
> > 	LOC_COMPUTED_ARG. 
> 
> Nobody reads a ChangeLog from beginning to end; they search it.  So
> even when the list of files and functions is a dozen times longer than
> the actual ChangeLog entry itself, it's best to actually list all the
> files explicitly, with no `*-tdep.c', `mips-{tdep,nat}.c', at all.
> It takes time and it's boring, but I think it's a good rule.

Okeydokey.

> 
> > Slightly updated accompanying text (from the april submission):
> > "
> > The only real things left to do is give some *real* description in 
> > locexpr_describe_location
> 
> Like something that prints out the expression?  Yeah, that would be
> nice, but it can get involved if you've got branches.  Maybe you can
> steal code from readelf; that prints location expressions.
I know, but the problem is pretty printing
> 
> > add tracepoint support
> 
> In general, you're looking at a Dwarf -> Agent Expression compiler.
> Yahoo.  I don't know if anyone's even using the tracepoint stuff; it
> seems okay to me to leave this unsupported for now.  (Although it
> seems to me that tracepoints would be The Thing for debugging the
> kernel.  Just write a kernel module that logs stuff to a buffer
> visible via /proc, write a custom gdbserver, and away you go,
> debugging page fault handlers.)
> 
> > and remove the dwarf2cfi.c evaluator .  None of these should be
> > blockers to committing it or reviewing it.
> 
> No, I agree.
> 
> > It would also be nice to use location expressions in *all* cases 
> > (currently, it only does it for variables) so you can get rid of 
> > decode_loc_desc, but i'll leave that for the next patch.
> 
> Makes sense.
> 
> > There are a few lines that are too long (<10, i *think*), sorry about 
> > that, i couldn't see an easy way to break them up and have them make 
> > sense.
> 
> Well, this does need to be corrected before you commit.  Come on, the
> rest of us all do it, I think you can handle the agony.  :)

It's not that I'm not going to do it, it's that i haven't figured out how 
to make it look nice.

You end up with it (gdb_indent) wanting to indent it like:


bob (fred, 5 *
     f (5 + 9 * 
        8, 7),
        5)

Or something equally silly (IE it makes it very hard to parse)

> 
> > Splitting this patch into loc_computed addition, and dwarf2 implementation 
> > of loc_computed, doesn't make it much smaller (loc_computed_addition is 
> > maybe 5% of the patch), so if size is a concern, sorry, but it's pretty 
> > much unsplittable beyond that (the biggest portion is the addition of the 
> > abstracted evaluator, but the dwarf2 implementation doesn't work without 
> > it).
> 
> It's not so much that a patch is too large or too small, it's the
> additional brain load from having to figure out which change each hunk
> belongs to.  I^HSome of us have limited resources to work with.
> 
> 
> The patch misses some cases:
> 
> - I think ch-exp.c:ch_lex and m2-exp.y:yylex need some additional
>   `case's for LOC_COMPUTED{,_ARG}.  I grant you, Chill is dead, but
>   they're easy tweaks.

Sure.

> - print_frame_args needs a case for LOC_COMPUTED_ARG.
> - print_block_frame_locals needs a case for LOC_COMPUTED.
> - print_frame_arg_vars needs a case for LOC_COMPUTED_ARG.

I could have sworn I had done these.
In fact, I know i did.

I must have had made some changes after posting it in april that were lost 
when i moved and a disk crashed (IE the gdb-patches posting was the only 
copy i had).

I'll go through it again and make sure nothing else got lost.
 > 
> > Index: symmisc.c
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/symmisc.c,v
> > retrieving revision 1.9
> > diff -c -3 -p -w -B -b -r1.9 symmisc.c
> > *** symmisc.c	15 May 2002 21:19:21 -0000	1.9
> > --- symmisc.c	8 Jul 2002 16:52:40 -0000
> > *************** print_partial_symbols (struct partial_sy
> > *** 862,867 ****
> > --- 862,871 ----
> >   	case LOC_OPTIMIZED_OUT:
> >   	  fputs_filtered ("optimized out", outfile);
> >   	  break;
> > + 	case LOC_COMPUTED:
> > + 	case LOC_COMPUTED_ARG:
> > + 	  fputs_filtered ("computed at runtime", outfile);
> > + 	  break;
> >   	default:
> >   	  fputs_filtered ("<invalid location>", outfile);
> >   	  break;
> 
> I guess we can't do anything more detailed here because we only have
> partial symbol table information --- the address class, but not the
> function group or the baton.  That's a pity, but I don't see how to do
> better.

The only time it's printed is if you maint print psymbols, happily, and 
if you are doing that, you've got worse problems :).


> > + /* This is a struct location function, this one returns 1 if we need a
> > +    frame to read the value of the variable. */
> > + static int 
> > + locexpr_read_needs_frame (void *baton)
> > + {
> > +   return 1;
> > + }
> 
> This needs to be smarter, in light of this code in find_var_value:
We can't know if it's got a constant address without looking through the 
location codes.
But since the expression evaluator is opaque, *it* is what looks at the 
actual location expressions, we (the reader) treat them as magic blocks of 
data.
:)
The simple workaround is to have a function in the same file as the 
evaluator that tells us if a location contains a register reference, since 
it has access to the data in question.


> 
> > +      case LOC_COMPUTED:
> > +      case LOC_COMPUTED_ARG:
> > +        {
> > +  	struct location_funcs *funcs = SYMBOL_LOCATION_FUNCS (var);
> > +  	void *baton = SYMBOL_LOCATION_BATON (var);
> > +  	
> > +  	if (frame == 0 && (funcs->read_needs_frame)(baton))
> > +  	  return 0;
> > +  	return (funcs->read_variable) (baton, frame);
> > +  	
> > +        }
> > +        break;
> 
> So read_var_value will return zero whenever it's given a
> LOC_COMPUTED{,_ARG} symbol and zero for a frame.  But read_var_value
> gets zero for the frame whenever you try to print a global or static
> variable when the program isn't running, even though the lack of a
> frame is harmless.  (value_struct_elt_for_reference seems to pass a
> frame of zero, too, but I'm not sure in what contexts that occurs.)
Wheee.


> 
> > !     LOC_INDIRECT,
> > !     
> > !     /* The variable address is computed by a set of location
> > !        functions.  */
> > !     LOC_COMPUTED,
> > !     
> > !     /* The variable is an argument, and it's address is computed by a
> > !        set of location functions.  */
> > !     LOC_COMPUTED_ARG
> > ! 
> > !   };
> 
> These comments should explicitly say how one computes the address ---
> or at least refer to the comment above `struct location_funcs', just
> below.
OKeydokey.

> 
> > *************** struct symbol
> > *** 660,665 ****
> > --- 712,719 ----
> >         {
> >   	/* Used by LOC_BASEREG and LOC_BASEREG_ARG.  */
> >   	short basereg;
> > + 	/* Location baton to pass to location functions.  */
> > + 	void *locbaton;
> >         }
> >       aux_value;
> >   
> > *************** struct symbol
> > *** 671,676 ****
> > --- 725,734 ----
> >       /* List of ranges where this symbol is active.  This is only
> >          used by alias symbols at the current time.  */
> >       struct range_list *ranges;
> > +     
> > +     /* Location functions for LOC_COMPUTED and LOC_COMPUTED_ARGS.  */
> > +     struct location_funcs *locfuncs;
> > +     
> >     };
> 
> I think both locbaton and locfuncs should be in the union (as a
> struct, of course).

Okey.
> > > *************** read_func_scope (struct die_info *die, s > 
> *** 1886,1891 ****
> > --- 1912,1931 ----
> >   
> >     new = push_context (0, lowpc);
> >     new->name = new_symbol (die, die->type, objfile, cu_header);
> > + 
> > +   /* If it has a location expression for fbreg, record it.  
> > +      Since not all symbols use location expressions exclusively yet,
> > +      we still need to decode it above. */
> > +   if (attr)
> > +     {
> > +       baton = obstack_alloc (&objfile->symbol_obstack, sizeof (struct locexpr_baton));
> > +       baton->sym = new->name;
> > +       baton->obj = objfile;
> > +       memcpy (&baton->locexpr, DW_BLOCK (attr), sizeof (struct dwarf_block));
> > +       SYMBOL_LOCATION_FUNCS (new->name) = &locexpr_funcs;
> > +       SYMBOL_LOCATION_BATON (new->name) = baton;
> > +     }
> > + 
> >     list_in_scope = &local_symbols;
> >   
> >     if (die->has_children)
> 
> Whoa, this is problematic.
> 
> - Big issue: you're assigning a meaning to the aux_value field of a
>   function's symbol, where previously it had none.

So?

The aux value field is there because "Some symbols require an additional 
value to be recorded on a per-symbol basis. Stash those values here".

Function symbols require an additional value to be recorded on a per 
symbol basis.
So I stashed it there.

Seemed to make sense at the time.

If you've got a better idea, i'm happy to implement it.
>   This at the very
>   least needs to be documented --- I almost didn't notice the issue,
>   since it isn't mentioned in the comments for `struct symbol'.
Oversight on my part.

>   And
>   while we certainly need some way to find the fbreg expression, I'm
>   sure that taking over generic fields of function symbols for a
>   rather Dwarf-specific optimization is not the right way to do this.

It's actually neither a generic field, nor dwarf specific, 
nor an optimization.

The field is per-symbol specific, and has no specific purpose given, other 
than to be a place to stash values.

It's not dwarf specific. Any time we need some function level 
value that is computed at runtime, to compute something *else* at runtime, 
we are gonna have to store the baton somewhere.  It is, 
in the debug info, attached to the function symbols, so it seemed to make 
sense that it should stay attached to the function symbol in gdb.
But, as I said, if you've got a better idea, i'm happy to implement it.
It's no skin off my back, and i'm not religious about where the fbreg is 
stored.
:)



> 
>   I think it would be better if all the batons for a function's
>   locals, etc. had a pointer to a shared thingy for the fbreg
>   expression. 

If you like, but IMHO, it just makes the code more complex, and less 
understandable to debug. It makes less sense from a perspective of someone 
trying to 
understand what's going on, because the pointer would only be different in 
different functions anyway.  And if they looked at the debug info, they 
would see the expression associated only with the function, and only 
appear once.

>    That means the Dwarf reader has to build this new fbreg
>   structure when it enters the function, and just leave a pointer to
>   it lying around for all the new batons to use, which is a bit icky.
>   But that's basically what we do now, with the `frame_base_reg' and
>   `frame_base_offset' global variables.

If you like, i'll do this, but i still don't see it as a better solution.  
It just seems to make it more complex, all for the added benefit of 
every reader with these types of expressions having to redo this type of 
thing.

> - Small issue: DW_BLOCK points into the .dwarf_info buffer, right?

Yup.

>   Are there any other cases where the symbol table points into
>   .dwarf_info?  The names get copied for partial symbols and full
>   symbols.  I think we need to copy the whole location expression into
>   the objfile's obstack.  Similarly for the DW_TAG_variable case.

Okeydokey.

Some year, when bfd supports mmap, ...

> 
> > +   struct dwarf_expr_baton  *debaton = (struct dwarf_expr_baton *)baton;
> 
> (a `debaton' being the fundamental particle of argument...  :) )

dwarf-expr-baton
:)

> 
> > + /* Read a register for the dwarf2 expression evaluator.  */
> > + CORE_ADDR dwarf_expr_read_reg (void *baton, int regnum)
> > + {
> > +   CORE_ADDR result;
> > +   struct dwarf_expr_baton  *debaton = (struct dwarf_expr_baton *)baton;
> > +   char * buf = (char *) alloca (MAX_REGISTER_RAW_SIZE);
> > +   get_saved_register (buf, NULL, NULL, debaton->frame, regnum, NULL);
> > +   result = extract_address  (buf, REGISTER_RAW_SIZE (regnum));
> > +   return result;
> > + }
> 
> Would `read_register' work here?

No, because we read_register gives the register for the *current* 
frame, and the frame *we want* is associated with a given evaluator baton. 
(debaton->frame).
So the answer is "for reading variables, debaton->frame is, AFAIK, always 
going to be the same as the frame read_register would use, but from an 
abstraction perspective, we shouldn't use this fact since it's using magic knowledge 
of how we happened to initalize the  evaluator context below, which is 
bad." 
IIRC, the whole reason for this at all is that it's necessary to be able 
to reuse the evaluator in dwarf2cfi.

> 
> Your post doesn't include dwarf2expr.[ch].

Whoops.
I forgot I didn't readd it with cvs add, so diff didn't diff it.

> 
> If your evaluator is based on the one in dwarf2cfi.c, did you
> incorporate Mark's recent fix?
It's not based on dwarf2cfi.

The routine that reads memory (remember, it's abstracted, as you 
requested) for dwarf2read's usage of it (location expressions) is in 
dwarf_expr_read_mem, which is simply:


/* Read memory for the dwarf2 expression evaluator.  */
CORE_ADDR dwarf_expr_read_mem (void *baton, CORE_ADDR addr, size_t len)
{
  return (CORE_ADDR) read_memory_unsigned_integer (addr, len);
}

Re: [RFA]: LOC_COMPUTED + abstracted dwarf2 evaluator, again

[Jim Blandy]

Daniel Berlin <dberlin@dberlin.org> writes:
> > > + /* This is a struct location function, this one returns 1 if we need a
> > > +    frame to read the value of the variable. */
> > > + static int 
> > > + locexpr_read_needs_frame (void *baton)
> > > + {
> > > +   return 1;
> > > + }
> > 
> > This needs to be smarter, in light of this code in find_var_value:
> We can't know if it's got a constant address without looking through the 
> location codes.
> But since the expression evaluator is opaque, *it* is what looks at the 
> actual location expressions, we (the reader) treat them as magic blocks of 
> data.
> :)
> The simple workaround is to have a function in the same file as the 
> evaluator that tells us if a location contains a register reference, since 
> it has access to the data in question.

Yes, that sounds like the right way to do it.  Keep all the knowledge
about DW_OP_* and their operands in the same place.  (Perhaps we could
put the dwarf expression printing code there, too, and they could be
driven by the same table.  But that should be a separate patch, please.)

> > > > *************** read_func_scope (struct die_info *die, s > 
> > *** 1886,1891 ****
> > > --- 1912,1931 ----
> > >   
> > >     new = push_context (0, lowpc);
> > >     new->name = new_symbol (die, die->type, objfile, cu_header);
> > > + 
> > > +   /* If it has a location expression for fbreg, record it.  
> > > +      Since not all symbols use location expressions exclusively yet,
> > > +      we still need to decode it above. */
> > > +   if (attr)
> > > +     {
> > > +       baton = obstack_alloc (&objfile->symbol_obstack, sizeof (struct locexpr_baton));
> > > +       baton->sym = new->name;
> > > +       baton->obj = objfile;
> > > +       memcpy (&baton->locexpr, DW_BLOCK (attr), sizeof (struct dwarf_block));
> > > +       SYMBOL_LOCATION_FUNCS (new->name) = &locexpr_funcs;
> > > +       SYMBOL_LOCATION_BATON (new->name) = baton;
> > > +     }
> > > + 
> > >     list_in_scope = &local_symbols;
> > >   
> > >     if (die->has_children)
> > 
> > Whoa, this is problematic.
> > 
> > - Big issue: you're assigning a meaning to the aux_value field of a
> >   function's symbol, where previously it had none.
> 
> So?
> 
> The aux value field is there because "Some symbols require an additional 
> value to be recorded on a per-symbol basis. Stash those values here".
> 
> Function symbols require an additional value to be recorded on a per 
> symbol basis.
> So I stashed it there.
> 
> Seemed to make sense at the time.

Well, right, but if it's data specific to a particular debug format,
well, that needs to go someplace debug-reader specific.


> 
> If you've got a better idea, i'm happy to implement it.
> >   This at the very
> >   least needs to be documented --- I almost didn't notice the issue,
> >   since it isn't mentioned in the comments for `struct symbol'.
> Oversight on my part.
> 
> >   And
> >   while we certainly need some way to find the fbreg expression, I'm
> >   sure that taking over generic fields of function symbols for a
> >   rather Dwarf-specific optimization is not the right way to do this.
> 
> It's actually neither a generic field, nor dwarf specific, 
> nor an optimization.
>
> The field is per-symbol specific, and has no specific purpose given, other 
> than to be a place to stash values.

You're right.  The fbreg info is strictly necessary for evaluating
Dwarf expressions --- it's not an optimization.

The "generic fields" bit is sloppy, too.  Here's what I should have
written:

It's important that one should be able to look at sym->aclass and know
exactly what aux_value means.  They're not really separate values ---
together aclass and aux_value form a discriminated union.  (This is
the same reason I wanted the location function pointer in a struct in
aux_value.)

As it stands, the patch effectively says, "when aclass == LOC_BLOCK,
aux_value holds debug-format-specific info."  I think if you
documented it that way, I'd mind less, but it still feels really
wrong.

> >   I think it would be better if all the batons for a function's
> >   locals, etc. had a pointer to a shared thingy for the fbreg
> >   expression. 
> 
> If you like, but IMHO, it just makes the code more complex, and less 
> understandable to debug. It makes less sense from a perspective of someone 
> trying to 
> understand what's going on, because the pointer would only be different in 
> different functions anyway.  And if they looked at the debug info, they 
> would see the expression associated only with the function, and only 
> appear once.

I agree with everything you say here.  The frame_base_reg and
frame_base_offset global vars are a crock.  But they are a crock local
to dwarf2read.c that *could* be replaced by something cleaner, as part
of a general cleanup of that area in Dwarf.  But the end result ---
having the batons point to a shared structure giving the data for
DW_OP_fbreg --- seems quite clean to me.

You won't need to do that function lookup by PC each time you print a
variable, either...

> >   Are there any other cases where the symbol table points into
> >   .dwarf_info?  The names get copied for partial symbols and full
> >   symbols.  I think we need to copy the whole location expression into
> >   the objfile's obstack.  Similarly for the DW_TAG_variable case.
> 
> Okeydokey.
> 
> Some year, when bfd supports mmap, ...

I used to be skeptical about your enthusiasm for mmap, but I'm being
persuaded.  I think I remember Ulrich Drepper talking about how he
found that mmap was much faster in libelf and libdwarf.

> > > +   struct dwarf_expr_baton  *debaton = (struct dwarf_expr_baton *)baton;
> > 
> > (a `debaton' being the fundamental particle of argument...  :) )
> 
> dwarf-expr-baton
> :)

Oh, that was clear.  It just hit me funny.  :)

> > > + /* Read a register for the dwarf2 expression evaluator.  */
> > > + CORE_ADDR dwarf_expr_read_reg (void *baton, int regnum)
> > > + {
> > > +   CORE_ADDR result;
> > > +   struct dwarf_expr_baton  *debaton = (struct dwarf_expr_baton *)baton;
> > > +   char * buf = (char *) alloca (MAX_REGISTER_RAW_SIZE);
> > > +   get_saved_register (buf, NULL, NULL, debaton->frame, regnum, NULL);
> > > +   result = extract_address  (buf, REGISTER_RAW_SIZE (regnum));
> > > +   return result;
> > > + }
> > 
> > Would `read_register' work here?
> 
> No, because we read_register gives the register for the *current* 
> frame, and the frame *we want* is associated with a given evaluator baton. 
> (debaton->frame).

Oh --- right.  It's a shame there isn't something like read_register
that takes a frame as an argument.  Or at least, I can't find one.

> So the answer is "for reading variables, debaton->frame is, AFAIK,
> always going to be the same as the frame read_register would use,
> but from an abstraction perspective, we shouldn't use this fact
> since it's using magic knowledge of how we happened to initalize the
> evaluator context below, which is bad."  IIRC, the whole reason for
> this at all is that it's necessary to be able to reuse the evaluator
> in dwarf2cfi.

No, I think it's absolutely correct to insist on using debaton->frame.
The fact that you have to allocate a little buffer and then parse it
with extract_address isn't your fault.

> > If your evaluator is based on the one in dwarf2cfi.c, did you
> > incorporate Mark's recent fix?
> It's not based on dwarf2cfi.
> 
> The routine that reads memory (remember, it's abstracted, as you 
> requested) for dwarf2read's usage of it (location expressions) is in 
> dwarf_expr_read_mem, which is simply:
> 
> 
> /* Read memory for the dwarf2 expression evaluator.  */
> CORE_ADDR dwarf_expr_read_mem (void *baton, CORE_ADDR addr, size_t len)
> {
>   return (CORE_ADDR) read_memory_unsigned_integer (addr, len);
> }

Right --- you've already had to replace exactly the part that Mark
fixed.

Re: [RFA]: LOC_COMPUTED + abstracted dwarf2 evaluator, again

[Jim Blandy]

Daniel Berlin <dberlin@dberlin.org> writes:
> + /* Evaluate a location description, given in THEBLOCK, in the
> +    context of frame FRAME. */
> + static struct value *
> + evaluate_loc_desc (struct symbol *var, struct frame_info *frame,
> + 		   struct dwarf_block *theblock, struct type *type)
> + {
> +   CORE_ADDR result;
> +   struct value * retval;
> +   struct dwarf_expr_baton *baton = xmalloc (sizeof (struct dwarf_expr_baton));
> +   struct dwarf_expr_context *ctx;
> +   
> +   retval = allocate_value(type);
> +   baton->var = var;
> +   baton->frame = frame;
> +   
> +   VALUE_LVAL (retval) = lval_memory;
> +   VALUE_BFD_SECTION (retval) = SYMBOL_BFD_SECTION (var);
> +   
> +   ctx = new_dwarf_expr_context ();
> +   ctx->read_reg_baton = baton;
> +   ctx->read_reg = dwarf_expr_read_reg;
> +   ctx->read_mem_baton = baton;
> +   ctx->read_mem = dwarf_expr_read_mem;
> +   ctx->get_frame_base_baton = baton;
> +   ctx->get_frame_base = dwarf_expr_frame_base;
> +   ctx->error = error;
> +   
> +   dwarf_expr_eval (ctx, theblock->data, theblock->size);
> +   
> +   if (ctx->in_reg)
> +     {
> +       store_typed_address (VALUE_CONTENTS_RAW (retval), 
> + 			   SYMBOL_TYPE (var), dwarf_expr_fetch (ctx, 0));
> +       VALUE_LVAL (retval) = not_lval;
> +     }
> +   else
> +     {
> +       result = dwarf_expr_fetch (ctx, 0);
> +       VALUE_LAZY (retval) = 1;
> +       VALUE_ADDRESS (retval) = result;
> +     }

This looks wrong.  If evaluating the Dwarf location expression yields
an `in_reg' result, shouldn't the value be an lval_register, with its
the address set to the register number?

I think the evaluator is confused in this regard, too; see my upcoming
post about that.

Re: [RFA]: LOC_COMPUTED + abstracted dwarf2 evaluator, again

[Daniel Berlin]

On 9 Jul 2002, Jim Blandy wrote:

> 
> Daniel Berlin <dberlin@dberlin.org> writes:
> > + /* Evaluate a location description, given in THEBLOCK, in the
> > +    context of frame FRAME. */
> > + static struct value *
> > + evaluate_loc_desc (struct symbol *var, struct frame_info *frame,
> > + 		   struct dwarf_block *theblock, struct type *type)
> > + {
> > +   CORE_ADDR result;
> > +   struct value * retval;
> > +   struct dwarf_expr_baton *baton = xmalloc (sizeof (struct dwarf_expr_baton));
> > +   struct dwarf_expr_context *ctx;
> > +   
> > +   retval = allocate_value(type);
> > +   baton->var = var;
> > +   baton->frame = frame;
> > +   
> > +   VALUE_LVAL (retval) = lval_memory;
> > +   VALUE_BFD_SECTION (retval) = SYMBOL_BFD_SECTION (var);
> > +   
> > +   ctx = new_dwarf_expr_context ();
> > +   ctx->read_reg_baton = baton;
> > +   ctx->read_reg = dwarf_expr_read_reg;
> > +   ctx->read_mem_baton = baton;
> > +   ctx->read_mem = dwarf_expr_read_mem;
> > +   ctx->get_frame_base_baton = baton;
> > +   ctx->get_frame_base = dwarf_expr_frame_base;
> > +   ctx->error = error;
> > +   
> > +   dwarf_expr_eval (ctx, theblock->data, theblock->size);
> > +   
> > +   if (ctx->in_reg)
> > +     {
> > +       store_typed_address (VALUE_CONTENTS_RAW (retval), 
> > + 			   SYMBOL_TYPE (var), dwarf_expr_fetch (ctx, 0));
> > +       VALUE_LVAL (retval) = not_lval;
> > +     }
> > +   else
> > +     {
> > +       result = dwarf_expr_fetch (ctx, 0);
> > +       VALUE_LAZY (retval) = 1;
> > +       VALUE_ADDRESS (retval) = result;
> > +     }
> 
> This looks wrong.  If evaluating the Dwarf location expression yields
> an `in_reg' result, shouldn't the value be an lval_register, with its
> the address set to the register number?

This doesn't work, in reality.

IIRC, you run into problems dereferencing the value, because it 
thinks it *only* lives in a register (IE it's not really a memory 
location), regardless of what you tell it.

I may be misremembering the exact reason why, but I know things don't work 
if you do this, even though it would seem to make sense (I had originally 
written it to do what you suggest).


> 
> I think the evaluator is confused in this regard, too; see my upcoming
> post about that.
> 
> 

Re: [RFA]: LOC_COMPUTED + abstracted dwarf2 evaluator, again

[Jim Blandy]

Daniel Berlin <dberlin@dberlin.org> writes:
> > > +   if (ctx->in_reg)
> > > +     {
> > > +       store_typed_address (VALUE_CONTENTS_RAW (retval), 
> > > + 			   SYMBOL_TYPE (var), dwarf_expr_fetch (ctx, 0));
> > > +       VALUE_LVAL (retval) = not_lval;
> > > +     }
> > > +   else
> > > +     {
> > > +       result = dwarf_expr_fetch (ctx, 0);
> > > +       VALUE_LAZY (retval) = 1;
> > > +       VALUE_ADDRESS (retval) = result;
> > > +     }
> > 
> > This looks wrong.  If evaluating the Dwarf location expression yields
> > an `in_reg' result, shouldn't the value be an lval_register, with its
> > the address set to the register number?
> 
> This doesn't work, in reality.
> 
> IIRC, you run into problems dereferencing the value, because it 
> thinks it *only* lives in a register (IE it's not really a memory 
> location), regardless of what you tell it.
> 
> I may be misremembering the exact reason why, but I know things don't work 
> if you do this, even though it would seem to make sense (I had originally 
> written it to do what you suggest).

The `in_reg' case needs to call value_from_register, not explicitly
set the value's `lval' and `address' fields, as I implied.  Calling
value_from_register will take care of finding registers that have been
spilled to the stack.

Re: [RFA]: LOC_COMPUTED + abstracted dwarf2 evaluator, again

[Daniel Berlin]

On 9 Jul 2002, Jim Blandy wrote:

> 
> Daniel Berlin <dberlin@dberlin.org> writes:
> > > > +   if (ctx->in_reg)
> > > > +     {
> > > > +       store_typed_address (VALUE_CONTENTS_RAW (retval), 
> > > > + 			   SYMBOL_TYPE (var), dwarf_expr_fetch (ctx, 0));
> > > > +       VALUE_LVAL (retval) = not_lval;
> > > > +     }
> > > > +   else
> > > > +     {
> > > > +       result = dwarf_expr_fetch (ctx, 0);
> > > > +       VALUE_LAZY (retval) = 1;
> > > > +       VALUE_ADDRESS (retval) = result;
> > > > +     }
> > > 
> > > This looks wrong.  If evaluating the Dwarf location expression yields
> > > an `in_reg' result, shouldn't the value be an lval_register, with its
> > > the address set to the register number?
> > 
> > This doesn't work, in reality.
> > 
> > IIRC, you run into problems dereferencing the value, because it 
> > thinks it *only* lives in a register (IE it's not really a memory 
> > location), regardless of what you tell it.
> > 
> > I may be misremembering the exact reason why, but I know things don't work 
> > if you do this, even though it would seem to make sense (I had originally 
> > written it to do what you suggest).
> 
> The `in_reg' case needs to call value_from_register, not explicitly
> set the value's `lval' and `address' fields, as I implied.  Calling
> value_from_register will take care of finding registers that have been
> spilled to the stack.

As you wish, but this is what i had tried next (or was it a cousin of 
value_from_register).
I ran into problems with pointers vs non-pointer types, and pass by 
reference vs pass by value.
I had to special case pointers and references, and do something slightly 
difference or else it wouldn't let us deref them.
Though it looks like value_from_register handles this, so maybe it was a 
sibling.
I wish i still had the old code to look at.



> 
> 

Re: [RFA]: LOC_COMPUTED + abstracted dwarf2 evaluator, again

[Daniel Jacobowitz]

On Mon, Jul 08, 2002 at 01:02:59PM -0400, Daniel Berlin wrote:
> For yer convenience,  I rediffed it against the current gdb sources.
> I'll update the changelog dates if it ever gets reviewed or approved.
> Slightly updated accompanying text (from the april submission):
> "
> The only real things left to do is give some *real* description in 
> locexpr_describe_location, add tracepoint support, and remove the 
> dwarf2cfi.c evaluator .  
> None of these should be blockers to committing it or reviewing it.
> 
> It would also be nice to use location expressions in *all* cases 
> (currently, it only does it for variables) so you can get rid of 
> decode_loc_desc, but i'll leave that for the next patch.
>  
> There are a few lines that are too long (<10, i *think*), sorry about 
> that, i couldn't see an easy way to break them up and have them make 
> sense.
> 
> Splitting this patch into loc_computed addition, and dwarf2 implementation 
> of loc_computed, doesn't make it much smaller (loc_computed_addition is 
> maybe 5% of the patch), so if size is a concern, sorry, but it's pretty 
> much unsplittable beyond that (the biggest portion is the addition of the 
> abstracted evaluator, but the dwarf2 implementation doesn't work without 
> it).
> 
> This patch has been tested on both powerpc-linux and 686-linux.
> "

FYI,

I've picked up this patch (well, an updated version Daniel sent me) and
I intend to get it working and resubmitted.  I've got it mostly passing
the testsuite now (there were a couple of problems, with frames and
such).  The tracepoint test will fail until that bit is implemented,
which I don't see as a blocker; I'll try to do that bit as a separate
patch afterwards.

I'm still working on cleaning up some bogosities, etc. etc., so it will
be another day or two.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer