Re: [PATCH 4/5] ARM: read_pieced_value do big endian processing only in case of valid gdb_regnum

[Victor Kamensky <victor dot kamensky at linaro dot org>]

Hi Yao,

I've attached my notes I captured while looking at
this issue. Some snippets from them repeated below
inline:

On 22 October 2014 02:27, Yao Qi <yao@codesourcery.com> wrote:
> Victor Kamensky <victor.kamensky@linaro.org> writes:
>
> Hi Victor,
> Could you please add more details in the commit message? for example....
>
>> During armv7b testing gdb.base/store.exp test was failling with
>> 'GDB internal error'. It turns out that compiler generated DWARF
>
> What is the 'GDB internal error'?  Is it like this?
>
> (gdb) PASS: gdb.base/store.exp: continue to wack_double
> print l^M
> gdb/regcache.c:178: internal-error: register_size: Assertion `regnum >= 0 && regnum < (gdbarch_num_regs (gdbarch) + gdbarch_num_pseudo_regs (gdbarch))' failed.^M
> A problem internal to GDB has been detected,

Yes, it is like this. I'll added into commit message.

> We've seen this internal error on (armv5te big-endian) for a while.
>
>> with non-existent register numbers. The compiler issue is present
>> in both little endian (armv7) and big endian (armv7b) (it is
>> separate issue). In both case gdbarch_dwarf2_reg_to_regnum returns
>
> Is there any PR opened for the compiler issue?  If there is, please
> mention it in the commit message, otherwise, please describe the
> mistakes in the compiler generated debug info, the snippet of
> 'readelf -wi' output, which shows the wrong register number, should be fine.

In both little endian and big endian cases compiler generate DW_OP_reg29-
DW_OP_reg31 something like this.

 <2><792>: Abbrev Number: 10 (DW_TAG_formal_parameter)
    <793>   DW_AT_name        : u
    <795>   DW_AT_decl_file   : 1
    <796>   DW_AT_decl_line   : 115
    <797>   DW_AT_type        : <0x57c>
    <79b>   DW_AT_location    : 6 byte block: 6d 93 4 6c 93 4
(DW_OP_reg29 (r29); DW_OP_piece: 4; DW_OP_reg28 (r28); DW_OP_piece: 4)

I strongly suspect that it is compiler error, but more accurately
it is hard to say, because I never saw a document where for given CPU
mapping from registers to DWARF reg numbers is defined. Have you
seen such document for example for ARM V7? In any case for this
test case Gdb believes that those register numbers are wrong. I.e we
can say for sure that gcc and gdb are disagrees.

>> -1 which is stored into gdb_regnum. But it cause severe problem
>> only in big endian case because in read_pieced_value and
>> write_pieced_value functions BFD_ENDIAN_BIG related processing
>> happen regardless of gdb_regnum value, and in case of gdb_regnum=-1,
>> it cause 'GDB internal error' and crash.
>>
>> Solution is to move BFD_ENDIAN_BIG related processing under
>> (gdb_regnum != -1) branch of processing.
>
> With your patch applied, the internal error is fixed.  How does GDB
> behave now?  What is the output for 'print l'?  In my case, it becomes:
>
> print l^M
> Unable to access DWARF register number 80^M
> (gdb) FAIL: gdb.base/store.exp: upvar float l; print old l, expecting -1

Yes it becomes the same as little endian result for the same test case.

The only issue addressed by patch that it makes big endian case behave
consistently with little endian - i.e don't do register_size call for unknown
register number

>> ---
>>  gdb/ChangeLog   |  6 ++++++
>>  gdb/dwarf2loc.c | 30 +++++++++++++++---------------
>>  2 files changed, 21 insertions(+), 15 deletions(-)
>>
>> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
>> index c32fb3f..6a735b8 100644
>> --- a/gdb/ChangeLog
>> +++ b/gdb/ChangeLog
>> @@ -1,5 +1,11 @@
>>  2014-10-13  Victor Kamensky  <victor.kamensky@linaro.org>
>>
>> +     * dwarf2loc.c (read_pieced_value): do BE processing only if
>> +     gdb_regnum is not -1.
>
> s/do/Do.  Looks you've fixed it in V2.
> s/BE/big endian/ because BE isn't very clear here.
>
>> +     (write_pieced_value): Ditto.
>> +
>> +2014-10-13  Victor Kamensky  <victor.kamensky@linaro.org>
>> +
>>       * arm-tdep.c: (extract_arm_insn): use dbarch_byte_order_for_code
>>       to read arm instruction.
>>
>> diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
>> index e347e59..fbe99bb 100644
>> --- a/gdb/dwarf2loc.c
>> +++ b/gdb/dwarf2loc.c
>> @@ -1686,20 +1686,20 @@ read_pieced_value (struct value *v)
>>           int gdb_regnum = gdbarch_dwarf2_reg_to_regnum (arch, p->v.regno);
>>           int reg_offset = source_offset;
>>
>> -         if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG
>> -             && this_size < register_size (arch, gdb_regnum))
>> -           {
>> -             /* Big-endian, and we want less than full size.  */
>> -             reg_offset = register_size (arch, gdb_regnum) - this_size;
>> -             /* We want the lower-order THIS_SIZE_BITS of the bytes
>> -                we extract from the register.  */
>> -             source_offset_bits += 8 * this_size - this_size_bits;
>> -           }
>> -
>>           if (gdb_regnum != -1)
>>             {
>>               int optim, unavail;
>>
>> +             if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG
>> +                 && this_size < register_size (arch, gdb_regnum))
>> +               {
>> +                 /* Big-endian, and we want less than full size.  */
>> +                 reg_offset = register_size (arch, gdb_regnum) - this_size;
>> +                 /* We want the lower-order THIS_SIZE_BITS of the bytes
>> +                    we extract from the register.  */
>> +                 source_offset_bits += 8 * this_size - this_size_bits;
>> +              }
>> +
>
> Nit: after the change, local variable 'reg_offset' is only used in the
> "if (gdb_regnum != -1) {}" block, so we can move 'reg_offset' into that
> block.

Ok, I'll make this change too. I'll update comment note with details
and repost the patch.

Thanks,
Victor

> --
> Yao (éå)

Attachment: gdb_armv7ab_badregnum.txt
Description: Text document