commit 9ace48f3d7d80ce09c5df60cccb433470410b11b

Author: Andreas Arnez <arnez@linux.vnet.ibm.com>

Date:   Tue Aug 19 15:42:13 2014 +0100

    This patch set mainly aims at improving the S/390 disassembler's

    readability and also fixes some minor issues.

      S/390: Split disassembler routine into smaller functions

      S/390: Fix disassembler's treatment of signed/unsigned operands

      S/390: Fix off-by-one error in disassembler initialization

      S/390: Simplify opcode search loop in disassembler

      S/390: Drop function pointer dereferences in disassembler

      S/390: Various minor simplifications in disassembler

### a/opcodes/ChangeLog

### b/opcodes/ChangeLog

## -1,3 +1,30 @@

+2014-08-19  Andreas Arnez  <arnez@linux.vnet.ibm.com>

+

+	* s390-dis.c (s390_insn_length, s390_insn_matches_opcode)

+	(s390_print_insn_with_opcode, opcode_mask_more_specific): New

+	static functions, code was moved from...

+	(print_insn_s390): ...here.

+	(s390_extract_operand): Adjust comment.  Change type of first

+	parameter from 'unsigned char *' to 'const bfd_byte *'.

+	(union operand_value): New.

+	(s390_extract_operand): Change return type to union operand_value.

+	Also avoid integer overflow in sign-extension.

+	(s390_print_insn_with_opcode): Adjust to changed return value from

+	s390_extract_operand().  Change "%i" printf format to "%u" for

+	unsigned values.

+	(init_disasm): Simplify initialization of opc_index[].  This also

+	fixes an access after the last element of s390_opcodes[].

+	(print_insn_s390): Simplify the opcode search loop.

+	Check architecture mask against all searched opcodes, not just the

+	first matching one.

+	(s390_print_insn_with_opcode): Drop function pointer dereferences

+	without effect.

+	(print_insn_s390): Likewise.

+	(s390_insn_length): Simplify formula for return value.

+	(s390_print_insn_with_opcode): Avoid special handling for the

+	separator before the first operand.  Use new local variable

+	'flags' in place of 'operand->flags'.

+

 2014-08-14  Mike Frysinger  <vapier@gentoo.org>

 	* bfin-dis.c (struct private): Change int's to bfd_boolean's.

--- a/opcodes/s390-dis.c

+++ b/opcodes/s390-dis.c

@@ -35,19 +35,15 @@ static int current_arch_mask = 0;

 static void

 init_disasm (struct disassemble_info *info)

 {

-  const struct s390_opcode *opcode;

-  const struct s390_opcode *opcode_end;

+  int i;

   const char *p;

   memset (opc_index, 0, sizeof (opc_index));

-  opcode_end = s390_opcodes + s390_num_opcodes;

-  for (opcode = s390_opcodes; opcode < opcode_end; opcode++)

-    {

-      opc_index[(int) opcode->opcode[0]] = opcode - s390_opcodes;

-      while ((opcode < opcode_end) &&

-	     (opcode[1].opcode[0] == opcode->opcode[0]))

-	opcode++;

-    }

+

+  /* Reverse order, such that each opc_index ends up pointing to the

+     first matching entry instead of the last.  */

+  for (i = s390_num_opcodes; i--; )

+    opc_index[s390_opcodes[i].opcode[0]] = i;

   for (p = info->disassembler_options; p != NULL; )

     {

@@ -69,15 +65,46 @@ init_disasm (struct disassemble_info *info)

   init_flag = 1;

 }

+/* Derive the length of an instruction from its first byte.  */

+

+static inline int

+s390_insn_length (const bfd_byte *buffer)

+{

+  /* 00xxxxxx -> 2, 01xxxxxx/10xxxxxx -> 4, 11xxxxxx -> 6.  */

+  return ((buffer[0] >> 6) + 3) & ~1U;

+}

+

+/* Match the instruction in BUFFER against the given OPCODE, excluding

+   the first byte.  */

+

+static inline int

+s390_insn_matches_opcode (const bfd_byte *buffer,

+			  const struct s390_opcode *opcode)

+{

+  return (buffer[1] & opcode->mask[1]) == opcode->opcode[1]

+    && (buffer[2] & opcode->mask[2]) == opcode->opcode[2]

+    && (buffer[3] & opcode->mask[3]) == opcode->opcode[3]

+    && (buffer[4] & opcode->mask[4]) == opcode->opcode[4]

+    && (buffer[5] & opcode->mask[5]) == opcode->opcode[5];

+}

+

+union operand_value

+{

+  int i;

+  unsigned int u;

+};

+

 /* Extracts an operand value from an instruction.  */

 /* We do not perform the shift operation for larl-type address

    operands here since that would lead to an overflow of the 32 bit

    integer value.  Instead the shift operation is done when printing

-   the operand in print_insn_s390.  */

+   the operand.  */

-static inline unsigned int

-s390_extract_operand (unsigned char *insn, const struct s390_operand *operand)

+static inline union operand_value

+s390_extract_operand (const bfd_byte *insn,

+		      const struct s390_operand *operand)

 {

+  union operand_value ret;

   unsigned int val;

   int bits;

@@ -99,15 +126,97 @@ s390_extract_operand (unsigned char *insn, const struct s390_operand *operand)

   if (operand->bits == 20 && operand->shift == 20)

     val = (val & 0xff) << 12 | (val & 0xfff00) >> 8;

-  /* Sign extend value if the operand is signed or pc relative.  */

-  if ((operand->flags & (S390_OPERAND_SIGNED | S390_OPERAND_PCREL))

-      && (val & (1U << (operand->bits - 1))))

-    val |= (-1U << (operand->bits - 1)) << 1;

+  /* Sign extend value if the operand is signed or pc relative.  Avoid

+     integer overflows.  */

+  if (operand->flags & (S390_OPERAND_SIGNED | S390_OPERAND_PCREL))

+    {

+      unsigned int m = 1U << (operand->bits - 1);

+

+      if (val >= m)

+	ret.i = (int) (val - m) - 1 - (int) (m - 1U);

+      else

+	ret.i = (int) val;

+    }

+  else if (operand->flags & S390_OPERAND_LENGTH)

+    /* Length x in an instruction has real length x + 1.  */

+    ret.u = val + 1;

+  else

+    ret.u = val;

+

+  return ret;

+}

+

+/* Print the S390 instruction in BUFFER, assuming that it matches the

+   given OPCODE.  */

+

+static void

+s390_print_insn_with_opcode (bfd_vma memaddr,

+			     struct disassemble_info *info,

+			     const bfd_byte *buffer,

+			     const struct s390_opcode *opcode)

+{

+  const unsigned char *opindex;

+  char separator;

+

+  /* Mnemonic.  */

+  info->fprintf_func (info->stream, "%s", opcode->name);

+

+  /* Operands.  */

+  separator = '\t';

+  for (opindex = opcode->operands; *opindex != 0; opindex++)

+    {

+      const struct s390_operand *operand = s390_operands + *opindex;

+      union operand_value val = s390_extract_operand (buffer, operand);

+      unsigned long flags = operand->flags;

+

+      if ((flags & S390_OPERAND_INDEX) && val.u == 0)

+	continue;

+      if ((flags & S390_OPERAND_BASE) &&

+	  val.u == 0 && separator == '(')

+	{

+	  separator = ',';

+	  continue;

+	}

+

+      info->fprintf_func (info->stream, "%c", separator);

+

+      if (flags & S390_OPERAND_GPR)

+	info->fprintf_func (info->stream, "%%r%u", val.u);

+      else if (flags & S390_OPERAND_FPR)

+	info->fprintf_func (info->stream, "%%f%u", val.u);

+      else if (flags & S390_OPERAND_AR)

+	info->fprintf_func (info->stream, "%%a%u", val.u);

+      else if (flags & S390_OPERAND_CR)

+	info->fprintf_func (info->stream, "%%c%u", val.u);

+      else if (flags & S390_OPERAND_PCREL)

+	info->print_address_func (memaddr + val.i + val.i, info);

+      else if (flags & S390_OPERAND_SIGNED)

+	info->fprintf_func (info->stream, "%i", val.i);

+      else

+	info->fprintf_func (info->stream, "%u", val.u);

+

+      if (flags & S390_OPERAND_DISP)

+	separator = '(';

+      else if (flags & S390_OPERAND_BASE)

+	{

+	  info->fprintf_func (info->stream, ")");

+	  separator = ',';

+	}

+      else

+	separator = ',';

+    }

+}

+

+/* Check whether opcode A's mask is more specific than that of B.  */

-  /* Length x in an instructions has real length x + 1.  */

-  if (operand->flags & S390_OPERAND_LENGTH)

-    val++;

-  return val;

+static int

+opcode_mask_more_specific (const struct s390_opcode *a,

+			   const struct s390_opcode *b)

+{

+  return (((int) a->mask[0] + a->mask[1] + a->mask[2]

+	   + a->mask[3] + a->mask[4] + a->mask[5])

+	  > ((int) b->mask[0] + b->mask[1] + b->mask[2]

+	     + b->mask[3] + b->mask[4] + b->mask[5]));

 }

 /* Print a S390 instruction.  */

@@ -116,11 +225,9 @@ int

 print_insn_s390 (bfd_vma memaddr, struct disassemble_info *info)

 {

   bfd_byte buffer[6];

-  const struct s390_opcode *opcode;

-  const struct s390_opcode *opcode_end;

+  const struct s390_opcode *opcode = NULL;

   unsigned int value;

   int status, opsize, bufsize;

-  char separator;

   if (init_flag == 0)

     init_disasm (info);

@@ -130,156 +237,72 @@ print_insn_s390 (bfd_vma memaddr, struct disassemble_info *info)

   /* Every S390 instruction is max 6 bytes long.  */

   memset (buffer, 0, 6);

-  status = (*info->read_memory_func) (memaddr, buffer, 6, info);

+  status = info->read_memory_func (memaddr, buffer, 6, info);

   if (status != 0)

     {

       for (bufsize = 0; bufsize < 6; bufsize++)

-	if ((*info->read_memory_func) (memaddr, buffer, bufsize + 1, info) != 0)

+	if (info->read_memory_func (memaddr, buffer, bufsize + 1, info) != 0)

 	  break;

       if (bufsize <= 0)

 	{

-	  (*info->memory_error_func) (status, memaddr, info);

+	  info->memory_error_func (status, memaddr, info);

 	  return -1;

 	}

-      /* Opsize calculation looks strange but it works

-	 00xxxxxx -> 2 bytes, 01xxxxxx/10xxxxxx -> 4 bytes,

-	 11xxxxxx -> 6 bytes.  */

-      opsize = ((((buffer[0] >> 6) + 1) >> 1) + 1) << 1;

+      opsize = s390_insn_length (buffer);

       status = opsize > bufsize;

     }

   else

     {

       bufsize = 6;

-      opsize = ((((buffer[0] >> 6) + 1) >> 1) + 1) << 1;

+      opsize = s390_insn_length (buffer);

     }

   if (status == 0)

     {

       const struct s390_opcode *op;

-      /* Find the first match in the opcode table.  */

-      opcode_end = s390_opcodes + s390_num_opcodes;

-      for (opcode = s390_opcodes + opc_index[(int) buffer[0]];

-	   (opcode < opcode_end) && (buffer[0] == opcode->opcode[0]);

-	   opcode++)

+      /* Find the "best match" in the opcode table.  */

+      for (op = s390_opcodes + opc_index[buffer[0]];

+	   op != s390_opcodes + s390_num_opcodes

+	     && op->opcode[0] == buffer[0];

+	   op++)

 	{

-	  const struct s390_operand *operand;

-	  const unsigned char *opindex;

-

-	  /* Check architecture.  */

-	  if (!(opcode->modes & current_arch_mask))

-	    continue;

-

-	  /* Check signature of the opcode.  */

-	  if ((buffer[1] & opcode->mask[1]) != opcode->opcode[1]

-	      || (buffer[2] & opcode->mask[2]) != opcode->opcode[2]

-	      || (buffer[3] & opcode->mask[3]) != opcode->opcode[3]

-	      || (buffer[4] & opcode->mask[4]) != opcode->opcode[4]

-	      || (buffer[5] & opcode->mask[5]) != opcode->opcode[5])

-	    continue;

-

-	  /* Advance to an opcode with a more specific mask.  */

-	  for (op = opcode + 1; op < opcode_end; op++)

-	    {

-	      if ((buffer[0] & op->mask[0]) != op->opcode[0])

-		break;

-

-	      if ((buffer[1] & op->mask[1]) != op->opcode[1]

-		  || (buffer[2] & op->mask[2]) != op->opcode[2]

-		  || (buffer[3] & op->mask[3]) != op->opcode[3]

-		  || (buffer[4] & op->mask[4]) != op->opcode[4]

-		  || (buffer[5] & op->mask[5]) != op->opcode[5])

-		continue;

-

-	      if (((int)opcode->mask[0] + opcode->mask[1] +

-		   opcode->mask[2] + opcode->mask[3] +

-		   opcode->mask[4] + opcode->mask[5]) <

-		  ((int)op->mask[0] + op->mask[1] +

-		   op->mask[2] + op->mask[3] +

-		   op->mask[4] + op->mask[5]))

-		opcode = op;

-	    }

-

-	  /* The instruction is valid.  */

-	  if (opcode->operands[0] != 0)

-	    (*info->fprintf_func) (info->stream, "%s\t", opcode->name);

-	  else

-	    (*info->fprintf_func) (info->stream, "%s", opcode->name);

-

-	  /* Extract the operands.  */

-	  separator = 0;

-	  for (opindex = opcode->operands; *opindex != 0; opindex++)

-	    {

-	      operand = s390_operands + *opindex;

-	      value = s390_extract_operand (buffer, operand);

-

-	      if ((operand->flags & S390_OPERAND_INDEX) && value == 0)

-		continue;

-	      if ((operand->flags & S390_OPERAND_BASE) &&

-		  value == 0 && separator == '(')

-		{

-		  separator = ',';

-		  continue;

-		}

-

-	      if (separator)

-		(*info->fprintf_func) (info->stream, "%c", separator);

-

-	      if (operand->flags & S390_OPERAND_GPR)

-		(*info->fprintf_func) (info->stream, "%%r%i", value);

-	      else if (operand->flags & S390_OPERAND_FPR)

-		(*info->fprintf_func) (info->stream, "%%f%i", value);

-	      else if (operand->flags & S390_OPERAND_AR)

-		(*info->fprintf_func) (info->stream, "%%a%i", value);

-	      else if (operand->flags & S390_OPERAND_CR)

-		(*info->fprintf_func) (info->stream, "%%c%i", value);

-	      else if (operand->flags & S390_OPERAND_PCREL)

-		(*info->print_address_func) (memaddr + (int)value + (int)value,

-					     info);

-	      else if (operand->flags & S390_OPERAND_SIGNED)

-		(*info->fprintf_func) (info->stream, "%i", (int) value);

-	      else

-		(*info->fprintf_func) (info->stream, "%u", value);

-

-	      if (operand->flags & S390_OPERAND_DISP)

-		{

-		  separator = '(';

-		}

-	      else if (operand->flags & S390_OPERAND_BASE)

-		{

-		  (*info->fprintf_func) (info->stream, ")");

-		  separator = ',';

-		}

-	      else

-		separator = ',';

-	    }

-

-	  /* Found instruction, printed it, return its size.  */

-	  return opsize;

+	  if ((op->modes & current_arch_mask)

+	      && s390_insn_matches_opcode (buffer, op)

+	      && (opcode == NULL

+		  || opcode_mask_more_specific (op, opcode)))

+	    opcode = op;

 	}

-      /* No matching instruction found, fall through to hex print.  */

     }

+  if (opcode != NULL)

+    {

+      /* The instruction is valid.  Print it and return its size.  */

+      s390_print_insn_with_opcode (memaddr, info, buffer, opcode);

+      return opsize;

+    }

+

+  /* Fall back to hex print.  */

   if (bufsize >= 4)

     {

       value = (unsigned int) buffer[0];

       value = (value << 8) + (unsigned int) buffer[1];

       value = (value << 8) + (unsigned int) buffer[2];

       value = (value << 8) + (unsigned int) buffer[3];

-      (*info->fprintf_func) (info->stream, ".long\t0x%08x", value);

+      info->fprintf_func (info->stream, ".long\t0x%08x", value);

       return 4;

     }

   else if (bufsize >= 2)

     {

       value = (unsigned int) buffer[0];

       value = (value << 8) + (unsigned int) buffer[1];

-      (*info->fprintf_func) (info->stream, ".short\t0x%04x", value);

+      info->fprintf_func (info->stream, ".short\t0x%04x", value);

       return 2;

     }

   else

     {

       value = (unsigned int) buffer[0];

-      (*info->fprintf_func) (info->stream, ".byte\t0x%02x", value);

+      info->fprintf_func (info->stream, ".byte\t0x%02x", value);

       return 1;

     }

 }