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

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

[Andreas Arnez]

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.

The new logic also respects the DW_OP_bit_piece offset for
DW_OP_stack_values.  Previously the offset was ignored.  Note that it is
still ignored for all other types of DWARF pieces.

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                            | 38 +++++++++++++-----------------
 gdb/testsuite/gdb.dwarf2/nonvar-access.exp | 21 ++++++++++++++---
 2 files changed, 35 insertions(+), 24 deletions(-)

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 4393c1f..764b3fb 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -1868,22 +1868,19 @@ read_pieced_value (struct value *v)
 
 	case DWARF_VALUE_STACK:
 	  {
-	    size_t n = this_size;
+	    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->offset + p->size > obj_size)
+	      goto skip_copy;
 
-		intermediate_buffer = val_bytes + source_offset;
-	      }
+	    /* Piece offset is from least significant bit end.  */
+	    if (bits_big_endian)
+	      source_offset_bits += obj_size - (p->offset + p->size);
+	    else
+	      source_offset_bits += p->offset;
+	    intermediate_buffer = value_contents_all (p->v.value);
+	    intermediate_buffer += source_offset_bits / 8;
 	  }
 	  break;
 
@@ -1903,22 +1900,21 @@ read_pieced_value (struct value *v)
 	  /* These bits show up as zeros -- but do not cause the value
 	     to be considered optimized-out.  */
 	case DWARF_VALUE_IMPLICIT_POINTER:
-	  break;
+	  goto skip_copy;
 
 	case DWARF_VALUE_OPTIMIZED_OUT:
 	  mark_value_bits_optimized_out (v, offset, this_size_bits);
-	  break;
+	  goto skip_copy;
 
 	default:
 	  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);
+      copy_bitwise (contents, dest_offset_bits,
+		    intermediate_buffer, source_offset_bits % 8,
+		    this_size_bits, bits_big_endian);
 
+    skip_copy:
       offset += this_size_bits;
     }
 }
diff --git a/gdb/testsuite/gdb.dwarf2/nonvar-access.exp b/gdb/testsuite/gdb.dwarf2/nonvar-access.exp
index 3cb5c49..69e01ca 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}
@@ -197,9 +209,12 @@ gdb_test "print/x *(char (*)\[5\]) implicit_a_ptr" \
 
 # 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>"
-- 
2.3.0

Re: [PATCH] PR gdb/21226: Take DWARF stack value pieces from LSB end

[Ulrich Weigand]

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.  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.

These assumptions look good to me.
 
> The new logic also respects the DW_OP_bit_piece offset for
> DW_OP_stack_values.  Previously the offset was ignored.  Note that it is
> still ignored for all other types of DWARF pieces.

I'm not really sure about that.  If we're going to support the offset,
shouldn't we then support it for all piece types?   I'm not sure it
is a good idea to support it for some but not others ...

>  	case DWARF_VALUE_STACK:
>  	  {
> -	    size_t n = this_size;
> +	    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);

c->addr_size is now unused.  This is a left-over from the days when stack
values were untyped, and were always assumed to be integers the size of
a target address.  Now that we have a proper typed stack, c->addr_size
is not really relevant anymore.  It should be completely removed from
the piece_closure data structure (and all code that initializes it).

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

I'm not sure I like those gotos :-(

> -      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);
> +      copy_bitwise (contents, dest_offset_bits,
> +		    intermediate_buffer, source_offset_bits % 8,
> +		    this_size_bits, bits_big_endian);
>  
> +    skip_copy:

At this point, it seems better to just duplicate the copy_bitwise
call to all those switch clauses that actually need it.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH] PR gdb/21226: Take DWARF stack value pieces from LSB end

[Ulrich Weigand]

Andreas Arnez wrote:

Sorry, I overlooked one other issue:

> +	    /* Piece offset is from least significant bit end.  */
> +	    if (bits_big_endian)
> +	      source_offset_bits += obj_size - (p->offset + p->size);
> +	    else
> +	      source_offset_bits += p->offset;

Should this really consult bits_big_endian, as opposed to the
regular byte order?  Note that in the DWARF_VALUE_REGISTER case,
we have the same issue, and there the byte order is consulted.

(This doesn't make much of a difference today since in GDB
bit order is always the same a byte order, but we might as
well get it right ...)

Also, is p->size the right value here?  Note that the code before
the loop might already have partially reduced the size and updated
the this_size(_bits) variables.  Again, I think this case basically
ought to work the same as the DWARF_VALUE_REGISTER case.  (Hmmm.
On the other hand, maybe the DWARF_VALUE_REGISTER case is wrong.
Either way, it should be the same :-))

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH] PR gdb/21226: Take DWARF stack value pieces from LSB end

[Andreas Arnez]

On Fri, Mar 10 2017, Ulrich Weigand wrote:

> 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.  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.
>
> These assumptions look good to me.
>
>> The new logic also respects the DW_OP_bit_piece offset for
>> DW_OP_stack_values.  Previously the offset was ignored.  Note that it is
>> still ignored for all other types of DWARF pieces.
>
> I'm not really sure about that.  If we're going to support the offset,
> shouldn't we then support it for all piece types?   I'm not sure it
> is a good idea to support it for some but not others ...

This patch specifically focuses on fixing the bug with stack value
pieces; I didn't want to intermingle it with fixes for the other piece
types.  So I'll drop the offset support from this patch and re-introduce
it with another patch that adds offset support for all piece types.

>
>>  	case DWARF_VALUE_STACK:
>>  	  {
>> -	    size_t n = this_size;
>> +	    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);
>
> c->addr_size is now unused.  This is a left-over from the days when stack
> values were untyped, and were always assumed to be integers the size of
> a target address.  Now that we have a proper typed stack, c->addr_size
> is not really relevant anymore.  It should be completely removed from
> the piece_closure data structure (and all code that initializes it).

OK, I'll look into providing a separate patch for that.

>
>> +	    /* Use zeroes if piece reaches beyond stack value.  */
>> +	    if (p->offset + p->size > obj_size)
>> +	      goto skip_copy;
>
> I'm not sure I like those gotos :-(
>
>> -      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);
>> +      copy_bitwise (contents, dest_offset_bits,
>> +		    intermediate_buffer, source_offset_bits % 8,
>> +		    this_size_bits, bits_big_endian);
>>  
>> +    skip_copy:
>
> At this point, it seems better to just duplicate the copy_bitwise
> call to all those switch clauses that actually need it.

OK.  (The original author seems to have thought differently, and I tried
to follow the existing scheme, to keep the changes local.)

>
> Bye,
> Ulrich

Re: [PATCH] PR gdb/21226: Take DWARF stack value pieces from LSB end

[Andreas Arnez]

On Fri, Mar 10 2017, Ulrich Weigand wrote:

> Andreas Arnez wrote:
>
> Sorry, I overlooked one other issue:
>
>> +	    /* Piece offset is from least significant bit end.  */
>> +	    if (bits_big_endian)
>> +	      source_offset_bits += obj_size - (p->offset + p->size);
>> +	    else
>> +	      source_offset_bits += p->offset;
>
> Should this really consult bits_big_endian, as opposed to the
> regular byte order?  Note that in the DWARF_VALUE_REGISTER case,
> we have the same issue, and there the byte order is consulted.

Using the byte order would strictly be more correct, yes.  As opposed to
register pieces, we would have to get it from a different gdbarch,
though.  I think the right one would be the objfile gdbarch of the
underlying CU, right?

>
> (This doesn't make much of a difference today since in GDB
> bit order is always the same a byte order, but we might as
> well get it right ...)
>
> Also, is p->size the right value here?  Note that the code before
> the loop might already have partially reduced the size and updated
> the this_size(_bits) variables.  Again, I think this case basically
> ought to work the same as the DWARF_VALUE_REGISTER case.  (Hmmm.
> On the other hand, maybe the DWARF_VALUE_REGISTER case is wrong.
> Either way, it should be the same :-))

Yeah, p->size is correct, and the code for DWARF_VALUE_REGISTER is
wrong.  Again, I didn't want to mix fixes for other piece types into
this patch.  So I'll add another patch for that.

--
Andreas

Re: [PATCH] PR gdb/21226: Take DWARF stack value pieces from LSB end

[Pedro Alves]

On 03/08/2017 06:26 PM, Andreas Arnez wrote:

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

I'm not exactly sure what that means, but doesn't "undefined" suggest
"optimized out" instead?

Thanks,
Pedro Alves

Re: [PATCH] PR gdb/21226: Take DWARF stack value pieces from LSB end

[Ulrich Weigand]

Andreas Arnez wrote:
> On Fri, Mar 10 2017, Ulrich Weigand wrote:
> 
> > Andreas Arnez wrote:
> >
> > Sorry, I overlooked one other issue:
> >
> >> +	    /* Piece offset is from least significant bit end.  */
> >> +	    if (bits_big_endian)
> >> +	      source_offset_bits += obj_size - (p->offset + p->size);
> >> +	    else
> >> +	      source_offset_bits += p->offset;
> >
> > Should this really consult bits_big_endian, as opposed to the
> > regular byte order?  Note that in the DWARF_VALUE_REGISTER case,
> > we have the same issue, and there the byte order is consulted.
> 
> Using the byte order would strictly be more correct, yes.  As opposed to
> register pieces, we would have to get it from a different gdbarch,
> though.  I think the right one would be the objfile gdbarch of the
> underlying CU, right?

That sounds right, and is compatible with what is done for full
DWARF_VALUE_STACK values in dwarf2_evaluate_loc_desc_full.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH] PR gdb/21226: Take DWARF stack value pieces from LSB end

[Andreas Arnez]

On Fri, Mar 10 2017, Ulrich Weigand wrote:

> Andreas Arnez wrote:
>> On Fri, Mar 10 2017, Ulrich Weigand wrote:
>> 
>> > Andreas Arnez wrote:
>> >
>> > Sorry, I overlooked one other issue:
>> >
>> >> +	    /* Piece offset is from least significant bit end.  */
>> >> +	    if (bits_big_endian)
>> >> +	      source_offset_bits += obj_size - (p->offset + p->size);
>> >> +	    else
>> >> +	      source_offset_bits += p->offset;
>> >
>> > Should this really consult bits_big_endian, as opposed to the
>> > regular byte order?  Note that in the DWARF_VALUE_REGISTER case,
>> > we have the same issue, and there the byte order is consulted.
>> 
>> Using the byte order would strictly be more correct, yes.  As opposed to
>> register pieces, we would have to get it from a different gdbarch,
>> though.  I think the right one would be the objfile gdbarch of the
>> underlying CU, right?
>
> That sounds right, and is compatible with what is done for full
> DWARF_VALUE_STACK values in dwarf2_evaluate_loc_desc_full.

Right -- which reminds me...  A month ago I provided a patch for that as
well:

  https://sourceware.org/ml/gdb-patches/2017-03/msg00041.html

--
Andreas

Re: [PATCH] PR gdb/21226: Take DWARF stack value pieces from LSB end

[Andreas Arnez]

On Fri, Mar 10 2017, Pedro Alves wrote:

> On 03/08/2017 06:26 PM, Andreas Arnez wrote:
>
>> * pieces reaching outside the stack value bits are considered undefined,
>>   and a zero value can be used instead.
>
> I'm not exactly sure what that means, but doesn't "undefined" suggest
> "optimized out" instead?

For instance, consider a 20-byte piece from a stack value that is only 8
bytes wide.  AFAIK, this is invalid DWARF.

If we want to handle this case more gracefully, we could mark the upper
12 bytes optimzed out and still use the lower 8 bytes.  Or we could
treat it as an implementation-defined extension and perform zero- (or
sign-) extension to the full piece size.  But so far I didn't see it in
the scope of this bug fix to add such logic, particularly since it
doesn't exist for any other piece type either.

--
Andreas