FYI: fix big-endian bug with DWARF_VALUE_STACK

FYI: fix big-endian bug with DWARF_VALUE_STACK

[Tom Tromey]

I'm checking this in on the trunk and the 7.1 branch.

Jakub noticed a bug with DWARF expressions on big-endian machines.
This is https://bugzilla.redhat.com/show_bug.cgi?id=567251

Built and regtested on x86-64 (compile farm) -- little-endian, of
course, so I also ran the relevant tests on a PPC64 box in the farm.

I do have a test case (see the bugzilla link), but it only works on
PPC64, and it relies on having a very new version of gas, so I did not
include it in the patch.

Tom

2010-02-23  Tom Tromey  <tromey@redhat.com>

	* dwarf2loc.c (read_pieced_value) <DWARF_VALUE_STACK>: Correctly
	handle big-endian values.
	(dwarf2_evaluate_loc_desc) <DWARF_VALUE_STACK>: Likewise.

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 6679d74..1c4d057 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -298,16 +298,14 @@ read_pieced_value (struct value *v)
 
 	case DWARF_VALUE_STACK:
 	  {
-	    gdb_byte bytes[sizeof (ULONGEST)];
 	    size_t n;
 	    int addr_size = gdbarch_addr_bit (c->arch) / 8;
-	    store_unsigned_integer (bytes, addr_size,
-				    gdbarch_byte_order (c->arch),
-				    p->v.expr.value);
 	    n = p->size;
 	    if (n > addr_size)
 	      n = addr_size;
-	    memcpy (contents + offset, bytes, n);
+	    store_unsigned_integer (contents + offset, n,
+				    gdbarch_byte_order (c->arch),
+				    p->v.expr.value);
 	  }
 	  break;
 
@@ -476,19 +474,17 @@ dwarf2_evaluate_loc_desc (struct symbol *var, struct frame_info *frame,
 
 	case DWARF_VALUE_STACK:
 	  {
-	    gdb_byte bytes[sizeof (ULONGEST)];
 	    ULONGEST value = (ULONGEST) dwarf_expr_fetch (ctx, 0);
 	    bfd_byte *contents;
 	    size_t n = ctx->addr_size;
 
-	    store_unsigned_integer (bytes, ctx->addr_size,
-				    gdbarch_byte_order (ctx->gdbarch),
-				    value);
 	    retval = allocate_value (SYMBOL_TYPE (var));
 	    contents = value_contents_raw (retval);
 	    if (n > TYPE_LENGTH (SYMBOL_TYPE (var)))
 	      n = TYPE_LENGTH (SYMBOL_TYPE (var));
-	    memcpy (contents, bytes, n);
+	    store_unsigned_integer (contents, n,
+				    gdbarch_byte_order (ctx->gdbarch),
+				    value);
 	  }
 	  break;
 

Re: FYI: fix big-endian bug with DWARF_VALUE_STACK

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Tom> Jakub noticed a bug with DWARF expressions on big-endian machines.
Tom> This is https://bugzilla.redhat.com/show_bug.cgi?id=567251

It was pointed out to me that this is a private bug, so you won't be
able to read it.

I can supply the test case if anybody wants it.

Tom

Re: FYI: fix big-endian bug with DWARF_VALUE_STACK

[Ulrich Weigand]

Tom Tromey wrote:

> 2010-02-23  Tom Tromey  <tromey@redhat.com>
> 
> 	* dwarf2loc.c (read_pieced_value) <DWARF_VALUE_STACK>: Correctly
> 	handle big-endian values.
> 	(dwarf2_evaluate_loc_desc) <DWARF_VALUE_STACK>: Likewise.

I'd recently been wondering about this piece of code (and in particular
its use of gdbarch_addr_bit) as well ...


> @@ -298,16 +298,14 @@ read_pieced_value (struct value *v)
>  
>  	case DWARF_VALUE_STACK:
>  	  {
> -	    gdb_byte bytes[sizeof (ULONGEST)];
>  	    size_t n;
>  	    int addr_size = gdbarch_addr_bit (c->arch) / 8;
> -	    store_unsigned_integer (bytes, addr_size,
> -				    gdbarch_byte_order (c->arch),
> -				    p->v.expr.value);
>  	    n = p->size;
>  	    if (n > addr_size)
>  	      n = addr_size;
> -	    memcpy (contents + offset, bytes, n);
> +	    store_unsigned_integer (contents + offset, n,
> +				    gdbarch_byte_order (c->arch),
> +				    p->v.expr.value);
>  	  }
>  	  break;

The original code looks broken to me too.  But even the new code
doesn't seem quite right: if the requested piece size p->size
is larger than addr_size, the remaining bytes of the output value
are left undefined (well, probably zeroed -- but that still doesn't
look correct on a big-endian machine ...).

Why would that code look at gdbarch_addr_bit at all?  I think this
should simply do something like:

	    store_unsigned_integer (contents + offset, p->size,
				    gdbarch_byte_order (c->arch),
				    p->v.expr.value);

> @@ -476,19 +474,17 @@ dwarf2_evaluate_loc_desc (struct symbol *var, struct frame_info *frame,
>  
>  	case DWARF_VALUE_STACK:
>  	  {
> -	    gdb_byte bytes[sizeof (ULONGEST)];
>  	    ULONGEST value = (ULONGEST) dwarf_expr_fetch (ctx, 0);
>  	    bfd_byte *contents;
>  	    size_t n = ctx->addr_size;
>  
> -	    store_unsigned_integer (bytes, ctx->addr_size,
> -				    gdbarch_byte_order (ctx->gdbarch),
> -				    value);
>  	    retval = allocate_value (SYMBOL_TYPE (var));
>  	    contents = value_contents_raw (retval);
>  	    if (n > TYPE_LENGTH (SYMBOL_TYPE (var)))
>  	      n = TYPE_LENGTH (SYMBOL_TYPE (var));
> -	    memcpy (contents, bytes, n);
> +	    store_unsigned_integer (contents, n,
> +				    gdbarch_byte_order (ctx->gdbarch),
> +				    value);
>  	  }
>  	  break;

Similiary here; why does this not simply use value_from_longest?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: FYI: fix big-endian bug with DWARF_VALUE_STACK

[Tom Tromey]

>>>>> "Ulrich" == Ulrich Weigand <uweigand@de.ibm.com> writes:

Tom> +	    store_unsigned_integer (contents + offset, n,
Tom> +				    gdbarch_byte_order (c->arch),
Tom> +				    p->v.expr.value);

Ulrich> The original code looks broken to me too.  But even the new code
Ulrich> doesn't seem quite right: if the requested piece size p->size
Ulrich> is larger than addr_size, the remaining bytes of the output value
Ulrich> are left undefined (well, probably zeroed -- but that still doesn't
Ulrich> look correct on a big-endian machine ...).

I asked about a similar case on dwarf-discuss, namely what if you have:

  DW_OP_implicit_value[len=5] DW_OP_piece[len=10]

The answer I got was something along the lines of "that is undefined,
don't do that".

The archives seem to be closed to the public, but if you are a
subscriber you can see:

    http://lists.dwarfstd.org/htdig.cgi/dwarf-discuss-dwarfstd.org/2009-June/000760.html

I think the same has to apply to your example.

Ulrich> Why would that code look at gdbarch_addr_bit at all?  I think this
Ulrich> should simply do something like:

Ulrich> 	    store_unsigned_integer (contents + offset, p->size,
Ulrich> 				    gdbarch_byte_order (c->arch),

Yeah ... I took another look and I still don't understand that use in
read_pieced_value.  Maybe a complaint is in order here.

Tom> @@ -476,19 +474,17 @@ dwarf2_evaluate_loc_desc (struct symbol *var, struct frame_info *frame,

Ulrich> Similiary here; why does this not simply use value_from_longest?

It didn't occur to me; but AFAIK, nothing prohibits a DWARF expression
from using DW_OP_stack_value to fill in a structure or union, and
value_from_longest won't work in that case.

Tom

Re: FYI: fix big-endian bug with DWARF_VALUE_STACK

[Ulrich Weigand]

Tom Tromey wrote:

> >>>>> "Ulrich" == Ulrich Weigand <uweigand@de.ibm.com> writes:
> Ulrich> The original code looks broken to me too.  But even the new code
> Ulrich> doesn't seem quite right: if the requested piece size p->size
> Ulrich> is larger than addr_size, the remaining bytes of the output value
> Ulrich> are left undefined (well, probably zeroed -- but that still doesn't
> Ulrich> look correct on a big-endian machine ...).
> 
> I asked about a similar case on dwarf-discuss, namely what if you have:
> 
>   DW_OP_implicit_value[len=5] DW_OP_piece[len=10]
> 
> The answer I got was something along the lines of "that is undefined,
> don't do that".

This seems somewhat different in that the DW_OP_implicit_value explicitly
provides data of a certain length.  On the other hand, DW_OP_stack_value
refers to the value on top of the stack, which -as I understand it- is a
numerical value, not a sequence of bytes of defined length.

So it would seem to me that you should be able to request that this value
be represented in any arbitrary length ...

However, maybe I'm missing something here, in particular as I haven't been
able to find a formal specification of DW_OP_stack_value -- I assume this
is in the current DWARF4 draft?  Is this available somewhere?

> Tom> @@ -476,19 +474,17 @@ dwarf2_evaluate_loc_desc (struct symbol *var, struct frame_info *frame,
> 
> Ulrich> Similiary here; why does this not simply use value_from_longest?
> 
> It didn't occur to me; but AFAIK, nothing prohibits a DWARF expression
> from using DW_OP_stack_value to fill in a structure or union, and
> value_from_longest won't work in that case.

OK, good point, so you'd have to use store_unsigned_integer directly.
Still, the reference to ctx->addr_size seems odd to me ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: FYI: fix big-endian bug with DWARF_VALUE_STACK

[Mark Wielaard]

On Wed, 2010-02-24 at 15:11 +0100, Ulrich Weigand wrote:
> However, maybe I'm missing something here, in particular as I haven't been
> able to find a formal specification of DW_OP_stack_value -- I assume this
> is in the current DWARF4 draft?  Is this available somewhere?

The latest drafts are here: http://www.dwarfstd.org/doc/

Re: FYI: fix big-endian bug with DWARF_VALUE_STACK

[Ulrich Weigand]

Mark Wielaard wrote:
> On Wed, 2010-02-24 at 15:11 +0100, Ulrich Weigand wrote:
> > However, maybe I'm missing something here, in particular as I haven't been
> > able to find a formal specification of DW_OP_stack_value -- I assume this
> > is in the current DWARF4 draft?  Is this available somewhere?
> 
> The latest drafts are here: http://www.dwarfstd.org/doc/

Thanks!

So in the latest draft we have:

  The DW_OP_stack_value operation specifies that the object does not exist
  in memory but its value is nonetheless known and is at the top of the DWARF
  expression stack. In this form of location description, the DWARF expression
  represents the actual value of the object, rather than its location.
  The DW_OP_stack_value operation terminates the expression.

and an example:

  DW_OP_lit1 DW_OP_stack_value DW_OP_piece 4
  DW_OP_breg3 0 DW_OP_breg4 0 DW_OP_plus DW_OP_stack_value DW_OP_piece 4

  The object value is found in an anonymous (virtual) location whose value
  consists of two parts, given in memory address order: the 4 byte value 1
  followed by the four byte value computed from the sum of the contents of
  r3 and r4.

This doesn't fully resolve the question for me, but at least there's no
explicit reference to the address size here ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: FYI: fix big-endian bug with DWARF_VALUE_STACK

[Tom Tromey]

>>>>> "Ulrich" == Ulrich Weigand <uweigand@de.ibm.com> writes:

Ulrich> This seems somewhat different in that the DW_OP_implicit_value
Ulrich> explicitly provides data of a certain length.  On the other
Ulrich> hand, DW_OP_stack_value refers to the value on top of the stack,
Ulrich> which -as I understand it- is a numerical value, not a sequence
Ulrich> of bytes of defined length.

Elements on the operand stack are explicitly address-sized.  From DWARF
1.11.1:

    Each element of the stack is the size of an address on the target
    machine.

Ulrich> So it would seem to me that you should be able to request that
Ulrich> this value be represented in any arbitrary length ...

That interpretation makes sense to me, but so does the one making it an
error.  I suspect we will never run across code that exercises this.  If
you want to change it, I have no objection.

Tom

Re: FYI: fix big-endian bug with DWARF_VALUE_STACK

[Ulrich Weigand]

Tom Tromey wrote:
> >>>>> "Ulrich" == Ulrich Weigand <uweigand@de.ibm.com> writes:
> Ulrich> So it would seem to me that you should be able to request that
> Ulrich> this value be represented in any arbitrary length ...
> 
> That interpretation makes sense to me, but so does the one making it an
> error.  I suspect we will never run across code that exercises this.  If
> you want to change it, I have no objection.

I agree that it is unlikely code will exercises this, and I'm fine
with leaving the check in.  However, we should at least use the proper
DWARF address size (ctx->addr_size), as is used everywhere else where
the size of values on the DWARF stack is considered, instead of using
gdbarch_addr_bit, which really refers to GDB's internal representation
of target addresses ...

The patch below implements this change; tested on powerpc64-linux.
Does this look reasonable to you?

Bye,
Ulrich

ChangeLog:

	* dwarf2loc.c (struct piece_closure): Remove ARCH member,
	add ADDR_SIZE member.
	(allocate_piece_closure): Update.
	(copy_pieced_value_closure): Likewise.
	(dwarf2_evaluate_loc_desc): Likewise.
	(read_pieced_value): Use DWARF address size instead of
	GDB's gdbarch_addr_bit as size of values on the DWARF stack.


Index: gdb/dwarf2loc.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2loc.c,v
retrieving revision 1.72
diff -u -p -r1.72 dwarf2loc.c
--- gdb/dwarf2loc.c	23 Feb 2010 16:14:36 -0000	1.72
+++ gdb/dwarf2loc.c	25 Feb 2010 18:36:02 -0000
@@ -232,8 +232,8 @@ struct piece_closure
   /* The number of pieces used to describe this variable.  */
   int n_pieces;
 
-  /* The architecture, used only for DWARF_VALUE_STACK.  */
-  struct gdbarch *arch;
+  /* The target address size, used only for DWARF_VALUE_STACK.  */
+  int addr_size;
 
   /* The pieces themselves.  */
   struct dwarf_expr_piece *pieces;
@@ -244,12 +244,12 @@ struct piece_closure
 
 static struct piece_closure *
 allocate_piece_closure (int n_pieces, struct dwarf_expr_piece *pieces,
-			struct gdbarch *arch)
+			int addr_size)
 {
   struct piece_closure *c = XZALLOC (struct piece_closure);
 
   c->n_pieces = n_pieces;
-  c->arch = arch;
+  c->addr_size = addr_size;
   c->pieces = XCALLOC (n_pieces, struct dwarf_expr_piece);
 
   memcpy (c->pieces, pieces, n_pieces * sizeof (struct dwarf_expr_piece));
@@ -298,13 +298,12 @@ read_pieced_value (struct value *v)
 
 	case DWARF_VALUE_STACK:
 	  {
-	    size_t n;
-	    int addr_size = gdbarch_addr_bit (c->arch) / 8;
-	    n = p->size;
-	    if (n > addr_size)
-	      n = addr_size;
+	    struct gdbarch *gdbarch = get_type_arch (value_type (v));
+	    size_t n = p->size;
+	    if (n > c->addr_size)
+	      n = c->addr_size;
 	    store_unsigned_integer (contents + offset, n,
-				    gdbarch_byte_order (c->arch),
+				    gdbarch_byte_order (gdbarch),
 				    p->v.expr.value);
 	  }
 	  break;
@@ -377,7 +376,7 @@ copy_pieced_value_closure (struct value 
 {
   struct piece_closure *c = (struct piece_closure *) value_computed_closure (v);
   
-  return allocate_piece_closure (c->n_pieces, c->pieces, c->arch);
+  return allocate_piece_closure (c->n_pieces, c->pieces, c->addr_size);
 }
 
 static void
@@ -439,7 +438,8 @@ dwarf2_evaluate_loc_desc (struct symbol 
       struct piece_closure *c;
       struct frame_id frame_id = get_frame_id (frame);
 
-      c = allocate_piece_closure (ctx->num_pieces, ctx->pieces, ctx->gdbarch);
+      c = allocate_piece_closure (ctx->num_pieces, ctx->pieces,
+				  ctx->addr_size);
       retval = allocate_computed_value (SYMBOL_TYPE (var),
 					&pieced_value_funcs,
 					c);

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: FYI: fix big-endian bug with DWARF_VALUE_STACK

[Tom Tromey]

>>>>> "Ulrich" == Ulrich Weigand <uweigand@de.ibm.com> writes:

Ulrich> The patch below implements this change; tested on powerpc64-linux.
Ulrich> Does this look reasonable to you?

Yes, it does.  Thanks for doing this.

Tom

Re: FYI: fix big-endian bug with DWARF_VALUE_STACK

[Ulrich Weigand]

Tom Tromey wrote:
> >>>>> "Ulrich" == Ulrich Weigand <uweigand@de.ibm.com> writes:
> 
> Ulrich> The patch below implements this change; tested on powerpc64-linux.
> Ulrich> Does this look reasonable to you?
> 
> Yes, it does.  Thanks for doing this.

OK, I've checked this in now.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com