diff mbox

[3/9] PR gdb/21226: Take DWARF stack value pieces from LSB end

Message ID 1491586736-21296-4-git-send-email-arnez@linux.vnet.ibm.com
State New
Headers show

Commit Message

Andreas Arnez April 7, 2017, 5:38 p.m. UTC
When taking a DW_OP_piece or DW_OP_bit_piece from a DW_OP_stack_value, the
existing logic always takes the piece from the lowest-addressed end, which
is wrong on big-endian targets.  The DWARF standard states that the
"DW_OP_bit_piece operation describes a sequence of bits using the least
significant bits of that value", and this also matches the current logic
in GCC.  For instance, the GCC guality test case pr54970.c fails on s390x
because of this.

This fix adjusts the piece accordingly on big-endian targets.  It is
assumed that:

* DW_OP_piece shall take the piece from the LSB end as well;

* pieces reaching outside the stack value bits are considered undefined,
  and a zero value can be used instead.

gdb/ChangeLog:

	PR gdb/21226
	* dwarf2loc.c (read_pieced_value): Anchor stack value pieces at
	the LSB end, independent of endianness.

gdb/testsuite/ChangeLog:

	PR gdb/21226
	* gdb.dwarf2/nonvar-access.exp: Add checks for verifying that
	stack value pieces are taken from the LSB end.
---
 gdb/dwarf2loc.c                            | 43 ++++++++++++++++--------------
 gdb/testsuite/gdb.dwarf2/nonvar-access.exp | 21 ++++++++++++---
 2 files changed, 41 insertions(+), 23 deletions(-)

Comments

Simon Marchi April 14, 2017, 3:36 a.m. UTC | #1
On 2017-04-07 13:38, Andreas Arnez wrote:
> When taking a DW_OP_piece or DW_OP_bit_piece from a DW_OP_stack_value, 
> the
> existing logic always takes the piece from the lowest-addressed end, 
> which
> is wrong on big-endian targets.

I'd like if you could clarify this (just here not necessarily in the 
patch).  DWARF locations are computed inside GDB, on the host.  So does 
it really depend on the target endianness, or it's that of the host, or 
both?

Let's consider these cases of remote debugging:

  host -> target
  x86  -> x86
  x86  -> s390
  s390 -> x86
  s390 -> s390

In which cases is the value found at the high memory address vs low 
memory address?

> The DWARF standard states that the
> "DW_OP_bit_piece operation describes a sequence of bits using the least
> significant bits of that value", and this also matches the current 
> logic
> in GCC.  For instance, the GCC guality test case pr54970.c fails on 
> s390x
> because of this.
> 
> This fix adjusts the piece accordingly on big-endian targets.  It is
> assumed that:
> 
> * DW_OP_piece shall take the piece from the LSB end as well;
> 
> * pieces reaching outside the stack value bits are considered 
> undefined,
>   and a zero value can be used instead.
> 
> gdb/ChangeLog:
> 
> 	PR gdb/21226
> 	* dwarf2loc.c (read_pieced_value): Anchor stack value pieces at
> 	the LSB end, independent of endianness.
> 
> gdb/testsuite/ChangeLog:
> 
> 	PR gdb/21226
> 	* gdb.dwarf2/nonvar-access.exp: Add checks for verifying that
> 	stack value pieces are taken from the LSB end.
> ---
>  gdb/dwarf2loc.c                            | 43 
> ++++++++++++++++--------------
>  gdb/testsuite/gdb.dwarf2/nonvar-access.exp | 21 ++++++++++++---
>  2 files changed, 41 insertions(+), 23 deletions(-)
> 
> diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
> index 496400a..09938c4 100644
> --- a/gdb/dwarf2loc.c
> +++ b/gdb/dwarf2loc.c
> @@ -1857,6 +1857,10 @@ read_pieced_value (struct value *v)
>  		if (unavail)
>  		  mark_value_bits_unavailable (v, offset, this_size_bits);
>  	      }
> +
> +	    copy_bitwise (contents, dest_offset_bits,
> +			  intermediate_buffer, source_offset_bits % 8,
> +			  this_size_bits, bits_big_endian);
>  	  }
>  	  break;
> 
> @@ -1865,26 +1869,28 @@ read_pieced_value (struct value *v)
>  			     p->v.mem.in_stack_memory,
>  			     p->v.mem.addr + source_offset,
>  			     buffer.data (), this_size);
> +	  copy_bitwise (contents, dest_offset_bits,
> +			intermediate_buffer, source_offset_bits % 8,
> +			this_size_bits, bits_big_endian);
>  	  break;
> 
>  	case DWARF_VALUE_STACK:
>  	  {
> -	    size_t n = this_size;
> +	    struct objfile *objfile = dwarf2_per_cu_objfile (c->per_cu);
> +	    struct gdbarch *objfile_gdbarch = get_objfile_arch (objfile);
> +	    ULONGEST obj_size = 8 * TYPE_LENGTH (value_type (p->v.value));

It would be really nice for the readers if you could put some comment 
like this, even though it may seem obvious to you:

   /* The size of a DWARF stack value.  */
   ULONGEST obj_size = 8 * TYPE_LENGTH (value_type (p->v.value));

I found I had to add them to the code to be able to follow.

> 
> -	    if (n > c->addr_size - source_offset)
> -	      n = (c->addr_size >= source_offset
> -		   ? c->addr_size - source_offset
> -		   : 0);
> -	    if (n == 0)
> -	      {
> -		/* Nothing.  */
> -	      }
> -	    else
> -	      {
> -		const gdb_byte *val_bytes = value_contents_all (p->v.value);

> +	    /* Use zeroes if piece reaches beyond stack value.  */
> +	    if (p->size > obj_size)
> +	      break;

Does this happen, for example, if a DWARF stack value is 32 bits long, 
but the piece is 64 bits?  I suppose that's not something we'd want a 
compiler to emit, and would be considered a bug in the compiler?

How does breaking out of the loop will use zeroes?  Is the value buffer 
cleared beforehand?

> 
> -		intermediate_buffer = val_bytes + source_offset;
> -	      }
> +	    /* Piece is anchored at least significant bit end.  */
> +	    if (gdbarch_byte_order (objfile_gdbarch) == BFD_ENDIAN_BIG)
> +	      source_offset_bits += obj_size - p->size;

Just a nit, but I find it more readable when there's an empty line 
between the if and the following lines not included in the if (so here, 
right where I cut the quote).  It reads like two separate sentences:

  - If the byte order is big endian, adjust offset in the source.
  - Copy bitwise from the source buffer to the destination buffer.

> +	    copy_bitwise (contents, dest_offset_bits,
> +			  value_contents_all (p->v.value),
> +			  source_offset_bits,
> +			  this_size_bits, bits_big_endian);

Thanks,

Simon
Andreas Arnez April 18, 2017, 4:31 p.m. UTC | #2
On Thu, Apr 13 2017, Simon Marchi wrote:

> On 2017-04-07 13:38, Andreas Arnez wrote:
>> When taking a DW_OP_piece or DW_OP_bit_piece from a DW_OP_stack_value,
>> the
>> existing logic always takes the piece from the lowest-addressed end,
>> which
>> is wrong on big-endian targets.
>
> I'd like if you could clarify this (just here not necessarily in the
> patch).  DWARF locations are computed inside GDB, on the host.  So does it
> really depend on the target endianness, or it's that of the host, or both?
>
> Let's consider these cases of remote debugging:
>
>  host -> target
>  x86  -> x86
>  x86  -> s390
>  s390 -> x86
>  s390 -> s390
>
> In which cases is the value found at the high memory address vs low memory
> address?

DWARF stack values are represented in the target architecture's
endianness, independent from the host.  For instance, if the target is
x86, then the DWARF stack uses little-endian byte order, even on s390
hosts.  See also dwarf_expr_context::execute_stack_op().

[...]

>>
>>  	case DWARF_VALUE_STACK:
>>  	  {
>> -	    size_t n = this_size;
>> +	    struct objfile *objfile = dwarf2_per_cu_objfile (c->per_cu);
>> +	    struct gdbarch *objfile_gdbarch = get_objfile_arch (objfile);
>> +	    ULONGEST obj_size = 8 * TYPE_LENGTH (value_type (p->v.value));
>
> It would be really nice for the readers if you could put some comment like
> this, even though it may seem obvious to you:
>
>   /* The size of a DWARF stack value.  */
>   ULONGEST obj_size = 8 * TYPE_LENGTH (value_type (p->v.value));
>
> I found I had to add them to the code to be able to follow.

OK, but it seems that the variable name 'obj_size' causes the confusion;
so I'd rather skip the comment, change the variable name to
stack_value_size_bits instead and let it speak for itself.

>
>>
>> -	    if (n > c->addr_size - source_offset)
>> -	      n = (c->addr_size >= source_offset
>> -		   ? c->addr_size - source_offset
>> -		   : 0);
>> -	    if (n == 0)
>> -	      {
>> -		/* Nothing.  */
>> -	      }
>> -	    else
>> -	      {
>> -		const gdb_byte *val_bytes = value_contents_all (p->v.value);
>
>> +	    /* Use zeroes if piece reaches beyond stack value.  */
>> +	    if (p->size > obj_size)
>> +	      break;
>
> Does this happen, for example, if a DWARF stack value is 32 bits long, but
> the piece is 64 bits?  I suppose that's not something we'd want a compiler
> to emit, and would be considered a bug in the compiler?

Yes, right now I would consider it a compiler bug.  And I haven't seen a
compiler emit such DWARF code yet.  This is just to avoid crashing GDB,
and to behave predictably instead if it occurs anyway.  (If you're
interested, I've written more thoughts about this topic in section 5,
"padding", of this article:
https://sourceware.org/ml/gdb/2016-01/msg00013.html)

>
> How does breaking out of the loop will use zeroes?  Is the value buffer
> cleared beforehand?

Yes, value_contents_raw returns a zeroed buffer.

>
>>
>> -		intermediate_buffer = val_bytes + source_offset;
>> -	      }
>> +	    /* Piece is anchored at least significant bit end.  */
>> +	    if (gdbarch_byte_order (objfile_gdbarch) == BFD_ENDIAN_BIG)
>> +	      source_offset_bits += obj_size - p->size;
>
> Just a nit, but I find it more readable when there's an empty line between
> the if and the following lines not included in the if (so here, right
> where I cut the quote).  It reads like two separate sentences:
>
>  - If the byte order is big endian, adjust offset in the source.
>  - Copy bitwise from the source buffer to the destination buffer.

OK.

>
>> +	    copy_bitwise (contents, dest_offset_bits,
>> +			  value_contents_all (p->v.value),
>> +			  source_offset_bits,
>> +			  this_size_bits, bits_big_endian);
>
> Thanks,
>
> Simon

Thanks,
Andreas
Simon Marchi April 18, 2017, 4:43 p.m. UTC | #3
On 2017-04-18 12:31, Andreas Arnez wrote:
>> It would be really nice for the readers if you could put some comment 
>> like
>> this, even though it may seem obvious to you:
>> 
>>   /* The size of a DWARF stack value.  */
>>   ULONGEST obj_size = 8 * TYPE_LENGTH (value_type (p->v.value));
>> 
>> I found I had to add them to the code to be able to follow.
> 
> OK, but it seems that the variable name 'obj_size' causes the 
> confusion;
> so I'd rather skip the comment, change the variable name to
> stack_value_size_bits instead and let it speak for itself.

I agree.

Thanks,

Simon
diff mbox

Patch

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 496400a..09938c4 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -1857,6 +1857,10 @@  read_pieced_value (struct value *v)
 		if (unavail)
 		  mark_value_bits_unavailable (v, offset, this_size_bits);
 	      }
+
+	    copy_bitwise (contents, dest_offset_bits,
+			  intermediate_buffer, source_offset_bits % 8,
+			  this_size_bits, bits_big_endian);
 	  }
 	  break;
 
@@ -1865,26 +1869,28 @@  read_pieced_value (struct value *v)
 			     p->v.mem.in_stack_memory,
 			     p->v.mem.addr + source_offset,
 			     buffer.data (), this_size);
+	  copy_bitwise (contents, dest_offset_bits,
+			intermediate_buffer, source_offset_bits % 8,
+			this_size_bits, bits_big_endian);
 	  break;
 
 	case DWARF_VALUE_STACK:
 	  {
-	    size_t n = this_size;
+	    struct objfile *objfile = dwarf2_per_cu_objfile (c->per_cu);
+	    struct gdbarch *objfile_gdbarch = get_objfile_arch (objfile);
+	    ULONGEST obj_size = 8 * TYPE_LENGTH (value_type (p->v.value));
 
-	    if (n > c->addr_size - source_offset)
-	      n = (c->addr_size >= source_offset
-		   ? c->addr_size - source_offset
-		   : 0);
-	    if (n == 0)
-	      {
-		/* Nothing.  */
-	      }
-	    else
-	      {
-		const gdb_byte *val_bytes = value_contents_all (p->v.value);
+	    /* Use zeroes if piece reaches beyond stack value.  */
+	    if (p->size > obj_size)
+	      break;
 
-		intermediate_buffer = val_bytes + source_offset;
-	      }
+	    /* Piece is anchored at least significant bit end.  */
+	    if (gdbarch_byte_order (objfile_gdbarch) == BFD_ENDIAN_BIG)
+	      source_offset_bits += obj_size - p->size;
+	    copy_bitwise (contents, dest_offset_bits,
+			  value_contents_all (p->v.value),
+			  source_offset_bits,
+			  this_size_bits, bits_big_endian);
 	  }
 	  break;
 
@@ -1898,6 +1904,9 @@  read_pieced_value (struct value *v)
 		   : 0);
 	    if (n != 0)
 	      intermediate_buffer = p->v.literal.data + source_offset;
+	    copy_bitwise (contents, dest_offset_bits,
+			  intermediate_buffer, source_offset_bits % 8,
+			  this_size_bits, bits_big_endian);
 	  }
 	  break;
 
@@ -1914,12 +1923,6 @@  read_pieced_value (struct value *v)
 	  internal_error (__FILE__, __LINE__, _("invalid location type"));
 	}
 
-      if (p->location != DWARF_VALUE_OPTIMIZED_OUT
-	  && p->location != DWARF_VALUE_IMPLICIT_POINTER)
-	copy_bitwise (contents, dest_offset_bits,
-		      intermediate_buffer, source_offset_bits % 8,
-		      this_size_bits, bits_big_endian);
-
       offset += this_size_bits;
     }
 }
diff --git a/gdb/testsuite/gdb.dwarf2/nonvar-access.exp b/gdb/testsuite/gdb.dwarf2/nonvar-access.exp
index 99f63cc..5406029 100644
--- a/gdb/testsuite/gdb.dwarf2/nonvar-access.exp
+++ b/gdb/testsuite/gdb.dwarf2/nonvar-access.exp
@@ -128,14 +128,26 @@  Dwarf::assemble $asm_file {
 		    {name def_t}
 		    {type :$struct_t_label}
 		    {location {
-			const1u 0
+			const2s -184
 			stack_value
 			bit_piece 9 0
-			const1s -1
+			const4u 1752286
 			stack_value
 			bit_piece 23 0
 		    } SPECIAL_expr}
 		}
+		# Composite location with some empty pieces.
+		DW_TAG_variable {
+		    {name part_def_a}
+		    {type :$array_a9_label}
+		    {location {
+			piece 3
+			const4u 0xf1927314
+			stack_value
+			piece 4
+			piece 2
+		    } SPECIAL_expr}
+		}
 		# Implicit location: immediate value.
 		DW_TAG_variable {
 		    {name def_implicit_s}
@@ -221,9 +233,12 @@  gdb_test "print/x *implicit_b_ptr" " = $val"
 
 # Byte-aligned fields, pieced together from DWARF stack values.
 gdb_test "print def_s" " = \\{a = 0, b = -1\\}"
+switch $endian { big {set val 0x92} little {set val 0x73} }
+gdb_test "print/x part_def_a\[4\]" " = $val"
+gdb_test "print/x part_def_a\[8\]" " = <optimized out>"
 
 # Non-byte-aligned fields, pieced together from DWARF stack values.
-gdb_test "print def_t" " = \\{a = 0, b = -1\\}"
+gdb_test "print def_t" " = \\{a = -184, b = 1752286\\}"
 
 # Simple variable without location.
 gdb_test "print undef_int" " = <optimized out>"