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

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

[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.
---
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)
+{
+  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,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);
       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.  */
+	  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
-- 
- Charlie

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

[Nelson Chu]

[-- Attachment #1: Type: text/plain, Size: 7179 bytes --]

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.


>    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.


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

[-- Attachment #2: Type: text/html, Size: 9646 bytes --]

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

[Charlie Jenkins]

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

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

[Jan Beulich]

On 19.12.2024 18:37, 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.

I did actually take a look, but then pretty quickly decided I must be missing
something: There's no disassembly of anything here afaics, and I also can't
see how a partial insn could be disassembled in the first place. Such can
only ever be displayed as raw hex data, I think. And that's what the patch
appears to be doing, just that it's description says otherwise. That plus the
lack of a testcase demonstrating the intended behavior made me simply stop
any effort here.

Jan

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

[Charlie Jenkins]

On Fri, Dec 20, 2024 at 11:38:01AM +0100, Jan Beulich wrote:
> On 19.12.2024 18:37, 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.
> 
> I did actually take a look, but then pretty quickly decided I must be missing
> something: There's no disassembly of anything here afaics, and I also can't
> see how a partial insn could be disassembled in the first place. Such can
> only ever be displayed as raw hex data, I think. And that's what the patch

I think this is just an issue with my terminology. I was referring to
raw hex data as the "disassembly". Should the title say "raw hex data"
instead of "disassembly"? I was thinking that "disassembly" included
any instruction data that was being output from objdump.

> appears to be doing, just that it's description says otherwise. That plus the
> lack of a testcase demonstrating the intended behavior made me simply stop
> any effort here.

I should have posted a minimal test case. The test case I was using was
the linux perf test case that I posted. Here is a better example where I
have the output on binutils 2.40, 2.41, and with this patch.

As a summary, on 2.40 the hex of the instruction is shown even if the
full instruction can't be dumped. On 2.41 it errors with "Address <addr> is
out of bounds." in this case. With this patch, it reverts to the 2.40
behavior of showing the hex, but also encodes the partial instruction
using assembler directives instead of printing the "out of bounds" error.

$ cat objdump.s
c.lw a0, 0(a0)
.option push
.option norvc
nop
.option pop

$ riscv64-linux-gnu-gcc -c objdump.s

Example 2.40:
$ ./objdump-2.40 --start-address 0 --stop-address 4 -d -z objdump.o

objdump.o:     file format elf64-littleriscv


Disassembly of section .text:

0000000000000000 <.text>:
   0:	4108                	lw	a0,0(a0)
   2:	13 00             	Address 0x2 is out of bounds.



Example 2.41
$ ./objdump-2.41 --start-address 0 --stop-address 4 -d -z objdump.o

objdump.o:     file format elf64-littleriscv


Disassembly of section .text:

0000000000000000 <.text>:
   0:	4108                	lw	a0,0(a0)
   2:	Address 0x2 is out of bounds.


Example with this patch:
$ ./binutils/objdump --start-address 0 --stop-address 4 -d -z objdump.o                                               [11:20:54]

objdump.o:     file format elf64-littleriscv


Disassembly of section .text:

0000000000000000 <.text>:
   0:   4108                    lw      a0,0(a0)
   2:   0013                    .short  0x0013


The difference here being that "Address <address> is out of bounds" is
replaced by the raw hex data.

> 
> Jan

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

[Jan Beulich]

On 20.12.2024 20:27, Charlie Jenkins wrote:
> On Fri, Dec 20, 2024 at 11:38:01AM +0100, Jan Beulich wrote:
>> On 19.12.2024 18:37, 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.
>>
>> I did actually take a look, but then pretty quickly decided I must be missing
>> something: There's no disassembly of anything here afaics, and I also can't
>> see how a partial insn could be disassembled in the first place. Such can
>> only ever be displayed as raw hex data, I think. And that's what the patch
> 
> I think this is just an issue with my terminology. I was referring to
> raw hex data as the "disassembly". Should the title say "raw hex data"
> instead of "disassembly"? I was thinking that "disassembly" included
> any instruction data that was being output from objdump.

That or "display" instead of "disassembly", if you ask me.

Jan

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

[Charlie Jenkins]

On Tue, Dec 24, 2024 at 09:22:06AM +0100, Jan Beulich wrote:
> On 20.12.2024 20:27, Charlie Jenkins wrote:
> > On Fri, Dec 20, 2024 at 11:38:01AM +0100, Jan Beulich wrote:
> >> On 19.12.2024 18:37, 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.
> >>
> >> I did actually take a look, but then pretty quickly decided I must be missing
> >> something: There's no disassembly of anything here afaics, and I also can't
> >> see how a partial insn could be disassembled in the first place. Such can
> >> only ever be displayed as raw hex data, I think. And that's what the patch
> > 
> > I think this is just an issue with my terminology. I was referring to
> > raw hex data as the "disassembly". Should the title say "raw hex data"
> > instead of "disassembly"? I was thinking that "disassembly" included
> > any instruction data that was being output from objdump.
> 
> That or "display" instead of "disassembly", if you ask me.

Sounds good, thank you for letting me know. Is there anything beyond
this wording that is of concern in this patch? Unless there is anything
else that needs to be changed, I can send a new version with the same
diff (containing Nelson's comments) but change "disassembly" to
"display" in the title and message.

- Charlie

> 
> Jan

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

[Maciej W. Rozycki]

On Thu, 2 Jan 2025, Charlie Jenkins wrote:

> > That or "display" instead of "disassembly", if you ask me.
> 
> Sounds good, thank you for letting me know. Is there anything beyond
> this wording that is of concern in this patch? Unless there is anything
> else that needs to be changed, I can send a new version with the same
> diff (containing Nelson's comments) but change "disassembly" to
> "display" in the title and message.

 I think relevant test cases need to be added to the testsuite, I guess 
under binutils/testsuite/binutils-all/riscv/ as this covers `objdump' 
rather than any of the other tools.  This is to guarantee things won't 
regress again by accident.

  Maciej

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

[Charlie Jenkins]

On Fri, Jan 03, 2025 at 06:22:34AM +0000, Maciej W. Rozycki wrote:
> On Thu, 2 Jan 2025, Charlie Jenkins wrote:
> 
> > > That or "display" instead of "disassembly", if you ask me.
> > 
> > Sounds good, thank you for letting me know. Is there anything beyond
> > this wording that is of concern in this patch? Unless there is anything
> > else that needs to be changed, I can send a new version with the same
> > diff (containing Nelson's comments) but change "disassembly" to
> > "display" in the title and message.
> 
>  I think relevant test cases need to be added to the testsuite, I guess 
> under binutils/testsuite/binutils-all/riscv/ as this covers `objdump' 
> rather than any of the other tools.  This is to guarantee things won't 
> regress again by accident.

Yes that is a good point, I will add a test for this.

- Charlie

> 
>   Maciej

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

[Jan Beulich]

On 03.01.2025 07:22, Maciej W. Rozycki wrote:
> On Thu, 2 Jan 2025, Charlie Jenkins wrote:
> 
>>> That or "display" instead of "disassembly", if you ask me.
>>
>> Sounds good, thank you for letting me know. Is there anything beyond
>> this wording that is of concern in this patch? Unless there is anything
>> else that needs to be changed, I can send a new version with the same
>> diff (containing Nelson's comments) but change "disassembly" to
>> "display" in the title and message.
> 
>  I think relevant test cases need to be added to the testsuite, I guess 
> under binutils/testsuite/binutils-all/riscv/ as this covers `objdump' 
> rather than any of the other tools.  This is to guarantee things won't 
> regress again by accident.

As to a testcase: Yes. As to where to put it: Almost all disassembler
tests really live in the gas testsuite, afaict.

Jan

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

[Andrew Burgess]

Nelson Chu <nelson@rivosinc.com> writes:

> 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.

I caught up on this thread today.  All the suggestions seem like good
advice to me.  But I have no issues with the underlying fix, it seems
like a good improvement to me.

Acked-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

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

[Nelson Chu]

[-- Attachment #1: Type: text/plain, Size: 733 bytes --]

Thanks Andrew, Jan, and Maciej.

I see the newest v4 patches, including test cases, so I committed them
after passed the regressions.

Thanks
Nelson

On Mon, Jan 6, 2025 at 6:31 PM Andrew Burgess <aburgess@redhat.com> wrote:

> Nelson Chu <nelson@rivosinc.com> writes:
>
> > 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.
>
> I caught up on this thread today.  All the suggestions seem like good
> advice to me.  But I have no issues with the underlying fix, it seems
> like a good improvement to me.
>
> Acked-By: Andrew Burgess <aburgess@redhat.com>
>
> Thanks,
> Andrew
>
>

[-- Attachment #2: Type: text/html, Size: 1277 bytes --]