Re: [PATCH] RISC-V: Fix disassembly of partial instructions

[Jiawei <jiawei@iscas.ac.cn>]

在 2024/12/14 4:50, Charlie Jenkins 写道:
> As of commit e43d8768d909 ("RISC-V: Fix disassemble fetch fail return
> value.") partial instructions are no longer disassembled. While that
> commit fixed the behavior of print_insn_riscv() returning the arbitrary
> status value upon failure, it caused the behavior of dumping
> instructions to change. Allow partial instructions to be disassembled
> once again and only return -1 if no part of the instruction was able to
> be disassembled.
>
> Fixes: e43d8768d909 ("RISC-V: Fix disassemble fetch fail return value.")
> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> ---
> When testing linux perf, I noticed that this behavior of objdump has
> changed. Before this patch and running `perf test` on riscv the
> following test fails due to objdump not returning all of the expected
> bytes.
>
> Bytes read differ from those read by objdump
> buf1 (dso):
> 0x97 0xf7 0x11 0x00 0x93 0x87 0xc7 0x7c 0x22 0x85 0x7c 0xec 0xef 0x50 0x80 0x12
> 0xa6 0x85 0xce 0x86 0x4a 0x86 0x22 0x85 0xef 0x50 0x40 0x40 0xa2 0x84 0x1d 0xc9
> 0x7c 0x58 0x85 0x8b 0x85 0xc3 0x1c 0x40 0xa1 0x8b 0x89 0xcf 0x83 0x27 0x04 0x0c
> 0x63 0x51 0xf0 0x04 0x97 0xf7 0x11 0x00 0x93 0x87 0x07 0x45 0xbe 0x86 0x58 0x70
> 0x74 0xec 0x7c 0xf3 0xa2 0x70 0x02 0x74 0x42 0x69 0xa2 0x69 0x26 0x85 0xe2 0x64
> 0x45 0x61 0x82 0x80 0x22 0x85 0xef 0x50 0x50 0x52 0x22 0x85 0xef 0x00 0xb1 0x39
> 0xa2 0x70 0x02 0x74 0x81 0x44 0x42 0x69 0xa2 0x69 0x26 0x85 0xe2 0x64 0x45 0x61
> 0x82 0x80 0x97 0x06 0x12 0x00 0x93 0x86 0xa6 0x8a 0x97 0xf7 0x11 0x00 0x93 0x87
>
> buf2 (objdump):
> 0x97 0xf7 0x11 0x00 0x93 0x87 0xc7 0x7c 0x22 0x85 0x7c 0xec 0xef 0x50 0x80 0x12
> 0xa6 0x85 0xce 0x86 0x4a 0x86 0x22 0x85 0xef 0x50 0x40 0x40 0xa2 0x84 0x1d 0xc9
> 0x7c 0x58 0x85 0x8b 0x85 0xc3 0x1c 0x40 0xa1 0x8b 0x89 0xcf 0x83 0x27 0x04 0x0c
> 0x63 0x51 0xf0 0x04 0x97 0xf7 0x11 0x00 0x93 0x87 0x07 0x45 0xbe 0x86 0x58 0x70
> 0x74 0xec 0x7c 0xf3 0xa2 0x70 0x02 0x74 0x42 0x69 0xa2 0x69 0x26 0x85 0xe2 0x64
> 0x45 0x61 0x82 0x80 0x22 0x85 0xef 0x50 0x50 0x52 0x22 0x85 0xef 0x00 0xb1 0x39
> 0xa2 0x70 0x02 0x74 0x81 0x44 0x42 0x69 0xa2 0x69 0x26 0x85 0xe2 0x64 0x45 0x61
> 0x82 0x80 0x97 0x06 0x12 0x00 0x93 0x86 0xa6 0x8a 0x97 0xf7 0x11 0x00 0xad 0x00
>
> ---- end(-1) ----
>   24: Object code reading                              : FAILED!
>
> After this patch, this test case no longer fails, as objdump returns the
> expected values.
> ---
>   opcodes/riscv-dis.c | 50 +++++++++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 45 insertions(+), 5 deletions(-)
>
> diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
> index 101380f93aafbd528ba0020371f0c43a85f41bd1..b0dc67c3a18caf7437a0a6d6229108299e8514a7 100644
> --- a/opcodes/riscv-dis.c
> +++ b/opcodes/riscv-dis.c
> @@ -1308,6 +1308,14 @@ 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)
> @@ -1360,13 +1368,28 @@ riscv_init_disasm_info (struct disassemble_info *info)
>     return true;
>   }
>   
> +/* Fetch an instruction. If only a partial instruction is able to be fetched,
> +   return the number of accessible bytes. */

And a hint there should be two spaces before the end .

> +
> +static bfd_vma
> +fetch_insn (bfd_vma memaddr, bfd_byte *packet, bfd_vma dump_size, struct disassemble_info *info, volatile int *status)
> +{
> +  do
> +    {
> +      *status = (*info->read_memory_func) (memaddr, packet, dump_size, info);
> +    }
> +  while(*status != 0 && dump_size-- > 1);
> +
> +  return dump_size;
> +}
> +
>   int
>   print_insn_riscv (bfd_vma memaddr, struct disassemble_info *info)
>   {
>     bfd_byte packet[RISCV_MAX_INSN_LEN];
>     insn_t insn = 0;
> -  bfd_vma dump_size;
> -  int status;
> +  volatile bfd_vma dump_size, bytes_fetched;
> +  volatile int status;
>     enum riscv_seg_mstate mstate;
>     int (*riscv_disassembler) (bfd_vma, insn_t, const bfd_byte *,
>   			     struct disassemble_info *);
> @@ -1398,24 +1421,41 @@ print_insn_riscv (bfd_vma memaddr, struct disassemble_info *info)
>     else
>       {
>         /* Get the first 2-bytes to check the lenghth of instruction.  */
> -      status = (*info->read_memory_func) (memaddr, packet, 2, info);
> +      bytes_fetched = fetch_insn(memaddr, packet, 2, info, &status);
>         if (status != 0)
>   	{
>   	  (*info->memory_error_func) (status, memaddr, info);
>   	  return -1;
>   	}
> +      else if (bytes_fetched != 2)
> +       {
> +	  /* Only the first byte was able to be read. Dump the partial instruction. */

Same case at here, besides that LGTM:)

Jiawei

> +	  dump_size = bytes_fetched;
> +	  info->bytes_per_chunk = dump_size;
> +	  riscv_disassembler = riscv_disassemble_data;
> +	 goto print;
> +       }
>         insn = (insn_t) bfd_getl16 (packet);
>         dump_size = riscv_insn_length (insn);
>         riscv_disassembler = riscv_disassemble_insn;
>       }
>   
> -  /* Fetch the instruction to dump.  */
> -  status = (*info->read_memory_func) (memaddr, packet, dump_size, info);
> +  bytes_fetched = fetch_insn(memaddr, packet, dump_size, info, &status);
> +
>     if (status != 0)
>       {
>         (*info->memory_error_func) (status, memaddr, info);
>         return -1;
>       }
> +  else if (bytes_fetched != dump_size)
> +    {
> +      dump_size = bytes_fetched;
> +      info->bytes_per_chunk = dump_size;
> +      riscv_disassembler = riscv_disassemble_data;
> +    }
> +
> +print:
> +
>     insn = (insn_t) bfd_get_bits (packet, dump_size * 8, false);
>   
>     return (*riscv_disassembler) (memaddr, insn, packet, info);
>
> ---
> base-commit: 978324718990b6b371d4eeeba02cfe13a0ebf120
> change-id: 20241121-fix_objdump_partial_insn-94e236f3db38