Re: set_value_component_location in apply_val_pretty_printer

[Yao Qi <qiyaoltc at gmail dot com>]

On Fri, Oct 28, 2016 at 08:58:33PM +0200, Ulrich Weigand wrote:
> Yao Qi wrote:
> 
> > I don't understand this piece of code in apply_val_pretty_printer,
> > why do we need to call set_value_component_location?
> > 
> >   set_value_component_location (value, val);
> >   /* set_value_component_location resets the address, so we may
> >      need to set it again.  */
> >   if (VALUE_LVAL (value) !=3D lval_internalvar
> >       && VALUE_LVAL (value) !=3D lval_internalvar_component
> >       && VALUE_LVAL (value) !=3D lval_computed)
> >     set_value_address (value, address + embedded_offset);
> > 
> > It was added by Tom in
> > https://sourceware.org/ml/gdb-patches/2010-06/msg00132.html
> > There wasn't much information in email and ChangeLog.
> 

> 
> It's reasonably simple to just create a new object of
> the correct type and having the correct contents.  However,
> some of the value printers also want to use the value's
> *location*.  Just allocating a fresh object would have
> no location information.   That's why the code above
> calls set_value_component_location to set the location
> of the new value to the same as the location of the old.
> 
> But this isn't really correct either, since we need the
> location of the *subobject*.  Now if the value resides
> in inferior memory, we can get there simply by adding
> the offset to the value address.  But that's not actually
> correct for values with other location types.

I don't see why it is not correct to set the value's location to the same as
the location of old.  The 'whole' object and 'component' object should have
the same location with different offsets (internalvar is an exception since 
the component's location is interanlvar_component), so
set_value_component_location  looks right to me.  However,
gdb{py,scm}_apply_val_pretty_printer are wrong to me that they use
value_from_contents_and_address and set_value_address, like this,

  value = value_from_contents_and_address (type, valaddr + embedded_offset,
					  address + embedded_offset);

  set_value_component_location (value, val);
  /* set_value_component_location resets the address, so we may
     need to set it again.  */
  if (VALUE_LVAL (value) != lval_internalvar
      && VALUE_LVAL (value) != lval_internalvar_component
      && VALUE_LVAL (value) != lval_computed)
    set_value_address (value, address + embedded_offset);

because the 'whole' object may not be from memory, as you pointed out.  I 
give a patch to this.

> 
> I think we should either add an offset parameter to
> set_value_component_location, and have it attempt to
> do the best thing possible to describe the subobject
> location, or maybe even provide a function that creates
> the subobject directly in one go, along the lines of
> value_primitive_field, except not based on struct
> types but simply using an offset and subobject type.
> 

value_primitive_field gives me some hints, and I use some in the patch.
Regression tested on x86_64-linux.  This patch fixes some asserts if I
restrict value_has_address that only returns true for lval_memory and
lval_register.  value_has_address patch is here,
https://sourceware.org/ml/gdb-patches/2016-10/msg00741.html

-- 
Yao (齐尧)

>From d02fbc6bf894588300bb0ff88b5dc059415f1e03 Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Fri, 11 Nov 2016 10:51:23 +0000
Subject: [PATCH] Create subobject value in pretty printer

Nowadays, we create a value of subobject in pretty printer with 'address'
being used,

  value = value_from_contents_and_address (type, valaddr + embedded_offset,
					   address + embedded_offset);

  set_value_component_location (value, val);
  /* set_value_component_location resets the address, so we may
     need to set it again.  */
  if (VALUE_LVAL (value) != lval_internalvar
      && VALUE_LVAL (value) != lval_internalvar_component
      && VALUE_LVAL (value) != lval_computed)
    set_value_address (value, address + embedded_offset);

value_from_contents_and_address creates a value from memory, but the
value we are pretty-printing may not from memory at all.

Instead of using value_from_contents_and_address, we create a value
of subobject with the same location as object's but different offset.
We avoid using address in this way.  As a result, parameter 'address'
in apply_val_pretty_printer is no longer needed, we can remove it in
next step.

We've already had the location of the 'whole' value, so it is safe
to assume we can create a value of 'component' or 'suboject' value
at the same location but with different offset.

gdb:

2016-11-11  Yao Qi  <yao.qi@linaro.org>

	* guile/scm-pretty-print.c (gdbscm_apply_val_pretty_printer):
	Don't call value_from_contents_and_address and
	set_value_address.  Call value_from_component.
	* python/py-prettyprint.c (gdbpy_apply_val_pretty_printer):
	Likewise.
	* value.c (value_from_component): New function.
	* value.h (value_from_component): Likewise.

diff --git a/gdb/guile/scm-pretty-print.c b/gdb/guile/scm-pretty-print.c
index 5253def..648ca53 100644
--- a/gdb/guile/scm-pretty-print.c
+++ b/gdb/guile/scm-pretty-print.c
@@ -985,16 +985,7 @@ gdbscm_apply_val_pretty_printer (const struct extension_language_defn *extlang,
   cleanups = make_cleanup (null_cleanup, NULL);
 
   /* Instantiate the printer.  */
-  value = value_from_contents_and_address (type, valaddr + embedded_offset,
-					   address + embedded_offset);
-
-  set_value_component_location (value, val);
-  /* set_value_component_location resets the address, so we may
-     need to set it again.  */
-  if (VALUE_LVAL (value) != lval_internalvar
-      && VALUE_LVAL (value) != lval_internalvar_component
-      && VALUE_LVAL (value) != lval_computed)
-    set_value_address (value, address + embedded_offset);
+  value = value_from_component (val, type, embedded_offset);
 
   val_obj = vlscm_scm_from_value (value);
   if (gdbscm_is_exception (val_obj))
diff --git a/gdb/python/py-prettyprint.c b/gdb/python/py-prettyprint.c
index cbc168d..4f5e7f7 100644
--- a/gdb/python/py-prettyprint.c
+++ b/gdb/python/py-prettyprint.c
@@ -726,16 +726,7 @@ gdbpy_apply_val_pretty_printer (const struct extension_language_defn *extlang,
   cleanups = ensure_python_env (gdbarch, language);
 
   /* Instantiate the printer.  */
-  value = value_from_contents_and_address (type, valaddr + embedded_offset,
-					   address + embedded_offset);
-
-  set_value_component_location (value, val);
-  /* set_value_component_location resets the address, so we may
-     need to set it again.  */
-  if (VALUE_LVAL (value) != lval_internalvar
-      && VALUE_LVAL (value) != lval_internalvar_component
-      && VALUE_LVAL (value) != lval_computed)
-    set_value_address (value, address + embedded_offset);
+  value = value_from_component (val, type, embedded_offset);
 
   val_obj = value_to_value_object (value);
   if (! val_obj)
diff --git a/gdb/value.c b/gdb/value.c
index 9def1b3..1bb61f1 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -3804,6 +3804,31 @@ value_from_history_ref (const char *h, const char **endp)
   return access_value_history (index);
 }
 
+/* Get the component value (offset by OFFSET bytes) of a struct or
+   union WHOLE.  Component's type is TYPE.  */
+
+struct value *
+value_from_component (struct value *whole, struct type *type, LONGEST offset)
+{
+  struct value *v;
+
+  if (value_lazy (whole))
+    v = allocate_value_lazy (type);
+  else
+    {
+      v = allocate_value (type);
+      value_contents_copy_raw (v, value_embedded_offset (v),
+			       whole, value_embedded_offset (whole) + offset,
+			       type_length_units (type));
+    }
+  v->offset = value_offset (whole) + offset + value_embedded_offset (whole);
+  set_value_component_location (v, whole);
+  VALUE_REGNUM (v) = VALUE_REGNUM (whole);
+  VALUE_FRAME_ID (v) = VALUE_FRAME_ID (whole);
+
+  return v;
+}
+
 struct value *
 coerce_ref_if_computed (const struct value *arg)
 {
diff --git a/gdb/value.h b/gdb/value.h
index f962508..c36eb6c 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -637,6 +637,8 @@ extern struct value *value_from_double (struct type *type, DOUBLEST num);
 extern struct value *value_from_decfloat (struct type *type,
 					  const gdb_byte *decbytes);
 extern struct value *value_from_history_ref (const char *, const char **);
+extern struct value *value_from_component (struct value *, struct type *,
+					   LONGEST);
 
 extern struct value *value_at (struct type *type, CORE_ADDR addr);
 extern struct value *value_at_lazy (struct type *type, CORE_ADDR addr);