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

[Jan Beulich <jbeulich@suse.com>]

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