Home Original page

[PATCHv2 1/3] opcodes/arc: Move instruction length logic to new function

Andrew Burgess andrew.burgess@embecosm.com
Thu Apr 14 12:40:00 GMT 2016 
* Nick Clifton <nickc@redhat.com> [2016-04-13 16:30:38 +0100]:

> Hi Andrew,
> 
> >> Would this function ever return a negative value ?  I assume not, so
> >> it would make sense for its return type to be "unsigned int".
> > 
> > I'm not sure is the answer.  I agree that arc_insn_length will never
> > return a negative, however, the return value from arc_insn_length is
> > used to prime a variable that is then the return value for
> > print_insn_arc, which is also defined to return 'int', and is part of
> > the disassembler API, and does return a negative value if there's an
> > error.
> > 
> > It was this relationship that originally lead me to make
> > arc_insn_length return an 'int'.
> > 
> > Given that it will only ever return small positive integers there
> > should be no problem making it return an unsigned value then casting
> > to int in print_insn_arc - would this be preferred?
> 
> Yes.  Or (better IMHO) just explicitly set the return value for 
> print_insn_arc based upon testing the return value from arc_insn_length.
> Ie something like:
> 
>   len = arc_insn_length (...);
>   if (len == 0)
>     return -1;
> 
> (I am assuming here that a returned length of zero should never happen
> with a valid instruction, and can therefore be used as an error return
> value).

Just so I know I've done what you imagined, here's the patch that I
think addresses your point.  Is this OK?  It's mostly the same except
for type changes associated with insnLen.

Thanks,
Andrew

---

Move the logic that calculates the instruction length out to a new
function.  Restructure the code to make it simpler.

opcodes/ChangeLog:

	* arc-dis.c (arc_insn_length): New function.
	(print_insn_arc): Use arc_insn_length, change insnLen to unsigned.
	(find_format): Change insnLen parameter to unsigned.
---
 opcodes/ChangeLog |  6 ++++++
 opcodes/arc-dis.c | 57 ++++++++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 50 insertions(+), 13 deletions(-)

diff --git a/opcodes/arc-dis.c b/opcodes/arc-dis.c
index eb8bd67..7757ef6 100644
--- a/opcodes/arc-dis.c
+++ b/opcodes/arc-dis.c
@@ -105,7 +105,7 @@ special_flag_p (const char *opname,
 /* Find proper format for the given opcode.  */
 static const struct arc_opcode *
 find_format (const struct arc_opcode *arc_table,
-	     unsigned *insn, int insnLen,
+	     unsigned *insn, unsigned int insnLen,
 	     unsigned isa_mask)
 {
   unsigned int i = 0;
@@ -309,6 +309,37 @@ get_auxreg (const struct arc_opcode *opcode,
     }
   return NULL;
 }
+
+/* Calculate the instruction length for an instruction starting with MSB
+   and LSB, the most and least significant byte.  The ISA_MASK is used to
+   filter the instructions considered to only those that are part of the
+   current architecture.
+
+   The instruction lengths are calculated from the ARC_OPCODE table, and
+   cached for later use.  */
+
+static unsigned int
+arc_insn_length (bfd_byte msb, struct disassemble_info *info)
+{
+  bfd_byte major_opcode = msb >> 3;
+
+  switch (info->mach)
+    {
+    case bfd_mach_arc_nps400:
+    case bfd_mach_arc_arc700:
+    case bfd_mach_arc_arc600:
+      return (major_opcode > 0xb) ? 2 : 4;
+      break;
+
+    case bfd_mach_arc_arcv2:
+      return (major_opcode > 0x7) ? 2 : 4;
+      break;
+
+    default:
+      abort ();
+    }
+}
+
 /* Disassemble ARC instructions.  */
 
 static int
@@ -318,7 +349,7 @@ print_insn_arc (bfd_vma memaddr,
   bfd_byte buffer[4];
   unsigned int lowbyte, highbyte;
   int status;
-  int insnLen = 0;
+  unsigned int insnLen;
   unsigned insn[2] = { 0, 0 };
   unsigned isa_mask;
   const unsigned char *opidx;
@@ -422,20 +453,19 @@ print_insn_arc (bfd_vma memaddr,
       return size;
     }
 
-  if ((((buffer[lowbyte] & 0xf8) > 0x38)
-       && ((buffer[lowbyte] & 0xf8) != 0x48))
-      || ((info->mach == bfd_mach_arc_arcv2)
-	  && ((buffer[lowbyte] & 0xF8) == 0x48)) /* FIXME! ugly.  */
-      )
+  insnLen = arc_insn_length (buffer[lowbyte], info);
+  switch (insnLen)
     {
-      /* This is a short instruction.  */
-      insnLen = 2;
+    case 2:
       insn[0] = (buffer[lowbyte] << 8) | buffer[highbyte];
-    }
-  else
-    {
-      insnLen = 4;
+      break;
 
+    default:
+      /* An unknown instruction is treated as being length 4.  This is
+         possibly not the best solution, but matches the behaviour that was
+         in place before the table based instruction length look-up was
+         introduced.  */
+    case 4:
       /* This is a long instruction: Read the remaning 2 bytes.  */
       status = (*info->read_memory_func) (memaddr + 2, &buffer[2], 2, info);
       if (status != 0)
@@ -444,6 +474,7 @@ print_insn_arc (bfd_vma memaddr,
 	  return -1;
 	}
       insn[0] = ARRANGE_ENDIAN (info, buffer);
+      break;
     }
 
   /* Set some defaults for the insn info.  */
-- 
2.5.1
More information about the Binutils mailing list