* [PATCH 0/2] RISC-V: Fix abort when displaying data and add test
@ 2025-02-12 5:08 Charlie Jenkins
2025-02-12 5:08 ` [PATCH 1/2] RISC-V: Fix abort when displaying .dword Charlie Jenkins
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Charlie Jenkins @ 2025-02-12 5:08 UTC (permalink / raw)
To: jiawei, Nelson Chu, Charlie Jenkins, Jan Beulich, Andrew Burgess
Cc: gdb-patches, Binutils
Commit 6a04e8230707 ("RISC-V: Fix display of partial instructions")
changed how objdump displays instructions. This had the unintented
side-effect that it allowed 6 byte instructions to be displayed which
happens when a .dword is attempted to be split into instructions. Add
support for 5, 6, and 7 byte instructions to remedy this
issue.
Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
---
Charlie Jenkins (2):
RISC-V: Fix abort when displaying .dword
RISC-V: Add testcase for 6 byte instruction
gas/testsuite/gas/riscv/dis-partial-insn-dword.d | 12 ++++++++++
gas/testsuite/gas/riscv/dis-partial-insn-dword.s | 2 ++
opcodes/riscv-dis.c | 29 +++++++++++++++++++++++-
3 files changed, 42 insertions(+), 1 deletion(-)
---
base-commit: 815d9a14cbbb3b81843f7566222c87fb22e7255d
change-id: 20250211-fix_gas_abort-6d1e28b4ad46
--
- Charlie
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/2] RISC-V: Fix abort when displaying .dword
2025-02-12 5:08 [PATCH 0/2] RISC-V: Fix abort when displaying data and add test Charlie Jenkins
@ 2025-02-12 5:08 ` Charlie Jenkins
2025-02-12 5:27 ` Jiawei
` (2 more replies)
2025-02-12 5:08 ` [PATCH 2/2] RISC-V: Add testcase for 6 byte instruction Charlie Jenkins
2025-02-12 5:20 ` [PATCH 0/2] RISC-V: Fix abort when displaying data and add test Nelson Chu
2 siblings, 3 replies; 12+ messages in thread
From: Charlie Jenkins @ 2025-02-12 5:08 UTC (permalink / raw)
To: jiawei, Nelson Chu, Charlie Jenkins, Jan Beulich, Andrew Burgess
Cc: gdb-patches, Binutils
In the normal case an instruction won't be split into 5, 6, or 7 byte
sections. However a .dword disassembled with -D can cause an instruction
to split across the 6 byte boundary. 6 byte instructions were not
supported so riscv_disassemble_data() would abort.
Forcing instructions to be at most 4 bytes causes other unintented
side-effects, so instead add entries to the switch statement to handle
instructions that are 5, 6, or 7 bytes.
Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
Fixes: 6a04e8230707 ("RISC-V: Fix display of partial instructions")
---
opcodes/riscv-dis.c | 29 ++++++++++++++++++++++++++++-
1 file changed, 28 insertions(+), 1 deletion(-)
diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index 367004d3341e46a5f72253cd70c7c2941912e84d..21bcbd32c7f226d13f1637ad192d8fe3c52e0d7a 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -1325,6 +1325,33 @@ riscv_disassemble_data (bfd_vma memaddr ATTRIBUTE_UNUSED,
(info->stream, dis_style_immediate, "0x%08lx",
(unsigned long) data);
break;
+ case 5:
+ info->bytes_per_line = 8;
+ (*info->fprintf_styled_func)
+ (info->stream, dis_style_assembler_directive, ".dword");
+ (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t");
+ (*info->fprintf_styled_func)
+ (info->stream, dis_style_immediate, "0x%010lx",
+ (unsigned long) data);
+ break;
+ case 6:
+ info->bytes_per_line = 8;
+ (*info->fprintf_styled_func)
+ (info->stream, dis_style_assembler_directive, ".dword");
+ (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t");
+ (*info->fprintf_styled_func)
+ (info->stream, dis_style_immediate, "0x%012lx",
+ (unsigned long) data);
+ break;
+ case 7:
+ info->bytes_per_line = 8;
+ (*info->fprintf_styled_func)
+ (info->stream, dis_style_assembler_directive, ".dword");
+ (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t");
+ (*info->fprintf_styled_func)
+ (info->stream, dis_style_immediate, "0x%014lx",
+ (unsigned long) data);
+ break;
case 8:
info->bytes_per_line = 8;
(*info->fprintf_styled_func)
@@ -1332,7 +1359,7 @@ riscv_disassemble_data (bfd_vma memaddr ATTRIBUTE_UNUSED,
(*info->fprintf_styled_func) (info->stream, dis_style_text, "\t");
(*info->fprintf_styled_func)
(info->stream, dis_style_immediate, "0x%016llx",
- (unsigned long long) data);
+ (unsigned long) data);
break;
default:
abort ();
--
2.43.0
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/2] RISC-V: Add testcase for 6 byte instruction
2025-02-12 5:08 [PATCH 0/2] RISC-V: Fix abort when displaying data and add test Charlie Jenkins
2025-02-12 5:08 ` [PATCH 1/2] RISC-V: Fix abort when displaying .dword Charlie Jenkins
@ 2025-02-12 5:08 ` Charlie Jenkins
2025-02-12 5:20 ` [PATCH 0/2] RISC-V: Fix abort when displaying data and add test Nelson Chu
2 siblings, 0 replies; 12+ messages in thread
From: Charlie Jenkins @ 2025-02-12 5:08 UTC (permalink / raw)
To: jiawei, Nelson Chu, Charlie Jenkins, Jan Beulich, Andrew Burgess
Cc: gdb-patches, Binutils
6-byte instructions were causing gas to abort. Add a test to ensure this
is avoided in the future.
Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
---
gas/testsuite/gas/riscv/dis-partial-insn-dword.d | 12 ++++++++++++
gas/testsuite/gas/riscv/dis-partial-insn-dword.s | 2 ++
2 files changed, 14 insertions(+)
diff --git a/gas/testsuite/gas/riscv/dis-partial-insn-dword.d b/gas/testsuite/gas/riscv/dis-partial-insn-dword.d
new file mode 100644
index 0000000000000000000000000000000000000000..79aada549c5a69c8e79a2a0b8009e08a903ff43b
--- /dev/null
+++ b/gas/testsuite/gas/riscv/dis-partial-insn-dword.d
@@ -0,0 +1,12 @@
+#as:
+#source: dis-partial-insn-dword.s
+#objdump: -D -j .text
+
+.*:[ ]+file format .*
+
+
+Disassembly of section .text:
+
+0+000 <target>:
+[ ]+0:[ ]+78f9[ ]+.insn[ ]+2, 0x78f9
+[ ]+2:[ ]+2f2f313421ff[ ]+.dword[ ]+0x2f2f313421ff
diff --git a/gas/testsuite/gas/riscv/dis-partial-insn-dword.s b/gas/testsuite/gas/riscv/dis-partial-insn-dword.s
new file mode 100644
index 0000000000000000000000000000000000000000..8aa94b1a8cbd57fb00bb4197697e7a8bb94a8bcb
--- /dev/null
+++ b/gas/testsuite/gas/riscv/dis-partial-insn-dword.s
@@ -0,0 +1,2 @@
+target:
+ .dword 0x2f2f313421ff78f9
--
2.43.0
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] RISC-V: Fix abort when displaying data and add test
2025-02-12 5:08 [PATCH 0/2] RISC-V: Fix abort when displaying data and add test Charlie Jenkins
2025-02-12 5:08 ` [PATCH 1/2] RISC-V: Fix abort when displaying .dword Charlie Jenkins
2025-02-12 5:08 ` [PATCH 2/2] RISC-V: Add testcase for 6 byte instruction Charlie Jenkins
@ 2025-02-12 5:20 ` Nelson Chu
2 siblings, 0 replies; 12+ messages in thread
From: Nelson Chu @ 2025-02-12 5:20 UTC (permalink / raw)
To: Charlie Jenkins, Nick Clifton, Alan Modra
Cc: jiawei, Jan Beulich, Andrew Burgess, gdb-patches, Binutils
[-- Attachment #1: Type: text/plain, Size: 1561 bytes --]
I think this resolved the segfault when dumping partial data which size
between 4 and 8. Before this series, we used to dump "out of bounds
address", so after adding this last piece of the puzzle will be perfect for
now. The only thing I can imagine is that we probably need to handle the
dump_size between 8 and 16 if supporting rv128, but we can handle that
later at that time.
Hi Nick, Hi Alan, Hi Jan,
I think we should also backport this to 2.44 branch, is that good?
Thanks a lot
Nelson
On Wed, Feb 12, 2025 at 1:08 PM Charlie Jenkins <charlie@rivosinc.com>
wrote:
> Commit 6a04e8230707 ("RISC-V: Fix display of partial instructions")
> changed how objdump displays instructions. This had the unintented
> side-effect that it allowed 6 byte instructions to be displayed which
> happens when a .dword is attempted to be split into instructions. Add
> support for 5, 6, and 7 byte instructions to remedy this
> issue.
>
> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> ---
> Charlie Jenkins (2):
> RISC-V: Fix abort when displaying .dword
> RISC-V: Add testcase for 6 byte instruction
>
> gas/testsuite/gas/riscv/dis-partial-insn-dword.d | 12 ++++++++++
> gas/testsuite/gas/riscv/dis-partial-insn-dword.s | 2 ++
> opcodes/riscv-dis.c | 29
> +++++++++++++++++++++++-
> 3 files changed, 42 insertions(+), 1 deletion(-)
> ---
> base-commit: 815d9a14cbbb3b81843f7566222c87fb22e7255d
> change-id: 20250211-fix_gas_abort-6d1e28b4ad46
> --
> - Charlie
>
>
[-- Attachment #2: Type: text/html, Size: 2098 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] RISC-V: Fix abort when displaying .dword
2025-02-12 5:08 ` [PATCH 1/2] RISC-V: Fix abort when displaying .dword Charlie Jenkins
@ 2025-02-12 5:27 ` Jiawei
2025-02-12 7:25 ` Alan Modra
2025-02-12 8:06 ` Jan Beulich
2 siblings, 0 replies; 12+ messages in thread
From: Jiawei @ 2025-02-12 5:27 UTC (permalink / raw)
To: Charlie Jenkins, Nelson Chu, Jan Beulich, Andrew Burgess
Cc: gdb-patches, Binutils
在 2025/2/12 13:08, Charlie Jenkins 写道:
> In the normal case an instruction won't be split into 5, 6, or 7 byte
> sections. However a .dword disassembled with -D can cause an instruction
> to split across the 6 byte boundary. 6 byte instructions were not
> supported so riscv_disassemble_data() would abort.
>
> Forcing instructions to be at most 4 bytes causes other unintented
> side-effects, so instead add entries to the switch statement to handle
> instructions that are 5, 6, or 7 bytes.
>
> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> Fixes: 6a04e8230707 ("RISC-V: Fix display of partial instructions")
> ---
> opcodes/riscv-dis.c | 29 ++++++++++++++++++++++++++++-
> 1 file changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
> index 367004d3341e46a5f72253cd70c7c2941912e84d..21bcbd32c7f226d13f1637ad192d8fe3c52e0d7a 100644
> --- a/opcodes/riscv-dis.c
> +++ b/opcodes/riscv-dis.c
> @@ -1325,6 +1325,33 @@ riscv_disassemble_data (bfd_vma memaddr ATTRIBUTE_UNUSED,
> (info->stream, dis_style_immediate, "0x%08lx",
> (unsigned long) data);
> break;
> + case 5:
> + info->bytes_per_line = 8;
> + (*info->fprintf_styled_func)
> + (info->stream, dis_style_assembler_directive, ".dword");
> + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t");
> + (*info->fprintf_styled_func)
> + (info->stream, dis_style_immediate, "0x%010lx",
> + (unsigned long) data);
> + break;
> + case 6:
> + info->bytes_per_line = 8;
> + (*info->fprintf_styled_func)
> + (info->stream, dis_style_assembler_directive, ".dword");
> + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t");
> + (*info->fprintf_styled_func)
> + (info->stream, dis_style_immediate, "0x%012lx",
> + (unsigned long) data);
> + break;
> + case 7:
> + info->bytes_per_line = 8;
> + (*info->fprintf_styled_func)
> + (info->stream, dis_style_assembler_directive, ".dword");
> + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t");
> + (*info->fprintf_styled_func)
> + (info->stream, dis_style_immediate, "0x%014lx",
> + (unsigned long) data);
> + break;
> case 8:
> info->bytes_per_line = 8;
> (*info->fprintf_styled_func)
> @@ -1332,7 +1359,7 @@ riscv_disassemble_data (bfd_vma memaddr ATTRIBUTE_UNUSED,
> (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t");
> (*info->fprintf_styled_func)
> (info->stream, dis_style_immediate, "0x%016llx",
> - (unsigned long long) data);
> + (unsigned long) data);
> break;
> default:
> abort ();
LGTM, thanks.
Jiawei
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] RISC-V: Fix abort when displaying .dword
2025-02-12 5:08 ` [PATCH 1/2] RISC-V: Fix abort when displaying .dword Charlie Jenkins
2025-02-12 5:27 ` Jiawei
@ 2025-02-12 7:25 ` Alan Modra
2025-02-12 8:06 ` Jan Beulich
2 siblings, 0 replies; 12+ messages in thread
From: Alan Modra @ 2025-02-12 7:25 UTC (permalink / raw)
To: Charlie Jenkins
Cc: jiawei, Nelson Chu, Jan Beulich, Andrew Burgess, gdb-patches, Binutils
On Tue, Feb 11, 2025 at 09:08:30PM -0800, Charlie Jenkins wrote:
> In the normal case an instruction won't be split into 5, 6, or 7 byte
> sections. However a .dword disassembled with -D can cause an instruction
> to split across the 6 byte boundary. 6 byte instructions were not
> supported so riscv_disassemble_data() would abort.
I think it is worse than just needing to cope with 5, 6, or 7 bytes.
riscv-dis.c:1444 sets dump_size from riscv_insn_length which
looks like it can be up to 22 bytes. A carefully constructed testcase
that gets this value from riscv_insn_length but then has only 20
bytes in the section will result in riscv_disassemble_data being
called with bytes_per_chunk of 20.
cat > xxx.s <<EOF
.byte 0x7f,0xef,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20
EOF
gas/as-new -o xxx.o xxx.s
binutils/objcopy --strip-symbol '$d' xxx.o xx.o
binutils/objdump -d xx.o
--
Alan Modra
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] RISC-V: Fix abort when displaying .dword
2025-02-12 5:08 ` [PATCH 1/2] RISC-V: Fix abort when displaying .dword Charlie Jenkins
2025-02-12 5:27 ` Jiawei
2025-02-12 7:25 ` Alan Modra
@ 2025-02-12 8:06 ` Jan Beulich
2025-02-12 8:57 ` Nelson Chu
2025-02-13 0:52 ` Charlie Jenkins
2 siblings, 2 replies; 12+ messages in thread
From: Jan Beulich @ 2025-02-12 8:06 UTC (permalink / raw)
To: Charlie Jenkins; +Cc: gdb-patches, Binutils, jiawei, Nelson Chu, Andrew Burgess
On 12.02.2025 06:08, Charlie Jenkins wrote:
> In the normal case an instruction won't be split into 5, 6, or 7 byte
> sections. However a .dword disassembled with -D can cause an instruction
> to split across the 6 byte boundary. 6 byte instructions were not
> supported so riscv_disassemble_data() would abort.
>
> Forcing instructions to be at most 4 bytes causes other unintented
> side-effects, so instead add entries to the switch statement to handle
> instructions that are 5, 6, or 7 bytes.
>
> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> Fixes: 6a04e8230707 ("RISC-V: Fix display of partial instructions")
> ---
> opcodes/riscv-dis.c | 29 ++++++++++++++++++++++++++++-
> 1 file changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
> index 367004d3341e46a5f72253cd70c7c2941912e84d..21bcbd32c7f226d13f1637ad192d8fe3c52e0d7a 100644
> --- a/opcodes/riscv-dis.c
> +++ b/opcodes/riscv-dis.c
> @@ -1325,6 +1325,33 @@ riscv_disassemble_data (bfd_vma memaddr ATTRIBUTE_UNUSED,
> (info->stream, dis_style_immediate, "0x%08lx",
> (unsigned long) data);
> break;
> + case 5:
> + info->bytes_per_line = 8;
> + (*info->fprintf_styled_func)
> + (info->stream, dis_style_assembler_directive, ".dword");
> + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t");
> + (*info->fprintf_styled_func)
> + (info->stream, dis_style_immediate, "0x%010lx",
> + (unsigned long) data);
This isn't going to have the intended effect on a 32-bit host.
> + break;
> + case 6:
> + info->bytes_per_line = 8;
> + (*info->fprintf_styled_func)
> + (info->stream, dis_style_assembler_directive, ".dword");
> + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t");
> + (*info->fprintf_styled_func)
> + (info->stream, dis_style_immediate, "0x%012lx",
> + (unsigned long) data);
> + break;
> + case 7:
> + info->bytes_per_line = 8;
> + (*info->fprintf_styled_func)
> + (info->stream, dis_style_assembler_directive, ".dword");
> + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t");
> + (*info->fprintf_styled_func)
> + (info->stream, dis_style_immediate, "0x%014lx",
> + (unsigned long) data);
> + break;
While this follows what's done in the 3-byte case, I don't consider this
(or the 3-byte logic) correct. When I see .dword, I expect what's printed
covers full 8 bytes. Imo fake .<N>byte directives (which the assembler
doesn't recognize for non-power-of-2 N) would be more logical to use.
Also, question to the maintainers: Can we perhaps stop this unnecessary
decoration of indirect function calls? E.g. (taking part of the above)
case 5:
info->bytes_per_line = 8;
info->fprintf_styled_func
(info->stream, dis_style_assembler_directive, ".dword");
info->fprintf_styled_func (info->stream, dis_style_text, "\t");
info->fprintf_styled_func
(info->stream, dis_style_immediate, "0x%010lx",
(unsigned long) data);
break;
has been perfectly fine to write for several decades, and is imo quite
a bit easier to read.
> @@ -1332,7 +1359,7 @@ riscv_disassemble_data (bfd_vma memaddr ATTRIBUTE_UNUSED,
> (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t");
> (*info->fprintf_styled_func)
> (info->stream, dis_style_immediate, "0x%016llx",
> - (unsigned long long) data);
> + (unsigned long) data);
This can't be correct; the compiler will complain about format specifier
disagreeing with type of the value to format, on 32-bit hosts.
Jan
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] RISC-V: Fix abort when displaying .dword
2025-02-12 8:06 ` Jan Beulich
@ 2025-02-12 8:57 ` Nelson Chu
2025-02-12 10:06 ` Jan Beulich
2025-02-13 0:52 ` Charlie Jenkins
1 sibling, 1 reply; 12+ messages in thread
From: Nelson Chu @ 2025-02-12 8:57 UTC (permalink / raw)
To: Jan Beulich
Cc: Charlie Jenkins, gdb-patches, Binutils, jiawei, Andrew Burgess
[-- Attachment #1: Type: text/plain, Size: 2691 bytes --]
On Wed, Feb 12, 2025 at 4:06 PM Jan Beulich <jbeulich@suse.com> wrote:
> While this follows what's done in the 3-byte case, I don't consider this
> (or the 3-byte logic) correct. When I see .dword, I expect what's printed
> covers full 8 bytes. Imo fake .<N>byte directives (which the assembler
> doesn't recognize for non-power-of-2 N) would be more logical to use.
>
> Also, question to the maintainers: Can we perhaps stop this unnecessary
> decoration of indirect function calls? E.g. (taking part of the above)
>
> case 5:
> info->bytes_per_line = 8;
> info->fprintf_styled_func
> (info->stream, dis_style_assembler_directive, ".dword");
> info->fprintf_styled_func (info->stream, dis_style_text, "\t");
> info->fprintf_styled_func
> (info->stream, dis_style_immediate, "0x%010lx",
> (unsigned long) data);
> break;
>
> has been perfectly fine to write for several decades, and is imo quite
> a bit easier to read.
>
Well if I see .word, I also expect to print the full 4 bytes, so probably
just remove the case 3, and then make sure that we only have 1/2/4/8 for
data, which means .byte/.short/.word/.dword. That is -
opcodes/riscv-dis.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index d67b2c2aaf0..fc774760f8c 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -1308,14 +1308,6 @@ riscv_disassemble_data (bfd_vma memaddr
ATTRIBUTE_UNUSED,
(*info->fprintf_styled_func)
(info->stream, dis_style_immediate, "0x%04x", (unsigned) data);
break;
- case 3:
- info->bytes_per_line = 7;
- (*info->fprintf_styled_func)
- (info->stream, dis_style_assembler_directive, ".word");
- (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t");
- (*info->fprintf_styled_func)
- (info->stream, dis_style_immediate, "0x%06x", (unsigned) data);
- break;
case 4:
info->bytes_per_line = 8;
(*info->fprintf_styled_func)
@@ -1497,6 +1489,10 @@ print_insn_riscv (bfd_vma memaddr, struct
disassemble_info *info)
}
else if (bytes_fetched != dump_size)
{
+ bytes_fetched = bytes_fetched >= 8
+ ? 8 : bytes_fetched >= 4
+ ? 4 : bytes_fetched >= 2
+ ? 2 : bytes_fetched;
dump_size = bytes_fetched;
info->bytes_per_chunk = dump_size;
riscv_disassembler = riscv_disassemble_data;
This may make stuff easier, and the logic is similar to riscv_data_length
when the length is 3.
Nelson
[-- Attachment #2: Type: text/html, Size: 3467 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] RISC-V: Fix abort when displaying .dword
2025-02-12 8:57 ` Nelson Chu
@ 2025-02-12 10:06 ` Jan Beulich
2025-02-12 22:15 ` Maciej W. Rozycki
0 siblings, 1 reply; 12+ messages in thread
From: Jan Beulich @ 2025-02-12 10:06 UTC (permalink / raw)
To: Nelson Chu; +Cc: Charlie Jenkins, gdb-patches, Binutils, jiawei, Andrew Burgess
On 12.02.2025 09:57, Nelson Chu wrote:
> On Wed, Feb 12, 2025 at 4:06 PM Jan Beulich <jbeulich@suse.com> wrote:
>
>> While this follows what's done in the 3-byte case, I don't consider this
>> (or the 3-byte logic) correct. When I see .dword, I expect what's printed
>> covers full 8 bytes. Imo fake .<N>byte directives (which the assembler
>> doesn't recognize for non-power-of-2 N) would be more logical to use.
>
> Well if I see .word, I also expect to print the full 4 bytes, so probably
> just remove the case 3, and then make sure that we only have 1/2/4/8 for
> data, which means .byte/.short/.word/.dword.
Except that there is no 4th byte to print in that case. That wants expressing
somehow, without taking a value out of thin air.
Jan
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] RISC-V: Fix abort when displaying .dword
2025-02-12 10:06 ` Jan Beulich
@ 2025-02-12 22:15 ` Maciej W. Rozycki
2025-02-13 0:51 ` Charlie Jenkins
0 siblings, 1 reply; 12+ messages in thread
From: Maciej W. Rozycki @ 2025-02-12 22:15 UTC (permalink / raw)
To: Jan Beulich
Cc: Nelson Chu, Charlie Jenkins, gdb-patches, Binutils, jiawei,
Andrew Burgess
On Wed, 12 Feb 2025, Jan Beulich wrote:
> >> While this follows what's done in the 3-byte case, I don't consider this
> >> (or the 3-byte logic) correct. When I see .dword, I expect what's printed
> >> covers full 8 bytes. Imo fake .<N>byte directives (which the assembler
> >> doesn't recognize for non-power-of-2 N) would be more logical to use.
> >
> > Well if I see .word, I also expect to print the full 4 bytes, so probably
> > just remove the case 3, and then make sure that we only have 1/2/4/8 for
> > data, which means .byte/.short/.word/.dword.
>
> Except that there is no 4th byte to print in that case. That wants expressing
> somehow, without taking a value out of thin air.
They could just do what backends for other fixed-length-instruction-word
targets do, such as Alpha or MIPS one:
$ dd if=/dev/zero of=partial-insn.img bs=1 count=7
$ alpha-linux-gnu-objdump -b binary -m alpha -D partial-insn.img
partial-insn.img: file format binary
Disassembly of section .data:
0000000000000000 <.data>:
0: 00 00 00 00 halt
4: Address 0x0000000000000004 is out of bounds.
$ mips-linux-gnu-objdump -b binary -m mips -D partial-insn.img
partial-insn.img: file format binary
Disassembly of section .data:
00000000 <.data>:
0: 00000000 nop
4: Address 0x4 is out of bounds.
$
There are other means available, such as the `-s' option or `readelf', for
dumping trailing partial data that is not a complete instruction and thus
has no meaning. I'm not sure if this is a corner case worth putting much
effort into, beyond just making sure the tools don't crash or produce
utter rubbish.
FWIW,
Maciej
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] RISC-V: Fix abort when displaying .dword
2025-02-12 22:15 ` Maciej W. Rozycki
@ 2025-02-13 0:51 ` Charlie Jenkins
0 siblings, 0 replies; 12+ messages in thread
From: Charlie Jenkins @ 2025-02-13 0:51 UTC (permalink / raw)
To: Maciej W. Rozycki
Cc: Jan Beulich, Nelson Chu, gdb-patches, Binutils, jiawei, Andrew Burgess
On Wed, Feb 12, 2025 at 10:15:31PM +0000, Maciej W. Rozycki wrote:
> On Wed, 12 Feb 2025, Jan Beulich wrote:
>
> > >> While this follows what's done in the 3-byte case, I don't consider this
> > >> (or the 3-byte logic) correct. When I see .dword, I expect what's printed
> > >> covers full 8 bytes. Imo fake .<N>byte directives (which the assembler
> > >> doesn't recognize for non-power-of-2 N) would be more logical to use.
> > >
> > > Well if I see .word, I also expect to print the full 4 bytes, so probably
> > > just remove the case 3, and then make sure that we only have 1/2/4/8 for
> > > data, which means .byte/.short/.word/.dword.
> >
> > Except that there is no 4th byte to print in that case. That wants expressing
> > somehow, without taking a value out of thin air.
>
> They could just do what backends for other fixed-length-instruction-word
> targets do, such as Alpha or MIPS one:
>
> $ dd if=/dev/zero of=partial-insn.img bs=1 count=7
> $ alpha-linux-gnu-objdump -b binary -m alpha -D partial-insn.img
>
> partial-insn.img: file format binary
>
>
> Disassembly of section .data:
>
> 0000000000000000 <.data>:
> 0: 00 00 00 00 halt
> 4: Address 0x0000000000000004 is out of bounds.
>
> $ mips-linux-gnu-objdump -b binary -m mips -D partial-insn.img
>
> partial-insn.img: file format binary
>
>
> Disassembly of section .data:
>
> 00000000 <.data>:
> 0: 00000000 nop
> 4: Address 0x4 is out of bounds.
>
> $
>
> There are other means available, such as the `-s' option or `readelf', for
> dumping trailing partial data that is not a complete instruction and thus
> has no meaning. I'm not sure if this is a corner case worth putting much
> effort into, beyond just making sure the tools don't crash or produce
> utter rubbish.
It came up because there was a Linux test that was using objdump to
check that instructions in memory was equal to the intructions in the
binary. It was using --stop-address to bound a region to look at and
that was sometimes splitting an instruction down the middle. This used
to work fine with binutils 2.41 on riscv, but a change was made that
caused this throw an out of bounds on 2.42. My original change was to
bring the behavior back in line with other architectures and what riscv
used to do. I feel like it's a somewhat useful feature.
- Charlie
>
> FWIW,
>
> Maciej
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] RISC-V: Fix abort when displaying .dword
2025-02-12 8:06 ` Jan Beulich
2025-02-12 8:57 ` Nelson Chu
@ 2025-02-13 0:52 ` Charlie Jenkins
1 sibling, 0 replies; 12+ messages in thread
From: Charlie Jenkins @ 2025-02-13 0:52 UTC (permalink / raw)
To: Jan Beulich; +Cc: gdb-patches, Binutils, jiawei, Nelson Chu, Andrew Burgess
On Wed, Feb 12, 2025 at 09:06:43AM +0100, Jan Beulich wrote:
> On 12.02.2025 06:08, Charlie Jenkins wrote:
> > In the normal case an instruction won't be split into 5, 6, or 7 byte
> > sections. However a .dword disassembled with -D can cause an instruction
> > to split across the 6 byte boundary. 6 byte instructions were not
> > supported so riscv_disassemble_data() would abort.
> >
> > Forcing instructions to be at most 4 bytes causes other unintented
> > side-effects, so instead add entries to the switch statement to handle
> > instructions that are 5, 6, or 7 bytes.
> >
> > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> > Fixes: 6a04e8230707 ("RISC-V: Fix display of partial instructions")
> > ---
> > opcodes/riscv-dis.c | 29 ++++++++++++++++++++++++++++-
> > 1 file changed, 28 insertions(+), 1 deletion(-)
> >
> > diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
> > index 367004d3341e46a5f72253cd70c7c2941912e84d..21bcbd32c7f226d13f1637ad192d8fe3c52e0d7a 100644
> > --- a/opcodes/riscv-dis.c
> > +++ b/opcodes/riscv-dis.c
> > @@ -1325,6 +1325,33 @@ riscv_disassemble_data (bfd_vma memaddr ATTRIBUTE_UNUSED,
> > (info->stream, dis_style_immediate, "0x%08lx",
> > (unsigned long) data);
> > break;
> > + case 5:
> > + info->bytes_per_line = 8;
> > + (*info->fprintf_styled_func)
> > + (info->stream, dis_style_assembler_directive, ".dword");
> > + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t");
> > + (*info->fprintf_styled_func)
> > + (info->stream, dis_style_immediate, "0x%010lx",
> > + (unsigned long) data);
>
> This isn't going to have the intended effect on a 32-bit host.
>
> > + break;
> > + case 6:
> > + info->bytes_per_line = 8;
> > + (*info->fprintf_styled_func)
> > + (info->stream, dis_style_assembler_directive, ".dword");
> > + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t");
> > + (*info->fprintf_styled_func)
> > + (info->stream, dis_style_immediate, "0x%012lx",
> > + (unsigned long) data);
> > + break;
> > + case 7:
> > + info->bytes_per_line = 8;
> > + (*info->fprintf_styled_func)
> > + (info->stream, dis_style_assembler_directive, ".dword");
> > + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t");
> > + (*info->fprintf_styled_func)
> > + (info->stream, dis_style_immediate, "0x%014lx",
> > + (unsigned long) data);
> > + break;
>
> While this follows what's done in the 3-byte case, I don't consider this
> (or the 3-byte logic) correct. When I see .dword, I expect what's printed
> covers full 8 bytes. Imo fake .<N>byte directives (which the assembler
> doesn't recognize for non-power-of-2 N) would be more logical to use.
That's a good point. I think the simplest thing to do would be to have
.short/.word/.dword for the 2/4/8 cases and then for anything outside of
that print .<N>byte. Similar to what is done with insn with printing
.insn <size>, <insn> when it is not identified.
- Charlie
>
> Also, question to the maintainers: Can we perhaps stop this unnecessary
> decoration of indirect function calls? E.g. (taking part of the above)
>
> case 5:
> info->bytes_per_line = 8;
> info->fprintf_styled_func
> (info->stream, dis_style_assembler_directive, ".dword");
> info->fprintf_styled_func (info->stream, dis_style_text, "\t");
> info->fprintf_styled_func
> (info->stream, dis_style_immediate, "0x%010lx",
> (unsigned long) data);
> break;
>
> has been perfectly fine to write for several decades, and is imo quite
> a bit easier to read.
>
> > @@ -1332,7 +1359,7 @@ riscv_disassemble_data (bfd_vma memaddr ATTRIBUTE_UNUSED,
> > (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t");
> > (*info->fprintf_styled_func)
> > (info->stream, dis_style_immediate, "0x%016llx",
> > - (unsigned long long) data);
> > + (unsigned long) data);
>
> This can't be correct; the compiler will complain about format specifier
> disagreeing with type of the value to format, on 32-bit hosts.
>
> Jan
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2025-02-13 0:58 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-02-12 5:08 [PATCH 0/2] RISC-V: Fix abort when displaying data and add test Charlie Jenkins
2025-02-12 5:08 ` [PATCH 1/2] RISC-V: Fix abort when displaying .dword Charlie Jenkins
2025-02-12 5:27 ` Jiawei
2025-02-12 7:25 ` Alan Modra
2025-02-12 8:06 ` Jan Beulich
2025-02-12 8:57 ` Nelson Chu
2025-02-12 10:06 ` Jan Beulich
2025-02-12 22:15 ` Maciej W. Rozycki
2025-02-13 0:51 ` Charlie Jenkins
2025-02-13 0:52 ` Charlie Jenkins
2025-02-12 5:08 ` [PATCH 2/2] RISC-V: Add testcase for 6 byte instruction Charlie Jenkins
2025-02-12 5:20 ` [PATCH 0/2] RISC-V: Fix abort when displaying data and add test Nelson Chu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox