[PATCH] RISC-V: Fix big endian disassembly of data.
Guy Benyei
guybe@nvidia.com
Tue Nov 30 15:05:20 GMT 2021
A month passed by without answer.
I also tried to contact assign@gnu.org, but no luck there. Does anyone still handle the FSF legal stuff?
Our legal department reviewed the old contract, and stated, that our old contracts are still hold, as Mellanox still exists as a company, as one of Nvidia's subsidiaries.
We're still interested in contributing code - but seems like there is no way we can do that. Not getting answers from assign@gnu.org means that even re-signing the copyright assignment would be impossible.
Thanks
Guy
-----Original Message-----
From: Nelson Chu <nelson.chu@sifive.com>
Sent: Thursday, November 4, 2021 10:59 AM
To: Nick Clifton <nickc@redhat.com>; Jim Wilson <jimw@sifive.com>
Cc: binutils@sourceware.org; Kito Cheng <kito.cheng@sifive.com>; Marcus Comstedt <marcus@mc.pp.se>; Guy Benyei <guybe@nvidia.com>
Subject: Re: [PATCH] RISC-V: Fix big endian disassembly of data.
External email: Use caution opening links or attachments
Hi Nick and Jim,
On Thu, Nov 4, 2021 at 3:51 PM Guy Benyei <guybe@nvidia.com> wrote:
>
> 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.
How do we usually deal with this situation? I do see the copyright of Mellanox, but I'm not sure what to do with the copyright after the merger of the companies.
Thanks
Nelson
> -----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