Re: [PATCH 1/2] RISC-V: Fix abort when displaying .dword

[Charlie Jenkins <charlie@rivosinc.com>]

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