[v3] RISC-V: Fix disassembly of partial instructions

Message ID 20241219-fix_objdump_partial_insn-v3-1-d5ed9af878d1@rivosinc.com
State New
Headers
Series [v3] RISC-V: Fix disassembly of partial instructions |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Test passed

Commit Message

Charlie Jenkins Dec. 19, 2024, 6:56 p.m. UTC
  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 v3:
- More formatting issues (Nelson)
- Link to v2: https://lore.kernel.org/r/20241216-fix_objdump_partial_insn-v2-1-8de88a115dbc@rivosinc.com

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 | 53 +++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 49 insertions(+), 4 deletions(-)


---
base-commit: 978324718990b6b371d4eeeba02cfe13a0ebf120
change-id: 20241121-fix_objdump_partial_insn-94e236f3db38
  

Patch

diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index 101380f93aafbd528ba0020371f0c43a85f41bd1..8f551f5bddb9289458b54dfee90d6c014941f9a3 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,12 +1368,31 @@  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;
+  bfd_vma dump_size, bytes_fetched;
   int status;
   enum riscv_seg_mstate mstate;
   int (*riscv_disassembler) (bfd_vma, insn_t, const bfd_byte *,
@@ -1398,24 +1425,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);