Change meaning of VALUE_FRAME_ID; rename to VALUE_NEXT_FRAME_ID
authorKevin Buettner <kevinb@redhat.com>
Wed, 28 Sep 2016 04:18:44 +0000 (21:18 -0700)
committerKevin Buettner <kevinb@redhat.com>
Wed, 16 Nov 2016 18:38:19 +0000 (11:38 -0700)
The VALUE_FRAME_ID macro provides access to a member in struct value
that's used to hold the frame id that's used when determining a
register's value or when assigning to a register.  The underlying
member has a long and obscure name.  I won't refer to it here, but
will simply refer to VALUE_FRAME_ID as if it's the struct value member
instead of being a convenient macro.

At the moment, without this patch in place, VALUE_FRAME_ID is set in
value_of_register_lazy() and several other locations to hold the frame
id of the frame passed to those functions.

VALUE_FRAME_ID is used in the lval_register case of
value_fetch_lazy().  To fetch the register's value, it calls
get_frame_register_value() which, in turn, calls
frame_unwind_register_value() with frame->next.

A python based unwinder may wish to determine the value of a register
or evaluate an expression containing a register.  When it does this,
value_fetch_lazy() will be called under some circumstances.  It will
attempt to determine the frame id associated with the frame passed to
it.  In so doing, it will end up back in the frame sniffer of the very
same python unwinder that's attempting to learn the value of a
register as part of the sniffing operation.  This recursion is not
desirable.

As noted above, when value_fetch_lazy() wants to fetch a register's
value, it does so (indirectly) by unwinding from frame->next.

With this in mind, a solution suggests itself:  Change VALUE_FRAME_ID
to hold the frame id associated with the next frame.  Then, when it
comes time to obtain the value associated with the register, we can
simply unwind from the frame corresponding to the frame id stored in
VALUE_FRAME_ID.  This neatly avoids the python unwinder recursion
problem by changing when the "next" operation occurs.  Instead of the
"next" operation occuring when the register value is fetched, it
occurs earlier on when assigning a frame id to VALUE_FRAME_ID.
(Thanks to Pedro for this suggestion.)

This patch implements this idea.

It builds on the patch "Distinguish sentinel frame from null frame".
Without that work in place, it's necessary to check for null_id at
several places and then obtain the sentinel frame.

It also renames most occurences of VALUE_FRAME_ID to
VALUE_NEXT_FRAME_ID to reflect the new meaning of this field.

There are several uses of VALUE_FRAME_ID which were not changed.  In
each case, the original meaning of VALUE_FRAME_ID is required to get
correct results.  In all but one of these uses, either
put_frame_register_bytes() or get_frame_register_bytes() is being
called with the frame value obtained from VALUE_FRAME_ID.  Both of
these functions perform some unwinding by performing a "->next"
operation on the frame passed to it.  If we were to use the new
VALUE_NEXT_FRAME_ID macro, this would effectively do two "->next"
operations, which is not what we want.

The VALUE_FRAME_ID macro has been redefined in terms of
VALUE_NEXT_FRAME_ID.  It simply fetches the previous frame's id,
providing this id as the value of the macro.

gdb/ChangeLog:

* value.h (VALUE_FRAME_ID): Rename to VALUE_NEXT_FRAME_ID. Update
comment.  Create new VALUE_FRAME_ID which is defined in terms of
VALUE_NEXT_FRAME_ID.
(deprecated_value_frame_id_hack): Rename to
deprecated_value_next_frame_id_hack.
* dwarf2loc.c, findvar.c, frame-unwind.c, sentinel-frame.c,
valarith.c, valops.c, value.c: Adjust nearly all occurences of
VALUE_FRAME_ID to VALUE_NEXT_FRAME_ID. Add comments for those
which did not change.
* value.c (struct value): Rename frame_id field to next_frame_id.
Update comment.
(deprecated_value_frame_id_hack): Rename to
deprecated_value_next_frame_id_hack.
(value_fetch_lazy): Call frame_unwind_register_value()
instead of get_frame_register_value().
* frame.c (get_prev_frame_id_by_id): New function.
* frame.h (get_prev_frame_id_by_id): Declare.
* dwarf2loc.c (dwarf2_evaluate_loc_desc_full): Make
VALUE_NEXT_FRAME_ID refer to the next frame.
* findvar.c (value_of_register_lazy): Likewise.
(default_value_from_register): Likewise.
(value_from_register): Likewise.
* frame_unwind.c (frame_unwind_got_optimized): Likewise.
* sentinel-frame.c (sentinel_frame_prev_register): Likewise.
* value.h (VALUE_FRAME_ID): Update comment describing this macro.

gdb/ChangeLog
gdb/dwarf2loc.c
gdb/findvar.c
gdb/frame-unwind.c
gdb/frame.c
gdb/frame.h
gdb/sentinel-frame.c
gdb/valarith.c
gdb/valops.c
gdb/value.c
gdb/value.h

index 9740a23..caad792 100644 (file)
@@ -1,5 +1,33 @@
 2016-11-16  Kevin Buettner  <kevinb@redhat.com>
 
+       * value.h (VALUE_FRAME_ID): Rename to VALUE_NEXT_FRAME_ID. Update
+       comment.  Create new VALUE_FRAME_ID which is defined in terms of
+       VALUE_NEXT_FRAME_ID.
+       (deprecated_value_frame_id_hack): Rename to
+       deprecated_value_next_frame_id_hack.
+       * dwarf2loc.c, findvar.c, frame-unwind.c, sentinel-frame.c,
+       valarith.c, valops.c, value.c: Adjust nearly all occurences of
+       VALUE_FRAME_ID to VALUE_NEXT_FRAME_ID.  Add comments for those
+       which did not change.
+       * value.c (struct value): Rename frame_id field to next_frame_id.
+       Update comment.
+       (deprecated_value_frame_id_hack): Rename to
+       deprecated_value_next_frame_id_hack.
+       (value_fetch_lazy): Call frame_unwind_register_value()
+       instead of get_frame_register_value().
+       * frame.c (get_prev_frame_id_by_id): New function.
+       * frame.h (get_prev_frame_id_by_id): Declare.
+       * dwarf2loc.c (dwarf2_evaluate_loc_desc_full): Make
+       VALUE_NEXT_FRAME_ID refer to the next frame.
+       * findvar.c (value_of_register_lazy): Likewise.
+       (default_value_from_register): Likewise.
+       (value_from_register): Likewise.
+       * frame_unwind.c (frame_unwind_got_optimized): Likewise.
+       * sentinel-frame.c (sentinel_frame_prev_register): Likewise.
+       * value.h (VALUE_FRAME_ID): Update comment describing this macro.
+
+2016-11-16  Kevin Buettner  <kevinb@redhat.com>
+
        * frame.h (enum frame_id_stack_status): Add FID_STACK_SENTINEL.
        (struct frame_id): Increase number of bits required for storing
        stack status to 3 from 2.
index 93aec1f..44dceda 100644 (file)
@@ -1672,13 +1672,18 @@ read_pieced_value (struct value *v)
   gdb_byte *contents;
   struct piece_closure *c
     = (struct piece_closure *) value_computed_closure (v);
-  struct frame_info *frame = frame_find_by_id (VALUE_FRAME_ID (v));
+  struct frame_info *frame;
   size_t type_len;
   size_t buffer_size = 0;
   std::vector<gdb_byte> buffer;
   int bits_big_endian
     = gdbarch_bits_big_endian (get_type_arch (value_type (v)));
 
+  /* VALUE_FRAME_ID is used instead of VALUE_NEXT_FRAME_ID here
+     because FRAME is passed to get_frame_register_bytes(), which
+     does its own "->next" operation.  */
+  frame = frame_find_by_id (VALUE_FRAME_ID (v));
+
   if (value_type (v) != value_enclosing_type (v))
     internal_error (__FILE__, __LINE__,
                    _("Should not be able to create a lazy value with "
@@ -1841,13 +1846,18 @@ write_pieced_value (struct value *to, struct value *from)
   const gdb_byte *contents;
   struct piece_closure *c
     = (struct piece_closure *) value_computed_closure (to);
-  struct frame_info *frame = frame_find_by_id (VALUE_FRAME_ID (to));
+  struct frame_info *frame;
   size_t type_len;
   size_t buffer_size = 0;
   std::vector<gdb_byte> buffer;
   int bits_big_endian
     = gdbarch_bits_big_endian (get_type_arch (value_type (to)));
 
+  /* VALUE_FRAME_ID is used instead of VALUE_NEXT_FRAME_ID here
+     because FRAME is passed to get_frame_register_bytes() and
+     put_frame_register_bytes(), both of which do their own "->next"
+     operations.  */
+  frame = frame_find_by_id (VALUE_FRAME_ID (to));
   if (frame == NULL)
     {
       mark_value_bytes_optimized_out (to, 0, TYPE_LENGTH (value_type (to)));
@@ -2301,7 +2311,10 @@ dwarf2_evaluate_loc_desc_full (struct type *type, struct frame_info *frame,
   if (ctx.num_pieces > 0)
     {
       struct piece_closure *c;
-      struct frame_id frame_id = get_frame_id (frame);
+      struct frame_id frame_id
+        = frame == NULL 
+         ? null_frame_id
+         : get_frame_id (get_next_frame_sentinel_okay (frame));
       ULONGEST bit_size = 0;
       int i;
 
@@ -2316,7 +2329,7 @@ dwarf2_evaluate_loc_desc_full (struct type *type, struct frame_info *frame,
         closure but before allocating the result.  */
       do_cleanups (value_chain);
       retval = allocate_computed_value (type, &pieced_value_funcs, c);
-      VALUE_FRAME_ID (retval) = frame_id;
+      VALUE_NEXT_FRAME_ID (retval) = frame_id;
       set_value_offset (retval, byte_offset);
     }
   else
index 6e28a29..bab717a 100644 (file)
@@ -283,17 +283,23 @@ value_of_register_lazy (struct frame_info *frame, int regnum)
 {
   struct gdbarch *gdbarch = get_frame_arch (frame);
   struct value *reg_val;
+  struct frame_info *next_frame;
 
   gdb_assert (regnum < (gdbarch_num_regs (gdbarch)
                        + gdbarch_num_pseudo_regs (gdbarch)));
 
-  /* We should have a valid (i.e. non-sentinel) frame.  */
-  gdb_assert (frame_id_p (get_frame_id (frame)));
+  gdb_assert (frame != NULL);
+
+  next_frame = get_next_frame_sentinel_okay (frame);
+
+  /* We should have a valid next frame.  */
+  gdb_assert (frame_id_p (get_frame_id (next_frame)));
 
   reg_val = allocate_value_lazy (register_type (gdbarch, regnum));
   VALUE_LVAL (reg_val) = lval_register;
   VALUE_REGNUM (reg_val) = regnum;
-  VALUE_FRAME_ID (reg_val) = get_frame_id (frame);
+  VALUE_NEXT_FRAME_ID (reg_val) = get_frame_id (next_frame);
+
   return reg_val;
 }
 
@@ -815,9 +821,17 @@ default_value_from_register (struct gdbarch *gdbarch, struct type *type,
 {
   int len = TYPE_LENGTH (type);
   struct value *value = allocate_value (type);
+  struct frame_info *frame;
 
   VALUE_LVAL (value) = lval_register;
-  VALUE_FRAME_ID (value) = frame_id;
+  frame = frame_find_by_id (frame_id);
+
+  if (frame == NULL)
+    frame_id = null_frame_id;
+  else
+    frame_id = get_frame_id (get_next_frame_sentinel_okay (frame));
+
+  VALUE_NEXT_FRAME_ID (value) = frame_id;
   VALUE_REGNUM (value) = regnum;
 
   /* Any structure stored in more than one register will always be
@@ -902,7 +916,7 @@ value_from_register (struct type *type, int regnum, struct frame_info *frame)
          including the location.  */
       v = allocate_value (type);
       VALUE_LVAL (v) = lval_register;
-      VALUE_FRAME_ID (v) = get_frame_id (frame);
+      VALUE_NEXT_FRAME_ID (v) = get_frame_id (get_next_frame_sentinel_okay (frame));
       VALUE_REGNUM (v) = regnum;
       ok = gdbarch_register_to_value (gdbarch, frame, regnum, type1,
                                      value_contents_raw (v), &optim,
@@ -950,7 +964,7 @@ address_from_register (int regnum, struct frame_info *frame)
      where the ID of FRAME is not yet known.  Calling value_from_register
      would therefore abort in get_frame_id.  However, since we only need
      a temporary value that is never used as lvalue, we actually do not
-     really need to set its VALUE_FRAME_ID.  Therefore, we re-implement
+     really need to set its VALUE_NEXT_FRAME_ID.  Therefore, we re-implement
      the core of value_from_register, but use the null_frame_id.  */
 
   /* Some targets require a special conversion routine even for plain
index 187e6c2..974a236 100644 (file)
@@ -210,7 +210,8 @@ frame_unwind_got_optimized (struct frame_info *frame, int regnum)
   mark_value_bytes_optimized_out (val, 0, TYPE_LENGTH (type));
   VALUE_LVAL (val) = lval_register;
   VALUE_REGNUM (val) = regnum;
-  VALUE_FRAME_ID (val) = get_frame_id (frame);
+  VALUE_NEXT_FRAME_ID (val)
+    = get_frame_id (get_next_frame_sentinel_okay (frame));
   return val;
 }
 
index 54dc833..02635e6 100644 (file)
@@ -2305,6 +2305,22 @@ get_prev_frame (struct frame_info *this_frame)
   return get_prev_frame_always (this_frame);
 }
 
+struct frame_id
+get_prev_frame_id_by_id (struct frame_id id)
+{
+  struct frame_id prev_id;
+  struct frame_info *frame;
+  
+  frame = frame_find_by_id (id);
+
+  if (frame != NULL)
+    prev_id = get_frame_id (get_prev_frame (frame));
+  else
+    prev_id = null_frame_id;
+
+  return prev_id;
+}
+
 CORE_ADDR
 get_frame_pc (struct frame_info *frame)
 {
index dc25ce9..e5c3d10 100644 (file)
@@ -331,6 +331,10 @@ extern struct frame_info *get_prev_frame_always (struct frame_info *);
    is not found.  */
 extern struct frame_info *frame_find_by_id (struct frame_id id);
 
+/* Given a frame's ID, find the previous frame's ID.  Returns null_frame_id
+   if the frame is not found.  */
+extern struct frame_id get_prev_frame_id_by_id (struct frame_id id);
+
 /* Base attributes of a frame: */
 
 /* The frame's `resume' address.  Where the program will resume in
index 6cd1bc3..eb827eb 100644 (file)
@@ -51,7 +51,7 @@ sentinel_frame_prev_register (struct frame_info *this_frame,
   struct value *value;
 
   value = regcache_cooked_read_value (cache->regcache, regnum);
-  VALUE_FRAME_ID (value) = get_frame_id (this_frame);
+  VALUE_NEXT_FRAME_ID (value) = sentinel_frame_id;
 
   return value;
 }
index de6fcfd..2c56110 100644 (file)
@@ -227,7 +227,7 @@ value_subscripted_rvalue (struct value *array, LONGEST index, int lowerbound)
 
   set_value_component_location (v, array);
   VALUE_REGNUM (v) = VALUE_REGNUM (array);
-  VALUE_FRAME_ID (v) = VALUE_FRAME_ID (array);
+  VALUE_NEXT_FRAME_ID (v) = VALUE_NEXT_FRAME_ID (array);
   set_value_offset (v, value_offset (array) + elt_offs);
   return v;
 }
index 40392e8..8a45641 100644 (file)
@@ -1112,8 +1112,15 @@ value_assign (struct value *toval, struct value *fromval)
        struct gdbarch *gdbarch;
        int value_reg;
 
-       /* Figure out which frame this is in currently.  */
+       /* Figure out which frame this is in currently.
+       
+          We use VALUE_FRAME_ID for obtaining the value's frame id instead of
+          VALUE_NEXT_FRAME_ID due to requiring a frame which may be passed to
+          put_frame_register_bytes() below.  That function will (eventually)
+          perform the necessary unwind operation by first obtaining the next
+          frame.  */
        frame = frame_find_by_id (VALUE_FRAME_ID (toval));
+
        value_reg = VALUE_REGNUM (toval);
 
        if (!frame)
@@ -1333,7 +1340,7 @@ address_of_variable (struct symbol *var, const struct block *b)
        struct frame_info *frame;
        const char *regname;
 
-       frame = frame_find_by_id (VALUE_FRAME_ID (val));
+       frame = frame_find_by_id (VALUE_NEXT_FRAME_ID (val));
        gdb_assert (frame);
 
        regname = gdbarch_register_name (get_frame_arch (frame),
@@ -3820,7 +3827,7 @@ value_slice (struct value *array, int lowbound, int length)
       }
 
     set_value_component_location (slice, array);
-    VALUE_FRAME_ID (slice) = VALUE_FRAME_ID (array);
+    VALUE_NEXT_FRAME_ID (slice) = VALUE_NEXT_FRAME_ID (array);
     set_value_offset (slice, value_offset (array) + offset);
   }
 
index 62c5e37..0575a55 100644 (file)
@@ -262,9 +262,11 @@ struct value
      bitfields.  */
   struct value *parent;
 
-  /* Frame register value is relative to.  This will be described in
-     the lval enum above as "lval_register".  */
-  struct frame_id frame_id;
+  /* Frame ID of "next" frame to which a register value is relative.  A
+     register value is indicated when the lval enum (above) is set to
+     lval_register.  So, if the register value is found relative to frame F,
+     then the frame id of F->next will be stored in next_frame_id.  */
+  struct frame_id next_frame_id;
 
   /* Type of the value.  */
   struct type *type;
@@ -943,7 +945,7 @@ allocate_value_lazy (struct type *type)
   val->enclosing_type = type;
   VALUE_LVAL (val) = not_lval;
   val->location.address = 0;
-  VALUE_FRAME_ID (val) = null_frame_id;
+  VALUE_NEXT_FRAME_ID (val) = null_frame_id;
   val->offset = 0;
   val->bitpos = 0;
   val->bitsize = 0;
@@ -1582,9 +1584,9 @@ deprecated_value_internalvar_hack (struct value *value)
 }
 
 struct frame_id *
-deprecated_value_frame_id_hack (struct value *value)
+deprecated_value_next_frame_id_hack (struct value *value)
 {
-  return &value->frame_id;
+  return &value->next_frame_id;
 }
 
 short *
@@ -1786,7 +1788,7 @@ value_copy (struct value *arg)
   val->offset = arg->offset;
   val->bitpos = arg->bitpos;
   val->bitsize = arg->bitsize;
-  VALUE_FRAME_ID (val) = VALUE_FRAME_ID (arg);
+  VALUE_NEXT_FRAME_ID (val) = VALUE_NEXT_FRAME_ID (arg);
   VALUE_REGNUM (val) = VALUE_REGNUM (arg);
   val->lazy = arg->lazy;
   val->embedded_offset = value_embedded_offset (arg);
@@ -3228,7 +3230,7 @@ value_primitive_field (struct value *arg1, LONGEST offset,
     }
   set_value_component_location (v, arg1);
   VALUE_REGNUM (v) = VALUE_REGNUM (arg1);
-  VALUE_FRAME_ID (v) = VALUE_FRAME_ID (arg1);
+  VALUE_NEXT_FRAME_ID (v) = VALUE_NEXT_FRAME_ID (arg1);
   return v;
 }
 
@@ -3974,7 +3976,7 @@ value_fetch_lazy (struct value *val)
     }
   else if (VALUE_LVAL (val) == lval_register)
     {
-      struct frame_info *frame;
+      struct frame_info *next_frame;
       int regnum;
       struct type *type = check_typedef (value_type (val));
       struct value *new_val = val, *mark = value_mark ();
@@ -3985,27 +3987,33 @@ value_fetch_lazy (struct value *val)
 
       while (VALUE_LVAL (new_val) == lval_register && value_lazy (new_val))
        {
-         struct frame_id frame_id = VALUE_FRAME_ID (new_val);
+         struct frame_id next_frame_id = VALUE_NEXT_FRAME_ID (new_val);
 
-         frame = frame_find_by_id (frame_id);
+         next_frame = frame_find_by_id (next_frame_id);
          regnum = VALUE_REGNUM (new_val);
 
-         gdb_assert (frame != NULL);
+         gdb_assert (next_frame != NULL);
 
          /* Convertible register routines are used for multi-register
             values and for interpretation in different types
             (e.g. float or int from a double register).  Lazy
             register values should have the register's natural type,
             so they do not apply.  */
-         gdb_assert (!gdbarch_convert_register_p (get_frame_arch (frame),
+         gdb_assert (!gdbarch_convert_register_p (get_frame_arch (next_frame),
                                                   regnum, type));
 
-         new_val = get_frame_register_value (frame, regnum);
+         /* FRAME was obtained, above, via VALUE_NEXT_FRAME_ID. 
+            Since a "->next" operation was performed when setting
+            this field, we do not need to perform a "next" operation
+            again when unwinding the register.  That's why
+            frame_unwind_register_value() is called here instead of
+            get_frame_register_value().  */
+         new_val = frame_unwind_register_value (next_frame, regnum);
 
          /* If we get another lazy lval_register value, it means the
-            register is found by reading it from the next frame.
-            get_frame_register_value should never return a value with
-            the frame id pointing to FRAME.  If it does, it means we
+            register is found by reading it from NEXT_FRAME's next frame.
+            frame_unwind_register_value should never return a value with
+            the frame id pointing to NEXT_FRAME.  If it does, it means we
             either have two consecutive frames with the same frame id
             in the frame chain, or some code is trying to unwind
             behind get_prev_frame's back (e.g., a frame unwind
@@ -4014,7 +4022,7 @@ value_fetch_lazy (struct value *val)
             in this situation.  */
          if (VALUE_LVAL (new_val) == lval_register
              && value_lazy (new_val)
-             && frame_id_eq (VALUE_FRAME_ID (new_val), frame_id))
+             && frame_id_eq (VALUE_NEXT_FRAME_ID (new_val), next_frame_id))
            internal_error (__FILE__, __LINE__,
                            _("infinite loop while fetching a register"));
        }
@@ -4034,6 +4042,9 @@ value_fetch_lazy (struct value *val)
       if (frame_debug)
        {
          struct gdbarch *gdbarch;
+         struct frame_info *frame;
+         /* VALUE_FRAME_ID is used here, instead of VALUE_NEXT_FRAME_ID,
+            so that the frame level will be shown correctly.  */
          frame = frame_find_by_id (VALUE_FRAME_ID (val));
          regnum = VALUE_REGNUM (val);
          gdbarch = get_frame_arch (frame);
index f962508..afcbcae 100644 (file)
@@ -434,10 +434,18 @@ extern void set_value_address (struct value *, CORE_ADDR);
 extern struct internalvar **deprecated_value_internalvar_hack (struct value *);
 #define VALUE_INTERNALVAR(val) (*deprecated_value_internalvar_hack (val))
 
-/* Frame register value is relative to.  This will be described in the
-   lval enum above as "lval_register".  */
-extern struct frame_id *deprecated_value_frame_id_hack (struct value *);
-#define VALUE_FRAME_ID(val) (*deprecated_value_frame_id_hack (val))
+/* Frame ID of "next" frame to which a register value is relative.  A
+   register value is indicated by VALUE_LVAL being set to lval_register.
+   So, if the register value is found relative to frame F, then the
+   frame id of F->next will be stored in VALUE_NEXT_FRAME_ID.  */
+extern struct frame_id *deprecated_value_next_frame_id_hack (struct value *);
+#define VALUE_NEXT_FRAME_ID(val) (*deprecated_value_next_frame_id_hack (val))
+
+/* Frame ID of frame to which a register value is relative.  This is
+   similar to VALUE_NEXT_FRAME_ID, above, but may not be assigned to. 
+   Note that VALUE_FRAME_ID effectively undoes the "next" operation
+   that was performed during the assignment to VALUE_NEXT_FRAME_ID.  */
+#define VALUE_FRAME_ID(val) (get_prev_frame_id_by_id (VALUE_NEXT_FRAME_ID (val)))
 
 /* Register number if the value is from a register.  */
 extern short *deprecated_value_regnum_hack (struct value *);