* [PATCH] RISC-V: fix JAL aliases ordering in opcode table
@ 2023-01-04 5:59 Aurelien Jarno
2023-01-04 7:43 ` Nelson Chu
2023-01-04 7:50 ` Jan Beulich
0 siblings, 2 replies; 6+ messages in thread
From: Aurelien Jarno @ 2023-01-04 5:59 UTC (permalink / raw)
To: binutils; +Cc: Jan Beulich, Aurelien Jarno
Commit 839189bc932e ("RISC-V: re-arrange opcode table for consistent
alias handling") reorder the instructions in the opcode tables,
including the various JAL aliases. In particular they are not ordered
anymore from the most specific to the less specific one. This causes the
form "JAL reg, imm" to emit a relocation with the register name. This
breaks various things like building Linux kernel modules.
This patch fixes the issue by restoring the original ordering of the JAL
aliases.
Tested on riscv64-linux
PR gas/29940
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29940
---
opcodes/riscv-opc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/opcodes/riscv-opc.c b/opcodes/riscv-opc.c
index 6b65296a3f2..89c6572519e 100644
--- a/opcodes/riscv-opc.c
+++ b/opcodes/riscv-opc.c
@@ -340,9 +340,9 @@ const struct riscv_opcode riscv_opcodes[] =
{"jalr", 0, INSN_CLASS_I, "d,s,j", MATCH_JALR, MASK_JALR, match_opcode, INSN_JSR },
{"j", 0, INSN_CLASS_C, "Ca", MATCH_C_J, MASK_C_J, match_opcode, INSN_ALIAS|INSN_BRANCH },
{"j", 0, INSN_CLASS_I, "a", MATCH_JAL, MASK_JAL|MASK_RD, match_opcode, INSN_ALIAS|INSN_BRANCH },
-{"jal", 0, INSN_CLASS_I, "a", MATCH_JAL|(X_RA << OP_SH_RD), MASK_JAL|MASK_RD, match_opcode, INSN_ALIAS|INSN_JSR },
{"jal", 0, INSN_CLASS_I, "d,a", MATCH_JAL, MASK_JAL, match_opcode, INSN_JSR },
{"jal", 32, INSN_CLASS_C, "Ca", MATCH_C_JAL, MASK_C_JAL, match_opcode, INSN_ALIAS|INSN_JSR },
+{"jal", 0, INSN_CLASS_I, "a", MATCH_JAL|(X_RA << OP_SH_RD), MASK_JAL|MASK_RD, match_opcode, INSN_ALIAS|INSN_JSR },
{"call", 0, INSN_CLASS_I, "d,c", (X_T1 << OP_SH_RS1), (int) M_CALL, match_never, INSN_MACRO },
{"call", 0, INSN_CLASS_I, "c", (X_RA << OP_SH_RS1)|(X_RA << OP_SH_RD), (int) M_CALL, match_never, INSN_MACRO },
{"tail", 0, INSN_CLASS_I, "c", (X_T1 << OP_SH_RS1), (int) M_CALL, match_never, INSN_MACRO },
--
2.35.1
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] RISC-V: fix JAL aliases ordering in opcode table
2023-01-04 5:59 [PATCH] RISC-V: fix JAL aliases ordering in opcode table Aurelien Jarno
@ 2023-01-04 7:43 ` Nelson Chu
2023-01-04 7:50 ` Jan Beulich
1 sibling, 0 replies; 6+ messages in thread
From: Nelson Chu @ 2023-01-04 7:43 UTC (permalink / raw)
To: Aurelien Jarno; +Cc: binutils, Jan Beulich
This solution is quite simple and clear, so it looks good to me. The
other solution is that - check for the integer register at the start
of the my_getExpression, just like what my_getSmallExpression did
before. Although my_getSmallExpression will also call
my_getExpression, so checking the integer register twice looks
redundant, but at least it is more safe and less error-prone. Or we
can find a way to only check the register once, and make the source
code clear. But since the release is close, I suggest we just accept
this simple solution in the short-term.
Thanks
Nelson
On Wed, Jan 4, 2023 at 2:00 PM Aurelien Jarno <aurelien@aurel32.net> wrote:
>
> Commit 839189bc932e ("RISC-V: re-arrange opcode table for consistent
> alias handling") reorder the instructions in the opcode tables,
> including the various JAL aliases. In particular they are not ordered
> anymore from the most specific to the less specific one. This causes the
> form "JAL reg, imm" to emit a relocation with the register name. This
> breaks various things like building Linux kernel modules.
>
> This patch fixes the issue by restoring the original ordering of the JAL
> aliases.
>
> Tested on riscv64-linux
> PR gas/29940
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29940
> ---
> opcodes/riscv-opc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/opcodes/riscv-opc.c b/opcodes/riscv-opc.c
> index 6b65296a3f2..89c6572519e 100644
> --- a/opcodes/riscv-opc.c
> +++ b/opcodes/riscv-opc.c
> @@ -340,9 +340,9 @@ const struct riscv_opcode riscv_opcodes[] =
> {"jalr", 0, INSN_CLASS_I, "d,s,j", MATCH_JALR, MASK_JALR, match_opcode, INSN_JSR },
> {"j", 0, INSN_CLASS_C, "Ca", MATCH_C_J, MASK_C_J, match_opcode, INSN_ALIAS|INSN_BRANCH },
> {"j", 0, INSN_CLASS_I, "a", MATCH_JAL, MASK_JAL|MASK_RD, match_opcode, INSN_ALIAS|INSN_BRANCH },
> -{"jal", 0, INSN_CLASS_I, "a", MATCH_JAL|(X_RA << OP_SH_RD), MASK_JAL|MASK_RD, match_opcode, INSN_ALIAS|INSN_JSR },
> {"jal", 0, INSN_CLASS_I, "d,a", MATCH_JAL, MASK_JAL, match_opcode, INSN_JSR },
> {"jal", 32, INSN_CLASS_C, "Ca", MATCH_C_JAL, MASK_C_JAL, match_opcode, INSN_ALIAS|INSN_JSR },
> +{"jal", 0, INSN_CLASS_I, "a", MATCH_JAL|(X_RA << OP_SH_RD), MASK_JAL|MASK_RD, match_opcode, INSN_ALIAS|INSN_JSR },
> {"call", 0, INSN_CLASS_I, "d,c", (X_T1 << OP_SH_RS1), (int) M_CALL, match_never, INSN_MACRO },
> {"call", 0, INSN_CLASS_I, "c", (X_RA << OP_SH_RS1)|(X_RA << OP_SH_RD), (int) M_CALL, match_never, INSN_MACRO },
> {"tail", 0, INSN_CLASS_I, "c", (X_T1 << OP_SH_RS1), (int) M_CALL, match_never, INSN_MACRO },
> --
> 2.35.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] RISC-V: fix JAL aliases ordering in opcode table
2023-01-04 5:59 [PATCH] RISC-V: fix JAL aliases ordering in opcode table Aurelien Jarno
2023-01-04 7:43 ` Nelson Chu
@ 2023-01-04 7:50 ` Jan Beulich
2023-01-05 7:56 ` Aurelien Jarno
1 sibling, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2023-01-04 7:50 UTC (permalink / raw)
To: Aurelien Jarno; +Cc: binutils
On 04.01.2023 06:59, Aurelien Jarno wrote:
> Commit 839189bc932e ("RISC-V: re-arrange opcode table for consistent
> alias handling") reorder the instructions in the opcode tables,
> including the various JAL aliases. In particular they are not ordered
> anymore from the most specific to the less specific one. This causes the
> form "JAL reg, imm" to emit a relocation with the register name. This
> breaks various things like building Linux kernel modules.
>
> This patch fixes the issue by restoring the original ordering of the JAL
> aliases.
I'm afraid reverting the "offending" hunk is only a workaround, as it
would re-introduce the disassembler side issue that the patch tried to
deal with (and which sadly looks to not be covered by anything in the
testsuite, or else you would have noticed breakage when testing your
change). Since I expect that in the course of table lookup it is still
the correct entry (in particular one with two rather than just one
operands) which is used, my suspicion is that something somewhere makes
a silent assumption upon the (original) ordering of table entries. It
would be that place which needs fixing then; I'll try to find time soon
to see if I can spot where that is.
Jan
> Tested on riscv64-linux
> PR gas/29940
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29940
> ---
> opcodes/riscv-opc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/opcodes/riscv-opc.c b/opcodes/riscv-opc.c
> index 6b65296a3f2..89c6572519e 100644
> --- a/opcodes/riscv-opc.c
> +++ b/opcodes/riscv-opc.c
> @@ -340,9 +340,9 @@ const struct riscv_opcode riscv_opcodes[] =
> {"jalr", 0, INSN_CLASS_I, "d,s,j", MATCH_JALR, MASK_JALR, match_opcode, INSN_JSR },
> {"j", 0, INSN_CLASS_C, "Ca", MATCH_C_J, MASK_C_J, match_opcode, INSN_ALIAS|INSN_BRANCH },
> {"j", 0, INSN_CLASS_I, "a", MATCH_JAL, MASK_JAL|MASK_RD, match_opcode, INSN_ALIAS|INSN_BRANCH },
> -{"jal", 0, INSN_CLASS_I, "a", MATCH_JAL|(X_RA << OP_SH_RD), MASK_JAL|MASK_RD, match_opcode, INSN_ALIAS|INSN_JSR },
> {"jal", 0, INSN_CLASS_I, "d,a", MATCH_JAL, MASK_JAL, match_opcode, INSN_JSR },
> {"jal", 32, INSN_CLASS_C, "Ca", MATCH_C_JAL, MASK_C_JAL, match_opcode, INSN_ALIAS|INSN_JSR },
> +{"jal", 0, INSN_CLASS_I, "a", MATCH_JAL|(X_RA << OP_SH_RD), MASK_JAL|MASK_RD, match_opcode, INSN_ALIAS|INSN_JSR },
> {"call", 0, INSN_CLASS_I, "d,c", (X_T1 << OP_SH_RS1), (int) M_CALL, match_never, INSN_MACRO },
> {"call", 0, INSN_CLASS_I, "c", (X_RA << OP_SH_RS1)|(X_RA << OP_SH_RD), (int) M_CALL, match_never, INSN_MACRO },
> {"tail", 0, INSN_CLASS_I, "c", (X_T1 << OP_SH_RS1), (int) M_CALL, match_never, INSN_MACRO },
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] RISC-V: fix JAL aliases ordering in opcode table
2023-01-04 7:50 ` Jan Beulich
@ 2023-01-05 7:56 ` Aurelien Jarno
2023-01-05 8:20 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Aurelien Jarno @ 2023-01-05 7:56 UTC (permalink / raw)
To: Jan Beulich; +Cc: binutils
On 2023-01-04 08:50, Jan Beulich via Binutils wrote:
> On 04.01.2023 06:59, Aurelien Jarno wrote:
> > Commit 839189bc932e ("RISC-V: re-arrange opcode table for consistent
> > alias handling") reorder the instructions in the opcode tables,
> > including the various JAL aliases. In particular they are not ordered
> > anymore from the most specific to the less specific one. This causes the
> > form "JAL reg, imm" to emit a relocation with the register name. This
> > breaks various things like building Linux kernel modules.
> >
> > This patch fixes the issue by restoring the original ordering of the JAL
> > aliases.
>
> I'm afraid reverting the "offending" hunk is only a workaround, as it
> would re-introduce the disassembler side issue that the patch tried to
> deal with (and which sadly looks to not be covered by anything in the
> testsuite, or else you would have noticed breakage when testing your
> change). Since I expect that in the course of table lookup it is still
> the correct entry (in particular one with two rather than just one
> operands) which is used, my suspicion is that something somewhere makes
> a silent assumption upon the (original) ordering of table entries. It
> would be that place which needs fixing then; I'll try to find time soon
> to see if I can spot where that is.
Thanks for the analysis. Do you think you have time to get a patch with
that fix before the 2.40 release?
Aurelien
--
Aurelien Jarno GPG: 4096R/1DDD8C9B
aurelien@aurel32.net http://www.aurel32.net
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] RISC-V: fix JAL aliases ordering in opcode table
2023-01-05 7:56 ` Aurelien Jarno
@ 2023-01-05 8:20 ` Jan Beulich
2023-01-08 13:53 ` Maciej W. Rozycki
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2023-01-05 8:20 UTC (permalink / raw)
To: binutils
On 05.01.2023 08:56, Aurelien Jarno wrote:
> On 2023-01-04 08:50, Jan Beulich via Binutils wrote:
>> On 04.01.2023 06:59, Aurelien Jarno wrote:
>>> Commit 839189bc932e ("RISC-V: re-arrange opcode table for consistent
>>> alias handling") reorder the instructions in the opcode tables,
>>> including the various JAL aliases. In particular they are not ordered
>>> anymore from the most specific to the less specific one. This causes the
>>> form "JAL reg, imm" to emit a relocation with the register name. This
>>> breaks various things like building Linux kernel modules.
>>>
>>> This patch fixes the issue by restoring the original ordering of the JAL
>>> aliases.
>>
>> I'm afraid reverting the "offending" hunk is only a workaround, as it
>> would re-introduce the disassembler side issue that the patch tried to
>> deal with (and which sadly looks to not be covered by anything in the
>> testsuite, or else you would have noticed breakage when testing your
>> change). Since I expect that in the course of table lookup it is still
>> the correct entry (in particular one with two rather than just one
>> operands) which is used, my suspicion is that something somewhere makes
>> a silent assumption upon the (original) ordering of table entries. It
>> would be that place which needs fixing then; I'll try to find time soon
>> to see if I can spot where that is.
>
> Thanks for the analysis. Do you think you have time to get a patch with
> that fix before the 2.40 release?
I hope to get to looking into this tomorrow; whether that'll result in a
patch I obviously can't tell yet. In any event Nelson's reply (which
crossed with mine) suggests the reason for the problem to be different.
Yet then still that's where the change wants to be (albeit I disagree
with his proposal, yet that's without having looked at the code at all),
rather than re-introducing the disassembler inconsistency. The latter
would certainly be a short-term option for the impending release, if a
proper solution can't be found in time or turns out too intrusive. But
then the title and description would need adjustment to reflect this
(e.g. it wouldn't be "fix" but maybe "restore" or "undo").
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] RISC-V: fix JAL aliases ordering in opcode table
2023-01-05 8:20 ` Jan Beulich
@ 2023-01-08 13:53 ` Maciej W. Rozycki
0 siblings, 0 replies; 6+ messages in thread
From: Maciej W. Rozycki @ 2023-01-08 13:53 UTC (permalink / raw)
To: Jan Beulich; +Cc: binutils
On Thu, 5 Jan 2023, Jan Beulich via Binutils wrote:
> >> I'm afraid reverting the "offending" hunk is only a workaround, as it
> >> would re-introduce the disassembler side issue that the patch tried to
> >> deal with (and which sadly looks to not be covered by anything in the
> >> testsuite, or else you would have noticed breakage when testing your
> >> change). Since I expect that in the course of table lookup it is still
> >> the correct entry (in particular one with two rather than just one
> >> operands) which is used, my suspicion is that something somewhere makes
> >> a silent assumption upon the (original) ordering of table entries. It
> >> would be that place which needs fixing then; I'll try to find time soon
> >> to see if I can spot where that is.
> >
> > Thanks for the analysis. Do you think you have time to get a patch with
> > that fix before the 2.40 release?
>
> I hope to get to looking into this tomorrow; whether that'll result in a
> patch I obviously can't tell yet. In any event Nelson's reply (which
> crossed with mine) suggests the reason for the problem to be different.
> Yet then still that's where the change wants to be (albeit I disagree
> with his proposal, yet that's without having looked at the code at all),
> rather than re-introducing the disassembler inconsistency. The latter
> would certainly be a short-term option for the impending release, if a
> proper solution can't be found in time or turns out too intrusive. But
> then the title and description would need adjustment to reflect this
> (e.g. it wouldn't be "fix" but maybe "restore" or "undo").
FWIW we have taken a similar approach in the MIPS port, which I believe
is where the RISC-V port took inspiration from, and by ordering the opcode
table carefully we get the desired results; see the various `j*' and other
instruction entries in opcodes/mips-opc.c. It has worked for some 30
years now, so it should be pretty straightforward to get this sorted here
as well.
Maciej
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-01-08 13:53 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-04 5:59 [PATCH] RISC-V: fix JAL aliases ordering in opcode table Aurelien Jarno
2023-01-04 7:43 ` Nelson Chu
2023-01-04 7:50 ` Jan Beulich
2023-01-05 7:56 ` Aurelien Jarno
2023-01-05 8:20 ` Jan Beulich
2023-01-08 13:53 ` Maciej W. Rozycki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).