[PATCH] RISC-V: Fix big endian disassembly of data.
Guy Benyei
guybe@nvidia.com
Thu Nov 4 07:51:15 GMT 2021
Hi,
Thanks for the answer.
After the NVIDIA/Mellanox merger, the Mellanox documents are still in place - I personally took the copyright assignment to our senior VP to sign (and he is still senior VP at NVIDIA). So please look up the Mellanox copyright assignment.
Thanks
Guy
-----Original Message-----
From: Nelson Chu <nelson.chu@sifive.com>
Sent: Thursday, November 4, 2021 9:32 AM
To: Guy Benyei <guybe@nvidia.com>
Cc: binutils@sourceware.org; Kito Cheng <kito.cheng@sifive.com>; Marcus Comstedt <marcus@mc.pp.se>
Subject: Re: [PATCH] RISC-V: Fix big endian disassembly of data.
External email: Use caution opening links or attachments
Hi Guy,
The change of opcodes/riscv-dis.c looks reasonable, and I'm OK with this change, thank you. But unfortunately I cannot find either your or Nvidia's copyright assignment of binutil, so this may cause a problem. However, the trivial 10-line change should be accepted without the copyright assignment, but I had heard that the rule is that - 10-line for all patches from the person total, not each patch individually. I think the 11-line change of opcodes/riscv-dis.c should be OK under this rule, so I would suggest that,
1. Keep the 11-line change of opcodes/riscv-dis.c, or you can reduce the number of lines.
2. Moving the changes of ChangLog files into the commit comments should be enough.
For the big endian data testcase, you probably can find another person who has the copyright and can help to send the patch. I would suggest that adding a simple big endian data test should be enough, since this will double the test cases of the mapping symbol, but in fact they are testing similar things. However, I can add and update all the testcases in the future patches, so you probably can only keep the changes of the opcodes/riscv-dis.c.
Thanks
Nelson
On Thu, Nov 4, 2021 at 2:38 PM Guy Benyei via Binutils <binutils@sourceware.org> wrote:
>
> Ping?
>
> -----Original Message-----
> From: Binutils <binutils-bounces+guybe=mellanox.com@sourceware.org> On
> Behalf Of Guy Benyei via Binutils
> Sent: Monday, November 1, 2021 9:55 AM
> To: binutils@sourceware.org
> Cc: Kito Cheng <kito.cheng@sifive.com>; Marcus Comstedt
> <marcus@mc.pp.se>
> Subject: [PATCH] RISC-V: Fix big endian disassembly of data.
>
> When disassembling big endian RISC-V binary, the data displayed with swapped endiannes. Binary generated from:
>
> .word 0x01020304
>
> was disassembled as:
>
> .word 0x04030201
>
> opcodes/
> * riscv-dis.c: Consider endiannes when printing data
>
> gas/
> * testsuite/gas/riscv/mapping-03b.d: check only for little endian
> * testsuite/gas/riscv/mapping-04b.d: likewise
> * testsuite/gas/riscv/mapping-norelax-03b.d: likewise
> * testsuite/gas/riscv/mapping-norelax-04b.d: likewise
> * testsuite/gas/riscv/mapping-03bbe.d: big endian data/code mapping test
> * testsuite/gas/riscv/mapping-04bbe.d: likewise
> * testsuite/gas/riscv/mapping-norelax-03bbe.d: likewise
> * testsuite/gas/riscv/mapping-norelax-04bbe.d: likewise
>
> Conflicts:
> gas/ChangeLog
> opcodes/ChangeLog
> ---
> gas/ChangeLog | 10 ++++++++++
> gas/testsuite/gas/riscv/mapping-03b.d | 2 +-
> gas/testsuite/gas/riscv/mapping-03bbe.d | 24 ++++++++++++++++++++++++
> gas/testsuite/gas/riscv/mapping-04b.d | 2 +-
> gas/testsuite/gas/riscv/mapping-04bbe.d | 23 +++++++++++++++++++++++
> gas/testsuite/gas/riscv/mapping-norelax-03b.d | 2 +-
> gas/testsuite/gas/riscv/mapping-norelax-03bbe.d | 25 +++++++++++++++++++++++++
> gas/testsuite/gas/riscv/mapping-norelax-04b.d | 2 +-
> gas/testsuite/gas/riscv/mapping-norelax-04bbe.d | 24 ++++++++++++++++++++++++
> opcodes/ChangeLog | 4 ++++
> opcodes/riscv-dis.c | 11 ++++++-----
> 11 files changed, 120 insertions(+), 9 deletions(-) create mode
> 100644 gas/testsuite/gas/riscv/mapping-03bbe.d
> create mode 100644 gas/testsuite/gas/riscv/mapping-04bbe.d
> create mode 100644 gas/testsuite/gas/riscv/mapping-norelax-03bbe.d
> create mode 100644 gas/testsuite/gas/riscv/mapping-norelax-04bbe.d
>
> diff --git a/gas/ChangeLog b/gas/ChangeLog index 1133847..ddfd613
> 100644
> --- a/gas/ChangeLog
> +++ b/gas/ChangeLog
> @@ -1,3 +1,13 @@
> +2021-11-01 Guy Benyei <guybe@nvidia.com>
> + * testsuite/gas/riscv/mapping-03b.d: check only for little endian
> + * testsuite/gas/riscv/mapping-04b.d: likewise
> + * testsuite/gas/riscv/mapping-norelax-03b.d: likewise
> + * testsuite/gas/riscv/mapping-norelax-04b.d: likewise
> + * testsuite/gas/riscv/mapping-03bbe.d: big endian data/code mapping test
> + * testsuite/gas/riscv/mapping-04bbe.d: likewise
> + * testsuite/gas/riscv/mapping-norelax-03bbe.d: likewise
> + * testsuite/gas/riscv/mapping-norelax-04bbe.d: likewise
> +
> 2021-10-28 Markus Klein <markus.klein@sma.de>
>
> PR 28436
> diff --git a/gas/testsuite/gas/riscv/mapping-03b.d
> b/gas/testsuite/gas/riscv/mapping-03b.d
> index f4f6726..ecb3e31 100644
> --- a/gas/testsuite/gas/riscv/mapping-03b.d
> +++ b/gas/testsuite/gas/riscv/mapping-03b.d
> @@ -1,4 +1,4 @@
> -#as:
> +#as: -mlittle-endian
> #source: mapping-03.s
> #objdump: -d
>
> diff --git a/gas/testsuite/gas/riscv/mapping-03bbe.d
> b/gas/testsuite/gas/riscv/mapping-03bbe.d
> new file mode 100644
> index 0000000..9e5b429
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/mapping-03bbe.d
> @@ -0,0 +1,24 @@
> +#as: -mbig-endian
> +#source: mapping-03.s
> +#objdump: -d
> +
> +.*:[ ]+file format .*
> +
> +
> +Disassembly of section .text:
> +
> +0+000 <.text>:
> +[ ]+0:[ ]+00a50533[ ]+add[ ]+a0,a0,a0
> +[ ]+4:[ ]+00000000[ ]+.word[ ]+0x00000000
> +[ ]+8:[ ]+00000013[ ]+nop
> +[ ]+c:[ ]+00000013[ ]+nop
> +[ ]+10:[ ]+00000013[ ]+nop
> +[ ]+14:[ ]+00000001[ ]+.word[ ]+0x00000001
> +[ ]+18:[ ]+00b585b3[ ]+add[ ]+a1,a1,a1
> +[ ]+1c:[ ]+02000000[ ]+.word[ ]+0x02000000
> +[ ]+20:[ ]+03[ ]+.byte[ ]+0x03
> +[ ]+21:[ ]+00000013[ ]+nop
> +[ ]+25:[ ]+00000013[ ]+nop
> +[ ]+29:[ ]+00000013[ ]+nop
> +[ ]+2d:[ ]+00000005[ ]+.word[ ]+0x00000005
> +#...
> diff --git a/gas/testsuite/gas/riscv/mapping-04b.d
> b/gas/testsuite/gas/riscv/mapping-04b.d
> index 9735498..5616220 100644
> --- a/gas/testsuite/gas/riscv/mapping-04b.d
> +++ b/gas/testsuite/gas/riscv/mapping-04b.d
> @@ -1,4 +1,4 @@
> -#as:
> +#as: -mlittle-endian
> #source: mapping-04.s
> #objdump: -d
>
> diff --git a/gas/testsuite/gas/riscv/mapping-04bbe.d
> b/gas/testsuite/gas/riscv/mapping-04bbe.d
> new file mode 100644
> index 0000000..c5339c8
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/mapping-04bbe.d
> @@ -0,0 +1,23 @@
> +#as: -mbig-endian
> +#source: mapping-04.s
> +#objdump: -d
> +
> +.*:[ ]+file format .*
> +
> +
> +Disassembly of section .text:
> +
> +0+000 <.text>:
> +[ ]+0:[ ]+00001001[ ]+.word[ ]+0x00001001
> +[ ]+4:[ ]+00001001[ ]+.word[ ]+0x00001001
> +[ ]+8:[ ]+01000000[ ]+.word[ ]+0x01000000
> +[ ]+c:[ ]+00[ ]+.byte[ ]+0x00
> +[ ]+d:[ ]+00000013[ ]+nop
> +[ ]+11:[ ]+00a50533[ ]+add[ ]+a0,a0,a0
> +[ ]+15:[ ]+20022002[ ]+.word[ ]+0x20022002
> +[ ]+19:[ ]+20022002[ ]+.word[ ]+0x20022002
> +[ ]+1d:[ ]+2002[ ]+.short[ ]+0x2002
> +[ ]+1f:[ ]+00b585b3[ ]+add[ ]+a1,a1,a1
> +[ ]+23:[ ]+0000[ ]+unimp
> +[ ]+25:[ ]+0000[ ]+unimp
> +#...
> diff --git a/gas/testsuite/gas/riscv/mapping-norelax-03b.d
> b/gas/testsuite/gas/riscv/mapping-norelax-03b.d
> index ad88888..e65a922 100644
> --- a/gas/testsuite/gas/riscv/mapping-norelax-03b.d
> +++ b/gas/testsuite/gas/riscv/mapping-norelax-03b.d
> @@ -1,4 +1,4 @@
> -#as: -mno-relax
> +#as: -mno-relax -mlittle-endian
> #source: mapping-03.s
> #objdump: -d
>
> diff --git a/gas/testsuite/gas/riscv/mapping-norelax-03bbe.d
> b/gas/testsuite/gas/riscv/mapping-norelax-03bbe.d
> new file mode 100644
> index 0000000..c8520fd
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/mapping-norelax-03bbe.d
> @@ -0,0 +1,25 @@
> +#as: -mno-relax -mbig-endian
> +#source: mapping-03.s
> +#objdump: -d
> +
> +.*:[ ]+file format .*
> +
> +
> +Disassembly of section .text:
> +
> +0+000 <.text>:
> +[ ]+0:[ ]+00a50533[ ]+add[ ]+a0,a0,a0
> +[ ]+4:[ ]+00000000[ ]+.word[ ]+0x00000000
> +[ ]+8:[ ]+00000013[ ]+nop
> +[ ]+c:[ ]+00000013[ ]+nop
> +[ ]+10:[ ]+00000001[ ]+.word[ ]+0x00000001
> +[ ]+14:[ ]+00b585b3[ ]+add[ ]+a1,a1,a1
> +[ ]+18:[ ]+02000000[ ]+.word[ ]+0x02000000
> +[ ]+1c:[ ]+03[ ]+.byte[ ]+0x03
> +[ ]+1d:[ ]+00[ ]+.byte[ ]+0x00
> +[ ]+1e:[ ]+0001[ ]+nop
> +[ ]+20:[ ]+00000005[ ]+.word[ ]+0x00000005
> +[ ]+24:[ ]+00000013[ ]+nop
> +[ ]+28:[ ]+00000013[ ]+nop
> +[ ]+2c:[ ]+00000013[ ]+nop
> +#...
> diff --git a/gas/testsuite/gas/riscv/mapping-norelax-04b.d
> b/gas/testsuite/gas/riscv/mapping-norelax-04b.d
> index 824a898..00a9dbd 100644
> --- a/gas/testsuite/gas/riscv/mapping-norelax-04b.d
> +++ b/gas/testsuite/gas/riscv/mapping-norelax-04b.d
> @@ -1,4 +1,4 @@
> -#as: -mno-relax
> +#as: -mno-relax -mlittle-endian
> #source: mapping-04.s
> #objdump: -d
>
> diff --git a/gas/testsuite/gas/riscv/mapping-norelax-04bbe.d
> b/gas/testsuite/gas/riscv/mapping-norelax-04bbe.d
> new file mode 100644
> index 0000000..0b987fa
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/mapping-norelax-04bbe.d
> @@ -0,0 +1,24 @@
> +#as: -mno-relax -mbig-endian
> +#source: mapping-04.s
> +#objdump: -d
> +
> +.*:[ ]+file format .*
> +
> +
> +Disassembly of section .text:
> +
> +0+000 <.text>:
> +[ ]+0:[ ]+00001001[ ]+.word[ ]+0x00001001
> +[ ]+4:[ ]+00001001[ ]+.word[ ]+0x00001001
> +[ ]+8:[ ]+01000000[ ]+.word[ ]+0x01000000
> +[ ]+c:[ ]+00[ ]+.byte[ ]+0x00
> +[ ]+d:[ ]+00[ ]+.byte[ ]+0x00
> +[ ]+e:[ ]+0001[ ]+nop
> +[ ]+10:[ ]+00a50533[ ]+add[ ]+a0,a0,a0
> +[ ]+14:[ ]+20022002[ ]+.word[ ]+0x20022002
> +[ ]+18:[ ]+20022002[ ]+.word[ ]+0x20022002
> +[ ]+1c:[ ]+2002[ ]+.short[ ]+0x2002
> +[ ]+1e:[ ]+00b585b3[ ]+add[ ]+a1,a1,a1
> +[ ]+22:[ ]+0001[ ]+nop
> +[ ]+24:[ ]+00000013[ ]+nop
> +#...
> diff --git a/opcodes/ChangeLog b/opcodes/ChangeLog index
> f0e4b72..0f37316 100644
> --- a/opcodes/ChangeLog
> +++ b/opcodes/ChangeLog
> @@ -1,3 +1,7 @@
> +2021-11-01 Guy Benyei <guybe@nvidia.com>
> +
> + * riscv-dis.c: Consider endiannes when printing data
> +
> 2021-10-27 Maciej W. Rozycki <macro@embecosm.com>
>
> * Makefile.am: Remove obsolete comment.
> diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c index
> 1a09440..56af8e0 100644
> --- a/opcodes/riscv-dis.c
> +++ b/opcodes/riscv-dis.c
> @@ -485,13 +485,9 @@ riscv_disassemble_insn (bfd_vma memaddr, insn_t
> word, disassemble_info *info)
>
> insnlen = riscv_insn_length (word);
>
> - /* RISC-V instructions are always little-endian. */
> - info->endian_code = BFD_ENDIAN_LITTLE;
> -
> info->bytes_per_chunk = insnlen % 4 == 0 ? 4 : 2;
> info->bytes_per_line = 8;
> /* We don't support constant pools, so this must be code. */
> - info->display_endian = info->endian_code;
> info->insn_info_valid = 1;
> info->branch_delay_insns = 0;
> info->data_size = 0;
> @@ -811,6 +807,9 @@ print_insn_riscv (bfd_vma memaddr, struct disassemble_info *info)
> else if (riscv_gpr_names == NULL)
> set_default_riscv_dis_options ();
>
> + /* RISC-V instructions are always little-endian. */
> + info->endian_code = BFD_ENDIAN_LITTLE;
> +
> mstate = riscv_search_mapping_symbol (memaddr, info);
> /* Save the last mapping state. */
> last_map_state = mstate;
> @@ -822,6 +821,7 @@ print_insn_riscv (bfd_vma memaddr, struct disassemble_info *info)
> dump_size = riscv_data_length (memaddr, info);
> info->bytes_per_chunk = dump_size;
> riscv_disassembler = riscv_disassemble_data;
> + info->display_endian = info->endian;
> }
> else
> {
> @@ -835,6 +835,7 @@ print_insn_riscv (bfd_vma memaddr, struct disassemble_info *info)
> insn = (insn_t) bfd_getl16 (packet);
> dump_size = riscv_insn_length (insn);
> riscv_disassembler = riscv_disassemble_insn;
> + info->display_endian = info->endian_code;
> }
>
> /* Fetch the instruction to dump. */ @@ -844,7 +845,7 @@ print_insn_riscv (bfd_vma memaddr, struct disassemble_info *info)
> (*info->memory_error_func) (status, memaddr, info);
> return status;
> }
> - insn = (insn_t) bfd_get_bits (packet, dump_size * 8, false);
> + insn = (insn_t) bfd_get_bits (packet, dump_size * 8,
> + info->display_endian == BFD_ENDIAN_BIG);
>
> return (*riscv_disassembler) (memaddr, insn, info); }
> --
> 1.8.3.1
>
More information about the Binutils
mailing list