[v3,2/2] RISC-V: Better support for long instructions (assembler)
Checks
Commit Message
From: Tsukasa OI <research_trasio@irq.a4lg.com>
Commit bb996692bd96 ("RISC-V/gas: allow generating up to 176-bit
instructions with .insn") tried to start supporting long instructions but
it was insufficient.
1. It heavily depended on the bignum internals (radix of 2^16),
2. It generates "value conflicts with instruction length" even if a big
number instruction encoding does not exceed its expected length,
3. Because long opcode was handled separately (from struct riscv_cl_insn),
some information like DWARF line number correspondence was missing and
To solve these problems, this commit:
1. Handles bignum (and its encodings) precisely and
2. Incorporates long opcode handling into regular instruction handling.
gas/ChangeLog:
* config/tc-riscv.c (struct riscv_cl_insn): Add long opcode field.
(create_insn) Clear long opcode marker.
(install_insn) Install longer opcode as well.
(s_riscv_insn) Likewise.
(riscv_ip_hardcode): Make big number handling stricter. Length and
the value conflicts only if the bignum size exceeds the expected
maximum length.
* testsuite/gas/riscv/insn.s: Add testcases such that big number
handling is required.
* testsuite/gas/riscv/insn.d: Likewise.
* testsuite/gas/riscv/insn-na.d: Likewise.
* testsuite/gas/riscv/insn-dwarf.d: Likewise.
---
gas/config/tc-riscv.c | 38 ++++++++++++++++++++++------
gas/testsuite/gas/riscv/insn-dwarf.d | 10 +++++++-
gas/testsuite/gas/riscv/insn-na.d | 8 ++++++
gas/testsuite/gas/riscv/insn.d | 22 ++++++++++++++++
gas/testsuite/gas/riscv/insn.s | 9 +++++++
5 files changed, 78 insertions(+), 9 deletions(-)
Comments
On 25.11.2022 03:17, Tsukasa OI wrote:
> From: Tsukasa OI <research_trasio@irq.a4lg.com>
>
> Commit bb996692bd96 ("RISC-V/gas: allow generating up to 176-bit
> instructions with .insn") tried to start supporting long instructions but
> it was insufficient.
>
> 1. It heavily depended on the bignum internals (radix of 2^16),
> 2. It generates "value conflicts with instruction length" even if a big
> number instruction encoding does not exceed its expected length,
> 3. Because long opcode was handled separately (from struct riscv_cl_insn),
> some information like DWARF line number correspondence was missing and
>
> To solve these problems, this commit:
>
> 1. Handles bignum (and its encodings) precisely and
> 2. Incorporates long opcode handling into regular instruction handling.
>
> gas/ChangeLog:
>
> * config/tc-riscv.c (struct riscv_cl_insn): Add long opcode field.
> (create_insn) Clear long opcode marker.
> (install_insn) Install longer opcode as well.
> (s_riscv_insn) Likewise.
> (riscv_ip_hardcode): Make big number handling stricter. Length and
> the value conflicts only if the bignum size exceeds the expected
> maximum length.
> * testsuite/gas/riscv/insn.s: Add testcases such that big number
> handling is required.
> * testsuite/gas/riscv/insn.d: Likewise.
> * testsuite/gas/riscv/insn-na.d: Likewise.
> * testsuite/gas/riscv/insn-dwarf.d: Likewise.
> ---
> gas/config/tc-riscv.c | 38 ++++++++++++++++++++++------
> gas/testsuite/gas/riscv/insn-dwarf.d | 10 +++++++-
> gas/testsuite/gas/riscv/insn-na.d | 8 ++++++
> gas/testsuite/gas/riscv/insn.d | 22 ++++++++++++++++
> gas/testsuite/gas/riscv/insn.s | 9 +++++++
> 5 files changed, 78 insertions(+), 9 deletions(-)
>
> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
> index 019545171f5e..2237062d8b45 100644
> --- a/gas/config/tc-riscv.c
> +++ b/gas/config/tc-riscv.c
> @@ -45,6 +45,9 @@ struct riscv_cl_insn
> /* The encoded instruction bits. */
> insn_t insn_opcode;
>
> + /* The long encoded instruction bits ([0] is non-zero if used). */
> + char insn_long_opcode[RISCV_MAX_INSN_LEN];
> +
> /* The frag that contains the instruction. */
> struct frag *frag;
>
> @@ -714,6 +717,7 @@ create_insn (struct riscv_cl_insn *insn, const struct riscv_opcode *mo)
> {
> insn->insn_mo = mo;
> insn->insn_opcode = mo->match;
> + insn->insn_long_opcode[0] = 0; /* Long insn has non-zero value. */
> insn->frag = NULL;
> insn->where = 0;
> insn->fixp = NULL;
> @@ -725,7 +729,10 @@ static void
> install_insn (const struct riscv_cl_insn *insn)
> {
> char *f = insn->frag->fr_literal + insn->where;
> - number_to_chars_littleendian (f, insn->insn_opcode, insn_length (insn));
> + if (insn->insn_long_opcode[0] != 0)
> + memcpy (f, insn->insn_long_opcode, insn_length (insn));
> + else
> + number_to_chars_littleendian (f, insn->insn_opcode, insn_length (insn));
> }
>
> /* Move INSN to offset WHERE in FRAG. Adjust the fixups accordingly
> @@ -3481,7 +3488,9 @@ riscv_ip_hardcode (char *str,
> values[num++] = (insn_t) imm_expr->X_add_number;
> break;
> case O_big:
> - values[num++] = generic_bignum[0];
> + /* Extract lower 32-bits of a big number.
> + Assume that generic_bignum_to_int32 work on such number. */
> + values[num++] = (insn_t) generic_bignum_to_int32 ();
> break;
> default:
> /* The first value isn't constant, so it should be
> @@ -3508,12 +3517,25 @@ riscv_ip_hardcode (char *str,
>
> if (imm_expr->X_op == O_big)
> {
> - if (bytes != imm_expr->X_add_number * CHARS_PER_LITTLENUM)
> + unsigned int llen = 0;
> + for (LITTLENUM_TYPE lval = generic_bignum[imm_expr->X_add_number - 1];
> + lval != 0; llen++)
> + lval >>= BITS_PER_CHAR;
> + unsigned int repr_bytes
> + = (imm_expr->X_add_number - 1) * CHARS_PER_LITTLENUM + llen;
> + if (bytes < repr_bytes)
> return _("value conflicts with instruction length");
> - char *f = frag_more (bytes);
> - for (num = 0; num < imm_expr->X_add_number; ++num)
> - number_to_chars_littleendian (f + num * CHARS_PER_LITTLENUM,
> - generic_bignum[num], CHARS_PER_LITTLENUM);
> + for (num = 0; num < imm_expr->X_add_number - 1; ++num)
> + number_to_chars_littleendian (
> + ip->insn_long_opcode + num * CHARS_PER_LITTLENUM,
> + generic_bignum[num],
> + CHARS_PER_LITTLENUM);
> + if (llen != 0)
> + number_to_chars_littleendian (
> + ip->insn_long_opcode + num * CHARS_PER_LITTLENUM,
> + generic_bignum[num],
> + llen);
> + memset(ip->insn_long_opcode + repr_bytes, 0, bytes - repr_bytes);
> return NULL;
> }
>
> @@ -4590,7 +4612,7 @@ s_riscv_insn (int x ATTRIBUTE_UNUSED)
> else
> as_bad ("%s `%s'", error.msg, error.statement);
> }
> - else if (imm_expr.X_op != O_big)
> + else
> {
> gas_assert (insn.insn_mo->pinfo != INSN_MACRO);
> append_insn (&insn, &imm_expr, imm_reloc);
> diff --git a/gas/testsuite/gas/riscv/insn-dwarf.d b/gas/testsuite/gas/riscv/insn-dwarf.d
> index 89dc8d58ff09..b8bd42dff18c 100644
> --- a/gas/testsuite/gas/riscv/insn-dwarf.d
> +++ b/gas/testsuite/gas/riscv/insn-dwarf.d
> @@ -74,5 +74,13 @@ insn.s +69 +0xf6.*
> insn.s +70 +0xfe.*
> insn.s +71 +0x108.*
> insn.s +72 +0x114.*
> -insn.s +- +0x12a
> +insn.s +74 +0x12a.*
> +insn.s +75 +0x134.*
> +insn.s +76 +0x13e.*
> +insn.s +77 +0x154.*
> +insn.s +78 +0x16a.*
> +insn.s +79 +0x180.*
> +insn.s +80 +0x196.*
> +insn.s +81 +0x1ac.*
> +insn.s +- +0x1c2
> #pass
> diff --git a/gas/testsuite/gas/riscv/insn-na.d b/gas/testsuite/gas/riscv/insn-na.d
> index 66dce71ebc21..6928ba9ba0f2 100644
> --- a/gas/testsuite/gas/riscv/insn-na.d
> +++ b/gas/testsuite/gas/riscv/insn-na.d
> @@ -73,3 +73,11 @@ Disassembly of section .text:
> [^:]+:[ ]+007f 0000 0000 0000 0000[ ]+[._a-z].*
> [^:]+:[ ]+0000107f 00000000 00000000[ ]+[._a-z].*
> [^:]+:[ ]+607f 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000[ ]+[._a-z].*
> +[^:]+:[ ]+007f 0000 0000 0000 8000[ ]+\.byte[ ]+0x7f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80
> +[^:]+:[ ]+007f 0000 0000 0000 8000[ ]+\.byte[ ]+0x7f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80
> +[^:]+:[ ]+607f 89ab 4567 0123 3210 7654 ba98 fedc 0000 0000 0000[ ]+\.byte[ ]+0x7f, 0x60, 0xab, 0x89, 0x67, 0x45, 0x23, 0x01, 0x10, 0x32, 0x54, 0x76, 0x98, 0xba, 0xdc, 0xfe, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
I have to admit that I still don't see what good the ".byte ..." part of
the expectations does for the purpose of the test. In the cover letter
you say "They are not 4-byte aligned (10 and 22-bytes) and unlikely to
change any time soon." But changing that is exactly my plan (unless
objected to by the arch maintainers): Showing insn components as bytes
is imo reasonable for RISC-V at most when things aren't even 2-byte
aligned. IOW I'd see these to be "disassembled" to ".2byte ...",
matching the "raw opcode" output left to .<N>byte. In fact when raw
opcodes are output I question the need for any .<N>byte - it's fully
redundant.
Bottom line: As before I'd prefer if these parts were dropped (to limit
the churn on the files when changing the .<N>byte granularity), but I'm
not going to insist. Apart from this the change looks good to me.
Jan
On 2022/11/25 17:15, Jan Beulich wrote:
> On 25.11.2022 03:17, Tsukasa OI wrote:
>> From: Tsukasa OI <research_trasio@irq.a4lg.com>
>>
>> Commit bb996692bd96 ("RISC-V/gas: allow generating up to 176-bit
>> instructions with .insn") tried to start supporting long instructions but
>> it was insufficient.
>>
>> 1. It heavily depended on the bignum internals (radix of 2^16),
>> 2. It generates "value conflicts with instruction length" even if a big
>> number instruction encoding does not exceed its expected length,
>> 3. Because long opcode was handled separately (from struct riscv_cl_insn),
>> some information like DWARF line number correspondence was missing and
>>
>> To solve these problems, this commit:
>>
>> 1. Handles bignum (and its encodings) precisely and
>> 2. Incorporates long opcode handling into regular instruction handling.
>>
>> gas/ChangeLog:
>>
>> * config/tc-riscv.c (struct riscv_cl_insn): Add long opcode field.
>> (create_insn) Clear long opcode marker.
>> (install_insn) Install longer opcode as well.
>> (s_riscv_insn) Likewise.
>> (riscv_ip_hardcode): Make big number handling stricter. Length and
>> the value conflicts only if the bignum size exceeds the expected
>> maximum length.
>> * testsuite/gas/riscv/insn.s: Add testcases such that big number
>> handling is required.
>> * testsuite/gas/riscv/insn.d: Likewise.
>> * testsuite/gas/riscv/insn-na.d: Likewise.
>> * testsuite/gas/riscv/insn-dwarf.d: Likewise.
>> ---
>> gas/config/tc-riscv.c | 38 ++++++++++++++++++++++------
>> gas/testsuite/gas/riscv/insn-dwarf.d | 10 +++++++-
>> gas/testsuite/gas/riscv/insn-na.d | 8 ++++++
>> gas/testsuite/gas/riscv/insn.d | 22 ++++++++++++++++
>> gas/testsuite/gas/riscv/insn.s | 9 +++++++
>> 5 files changed, 78 insertions(+), 9 deletions(-)
>>
>> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
>> index 019545171f5e..2237062d8b45 100644
>> --- a/gas/config/tc-riscv.c
>> +++ b/gas/config/tc-riscv.c
>> @@ -45,6 +45,9 @@ struct riscv_cl_insn
>> /* The encoded instruction bits. */
>> insn_t insn_opcode;
>>
>> + /* The long encoded instruction bits ([0] is non-zero if used). */
>> + char insn_long_opcode[RISCV_MAX_INSN_LEN];
>> +
>> /* The frag that contains the instruction. */
>> struct frag *frag;
>>
>> @@ -714,6 +717,7 @@ create_insn (struct riscv_cl_insn *insn, const struct riscv_opcode *mo)
>> {
>> insn->insn_mo = mo;
>> insn->insn_opcode = mo->match;
>> + insn->insn_long_opcode[0] = 0; /* Long insn has non-zero value. */
>> insn->frag = NULL;
>> insn->where = 0;
>> insn->fixp = NULL;
>> @@ -725,7 +729,10 @@ static void
>> install_insn (const struct riscv_cl_insn *insn)
>> {
>> char *f = insn->frag->fr_literal + insn->where;
>> - number_to_chars_littleendian (f, insn->insn_opcode, insn_length (insn));
>> + if (insn->insn_long_opcode[0] != 0)
>> + memcpy (f, insn->insn_long_opcode, insn_length (insn));
>> + else
>> + number_to_chars_littleendian (f, insn->insn_opcode, insn_length (insn));
>> }
>>
>> /* Move INSN to offset WHERE in FRAG. Adjust the fixups accordingly
>> @@ -3481,7 +3488,9 @@ riscv_ip_hardcode (char *str,
>> values[num++] = (insn_t) imm_expr->X_add_number;
>> break;
>> case O_big:
>> - values[num++] = generic_bignum[0];
>> + /* Extract lower 32-bits of a big number.
>> + Assume that generic_bignum_to_int32 work on such number. */
>> + values[num++] = (insn_t) generic_bignum_to_int32 ();
>> break;
>> default:
>> /* The first value isn't constant, so it should be
>> @@ -3508,12 +3517,25 @@ riscv_ip_hardcode (char *str,
>>
>> if (imm_expr->X_op == O_big)
>> {
>> - if (bytes != imm_expr->X_add_number * CHARS_PER_LITTLENUM)
>> + unsigned int llen = 0;
>> + for (LITTLENUM_TYPE lval = generic_bignum[imm_expr->X_add_number - 1];
>> + lval != 0; llen++)
>> + lval >>= BITS_PER_CHAR;
>> + unsigned int repr_bytes
>> + = (imm_expr->X_add_number - 1) * CHARS_PER_LITTLENUM + llen;
>> + if (bytes < repr_bytes)
>> return _("value conflicts with instruction length");
>> - char *f = frag_more (bytes);
>> - for (num = 0; num < imm_expr->X_add_number; ++num)
>> - number_to_chars_littleendian (f + num * CHARS_PER_LITTLENUM,
>> - generic_bignum[num], CHARS_PER_LITTLENUM);
>> + for (num = 0; num < imm_expr->X_add_number - 1; ++num)
>> + number_to_chars_littleendian (
>> + ip->insn_long_opcode + num * CHARS_PER_LITTLENUM,
>> + generic_bignum[num],
>> + CHARS_PER_LITTLENUM);
>> + if (llen != 0)
>> + number_to_chars_littleendian (
>> + ip->insn_long_opcode + num * CHARS_PER_LITTLENUM,
>> + generic_bignum[num],
>> + llen);
>> + memset(ip->insn_long_opcode + repr_bytes, 0, bytes - repr_bytes);
>> return NULL;
>> }
>>
>> @@ -4590,7 +4612,7 @@ s_riscv_insn (int x ATTRIBUTE_UNUSED)
>> else
>> as_bad ("%s `%s'", error.msg, error.statement);
>> }
>> - else if (imm_expr.X_op != O_big)
>> + else
>> {
>> gas_assert (insn.insn_mo->pinfo != INSN_MACRO);
>> append_insn (&insn, &imm_expr, imm_reloc);
>> diff --git a/gas/testsuite/gas/riscv/insn-dwarf.d b/gas/testsuite/gas/riscv/insn-dwarf.d
>> index 89dc8d58ff09..b8bd42dff18c 100644
>> --- a/gas/testsuite/gas/riscv/insn-dwarf.d
>> +++ b/gas/testsuite/gas/riscv/insn-dwarf.d
>> @@ -74,5 +74,13 @@ insn.s +69 +0xf6.*
>> insn.s +70 +0xfe.*
>> insn.s +71 +0x108.*
>> insn.s +72 +0x114.*
>> -insn.s +- +0x12a
>> +insn.s +74 +0x12a.*
>> +insn.s +75 +0x134.*
>> +insn.s +76 +0x13e.*
>> +insn.s +77 +0x154.*
>> +insn.s +78 +0x16a.*
>> +insn.s +79 +0x180.*
>> +insn.s +80 +0x196.*
>> +insn.s +81 +0x1ac.*
>> +insn.s +- +0x1c2
>> #pass
>> diff --git a/gas/testsuite/gas/riscv/insn-na.d b/gas/testsuite/gas/riscv/insn-na.d
>> index 66dce71ebc21..6928ba9ba0f2 100644
>> --- a/gas/testsuite/gas/riscv/insn-na.d
>> +++ b/gas/testsuite/gas/riscv/insn-na.d
>> @@ -73,3 +73,11 @@ Disassembly of section .text:
>> [^:]+:[ ]+007f 0000 0000 0000 0000[ ]+[._a-z].*
>> [^:]+:[ ]+0000107f 00000000 00000000[ ]+[._a-z].*
>> [^:]+:[ ]+607f 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000[ ]+[._a-z].*
>> +[^:]+:[ ]+007f 0000 0000 0000 8000[ ]+\.byte[ ]+0x7f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80
>> +[^:]+:[ ]+007f 0000 0000 0000 8000[ ]+\.byte[ ]+0x7f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80
>> +[^:]+:[ ]+607f 89ab 4567 0123 3210 7654 ba98 fedc 0000 0000 0000[ ]+\.byte[ ]+0x7f, 0x60, 0xab, 0x89, 0x67, 0x45, 0x23, 0x01, 0x10, 0x32, 0x54, 0x76, 0x98, 0xba, 0xdc, 0xfe, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>
> I have to admit that I still don't see what good the ".byte ..." part of
> the expectations does for the purpose of the test. In the cover letter
> you say "They are not 4-byte aligned (10 and 22-bytes) and unlikely to
> change any time soon." But changing that is exactly my plan (unless
> objected to by the arch maintainers): Showing insn components as bytes
> is imo reasonable for RISC-V at most when things aren't even 2-byte
> aligned. IOW I'd see these to be "disassembled" to ".2byte ...",
> matching the "raw opcode" output left to .<N>byte. In fact when raw
> opcodes are output I question the need for any .<N>byte - it's fully
> redundant>
> Bottom line: As before I'd prefer if these parts were dropped (to limit
> the churn on the files when changing the .<N>byte granularity), but I'm
> not going to insist. Apart from this the change looks good to me.
>
> Jan
>
Okay, I have to admit that I misunderstood your intent.
Quoting my PATCH v1 cover letter:
> [Disassembler: Instruction is trimmed with 64-bits]
>
> In this section, we reuse the object file generated by the section above
> (two 22-byte instructions).
>
> 0000000000000000 <.text>:
> 0: 607f 0000 0000 0000 .byte 0x7f, 0x60, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
> 8: 0000 0000 0000 0000
> 10: 0000 0000 0000
> 16: 607f 33cc 55aa cdef .byte 0x7f, 0x60, 0xcc, 0x33, 0xaa, 0x55, 0xef, 0xcd, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
> 1e: 89ab 4567 0123 3210 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 26: 7654 ba98 fedc zeroed out after first 64-bits
>
> See ".byte" at the address 0x16. It's trimmed at 64-bits.
> The resolution is simple. If we simply add a char* argument (containing all
> instruction bits), we can easily resolve this.
>
> 0000000000000000 <.text>:
> 0: 607f 0000 0000 0000 .byte 0x7f, 0x60, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
> 8: 0000 0000 0000 0000
> 10: 0000 0000 0000
> 16: 607f 33cc 55aa cdef .byte 0x7f, 0x60, 0xcc, 0x33, 0xaa, 0x55, 0xef, 0xcd, 0xab, 0x89, 0x67, 0x45, 0x23, 0x01, 0x10, 0x32, 0x54, 0x76, 0x98, 0xba, 0xdc, 0xfe
> 1e: 89ab 4567 0123 3210 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 26: 7654 ba98 fedc all instruction bits are correct
At least, I want to test whether disassembly of this part (after first
64-bits of an instruction) is fixed. That is exactly what's fixed in
PATCH 1/2 and I want to make sure that this part is changed correctly.
If you change the output, you can freely remove or replace the testcase
according to the new output format but until that happens, I want to
keep those. What am I missing?
Tsukasa
On 25.11.2022 09:39, Tsukasa OI wrote:
> On 2022/11/25 17:15, Jan Beulich wrote:
>> On 25.11.2022 03:17, Tsukasa OI wrote:
>>> --- a/gas/testsuite/gas/riscv/insn-na.d
>>> +++ b/gas/testsuite/gas/riscv/insn-na.d
>>> @@ -73,3 +73,11 @@ Disassembly of section .text:
>>> [^:]+:[ ]+007f 0000 0000 0000 0000[ ]+[._a-z].*
>>> [^:]+:[ ]+0000107f 00000000 00000000[ ]+[._a-z].*
>>> [^:]+:[ ]+607f 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000[ ]+[._a-z].*
>>> +[^:]+:[ ]+007f 0000 0000 0000 8000[ ]+\.byte[ ]+0x7f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80
>>> +[^:]+:[ ]+007f 0000 0000 0000 8000[ ]+\.byte[ ]+0x7f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80
>>> +[^:]+:[ ]+607f 89ab 4567 0123 3210 7654 ba98 fedc 0000 0000 0000[ ]+\.byte[ ]+0x7f, 0x60, 0xab, 0x89, 0x67, 0x45, 0x23, 0x01, 0x10, 0x32, 0x54, 0x76, 0x98, 0xba, 0xdc, 0xfe, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>>
>> I have to admit that I still don't see what good the ".byte ..." part of
>> the expectations does for the purpose of the test. In the cover letter
>> you say "They are not 4-byte aligned (10 and 22-bytes) and unlikely to
>> change any time soon." But changing that is exactly my plan (unless
>> objected to by the arch maintainers): Showing insn components as bytes
>> is imo reasonable for RISC-V at most when things aren't even 2-byte
>> aligned. IOW I'd see these to be "disassembled" to ".2byte ...",
>> matching the "raw opcode" output left to .<N>byte. In fact when raw
>> opcodes are output I question the need for any .<N>byte - it's fully
>> redundant>
>> Bottom line: As before I'd prefer if these parts were dropped (to limit
>> the churn on the files when changing the .<N>byte granularity), but I'm
>> not going to insist. Apart from this the change looks good to me.
>
> Okay, I have to admit that I misunderstood your intent.
>
> Quoting my PATCH v1 cover letter:
>
>> [Disassembler: Instruction is trimmed with 64-bits]
>>
>> In this section, we reuse the object file generated by the section above
>> (two 22-byte instructions).
>>
>> 0000000000000000 <.text>:
>> 0: 607f 0000 0000 0000 .byte 0x7f, 0x60, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>> 8: 0000 0000 0000 0000
>> 10: 0000 0000 0000
>> 16: 607f 33cc 55aa cdef .byte 0x7f, 0x60, 0xcc, 0x33, 0xaa, 0x55, 0xef, 0xcd, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>> 1e: 89ab 4567 0123 3210 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> 26: 7654 ba98 fedc zeroed out after first 64-bits
>>
>> See ".byte" at the address 0x16. It's trimmed at 64-bits.
>> The resolution is simple. If we simply add a char* argument (containing all
>> instruction bits), we can easily resolve this.
>>
>> 0000000000000000 <.text>:
>> 0: 607f 0000 0000 0000 .byte 0x7f, 0x60, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>> 8: 0000 0000 0000 0000
>> 10: 0000 0000 0000
>> 16: 607f 33cc 55aa cdef .byte 0x7f, 0x60, 0xcc, 0x33, 0xaa, 0x55, 0xef, 0xcd, 0xab, 0x89, 0x67, 0x45, 0x23, 0x01, 0x10, 0x32, 0x54, 0x76, 0x98, 0xba, 0xdc, 0xfe
>> 1e: 89ab 4567 0123 3210 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> 26: 7654 ba98 fedc all instruction bits are correct
>
> At least, I want to test whether disassembly of this part (after first
> 64-bits of an instruction) is fixed. That is exactly what's fixed in
> PATCH 1/2 and I want to make sure that this part is changed correctly.
Which you already achieve by the raw opcode output
607f 89ab 4567 0123 3210 7654 ba98 fedc 0000 0000 0000
The subsequent
.byte[ ]+0x7f, 0x60, 0xab, 0x89, 0x67, 0x45, 0x23, 0x01, 0x10, 0x32, 0x54, 0x76, 0x98, 0xba, 0xdc, 0xfe, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
does not check anything the first part didn't already check.
> If you change the output, you can freely remove or replace the testcase
> according to the new output format but until that happens, I want to
> keep those. What am I missing?
I can only restate what I've said earlier: Testcases would imo better be
supplying as strict as necessary but as relaxed as possible expectations.
If it was truly the _intention_ for .byte (and not e.g. .2byte) to be
used here, _then_ it would make sense to have this be part of the
expectations. And then, by recording it that way, you also raise the
barrier of someone changing the behavior - after all the testcase then
says it is intended to be that way (rather than, as I assume here, it
being merely "the way it is").
And yes, if you look at other testcase expectations you will frequently
find way too strict expectations (often enough people simply take the
output of the dumping tool, massage it suitably to be regex-es, and be
done - sometimes with the effect of event recording outright wrong
expectations, simply because huge output is cumbersome to check for
correctness). In many cases this has resulted in unnecessarily big code
churn when extending such testcases, or unhelpful mishmash because of
people then always adding to the end instead of at a more sensible
place (e.g. next to related stuff). Hence in particular for a still
relatively new and tidy port like RISC-V is, it would seem desirable to
me to learn from mistakes made elsewhere before.
Yet to reiterate - I'm not going to insist, first and foremost because
binutils, unlike other projects, has overall relatively relaxed
acceptance criteria for patches.
Jan
On 2022/11/25 18:04, Jan Beulich wrote:
> On 25.11.2022 09:39, Tsukasa OI wrote:
>> On 2022/11/25 17:15, Jan Beulich wrote:
>>> On 25.11.2022 03:17, Tsukasa OI wrote:
>>>> --- a/gas/testsuite/gas/riscv/insn-na.d
>>>> +++ b/gas/testsuite/gas/riscv/insn-na.d
>>>> @@ -73,3 +73,11 @@ Disassembly of section .text:
>>>> [^:]+:[ ]+007f 0000 0000 0000 0000[ ]+[._a-z].*
>>>> [^:]+:[ ]+0000107f 00000000 00000000[ ]+[._a-z].*
>>>> [^:]+:[ ]+607f 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000[ ]+[._a-z].*
>>>> +[^:]+:[ ]+007f 0000 0000 0000 8000[ ]+\.byte[ ]+0x7f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80
>>>> +[^:]+:[ ]+007f 0000 0000 0000 8000[ ]+\.byte[ ]+0x7f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80
>>>> +[^:]+:[ ]+607f 89ab 4567 0123 3210 7654 ba98 fedc 0000 0000 0000[ ]+\.byte[ ]+0x7f, 0x60, 0xab, 0x89, 0x67, 0x45, 0x23, 0x01, 0x10, 0x32, 0x54, 0x76, 0x98, 0xba, 0xdc, 0xfe, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>>>
>>> I have to admit that I still don't see what good the ".byte ..." part of
>>> the expectations does for the purpose of the test. In the cover letter
>>> you say "They are not 4-byte aligned (10 and 22-bytes) and unlikely to
>>> change any time soon." But changing that is exactly my plan (unless
>>> objected to by the arch maintainers): Showing insn components as bytes
>>> is imo reasonable for RISC-V at most when things aren't even 2-byte
>>> aligned. IOW I'd see these to be "disassembled" to ".2byte ...",
>>> matching the "raw opcode" output left to .<N>byte. In fact when raw
>>> opcodes are output I question the need for any .<N>byte - it's fully
>>> redundant>
>>> Bottom line: As before I'd prefer if these parts were dropped (to limit
>>> the churn on the files when changing the .<N>byte granularity), but I'm
>>> not going to insist. Apart from this the change looks good to me.
>>
>> Okay, I have to admit that I misunderstood your intent.
>>
>> Quoting my PATCH v1 cover letter:
>>
>>> [Disassembler: Instruction is trimmed with 64-bits]
>>>
>>> In this section, we reuse the object file generated by the section above
>>> (two 22-byte instructions).
>>>
>>> 0000000000000000 <.text>:
>>> 0: 607f 0000 0000 0000 .byte 0x7f, 0x60, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>>> 8: 0000 0000 0000 0000
>>> 10: 0000 0000 0000
>>> 16: 607f 33cc 55aa cdef .byte 0x7f, 0x60, 0xcc, 0x33, 0xaa, 0x55, 0xef, 0xcd, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>>> 1e: 89ab 4567 0123 3210 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>> 26: 7654 ba98 fedc zeroed out after first 64-bits
>>>
>>> See ".byte" at the address 0x16. It's trimmed at 64-bits.
>>> The resolution is simple. If we simply add a char* argument (containing all
>>> instruction bits), we can easily resolve this.
>>>
>>> 0000000000000000 <.text>:
>>> 0: 607f 0000 0000 0000 .byte 0x7f, 0x60, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>>> 8: 0000 0000 0000 0000
>>> 10: 0000 0000 0000
>>> 16: 607f 33cc 55aa cdef .byte 0x7f, 0x60, 0xcc, 0x33, 0xaa, 0x55, 0xef, 0xcd, 0xab, 0x89, 0x67, 0x45, 0x23, 0x01, 0x10, 0x32, 0x54, 0x76, 0x98, 0xba, 0xdc, 0xfe
>>> 1e: 89ab 4567 0123 3210 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>> 26: 7654 ba98 fedc all instruction bits are correct
>>
>> At least, I want to test whether disassembly of this part (after first
>> 64-bits of an instruction) is fixed. That is exactly what's fixed in
>> PATCH 1/2 and I want to make sure that this part is changed correctly.
>
> Which you already achieve by the raw opcode output
>
> 607f 89ab 4567 0123 3210 7654 ba98 fedc 0000 0000 0000
>
> The subsequent
>
> .byte[ ]+0x7f, 0x60, 0xab, 0x89, 0x67, 0x45, 0x23, 0x01, 0x10, 0x32, 0x54, 0x76, 0x98, 0xba, 0xdc, 0xfe, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>
> does not check anything the first part didn't already check.
That's simply not true.
Again, quoting from PATCH v1 cover letter (BEFORE THE PATCH
[disassembler part]):
> 16: 607f 33cc 55aa cdef .byte 0x7f, 0x60, 0xcc, 0x33, 0xaa, 0x55, 0xef, 0xcd, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
> 1e: 89ab 4567 0123 3210 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 26: 7654 ba98 fedc zeroed out after first 64-bits
Compare hexdump and printed bytes (in ".byte"). You can see that "1e:
89ab..." are different from corresponding ".byte" (0x00, 0x00...).
They are completely separate and PATCH 1/2 only changes ".byte" output.
If ".byte[ ]+0x7f, 0x60..." does not check anything the first part
didn't already check, the dump would look like this:
> 16: 607f 33cc 55aa cdef .byte 0x7f, 0x60, 0xcc, 0x33, 0xaa, 0x55, 0xef, 0xcd, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
> 1e: 0000 0000 0000 0000 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 26: 0000 0000 0000 zeroed out after first 64-bits (but matches to hexdump)
If the hexdump and ".byte" output always matches EVEN BEFORE THE FIX,
that's what I can call "redundant". But in reality, they do not.
That's why I want to leave a test to make sure that the issue is fixed
(now hexdump and ".byte" output always matches).
Tsukasa
>
>> If you change the output, you can freely remove or replace the testcase
>> according to the new output format but until that happens, I want to
>> keep those. What am I missing?
>
> I can only restate what I've said earlier: Testcases would imo better be
> supplying as strict as necessary but as relaxed as possible expectations.
> If it was truly the _intention_ for .byte (and not e.g. .2byte) to be
> used here, _then_ it would make sense to have this be part of the
> expectations. And then, by recording it that way, you also raise the
> barrier of someone changing the behavior - after all the testcase then
> says it is intended to be that way (rather than, as I assume here, it
> being merely "the way it is").
>
> And yes, if you look at other testcase expectations you will frequently
> find way too strict expectations (often enough people simply take the
> output of the dumping tool, massage it suitably to be regex-es, and be
> done - sometimes with the effect of event recording outright wrong
> expectations, simply because huge output is cumbersome to check for
> correctness). In many cases this has resulted in unnecessarily big code
> churn when extending such testcases, or unhelpful mishmash because of
> people then always adding to the end instead of at a more sensible
> place (e.g. next to related stuff). Hence in particular for a still
> relatively new and tidy port like RISC-V is, it would seem desirable to
> me to learn from mistakes made elsewhere before.
>
> Yet to reiterate - I'm not going to insist, first and foremost because
> binutils, unlike other projects, has overall relatively relaxed
> acceptance criteria for patches.
>
> Jan
>
On 25.11.2022 10:18, Tsukasa OI wrote:
> On 2022/11/25 18:04, Jan Beulich wrote:
>> On 25.11.2022 09:39, Tsukasa OI wrote:
>>> On 2022/11/25 17:15, Jan Beulich wrote:
>>>> On 25.11.2022 03:17, Tsukasa OI wrote:
>>>>> --- a/gas/testsuite/gas/riscv/insn-na.d
>>>>> +++ b/gas/testsuite/gas/riscv/insn-na.d
>>>>> @@ -73,3 +73,11 @@ Disassembly of section .text:
>>>>> [^:]+:[ ]+007f 0000 0000 0000 0000[ ]+[._a-z].*
>>>>> [^:]+:[ ]+0000107f 00000000 00000000[ ]+[._a-z].*
>>>>> [^:]+:[ ]+607f 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000[ ]+[._a-z].*
>>>>> +[^:]+:[ ]+007f 0000 0000 0000 8000[ ]+\.byte[ ]+0x7f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80
>>>>> +[^:]+:[ ]+007f 0000 0000 0000 8000[ ]+\.byte[ ]+0x7f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80
>>>>> +[^:]+:[ ]+607f 89ab 4567 0123 3210 7654 ba98 fedc 0000 0000 0000[ ]+\.byte[ ]+0x7f, 0x60, 0xab, 0x89, 0x67, 0x45, 0x23, 0x01, 0x10, 0x32, 0x54, 0x76, 0x98, 0xba, 0xdc, 0xfe, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>>>>
>>>> I have to admit that I still don't see what good the ".byte ..." part of
>>>> the expectations does for the purpose of the test. In the cover letter
>>>> you say "They are not 4-byte aligned (10 and 22-bytes) and unlikely to
>>>> change any time soon." But changing that is exactly my plan (unless
>>>> objected to by the arch maintainers): Showing insn components as bytes
>>>> is imo reasonable for RISC-V at most when things aren't even 2-byte
>>>> aligned. IOW I'd see these to be "disassembled" to ".2byte ...",
>>>> matching the "raw opcode" output left to .<N>byte. In fact when raw
>>>> opcodes are output I question the need for any .<N>byte - it's fully
>>>> redundant>
>>>> Bottom line: As before I'd prefer if these parts were dropped (to limit
>>>> the churn on the files when changing the .<N>byte granularity), but I'm
>>>> not going to insist. Apart from this the change looks good to me.
>>>
>>> Okay, I have to admit that I misunderstood your intent.
>>>
>>> Quoting my PATCH v1 cover letter:
>>>
>>>> [Disassembler: Instruction is trimmed with 64-bits]
>>>>
>>>> In this section, we reuse the object file generated by the section above
>>>> (two 22-byte instructions).
>>>>
>>>> 0000000000000000 <.text>:
>>>> 0: 607f 0000 0000 0000 .byte 0x7f, 0x60, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>>>> 8: 0000 0000 0000 0000
>>>> 10: 0000 0000 0000
>>>> 16: 607f 33cc 55aa cdef .byte 0x7f, 0x60, 0xcc, 0x33, 0xaa, 0x55, 0xef, 0xcd, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>>>> 1e: 89ab 4567 0123 3210 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>> 26: 7654 ba98 fedc zeroed out after first 64-bits
>>>>
>>>> See ".byte" at the address 0x16. It's trimmed at 64-bits.
>>>> The resolution is simple. If we simply add a char* argument (containing all
>>>> instruction bits), we can easily resolve this.
>>>>
>>>> 0000000000000000 <.text>:
>>>> 0: 607f 0000 0000 0000 .byte 0x7f, 0x60, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>>>> 8: 0000 0000 0000 0000
>>>> 10: 0000 0000 0000
>>>> 16: 607f 33cc 55aa cdef .byte 0x7f, 0x60, 0xcc, 0x33, 0xaa, 0x55, 0xef, 0xcd, 0xab, 0x89, 0x67, 0x45, 0x23, 0x01, 0x10, 0x32, 0x54, 0x76, 0x98, 0xba, 0xdc, 0xfe
>>>> 1e: 89ab 4567 0123 3210 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>> 26: 7654 ba98 fedc all instruction bits are correct
>>>
>>> At least, I want to test whether disassembly of this part (after first
>>> 64-bits of an instruction) is fixed. That is exactly what's fixed in
>>> PATCH 1/2 and I want to make sure that this part is changed correctly.
>>
>> Which you already achieve by the raw opcode output
>>
>> 607f 89ab 4567 0123 3210 7654 ba98 fedc 0000 0000 0000
>>
>> The subsequent
>>
>> .byte[ ]+0x7f, 0x60, 0xab, 0x89, 0x67, 0x45, 0x23, 0x01, 0x10, 0x32, 0x54, 0x76, 0x98, 0xba, 0xdc, 0xfe, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>>
>> does not check anything the first part didn't already check.
>
> That's simply not true.
>
> Again, quoting from PATCH v1 cover letter (BEFORE THE PATCH
> [disassembler part]):
>
>> 16: 607f 33cc 55aa cdef .byte 0x7f, 0x60, 0xcc, 0x33, 0xaa, 0x55, 0xef, 0xcd, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>> 1e: 89ab 4567 0123 3210 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> 26: 7654 ba98 fedc zeroed out after first 64-bits
>
> Compare hexdump and printed bytes (in ".byte"). You can see that "1e:
> 89ab..." are different from corresponding ".byte" (0x00, 0x00...).
> They are completely separate and PATCH 1/2 only changes ".byte" output.
>
> If ".byte[ ]+0x7f, 0x60..." does not check anything the first part
> didn't already check, the dump would look like this:
>
>> 16: 607f 33cc 55aa cdef .byte 0x7f, 0x60, 0xcc, 0x33, 0xaa, 0x55, 0xef, 0xcd, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>> 1e: 0000 0000 0000 0000 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> 26: 0000 0000 0000 zeroed out after first 64-bits (but matches to hexdump)
>
> If the hexdump and ".byte" output always matches EVEN BEFORE THE FIX,
> that's what I can call "redundant". But in reality, they do not.
> That's why I want to leave a test to make sure that the issue is fixed
> (now hexdump and ".byte" output always matches).
Oh, apologies: Disassembly then was wrong _only_ for the .byte part, but
_not_ for the raw encoding? While indeed (going back) you said so in the
original cover letter, the description of patch 1 doesn't make this
clear. In that case, yes, I agree that the .byte part wants to be part
of the expectations (but patch 1's description also wants adjusting).
Jan
On 2022/11/25 18:56, Jan Beulich wrote:
> On 25.11.2022 10:18, Tsukasa OI wrote:
>> On 2022/11/25 18:04, Jan Beulich wrote:
>>> On 25.11.2022 09:39, Tsukasa OI wrote:
>>>> On 2022/11/25 17:15, Jan Beulich wrote:
>>>>> On 25.11.2022 03:17, Tsukasa OI wrote:
>>>>>> --- a/gas/testsuite/gas/riscv/insn-na.d
>>>>>> +++ b/gas/testsuite/gas/riscv/insn-na.d
>>>>>> @@ -73,3 +73,11 @@ Disassembly of section .text:
>>>>>> [^:]+:[ ]+007f 0000 0000 0000 0000[ ]+[._a-z].*
>>>>>> [^:]+:[ ]+0000107f 00000000 00000000[ ]+[._a-z].*
>>>>>> [^:]+:[ ]+607f 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000[ ]+[._a-z].*
>>>>>> +[^:]+:[ ]+007f 0000 0000 0000 8000[ ]+\.byte[ ]+0x7f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80
>>>>>> +[^:]+:[ ]+007f 0000 0000 0000 8000[ ]+\.byte[ ]+0x7f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80
>>>>>> +[^:]+:[ ]+607f 89ab 4567 0123 3210 7654 ba98 fedc 0000 0000 0000[ ]+\.byte[ ]+0x7f, 0x60, 0xab, 0x89, 0x67, 0x45, 0x23, 0x01, 0x10, 0x32, 0x54, 0x76, 0x98, 0xba, 0xdc, 0xfe, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>>>>>
>>>>> I have to admit that I still don't see what good the ".byte ..." part of
>>>>> the expectations does for the purpose of the test. In the cover letter
>>>>> you say "They are not 4-byte aligned (10 and 22-bytes) and unlikely to
>>>>> change any time soon." But changing that is exactly my plan (unless
>>>>> objected to by the arch maintainers): Showing insn components as bytes
>>>>> is imo reasonable for RISC-V at most when things aren't even 2-byte
>>>>> aligned. IOW I'd see these to be "disassembled" to ".2byte ...",
>>>>> matching the "raw opcode" output left to .<N>byte. In fact when raw
>>>>> opcodes are output I question the need for any .<N>byte - it's fully
>>>>> redundant>
>>>>> Bottom line: As before I'd prefer if these parts were dropped (to limit
>>>>> the churn on the files when changing the .<N>byte granularity), but I'm
>>>>> not going to insist. Apart from this the change looks good to me.
>>>>
>>>> Okay, I have to admit that I misunderstood your intent.
>>>>
>>>> Quoting my PATCH v1 cover letter:
>>>>
>>>>> [Disassembler: Instruction is trimmed with 64-bits]
>>>>>
>>>>> In this section, we reuse the object file generated by the section above
>>>>> (two 22-byte instructions).
>>>>>
>>>>> 0000000000000000 <.text>:
>>>>> 0: 607f 0000 0000 0000 .byte 0x7f, 0x60, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>>>>> 8: 0000 0000 0000 0000
>>>>> 10: 0000 0000 0000
>>>>> 16: 607f 33cc 55aa cdef .byte 0x7f, 0x60, 0xcc, 0x33, 0xaa, 0x55, 0xef, 0xcd, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>>>>> 1e: 89ab 4567 0123 3210 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>>> 26: 7654 ba98 fedc zeroed out after first 64-bits
>>>>>
>>>>> See ".byte" at the address 0x16. It's trimmed at 64-bits.
>>>>> The resolution is simple. If we simply add a char* argument (containing all
>>>>> instruction bits), we can easily resolve this.
>>>>>
>>>>> 0000000000000000 <.text>:
>>>>> 0: 607f 0000 0000 0000 .byte 0x7f, 0x60, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>>>>> 8: 0000 0000 0000 0000
>>>>> 10: 0000 0000 0000
>>>>> 16: 607f 33cc 55aa cdef .byte 0x7f, 0x60, 0xcc, 0x33, 0xaa, 0x55, 0xef, 0xcd, 0xab, 0x89, 0x67, 0x45, 0x23, 0x01, 0x10, 0x32, 0x54, 0x76, 0x98, 0xba, 0xdc, 0xfe
>>>>> 1e: 89ab 4567 0123 3210 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>>> 26: 7654 ba98 fedc all instruction bits are correct
>>>>
>>>> At least, I want to test whether disassembly of this part (after first
>>>> 64-bits of an instruction) is fixed. That is exactly what's fixed in
>>>> PATCH 1/2 and I want to make sure that this part is changed correctly.
>>>
>>> Which you already achieve by the raw opcode output
>>>
>>> 607f 89ab 4567 0123 3210 7654 ba98 fedc 0000 0000 0000
>>>
>>> The subsequent
>>>
>>> .byte[ ]+0x7f, 0x60, 0xab, 0x89, 0x67, 0x45, 0x23, 0x01, 0x10, 0x32, 0x54, 0x76, 0x98, 0xba, 0xdc, 0xfe, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>>>
>>> does not check anything the first part didn't already check.
>>
>> That's simply not true.
>>
>> Again, quoting from PATCH v1 cover letter (BEFORE THE PATCH
>> [disassembler part]):
>>
>>> 16: 607f 33cc 55aa cdef .byte 0x7f, 0x60, 0xcc, 0x33, 0xaa, 0x55, 0xef, 0xcd, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>>> 1e: 89ab 4567 0123 3210 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>> 26: 7654 ba98 fedc zeroed out after first 64-bits
>>
>> Compare hexdump and printed bytes (in ".byte"). You can see that "1e:
>> 89ab..." are different from corresponding ".byte" (0x00, 0x00...).
>> They are completely separate and PATCH 1/2 only changes ".byte" output.
>>
>> If ".byte[ ]+0x7f, 0x60..." does not check anything the first part
>> didn't already check, the dump would look like this:
>>
>>> 16: 607f 33cc 55aa cdef .byte 0x7f, 0x60, 0xcc, 0x33, 0xaa, 0x55, 0xef, 0xcd, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>>> 1e: 0000 0000 0000 0000 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>> 26: 0000 0000 0000 zeroed out after first 64-bits (but matches to hexdump)
>>
>> If the hexdump and ".byte" output always matches EVEN BEFORE THE FIX,
>> that's what I can call "redundant". But in reality, they do not.
>> That's why I want to leave a test to make sure that the issue is fixed
>> (now hexdump and ".byte" output always matches).
>
> Oh, apologies: Disassembly then was wrong _only_ for the .byte part, but
> _not_ for the raw encoding? While indeed (going back) you said so in the
> original cover letter, the description of patch 1 doesn't make this
> clear. In that case, yes, I agree that the .byte part wants to be part
> of the expectations (but patch 1's description also wants adjusting).
>
> Jan
>
I'm so happy to resolve misunderstandings between us and I sincerely
apologize for misleading text. Hmm... splitting to three patches
(instead of two) might be better on PATCH v4.
1. Disassembler Fix
(clarify exactly what is going to be fixed: ".byte" output)
2. Assembler Fix
3. New testcases
(clarify that it tests both disassembler and assembler fixes)
PATCH v4 may not be submitted today but I will work on it.
Thanks,
Tsukasa
@@ -45,6 +45,9 @@ struct riscv_cl_insn
/* The encoded instruction bits. */
insn_t insn_opcode;
+ /* The long encoded instruction bits ([0] is non-zero if used). */
+ char insn_long_opcode[RISCV_MAX_INSN_LEN];
+
/* The frag that contains the instruction. */
struct frag *frag;
@@ -714,6 +717,7 @@ create_insn (struct riscv_cl_insn *insn, const struct riscv_opcode *mo)
{
insn->insn_mo = mo;
insn->insn_opcode = mo->match;
+ insn->insn_long_opcode[0] = 0; /* Long insn has non-zero value. */
insn->frag = NULL;
insn->where = 0;
insn->fixp = NULL;
@@ -725,7 +729,10 @@ static void
install_insn (const struct riscv_cl_insn *insn)
{
char *f = insn->frag->fr_literal + insn->where;
- number_to_chars_littleendian (f, insn->insn_opcode, insn_length (insn));
+ if (insn->insn_long_opcode[0] != 0)
+ memcpy (f, insn->insn_long_opcode, insn_length (insn));
+ else
+ number_to_chars_littleendian (f, insn->insn_opcode, insn_length (insn));
}
/* Move INSN to offset WHERE in FRAG. Adjust the fixups accordingly
@@ -3481,7 +3488,9 @@ riscv_ip_hardcode (char *str,
values[num++] = (insn_t) imm_expr->X_add_number;
break;
case O_big:
- values[num++] = generic_bignum[0];
+ /* Extract lower 32-bits of a big number.
+ Assume that generic_bignum_to_int32 work on such number. */
+ values[num++] = (insn_t) generic_bignum_to_int32 ();
break;
default:
/* The first value isn't constant, so it should be
@@ -3508,12 +3517,25 @@ riscv_ip_hardcode (char *str,
if (imm_expr->X_op == O_big)
{
- if (bytes != imm_expr->X_add_number * CHARS_PER_LITTLENUM)
+ unsigned int llen = 0;
+ for (LITTLENUM_TYPE lval = generic_bignum[imm_expr->X_add_number - 1];
+ lval != 0; llen++)
+ lval >>= BITS_PER_CHAR;
+ unsigned int repr_bytes
+ = (imm_expr->X_add_number - 1) * CHARS_PER_LITTLENUM + llen;
+ if (bytes < repr_bytes)
return _("value conflicts with instruction length");
- char *f = frag_more (bytes);
- for (num = 0; num < imm_expr->X_add_number; ++num)
- number_to_chars_littleendian (f + num * CHARS_PER_LITTLENUM,
- generic_bignum[num], CHARS_PER_LITTLENUM);
+ for (num = 0; num < imm_expr->X_add_number - 1; ++num)
+ number_to_chars_littleendian (
+ ip->insn_long_opcode + num * CHARS_PER_LITTLENUM,
+ generic_bignum[num],
+ CHARS_PER_LITTLENUM);
+ if (llen != 0)
+ number_to_chars_littleendian (
+ ip->insn_long_opcode + num * CHARS_PER_LITTLENUM,
+ generic_bignum[num],
+ llen);
+ memset(ip->insn_long_opcode + repr_bytes, 0, bytes - repr_bytes);
return NULL;
}
@@ -4590,7 +4612,7 @@ s_riscv_insn (int x ATTRIBUTE_UNUSED)
else
as_bad ("%s `%s'", error.msg, error.statement);
}
- else if (imm_expr.X_op != O_big)
+ else
{
gas_assert (insn.insn_mo->pinfo != INSN_MACRO);
append_insn (&insn, &imm_expr, imm_reloc);
@@ -74,5 +74,13 @@ insn.s +69 +0xf6.*
insn.s +70 +0xfe.*
insn.s +71 +0x108.*
insn.s +72 +0x114.*
-insn.s +- +0x12a
+insn.s +74 +0x12a.*
+insn.s +75 +0x134.*
+insn.s +76 +0x13e.*
+insn.s +77 +0x154.*
+insn.s +78 +0x16a.*
+insn.s +79 +0x180.*
+insn.s +80 +0x196.*
+insn.s +81 +0x1ac.*
+insn.s +- +0x1c2
#pass
@@ -73,3 +73,11 @@ Disassembly of section .text:
[^:]+:[ ]+007f 0000 0000 0000 0000[ ]+[._a-z].*
[^:]+:[ ]+0000107f 00000000 00000000[ ]+[._a-z].*
[^:]+:[ ]+607f 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000[ ]+[._a-z].*
+[^:]+:[ ]+007f 0000 0000 0000 8000[ ]+\.byte[ ]+0x7f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80
+[^:]+:[ ]+007f 0000 0000 0000 8000[ ]+\.byte[ ]+0x7f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80
+[^:]+:[ ]+607f 89ab 4567 0123 3210 7654 ba98 fedc 0000 0000 0000[ ]+\.byte[ ]+0x7f, 0x60, 0xab, 0x89, 0x67, 0x45, 0x23, 0x01, 0x10, 0x32, 0x54, 0x76, 0x98, 0xba, 0xdc, 0xfe, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
+[^:]+:[ ]+607f 89ab 4567 0123 3210 7654 ba98 fedc 0000 0000 0000[ ]+\.byte[ ]+0x7f, 0x60, 0xab, 0x89, 0x67, 0x45, 0x23, 0x01, 0x10, 0x32, 0x54, 0x76, 0x98, 0xba, 0xdc, 0xfe, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
+[^:]+:[ ]+607f 33cc 55aa cdef 89ab 4567 0123 3210 7654 ba98 00dc[ ]+\.byte[ ]+0x7f, 0x60, 0xcc, 0x33, 0xaa, 0x55, 0xef, 0xcd, 0xab, 0x89, 0x67, 0x45, 0x23, 0x01, 0x10, 0x32, 0x54, 0x76, 0x98, 0xba, 0xdc, 0x00
+[^:]+:[ ]+607f 33cc 55aa cdef 89ab 4567 0123 3210 7654 ba98 00dc[ ]+\.byte[ ]+0x7f, 0x60, 0xcc, 0x33, 0xaa, 0x55, 0xef, 0xcd, 0xab, 0x89, 0x67, 0x45, 0x23, 0x01, 0x10, 0x32, 0x54, 0x76, 0x98, 0xba, 0xdc, 0x00
+[^:]+:[ ]+607f 33cc 55aa cdef 89ab 4567 0123 3210 7654 ba98 fedc[ ]+\.byte[ ]+0x7f, 0x60, 0xcc, 0x33, 0xaa, 0x55, 0xef, 0xcd, 0xab, 0x89, 0x67, 0x45, 0x23, 0x01, 0x10, 0x32, 0x54, 0x76, 0x98, 0xba, 0xdc, 0xfe
+[^:]+:[ ]+607f 33cc 55aa cdef 89ab 4567 0123 3210 7654 ba98 fedc[ ]+\.byte[ ]+0x7f, 0x60, 0xcc, 0x33, 0xaa, 0x55, 0xef, 0xcd, 0xab, 0x89, 0x67, 0x45, 0x23, 0x01, 0x10, 0x32, 0x54, 0x76, 0x98, 0xba, 0xdc, 0xfe
@@ -92,3 +92,25 @@ Disassembly of section .text:
[^:]+:[ ]+607f 0000 0000 0000[ ]+[._a-z].*
[^:]+:[ ]+0000 0000 0000 0000 ?
[^:]+:[ ]+0000 0000 0000 ?
+[^:]+:[ ]+007f 0000 0000 0000[ ]+\.byte[ ]+0x7f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80
+[^:]+:[ ]+8000 ?
+[^:]+:[ ]+007f 0000 0000 0000[ ]+\.byte[ ]+0x7f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80
+[^:]+:[ ]+8000 ?
+[^:]+:[ ]+607f 89ab 4567 0123[ ]+\.byte[ ]+0x7f, 0x60, 0xab, 0x89, 0x67, 0x45, 0x23, 0x01, 0x10, 0x32, 0x54, 0x76, 0x98, 0xba, 0xdc, 0xfe, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
+[^:]+:[ ]+3210 7654 ba98 fedc ?
+[^:]+:[ ]+0000 0000 0000 ?
+[^:]+:[ ]+607f 89ab 4567 0123[ ]+\.byte[ ]+0x7f, 0x60, 0xab, 0x89, 0x67, 0x45, 0x23, 0x01, 0x10, 0x32, 0x54, 0x76, 0x98, 0xba, 0xdc, 0xfe, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
+[^:]+:[ ]+3210 7654 ba98 fedc ?
+[^:]+:[ ]+0000 0000 0000 ?
+[^:]+:[ ]+607f 33cc 55aa cdef[ ]+\.byte[ ]+0x7f, 0x60, 0xcc, 0x33, 0xaa, 0x55, 0xef, 0xcd, 0xab, 0x89, 0x67, 0x45, 0x23, 0x01, 0x10, 0x32, 0x54, 0x76, 0x98, 0xba, 0xdc, 0x00
+[^:]+:[ ]+89ab 4567 0123 3210 ?
+[^:]+:[ ]+7654 ba98 00dc ?
+[^:]+:[ ]+607f 33cc 55aa cdef[ ]+\.byte[ ]+0x7f, 0x60, 0xcc, 0x33, 0xaa, 0x55, 0xef, 0xcd, 0xab, 0x89, 0x67, 0x45, 0x23, 0x01, 0x10, 0x32, 0x54, 0x76, 0x98, 0xba, 0xdc, 0x00
+[^:]+:[ ]+89ab 4567 0123 3210 ?
+[^:]+:[ ]+7654 ba98 00dc ?
+[^:]+:[ ]+607f 33cc 55aa cdef[ ]+\.byte[ ]+0x7f, 0x60, 0xcc, 0x33, 0xaa, 0x55, 0xef, 0xcd, 0xab, 0x89, 0x67, 0x45, 0x23, 0x01, 0x10, 0x32, 0x54, 0x76, 0x98, 0xba, 0xdc, 0xfe
+[^:]+:[ ]+89ab 4567 0123 3210 ?
+[^:]+:[ ]+7654 ba98 fedc ?
+[^:]+:[ ]+607f 33cc 55aa cdef[ ]+\.byte[ ]+0x7f, 0x60, 0xcc, 0x33, 0xaa, 0x55, 0xef, 0xcd, 0xab, 0x89, 0x67, 0x45, 0x23, 0x01, 0x10, 0x32, 0x54, 0x76, 0x98, 0xba, 0xdc, 0xfe
+[^:]+:[ ]+89ab 4567 0123 3210 ?
+[^:]+:[ ]+7654 ba98 fedc ?
@@ -70,3 +70,12 @@ target:
.insn 10, 0x007f
.insn 12, 0x107f
.insn 22, 0x607f
+
+ .insn 0x8000000000000000007f
+ .insn 10, 0x8000000000000000007f
+ .insn 0x000000000000fedcba98765432100123456789ab607f
+ .insn 22, 0x000000000000fedcba98765432100123456789ab607f
+ .insn 0x00dcba98765432100123456789abcdef55aa33cc607f
+ .insn 22, 0x00dcba98765432100123456789abcdef55aa33cc607f
+ .insn 0xfedcba98765432100123456789abcdef55aa33cc607f
+ .insn 22, 0xfedcba98765432100123456789abcdef55aa33cc607f