store.exp failure on i686-linux with newer gcc's

store.exp failure on i686-linux with newer gcc's

[Doug Evans]

Hi.

Anyone working on this failure?

FAIL: gdb.base/store.exp: var longest l; print new l, expecting -1
[should read "expecting 4", fixed in separate patch]
FAIL: gdb.base/store.exp: var longest l; print incremented l, expecting 2
FAIL: gdb.base/store.exp: upvar longest l; print new l, expecting 4
FAIL: gdb.base/store.exp: var struct 4 u; print new u, expecting {s =
\{1, 2, 3, 4}}
FAIL: gdb.base/store.exp: up struct 4 u; print new u, expecting {s =
\{1, 2, 3, 4}}

Newer gcc's can store a 64 bit int in non-contiguous registers and use
DW_OP_piece to mark each piece.

value->lval is set to not_lval and value_assign doesn't like that so
gdb refuses to set the variable with "Left operand of assignment is
not an lvalue."

It seems like we need to not mark the value as not_lval and teach the
relevant pieces how to handle such values.
Anyone thought about how they want this done?

Re: store.exp failure on i686-linux with newer gcc's

[Joel Brobecker]

> value->lval is set to not_lval and value_assign doesn't like that so
> gdb refuses to set the variable with "Left operand of assignment is
> not an lvalue."

IIRC, this is a know limitation in GDB. Daniel wrote the piece that
allows us to read variables that are spread across several locations
like this.  But i don't think anyone has thought about being able
to write to such a variable.

-- 
Joel

Re: store.exp failure on i686-linux with newer gcc's

[Pedro Alves]

On Thursday 03 September 2009 22:50:13, Doug Evans wrote:

> Newer gcc's can store a 64 bit int in non-contiguous registers and use
> DW_OP_piece to mark each piece.
> 
> value->lval is set to not_lval and value_assign doesn't like that so
> gdb refuses to set the variable with "Left operand of assignment is
> not an lvalue."
> 
> It seems like we need to not mark the value as not_lval and teach the
> relevant pieces how to handle such values.
> Anyone thought about how they want this done?

Nathan Froyd has a patch for this, using the lval_computed
machinery.

-- 
Pedro Alves

Re: store.exp failure on i686-linux with newer gcc's

[Doug Evans]

On Thu, Sep 3, 2009 at 3:03 PM, Pedro Alves<pedro@codesourcery.com> wrote:
> On Thursday 03 September 2009 22:50:13, Doug Evans wrote:
>
>> Newer gcc's can store a 64 bit int in non-contiguous registers and use
>> DW_OP_piece to mark each piece.
>>
>> value->lval is set to not_lval and value_assign doesn't like that so
>> gdb refuses to set the variable with "Left operand of assignment is
>> not an lvalue."
>>
>> It seems like we need to not mark the value as not_lval and teach the
>> relevant pieces how to handle such values.
>> Anyone thought about how they want this done?
>
> Nathan Froyd has a patch for this, using the lval_computed
> machinery.

Good thing I checked first. :-)
Thanks.

What's the status? [if I may ask - no rush, just curious]

Re: store.exp failure on i686-linux with newer gcc's

[Nathan Froyd]

On Thu, Sep 03, 2009 at 03:17:32PM -0700, Doug Evans wrote:
> On Thu, Sep 3, 2009 at 3:03 PM, Pedro Alves<pedro@codesourcery.com> wrote:
> > On Thursday 03 September 2009 22:50:13, Doug Evans wrote:
> >> Newer gcc's can store a 64 bit int in non-contiguous registers and use
> >> DW_OP_piece to mark each piece.
> >>
> >> value->lval is set to not_lval and value_assign doesn't like that so
> >> gdb refuses to set the variable with "Left operand of assignment is
> >> not an lvalue."
> >
> > Nathan Froyd has a patch for this, using the lval_computed
> > machinery.
> 
> Good thing I checked first. :-)
> Thanks.
> 
> What's the status? [if I may ask - no rush, just curious]

Here's the patch we have in our internal tree, applied to gdb HEAD or
thereabouts.  (We first found the problem with soft-float doubles on
32-bit targets.)  It passes tests here; I'd be curious to hear if it
works for you as well.

If it does, maybe somebody will be so kind as to approve it in this
thread rather than waiting for a resubmit. :)

-Nathan

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 3064089..607c551 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,15 @@
+2009-09-03  Nathan Froyd  <froydnj@codesourcery.com>
+
+	* dwarf2loc.c (struct piece_closure): New.
+	(allocate_piece_closure): New function.
+	(read_pieced_value): New function.
+	(write_pieced_value): New function.
+	(copy_pieced_value_closure): New function.
+	(free_pieced_value_closure): New function.
+	(pieced_value_funcs): Define.
+	(dwarf2_evaluate_loc_desc): Return a computed value for a variable
+	described with pieces.
+
 2009-09-03  Pierre Muller <muller@ics.u-strasbg.fr>
 	    Richard Earnshaw <rearnsha@arm.com>
 
diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index dc150a7..c3f6d40 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -215,6 +215,125 @@ dwarf_expr_tls_address (void *baton, CORE_ADDR offset)
   return target_translate_tls_address (debaton->objfile, offset);
 }
 
+struct piece_closure
+{
+  /* The number of pieces used to describe this variable.  */
+  int n_pieces;
+
+  /* The pieces themselves.  */
+  struct dwarf_expr_piece *pieces;
+};
+
+/* Allocate a closure for a value formed from separately-described
+   PIECES.  */
+
+static struct piece_closure *
+allocate_piece_closure (int n_pieces, struct dwarf_expr_piece *pieces)
+{
+  struct piece_closure *c = XZALLOC (struct piece_closure);
+
+  c->n_pieces = n_pieces;
+  c->pieces = XCALLOC (n_pieces, struct dwarf_expr_piece);
+
+  memcpy (c->pieces, pieces, n_pieces * sizeof (struct dwarf_expr_piece));
+
+  return c;
+}
+
+static void
+read_pieced_value (struct value *v)
+{
+  int i;
+  long offset = 0;
+  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));
+
+  contents = value_contents_raw (v);
+  for (i = 0; i < c->n_pieces; i++)
+    {
+      struct dwarf_expr_piece *p = &c->pieces[i];
+
+      if (frame == NULL)
+	{
+	  memset (contents + offset, 0, p->size);
+	  set_value_optimized_out (v, 1);
+	}
+      else if (p->in_reg)
+	{
+	  struct gdbarch *arch = get_frame_arch (frame);
+	  gdb_byte regval[MAX_REGISTER_SIZE];
+	  int gdb_regnum = gdbarch_dwarf2_reg_to_regnum (arch, p->value);
+
+	  get_frame_register (frame, gdb_regnum, regval);
+	  memcpy (contents + offset, regval, p->size);
+	}
+      else
+	{
+	  read_memory (p->value, contents + offset, p->size);
+	}
+      offset += p->size;
+    }
+}
+
+static void
+write_pieced_value (struct value *to, struct value *from)
+{
+  int i;
+  long offset = 0;
+  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));
+
+  if (frame == NULL)
+    {
+      set_value_optimized_out (to, 1);
+      return;
+    }
+
+  contents = value_contents_raw (from);
+  for (i = 0; i < c->n_pieces; i++)
+    {
+      struct dwarf_expr_piece *p = &c->pieces[i];
+      if (p->in_reg)
+	{
+	  struct gdbarch *arch = get_frame_arch (frame);
+	  int gdb_regnum = gdbarch_dwarf2_reg_to_regnum (arch, p->value);
+	  put_frame_register (frame, gdb_regnum, contents + offset);
+	}
+      else
+	{
+	  write_memory (p->value, contents + offset, p->size);
+	}
+      offset += p->size;
+    }
+}
+
+static void *
+copy_pieced_value_closure (struct value *v)
+{
+  struct piece_closure *c = (struct piece_closure *) value_computed_closure (v);
+  
+  return allocate_piece_closure (c->n_pieces, c->pieces);
+}
+
+static void
+free_pieced_value_closure (struct value *v)
+{
+  struct piece_closure *c = (struct piece_closure *) value_computed_closure (v);
+
+  xfree (c->pieces);
+  xfree (c);
+}
+
+/* Functions for accessing a variable described by DW_OP_piece.  */
+static struct lval_funcs pieced_value_funcs = {
+  read_pieced_value,
+  write_pieced_value,
+  copy_pieced_value_closure,
+  free_pieced_value_closure
+};
+
 /* Evaluate a location description, starting at DATA and with length
    SIZE, to find the current location of variable VAR in the context
    of FRAME.  */
@@ -254,29 +373,14 @@ dwarf2_evaluate_loc_desc (struct symbol *var, struct frame_info *frame,
   dwarf_expr_eval (ctx, data, size);
   if (ctx->num_pieces > 0)
     {
-      int i;
-      long offset = 0;
-      bfd_byte *contents;
-
-      retval = allocate_value (SYMBOL_TYPE (var));
-      contents = value_contents_raw (retval);
-      for (i = 0; i < ctx->num_pieces; i++)
-	{
-	  struct dwarf_expr_piece *p = &ctx->pieces[i];
-	  if (p->in_reg)
-	    {
-	      struct gdbarch *arch = get_frame_arch (frame);
-	      bfd_byte regval[MAX_REGISTER_SIZE];
-	      int gdb_regnum = gdbarch_dwarf2_reg_to_regnum (arch, p->value);
-	      get_frame_register (frame, gdb_regnum, regval);
-	      memcpy (contents + offset, regval, p->size);
-	    }
-	  else /* In memory?  */
-	    {
-	      read_memory (p->value, contents + offset, p->size);
-	    }
-	  offset += p->size;
-	}
+      struct piece_closure *c;
+      struct frame_id frame_id = get_frame_id (frame);
+
+      c = allocate_piece_closure (ctx->num_pieces, ctx->pieces);
+      retval = allocate_computed_value (SYMBOL_TYPE (var),
+					&pieced_value_funcs,
+					c);
+      VALUE_FRAME_ID (retval) = frame_id;
     }
   else if (ctx->in_reg)
     {

Re: store.exp failure on i686-linux with newer gcc's

[Tom Tromey]

>>>>> "Nathan" == Nathan Froyd <froydnj@codesourcery.com> writes:

Nathan> If it does, maybe somebody will be so kind as to approve it in this
Nathan> thread rather than waiting for a resubmit. :)

Nice patch.

This conflicts with the DW_OP_*_value patch, but I looked and for the
most part it does not seem to be too hard to reconcile them.

Nathan> +static void
Nathan> +write_pieced_value (struct value *to, struct value *from)
Nathan> +{
[...]
Nathan> +  struct frame_info *frame = frame_find_by_id (VALUE_FRAME_ID (to));
Nathan> +
Nathan> +  if (frame == NULL)
Nathan> +    {
Nathan> +      set_value_optimized_out (to, 1);

What impact does this have on the error messages the user sees?
I didn't try to trace through the code; will setting a variable in this
scenario give an error?  (Or silently do nothing?)

Second, this seems like a change in behavior for the value history.
Suppose the user does "print local", which uses this machinery.
Then the inferior exits.  Then the user does "print $1" or whatever...
this ought to print the same value, but now I think the user will get an
error.  So, perhaps it is better to continue to copy in the bits
eagerly instead of postponing them to read_pieced_value.

Nathan> +  struct piece_closure *c = (struct piece_closure *) value_computed_closure (v);

I wish the closure were just passed as an argument to all these
callbacks, but that is just a drive-by complaint, not your problem.

Tom

Re: store.exp failure on i686-linux with newer gcc's

[Nathan Froyd]

On Fri, Sep 04, 2009 at 10:04:39AM -0600, Tom Tromey wrote:
> Nice patch.

Thanks.

> Nathan> +static void
> Nathan> +write_pieced_value (struct value *to, struct value *from)
> Nathan> +{
> [...]
> Nathan> +  struct frame_info *frame = frame_find_by_id (VALUE_FRAME_ID (to));
> Nathan> +
> Nathan> +  if (frame == NULL)
> Nathan> +    {
> Nathan> +      set_value_optimized_out (to, 1);
> 
> What impact does this have on the error messages the user sees?
> I didn't try to trace through the code; will setting a variable in this
> scenario give an error?  (Or silently do nothing?)
> 
> Second, this seems like a change in behavior for the value history.
> Suppose the user does "print local", which uses this machinery.
> Then the inferior exits.  Then the user does "print $1" or whatever...
> this ought to print the same value, but now I think the user will get an
> error.

I'm not really sure if that case is even reachable (likewise for the
read case); I'd be happy to consider scenarios where it might be.  If
you do something like:

(gdb) break foo
(gdb) cont
(gdb) p local
$3 = ...
(gdb) finish
(gdb) p $3

that works just fine.  Similarly if you let the inferior finish.
Writing to $3 is disallowed ("Left operand of assignment is not a
modifiable lvalue.")  I think everything works as you would expect.

>   So, perhaps it is better to continue to copy in the bits eagerly
> instead of postponing them to read_pieced_value.

I'm not exactly sure what you mean here.  At least for values where some
of the pieces live in registers, if you don't have the frame for the
value, you can't write to the register, correct?

-Nathan

Re: store.exp failure on i686-linux with newer gcc's

[Tom Tromey]

>>>>> "Nathan" == Nathan Froyd <froydnj@codesourcery.com> writes:

>> So, perhaps it is better to continue to copy in the bits eagerly
>> instead of postponing them to read_pieced_value.

Nathan> I'm not exactly sure what you mean here.  At least for values where some
Nathan> of the pieces live in registers, if you don't have the frame for the
Nathan> value, you can't write to the register, correct?

Oh, never mind, I was confused.

I see now that read_pieced_value will only be called to fill in a lazy
value.  So, you are correct, this can't happen for values on the
history.

This patch is ok.

Tom

Re: store.exp failure on i686-linux with newer gcc's

[Nathan Froyd]

On Fri, Sep 04, 2009 at 12:38:23PM -0600, Tom Tromey wrote:
> I see now that read_pieced_value will only be called to fill in a lazy
> value.  So, you are correct, this can't happen for values on the
> history.
> 
> This patch is ok.

Thanks, committed.

-Nathan

Re: store.exp failure on i686-linux with newer gcc's

[Doug Evans]

On Fri, Sep 4, 2009 at 1:17 PM, Nathan Froyd<froydnj@codesourcery.com> wrote:
> On Fri, Sep 04, 2009 at 12:38:23PM -0600, Tom Tromey wrote:
>> I see now that read_pieced_value will only be called to fill in a lazy
>> value.  So, you are correct, this can't happen for values on the
>> history.
>>
>> This patch is ok.
>
> Thanks, committed.

Thanks!

Re: store.exp failure on i686-linux with newer gcc's

[Tom Tromey]

>>>>> "Nathan" == Nathan Froyd <froydnj@codesourcery.com> writes:

I've been thinking about this patch more and I have one more question.

Nathan> +	  struct gdbarch *arch = get_frame_arch (frame);

Nathan> +      struct frame_id frame_id = get_frame_id (frame);
Nathan> +
Nathan> +      c = allocate_piece_closure (ctx->num_pieces, ctx->pieces);
Nathan> +      retval = allocate_computed_value (SYMBOL_TYPE (var),
Nathan> +					&pieced_value_funcs,
Nathan> +					c);
Nathan> +      VALUE_FRAME_ID (retval) = frame_id;


My understanding is that we can evaluate dwarf expressions that do not
need a frame.  Will this code do the right thing in that situation?

I am not sure how to construct a situation like that.  Maybe someone
else knows.

Tom

Re: store.exp failure on i686-linux with newer gcc's

[Daniel Jacobowitz]

On Fri, Sep 04, 2009 at 03:23:14PM -0600, Tom Tromey wrote:
> >>>>> "Nathan" == Nathan Froyd <froydnj@codesourcery.com> writes:
> 
> I've been thinking about this patch more and I have one more question.
> 
> Nathan> +	  struct gdbarch *arch = get_frame_arch (frame);
> 
> Nathan> +      struct frame_id frame_id = get_frame_id (frame);
> Nathan> +
> Nathan> +      c = allocate_piece_closure (ctx->num_pieces, ctx->pieces);
> Nathan> +      retval = allocate_computed_value (SYMBOL_TYPE (var),
> Nathan> +					&pieced_value_funcs,
> Nathan> +					c);
> Nathan> +      VALUE_FRAME_ID (retval) = frame_id;
> 
> 
> My understanding is that we can evaluate dwarf expressions that do not
> need a frame.  Will this code do the right thing in that situation?

I *think* so - the frame ID would only be consulted if there were
registers.  I assume we can't get here when the inferior is not
running, since we're passed a frame.  Or is that overly optimistic?

> I am not sure how to construct a situation like that.  Maybe someone
> else knows.

I don't know how to make GCC do it, but SRA or struct-reorg on a
global variable could do that.

-- 
Daniel Jacobowitz
CodeSourcery

Re: store.exp failure on i686-linux with newer gcc's

[Tom Tromey]

>>>>> "Daniel" == Daniel Jacobowitz <drow@false.org> writes:

Daniel> I *think* so - the frame ID would only be consulted if there were
Daniel> registers.  I assume we can't get here when the inferior is not
Daniel> running, since we're passed a frame.  Or is that overly optimistic?

No, I think you are correct.
Thanks for the reassurance.

Tom