[ppc] Simplify VLE handling in print_insn_powerpc()

Message ID 63100274-9bf8-544c-a58e-44f4562f7b27@vnet.ibm.com
State New
Headers show
Series
  • [ppc] Simplify VLE handling in print_insn_powerpc()
Related show

Commit Message

Peter Bergner May 7, 2018, 7:29 p.m.
Here is another cleanup for some VLE code I noticed while scanning the code.
This consolidates two duplicate calls to memory_error_func.  In the case where
our initial 4-byte read and later VLE 2-byte read both fail, I've changed it so
that we return the error status of the 4-byte read instead of the 2-byte read.
This seems more "correct" to me, since the second 2-byte read is really just
speculative/hopefull.  If you don't agree with that, I can always replace
vle_status with status.  The code is still simpler either way.

Alan, does the following look ok to you?  There were no differences in
the testsuite runs.

Peter

	* ppc-dis.c (print_insn_powerpc) <insn_is_short>: Replace this...
	<insn_length>: ...with this.  Update usage.
	Remove duplicate call to *info->memory_error_func.  Return error status
	of 4-byte read.

Comments

Alan Modra May 7, 2018, 11:47 p.m. | #1
On Mon, May 07, 2018 at 02:29:54PM -0500, Peter Bergner wrote:
> Here is another cleanup for some VLE code I noticed while scanning the code.

> This consolidates two duplicate calls to memory_error_func.  In the case where

> our initial 4-byte read and later VLE 2-byte read both fail, I've changed it so

> that we return the error status of the 4-byte read instead of the 2-byte read.

> This seems more "correct" to me, since the second 2-byte read is really just

> speculative/hopefull.  If you don't agree with that, I can always replace

> vle_status with status.  The code is still simpler either way.


I don't think it matters either way, but the code is simpler without
vle_status so I'd prefer that you do return the second 2-byte read
status.  OK with that change

> -	insn_is_short = PPC_OP_SE_VLE(opcode->mask);

> +      if (opcode != NULL && PPC_OP_SE_VLE(opcode->mask))


and since you're editing the code, please fix the formatting here.

-- 
Alan Modra
Australia Development Lab, IBM
Peter Bergner May 8, 2018, 1:50 a.m. | #2
On 5/7/18 6:47 PM, Alan Modra wrote:
> I don't think it matters either way, but the code is simpler without

> vle_status so I'd prefer that you do return the second 2-byte read

> status.  OK with that change


Done.


>> -	insn_is_short = PPC_OP_SE_VLE(opcode->mask);

>> +      if (opcode != NULL && PPC_OP_SE_VLE(opcode->mask))

> 

> and since you're editing the code, please fix the formatting here.


Ah, thanks for spotting the extra space.  Fixed and committed.  Thanks!

Peter

Patch

diff --git a/opcodes/ppc-dis.c b/opcodes/ppc-dis.c
index f8b0292c4e..373bfd2652 100644
--- a/opcodes/ppc-dis.c
+++ b/opcodes/ppc-dis.c
@@ -660,28 +660,24 @@  print_insn_powerpc (bfd_vma memaddr,
   int status;
   uint64_t insn;
   const struct powerpc_opcode *opcode;
-  bfd_boolean insn_is_short;
+  int insn_length = 4;  /* Assume we have a normal 4-byte instruction.  */
 
   status = (*info->read_memory_func) (memaddr, buffer, 4, info);
+
+  /* The final instruction may be a 2-byte VLE insn.  */
+  if (status != 0 && (dialect & PPC_OPCODE_VLE) != 0)
+    {
+      /* Clear buffer so unused bytes will not have garbage in them.  */
+      buffer[0] = buffer[1] = buffer[2] = buffer[3] = 0;
+      int vle_status = (*info->read_memory_func) (memaddr, buffer, 2, info);
+      if (vle_status == 0)
+	status = 0;
+    }
+
   if (status != 0)
     {
-      /* The final instruction may be a 2-byte VLE insn.  */
-      if ((dialect & PPC_OPCODE_VLE) != 0)
-        {
-          /* Clear buffer so unused bytes will not have garbage in them.  */
-          buffer[0] = buffer[1] = buffer[2] = buffer[3] = 0;
-          status = (*info->read_memory_func) (memaddr, buffer, 2, info);
-          if (status != 0)
-            {
-              (*info->memory_error_func) (status, memaddr, info);
-              return -1;
-            }
-        }
-      else
-        {
-          (*info->memory_error_func) (status, memaddr, info);
-          return -1;
-        }
+      (*info->memory_error_func) (status, memaddr, info);
+      return -1;
     }
 
   if (bigendian)
@@ -691,12 +687,15 @@  print_insn_powerpc (bfd_vma memaddr,
 
   /* Get the major opcode of the insn.  */
   opcode = NULL;
-  insn_is_short = FALSE;
   if ((dialect & PPC_OPCODE_VLE) != 0)
     {
       opcode = lookup_vle (insn);
-      if (opcode != NULL)
-	insn_is_short = PPC_OP_SE_VLE(opcode->mask);
+      if (opcode != NULL && PPC_OP_SE_VLE(opcode->mask))
+	{
+	  /* The operands will be fetched out of the 16-bit instruction.  */
+	  insn >>= 16;
+	  insn_length = 2;
+	}
     }
   if (opcode == NULL && (dialect & PPC_OPCODE_SPE2) != 0)
     opcode = lookup_spe2 (insn);
@@ -718,10 +717,6 @@  print_insn_powerpc (bfd_vma memaddr,
       else
 	(*info->fprintf_func) (info->stream, "%s", opcode->name);
 
-      if (insn_is_short)
-        /* The operands will be fetched out of the 16-bit instruction.  */
-        insn >>= 16;
-
       /* Now extract and print the operands.  */
       need_comma = 0;
       need_paren = 0;
@@ -813,16 +808,8 @@  print_insn_powerpc (bfd_vma memaddr,
 	    }
 	}
 
-      /* We have found and printed an instruction.
-         If it was a short VLE instruction we have more to do.  */
-      if (insn_is_short)
-        {
-          memaddr += 2;
-          return 2;
-        }
-      else
-        /* Otherwise, return.  */
-        return 4;
+      /* We have found and printed an instruction.  */
+      return insn_length;
     }
 
   /* We could not find a match.  */