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

[Charlie Jenkins <charlie@rivosinc.com>]

On Fri, Dec 20, 2024 at 01:37:37AM +0800, Nelson Chu wrote:
> Some minor GNU coding styles as follows.  Also cc Jan and Andrew, hope they
> still have time in their busy schedules can help to see if there are some
> side effects.
> 
> Thanks
> Nelson
> 
> On Tue, Dec 17, 2024 at 6:23 AM Charlie Jenkins <charlie@rivosinc.com>
> wrote:
> 
> > 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.
> > ---
> > Changes in v2:
> > - Fix comment spacing (Jiawei)
> > - Link to v1:
> > https://lore.kernel.org/r/20241213-fix_objdump_partial_insn-v1-1-7a4963e655d5@rivosinc.com
> > ---
> >  opcodes/riscv-dis.c | 51
> > ++++++++++++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 46 insertions(+), 5 deletions(-)
> >
> > diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
> > index
> > 101380f93aafbd528ba0020371f0c43a85f41bd1..492135d4642a29d86757ba0d2ec8261dab66275f
> > 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.  */
> > +
> > +static bfd_vma
> > +fetch_insn (bfd_vma memaddr, bfd_byte *packet, bfd_vma dump_size, struct
> > disassemble_info *info, volatile int *status)
> >
> 
> over 80 char per line.
> 
> 
> > +{
> > +  do
> > +    {
> > +      *status = (*info->read_memory_func) (memaddr, packet, dump_size,
> > info);
> > +    }
> > +  while(*status != 0 && dump_size-- > 1);
> >
> 
> Need space between while and left '('
> 
> 
> > +
> > +  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;
> >
> 
> Not sure if it needed to be volatile or not.

Oh, I didn't mean to commit that...

> 
> 
> >    enum riscv_seg_mstate mstate;
> >    int (*riscv_disassembler) (bfd_vma, insn_t, const bfd_byte *,
> >                              struct disassemble_info *);
> > @@ -1398,24 +1421,42 @@ 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);
> >
> 
> Need space after the function fetch_insn.
> 
> 
> >        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.  */
> >
> 
> Two spaces after '.'
> 
> 
> > +         dump_size = bytes_fetched;
> > +         info->bytes_per_chunk = dump_size;
> > +         riscv_disassembler = riscv_disassemble_data;
> > +        goto print;
> >
> 
> Indent, seems to miss a space.
> 
> 
> > +       }
> >        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);
> >
> 
> Need space after the function fetch_insn.
> 
> 
> > +
> >    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:
> >
> 
> Indent, one space needed for the label.
> 

Thanks for the formatting info, I will make those changes and send
another version.

- Charlie

> 
> > +
> >    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
> > --
> > - Charlie
> >
> >