[gdb/tdep] Fix gdb.base/store.exp on s390x

  On s390x-linux, I get:
...
(gdb) print l^M
$29 = 0^M
(gdb) FAIL: gdb.base/store.exp: var doublest l; print old l, expecting -1
...

So, we're in wack_doublest trying to print l, which is a copy of parameter u:
...
  register doublest l = u, r = v;
...
which does have the expected value:
...
(gdb) p u
$1 = -1
...
which is a long double, 16 bytes and looks like this:
...
(gdb) p /x u
$3 = 0xbfff0000000000000000000000000000
...

Parameter u is passed in two registers:
...
 <2><6a5>: Abbrev Number: 15 (DW_TAG_formal_parameter)
    <6a6>   DW_AT_name        : v
    <69e>   DW_AT_location    : 6 byte block: 50 93 8 51 93 8 \
      (DW_OP_reg0 (r0); DW_OP_piece: 8; DW_OP_reg1 (r1); DW_OP_piece: 8)
...
and indeed we find the msw in r0 and the lsw in r1:
...
(gdb) p /x $r0
$4 = 0xbfff000000000000
(gdb) p /x $r1
$5 = 0x0
(gdb)
...

Likewise, variable l consists of two registers:
...
 <2><6b5>: Abbrev Number: 13 (DW_TAG_variable)
    <6b6>   DW_AT_name        : l
    <6be>   DW_AT_location    : 6 byte block: 68 93 8 69 93 8 \
      (DW_OP_reg24 (f8); DW_OP_piece: 8; DW_OP_reg25 (f10); DW_OP_piece: 8)
...
and we find the same values there:
...
(gdb) p /x $f8
$6 = 0xbfff000000000000
(gdb) p /x $f10
$7 = 0x0
...

So, we get the expected results when fetching the value from two gprs, but not
when fetching the value from two fprs.

The root cause for this seems to be this bit in s390_dwarf_reg_to_regnum:
...
  if (tdep->v0_full_regnum == -1)
    {
      ...
    }
  else
    {
      if (gdb_reg >= S390_F0_REGNUM && gdb_reg <= S390_F15_REGNUM)
       gdb_reg = gdb_reg - S390_F0_REGNUM + tdep->v0_full_regnum;
    }
...

So, instead of fetching the values of f8 and f10, we fetch the values of v8
and v10, which look like this:
...
(gdb) p /x $v8
$10 = {v4_float = {0xbfff0000, 0x0, 0x0, 0x0}, v2_double = {0xbfff000000000000, 0x0},
  v16_int8 = {0xbf, 0xff, 0x0 <repeats 14 times>}, v8_int16 = {0xbfff, 0x0, 0x0, 0x0, 0x0,
    0x0, 0x0, 0x0}, v4_int32 = {0xbfff0000, 0x0, 0x0, 0x0}, v2_int64 = {0xbfff000000000000,
    0x0}, uint128 = 0xbfff0000000000000000000000000000}
(gdb) p /x $v10
$11 = {v4_float = {0x0, 0x0, 0x0, 0x0}, v2_double = {0x0, 0x0}, v16_int8 = {
    0x0 <repeats 16 times>}, v8_int16 = {0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}, v4_int32 = {
    0x0, 0x0, 0x0, 0x0}, v2_int64 = {0x0, 0x0}, uint128 = 0x0}
...

Because v8 is 16 bytes and we fetch only 8 bytes, this bit in rw_pieced_value is
activated:
...
		/* Big-endian, and we want less than full size.  */
		bits_to_skip += reg_bits - (p->offset + p->size);
...
and bits_to_skip gets set to 64, which gets us the value 0x0 instead of
0xbfff000000000000.

So, is there a bug in rw_pieced_value?

The DWARF standard says about DW_OP_piece:
...
If the piece is located in a register, but does not occupy the entire
register, the placement of the piece within that register is defined by the
ABI.
...

The interpretation seems to be to get the lsw of the register, possibly
because this is documented for DW_OP_bit_piece:
...
If the location is a register, the offset is from the least
significant bit end of the register.
...

I don't understand this area well enough to decide whether there's a bug.

Fix this by simply avoiding the unnecessary conversion from f0 to v0 in
s390_dwarf_reg_to_regnum.

Tested on s390x-linux.

Fixes test-cases:
- gdb.ada/O2_float_param.exp
- gdb.base/savedregs.exp
- gdb.base/store.exp
---
 gdb/s390-tdep.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)


base-commit: a042adc98554870ba21e3cda5211a28fad98c217
  

Hello Tom,

Thanks for addressing this.  I'd like this to be fixed, too, but
unfortunately I don't think it's that simple.  See below.

On Thu, Dec 05 2024, Tom de Vries wrote:

> On s390x-linux, I get:
> ...
> (gdb) print l^M
> $29 = 0^M
> (gdb) FAIL: gdb.base/store.exp: var doublest l; print old l, expecting -1
> ...
>
> So, we're in wack_doublest trying to print l, which is a copy of parameter u:

[...]

> Because v8 is 16 bytes and we fetch only 8 bytes, this bit in rw_pieced_value is
> activated:
> ...
> 		/* Big-endian, and we want less than full size.  */
> 		bits_to_skip += reg_bits - (p->offset + p->size);
> ...
> and bits_to_skip gets set to 64, which gets us the value 0x0 instead of
> 0xbfff000000000000.
>
> So, is there a bug in rw_pieced_value?

Yes.  Well.  If it were only one bug.

> The DWARF standard says about DW_OP_piece:
> ...
> If the piece is located in a register, but does not occupy the entire
> register, the placement of the piece within that register is defined by the
> ABI.
> ...
>
> The interpretation seems to be to get the lsw of the register, possibly
> because this is documented for DW_OP_bit_piece:
> ...
> If the location is a register, the offset is from the least
> significant bit end of the register.
> ...
>
> I don't understand this area well enough to decide whether there's a bug.

Yes, there's certainly a bug.  Or rather multiple bugs.

As you already noticed, there's an inconsistency in the DWARF standard.
In my view DW_OP_piece should just be a special case of DW_OP_bit_piece,
and the placement of both should be defined by the ABI.  Forcing bit
pieces to be taken from the LSB while allowing byte pieces to be placed
according to an ABI-defined rule doesn't make any sense.  So I tried to
resolve that inconsistency by changing the wording, but I wasn't able to
convince the DWARF committee that there's even an issue.

Even without fixing the DWARF standard, we could just decide to take
DWARF bit pieces from wherever the ABI states.  See section 1.6 "DWARF
Definition" in the current s390x ABI:

  https://github.com/IBM/s390x-abi

This is currently not implemented fully in GDB.  For this we would need
an architecture callback similar to the gdbarch method
`value_from_register' that can deal with bit slices, perhaps something
like `bit_piece_from_register'.

Last time I looked, GCC also had an issue in this area and produced
inconsistent bit piece offsets for vector registers on s390x.

And GDB probably still has the issue that it can't unwind part of a
register, such as f0 within v0.  Which means that you always get
`undefined' for values in floating-point registers on any higher frame.

FYI, I held a presentation about DWARF pieces at FOSDEM18:

  https://archive.fosdem.org/2018/schedule/event/dwarfpieces/

Where I also listed this bug as an open issue.

> Fix this by simply avoiding the unnecessary conversion from f0 to v0 in
> s390_dwarf_reg_to_regnum.

I don't think that's the correct way of fixing this.  Note that f0 and
v0 use the same DWARF register number.  If a DWARF location refers to
more than the first 64 bits from a vector register v0-v15, I expect your
patch to fail.  Did you try this?
  

On 12/17/24 18:54, Andreas Arnez wrote:
> Hello Tom,
> 
> Thanks for addressing this.  I'd like this to be fixed, too, but
> unfortunately I don't think it's that simple.  See below.
> 

Hi Andreas,

thanks for the review.

> On Thu, Dec 05 2024, Tom de Vries wrote:
> 
>> On s390x-linux, I get:
>> ...
>> (gdb) print l^M
>> $29 = 0^M
>> (gdb) FAIL: gdb.base/store.exp: var doublest l; print old l, expecting -1
>> ...
>>
>> So, we're in wack_doublest trying to print l, which is a copy of parameter u:
> 
> [...]
> 
>> Because v8 is 16 bytes and we fetch only 8 bytes, this bit in rw_pieced_value is
>> activated:
>> ...
>> 		/* Big-endian, and we want less than full size.  */
>> 		bits_to_skip += reg_bits - (p->offset + p->size);
>> ...
>> and bits_to_skip gets set to 64, which gets us the value 0x0 instead of
>> 0xbfff000000000000.
>>
>> So, is there a bug in rw_pieced_value?
> 
> Yes.  Well.  If it were only one bug.
> 

Aha.

>> The DWARF standard says about DW_OP_piece:
>> ...
>> If the piece is located in a register, but does not occupy the entire
>> register, the placement of the piece within that register is defined by the
>> ABI.
>> ...
>>
>> The interpretation seems to be to get the lsw of the register, possibly
>> because this is documented for DW_OP_bit_piece:
>> ...
>> If the location is a register, the offset is from the least
>> significant bit end of the register.
>> ...
>>
>> I don't understand this area well enough to decide whether there's a bug.
> 
> Yes, there's certainly a bug.  Or rather multiple bugs.
> 
> As you already noticed, there's an inconsistency in the DWARF standard.
> In my view DW_OP_piece should just be a special case of DW_OP_bit_piece,
> and the placement of both should be defined by the ABI.  Forcing bit
> pieces to be taken from the LSB while allowing byte pieces to be placed
> according to an ABI-defined rule doesn't make any sense.  So I tried to
> resolve that inconsistency by changing the wording, but I wasn't able to
> convince the DWARF committee that there's even an issue.
> 
> Even without fixing the DWARF standard, we could just decide to take
> DWARF bit pieces from wherever the ABI states.  See section 1.6 "DWARF
> Definition" in the current s390x ABI:
> 
>    https://github.com/IBM/s390x-abi
> 
> This is currently not implemented fully in GDB.  For this we would need
> an architecture callback similar to the gdbarch method
> `value_from_register' that can deal with bit slices, perhaps something
> like `bit_piece_from_register'.
> 

In a v2, I've added a new gdbarch hook gdbarch_dwarf2_reg_piece_offset, 
allowing the s390x architecture to define an implementation to specify 
the ABI-specific behaviour.

For now, I've kept the focus on DW_OP_piece, because that's the one the 
standard say has ABI-specific behaviour.

> Last time I looked, GCC also had an issue in this area and produced
> inconsistent bit piece offsets for vector registers on s390x.
> 

It's possible to work around compiler bugs in gdb, so you could file a 
gdb enhancement PR to request that.

> And GDB probably still has the issue that it can't unwind part of a
> register, such as f0 within v0.  Which means that you always get
> `undefined' for values in floating-point registers on any higher frame.
> 

Yeah, I saw that the two additional test-cases that were fixed in v1 are 
no longer fixed in v2, I suppose they're instances of that problem.

> FYI, I held a presentation about DWARF pieces at FOSDEM18:
> 
>    https://archive.fosdem.org/2018/schedule/event/dwarfpieces/
> 
> Where I also listed this bug as an open issue.
> 

Ack, I've watched the presentation and listed it as reference in v2.

>> Fix this by simply avoiding the unnecessary conversion from f0 to v0 in
>> s390_dwarf_reg_to_regnum.
> 
> I don't think that's the correct way of fixing this.  Note that f0 and
> v0 use the same DWARF register number.  If a DWARF location refers to
> more than the first 64 bits from a vector register v0-v15, I expect your
> patch to fail.  Did you try this?
> 

No, but no test-case failed either during testing, so there's room for a 
test-case there.

As implied earlier, I'm no longer pursuing this approach in v2, 
submitted here ( 
https://sourceware.org/pipermail/gdb-patches/2024-December/214235.html ).

Thanks,
- Tom