[rfc] Remove deprecated_safe_get_selected_frame call from read_var_value

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

Hello,

it turns out the patch in:
http://sourceware.org/ml/gdb-patches/2008-08/msg00565.html

doesn't really fix the problem, because read_var_value will
use the default selected frame when called with a NULL frame,
so it might generate lval_register values even in that case.

However, as noted in a comment, this behaviour of read_var_value
is questionable in any case:
   /* FIXME drow/2003-09-06: this call to the selected frame should be
      pushed upwards to the callers.  */

I've reviewed all callers of read_var_value, and most either already
pass in a non-NULL frame value, or else deliberately pass in NULL
because they query a global/static variable.

The only exceptions are value_of_variable, where the get_selected_frame
call can be easily pushed to, and locate_var_value, which sometimes does
get called with a NULL frame, even for frame-local variables.  Note that
code like:

       else if (symbol_read_needs_frame (var))
 	return
 	  locate_var_value
 	  (var,
 	   block_innermost_frame (exp->elts[pc + 1].block));
       else
 	return locate_var_value (var, NULL);

in eval.c:evaluate_subexp_for_address doesn't have quite the effect one
might think it has, because the parsers may set exp->elts[..].block
to NULL to indicate the "current block"; in this case the
block_innermost_frame call will return a NULL frame as well.

This can cause assertion failures when attempting to print the address
of a variable that is held in a register.


On the other hand, locate_var_value is only called from eval.c routines,
where we'd really prefer an interface like value_of_variable instead of
an interface like read_var_value.  The following patch therefore removes
locate_var_value in favor of a new address_of_variable routine -- which
because of lazy evaluation now simply calls value_of_variable.

This fixes the problems mentioned above.

Tested with no regression on powerpc-linux.

Comments welcome -- does this look right the right thing to do?

Bye,
Ulrich


ChangeLog:

	* value.h (address_of_variable): Add prototype.
	(locate_var_value): Remove prototype.

	* findvar.c (read_var_value): Do not attempt to default frame
	to selected frame.
	(locate_var_value): Remove function.
	* valops.c (value_of_variable): Retrieve selected frame for
	symbols that require a frame when called with NULL block.
	* valops.c (address_of_variable): New function.

	* eval.c (evaluate_subexp_for_address): Call address_of_variable
	instead of calling locate_var_value.
	(evaluate_subexp_with_coercion): Likewise.


Index: gdb/eval.c
===================================================================
RCS file: /cvs/src/src/gdb/eval.c,v
retrieving revision 1.104
diff -c -p -r1.104 eval.c
*** gdb/eval.c	3 Jan 2009 05:57:51 -0000	1.104
--- gdb/eval.c	13 Jan 2009 18:27:59 -0000
*************** evaluate_subexp_for_address (struct expr
*** 2560,2572 ****
  	  return
  	    value_zero (type, not_lval);
  	}
-       else if (symbol_read_needs_frame (var))
- 	return
- 	  locate_var_value
- 	  (var,
- 	   block_innermost_frame (exp->elts[pc + 1].block));
        else
! 	return locate_var_value (var, NULL);
  
      case OP_SCOPE:
        tem = longest_to_int (exp->elts[pc + 2].longconst);
--- 2560,2567 ----
  	  return
  	    value_zero (type, not_lval);
  	}
        else
! 	return address_of_variable (var, exp->elts[pc + 1].block);
  
      case OP_SCOPE:
        tem = longest_to_int (exp->elts[pc + 2].longconst);
*************** evaluate_subexp_with_coercion (struct ex
*** 2620,2625 ****
--- 2615,2621 ----
    int pc;
    struct value *val;
    struct symbol *var;
+   struct type *type;
  
    pc = (*pos);
    op = exp->elts[pc].opcode;
*************** evaluate_subexp_with_coercion (struct ex
*** 2628,2641 ****
      {
      case OP_VAR_VALUE:
        var = exp->elts[pc + 2].symbol;
!       if (TYPE_CODE (check_typedef (SYMBOL_TYPE (var))) == TYPE_CODE_ARRAY
  	  && CAST_IS_CONVERSION)
  	{
  	  (*pos) += 4;
! 	  val =
! 	    locate_var_value
! 	    (var, block_innermost_frame (exp->elts[pc + 1].block));
! 	  return value_cast (lookup_pointer_type (TYPE_TARGET_TYPE (check_typedef (SYMBOL_TYPE (var)))),
  			     val);
  	}
        /* FALLTHROUGH */
--- 2624,2636 ----
      {
      case OP_VAR_VALUE:
        var = exp->elts[pc + 2].symbol;
!       type = check_typedef (SYMBOL_TYPE (var));
!       if (TYPE_CODE (type) == TYPE_CODE_ARRAY
  	  && CAST_IS_CONVERSION)
  	{
  	  (*pos) += 4;
! 	  val = address_of_variable (var, exp->elts[pc + 1].block);
! 	  return value_cast (lookup_pointer_type (TYPE_TARGET_TYPE (type)),
  			     val);
  	}
        /* FALLTHROUGH */
Index: gdb/findvar.c
===================================================================
RCS file: /cvs/src/src/gdb/findvar.c,v
retrieving revision 1.120
diff -c -p -r1.120 findvar.c
*** gdb/findvar.c	3 Jan 2009 05:57:51 -0000	1.120
--- gdb/findvar.c	13 Jan 2009 18:27:59 -0000
*************** symbol_read_needs_frame (struct symbol *
*** 382,389 ****
  /* Given a struct symbol for a variable,
     and a stack frame id, read the value of the variable
     and return a (pointer to a) struct value containing the value. 
!    If the variable cannot be found, return a zero pointer.
!    If FRAME is NULL, use the selected frame.  */
  
  struct value *
  read_var_value (struct symbol *var, struct frame_info *frame)
--- 382,388 ----
  /* Given a struct symbol for a variable,
     and a stack frame id, read the value of the variable
     and return a (pointer to a) struct value containing the value. 
!    If the variable cannot be found, return a zero pointer.  */
  
  struct value *
  read_var_value (struct symbol *var, struct frame_info *frame)
*************** read_var_value (struct symbol *var, stru
*** 405,414 ****
  
    len = TYPE_LENGTH (type);
  
!   /* FIXME drow/2003-09-06: this call to the selected frame should be
!      pushed upwards to the callers.  */
!   if (frame == NULL)
!     frame = deprecated_safe_get_selected_frame ();
  
    switch (SYMBOL_CLASS (var))
      {
--- 404,411 ----
  
    len = TYPE_LENGTH (type);
  
!   if (symbol_read_needs_frame (var))
!     gdb_assert (frame);
  
    switch (SYMBOL_CLASS (var))
      {
*************** address_from_register (struct type *type
*** 657,714 ****
  
    return result;
  }
- 
- 
- /* Given a struct symbol for a variable or function,
-    and a stack frame id, 
-    return a (pointer to a) struct value containing the properly typed
-    address.  */
- 
- struct value *
- locate_var_value (struct symbol *var, struct frame_info *frame)
- {
-   struct gdbarch *gdbarch;
-   CORE_ADDR addr = 0;
-   struct type *type = SYMBOL_TYPE (var);
-   struct value *lazy_value;
- 
-   /* Evaluate it first; if the result is a memory address, we're fine.
-      Lazy evaluation pays off here. */
- 
-   lazy_value = read_var_value (var, frame);
-   if (lazy_value == 0)
-     error (_("Address of \"%s\" is unknown."), SYMBOL_PRINT_NAME (var));
- 
-   if ((VALUE_LVAL (lazy_value) == lval_memory && value_lazy (lazy_value))
-       || TYPE_CODE (type) == TYPE_CODE_FUNC)
-     {
-       struct value *val;
- 
-       addr = VALUE_ADDRESS (lazy_value);
-       val = value_from_pointer (lookup_pointer_type (type), addr);
-       return val;
-     }
- 
-   /* Not a memory address; check what the problem was.  */
-   switch (VALUE_LVAL (lazy_value))
-     {
-     case lval_register:
-       gdb_assert (frame);
-       gdbarch = get_frame_arch (frame);
-       gdb_assert (gdbarch_register_name
- 		   (gdbarch, VALUE_REGNUM (lazy_value)) != NULL
- 		  && *gdbarch_register_name
- 		    (gdbarch, VALUE_REGNUM (lazy_value)) != '\0');
-       error (_("Address requested for identifier "
- 	       "\"%s\" which is in register $%s"),
-             SYMBOL_PRINT_NAME (var), 
- 	    gdbarch_register_name (gdbarch, VALUE_REGNUM (lazy_value)));
-       break;
- 
-     default:
-       error (_("Can't take address of \"%s\" which isn't an lvalue."),
- 	     SYMBOL_PRINT_NAME (var));
-       break;
-     }
-   return 0;			/* For lint -- never reached */
- }
--- 654,656 ----
Index: gdb/valops.c
===================================================================
RCS file: /cvs/src/src/gdb/valops.c,v
retrieving revision 1.206
diff -c -p -r1.206 valops.c
*** gdb/valops.c	13 Jan 2009 10:34:31 -0000	1.206
--- gdb/valops.c	13 Jan 2009 18:28:00 -0000
*************** struct value *
*** 988,998 ****
  value_of_variable (struct symbol *var, struct block *b)
  {
    struct value *val;
!   struct frame_info *frame = NULL;
  
!   if (!b)
!     frame = NULL;		/* Use selected frame.  */
!   else if (symbol_read_needs_frame (var))
      {
        frame = block_innermost_frame (b);
        if (!frame)
--- 988,1000 ----
  value_of_variable (struct symbol *var, struct block *b)
  {
    struct value *val;
!   struct frame_info *frame;
  
!   if (!symbol_read_needs_frame (var))
!     frame = NULL;
!   else if (!b)
!     frame = get_selected_frame (_("No frame selected."));
!   else
      {
        frame = block_innermost_frame (b);
        if (!frame)
*************** value_of_variable (struct symbol *var, s
*** 1013,1018 ****
--- 1015,1068 ----
    return val;
  }
  
+ struct value *
+ address_of_variable (struct symbol *var, struct block *b)
+ {
+   struct type *type = SYMBOL_TYPE (var);
+   struct value *val;
+ 
+   /* Evaluate it first; if the result is a memory address, we're fine.
+      Lazy evaluation pays off here. */
+ 
+   val = value_of_variable (var, b);
+ 
+   if ((VALUE_LVAL (val) == lval_memory && value_lazy (val))
+       || TYPE_CODE (type) == TYPE_CODE_FUNC)
+     {
+       CORE_ADDR addr = VALUE_ADDRESS (val);
+       return value_from_pointer (lookup_pointer_type (type), addr);
+     }
+ 
+   /* Not a memory address; check what the problem was.  */
+   switch (VALUE_LVAL (val))
+     {
+     case lval_register:
+       {
+ 	struct frame_info *frame;
+ 	const char *regname;
+ 
+ 	frame = frame_find_by_id (VALUE_FRAME_ID (val));
+ 	gdb_assert (frame);
+ 
+ 	regname = gdbarch_register_name (get_frame_arch (frame),
+ 					 VALUE_REGNUM (val));
+ 	gdb_assert (regname && *regname);
+ 
+ 	error (_("Address requested for identifier "
+ 		 "\"%s\" which is in register $%s"),
+ 	       SYMBOL_PRINT_NAME (var), regname);
+ 	break;
+       }
+ 
+     default:
+       error (_("Can't take address of \"%s\" which isn't an lvalue."),
+ 	     SYMBOL_PRINT_NAME (var));
+       break;
+     }
+ 
+   return val;
+ }
+ 
  /* Return one if VAL does not live in target memory, but should in order
     to operate on it.  Otherwise return zero.  */
  
Index: gdb/value.h
===================================================================
RCS file: /cvs/src/src/gdb/value.h,v
retrieving revision 1.128
diff -c -p -r1.128 value.h
*** gdb/value.h	13 Jan 2009 10:34:31 -0000	1.128
--- gdb/value.h	13 Jan 2009 18:28:00 -0000
*************** extern CORE_ADDR address_from_register (
*** 310,315 ****
--- 310,317 ----
  
  extern struct value *value_of_variable (struct symbol *var, struct block *b);
  
+ extern struct value *address_of_variable (struct symbol *var, struct block *b);
+ 
  extern struct value *value_of_register (int regnum, struct frame_info *frame);
  
  struct value *value_of_register_lazy (struct frame_info *frame, int regnum);
*************** extern int symbol_read_needs_frame (stru
*** 319,327 ****
  extern struct value *read_var_value (struct symbol *var,
  				     struct frame_info *frame);
  
- extern struct value *locate_var_value (struct symbol *var,
- 				       struct frame_info *frame);
- 
  extern struct value *allocate_value (struct type *type);
  extern struct value *allocate_value_lazy (struct type *type);
  extern void allocate_value_contents (struct value *value);
--- 321,326 ----
-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com