[v2] Fix ARM machine state testcase failures

Message ID 54464B82.6030703@codesourcery.com
State New, archived
Headers

Commit Message

Luis Machado Oct. 21, 2014, 12:03 p.m. UTC
  Hi,

This is v2 of the original patch. Compared to v1, i've taken Yao's 
suggestion and dropped special handling of push/pop instructions.

While at it, i noticed the original functions had some issues like 
unused variables and broken identation, so i went ahead and completely 
rewrote the functions handling ARM mode opcodes 010 and 100.

I see the same results as before, with no regressions.

How does it look now?

Luis
  

Comments

Luis Machado Oct. 21, 2014, 12:16 p.m. UTC | #1
On 10/21/2014 10:03 AM, Luis Machado wrote:
> -      if (bit (arm_insn_r->arm_insn, 20) && !bit (arm_insn_r->arm_insn, 22))
> -        {
> -          register_bits = bits (arm_insn_r->arm_insn, 0, 15);
> -          no_of_regs = 15;
> -        }
> -      else
> -        {
> -          register_bits = bits (arm_insn_r->arm_insn, 0, 14);
> -          no_of_regs = 14;
> -        }

Forgot to mention. I saw this bit of code but did not find signs of 
these patterns in the documentation i have access to. I suppose it may 
be some older encoding of the instructions (26-bit)?

The comments don't really make it clear enough. If someone knows more 
about this distinction, please do share the details.

Luis
  
Yao Qi Oct. 21, 2014, 12:46 p.m. UTC | #2
Luis Machado <lgustavo@codesourcery.com> writes:

> While at it, i noticed the original functions had some issues like
> unused variables and broken identation, so i went ahead and completely
> rewrote the functions handling ARM mode opcodes 010 and 100.

It's great to make code cleaner, but better to put code refactor and bug
fixing into separate patches.

> I see the same results as before, with no regressions.
>
> How does it look now?
>
> Luis
>
> 2014-10-21  Luis Machado  <lgustavo@codesourcery.com>
>
> 	* arm-tdep.c (INSN_S_L_BIT_NUM): Document.
> 	(arm_record_ld_st_imm_offset): Reimplement to cover all
> 	load/store cases for ARM opcode 010.
> 	(arm_record_ld_st_multiple): Reimplement to cover all
> 	load/store cases for ARM opcode 100.
>

With these comments below addressed, this patch is OK to me.

>  
> -/* Handling opcode 100 insns.  */
> +/* Handle ARM mode instructions with opcode 100.  */
>  
>  static int
>  arm_record_ld_st_multiple (insn_decode_record *arm_insn_r)
>  {
>    struct regcache *reg_cache = arm_insn_r->regcache;
> -
> -  uint32_t register_list[16] = {0}, register_count = 0, register_bits = 0;
> -  uint32_t reg_src1 = 0, addr_mode = 0, no_of_regs = 0;
> -  uint32_t start_address = 0, index = 0;
> +  uint32_t register_count = 0, register_bits;
> +  uint32_t reg_base, addr_mode;
>    uint32_t record_buf[24], record_buf_mem[48];
> +  uint32_t wback;
> +  ULONGEST u_regval;
>  
> -  ULONGEST u_regval[2] = {0};
> +  /* Fetch the list of registers.  */
> +  register_bits = bits (arm_insn_r->arm_insn, 0, 15);
> +  arm_insn_r->reg_rec_count = 0;
> +
> +  /* Fetch the base register that contains the address we are loading data
> +     to.  */
> +  reg_base = bits (arm_insn_r->arm_insn, 16, 19);
>  
> -  /* This mode is exclusively for load and store multiple.  */
> -  /* Handle incremenrt after/before and decrment after.before mode;
> -     Rn is changing depending on W bit, but as of now we store Rn too
> -     without optimization.  */
> +  /* Calculate wback.  */
> +  wback = (bit (arm_insn_r->arm_insn, 24) == 0)
> +	  || (bit (arm_insn_r->arm_insn, 21) == 1);

Bit 24 is zero, so we only need to check bit 21.

>  
>    if (bit (arm_insn_r->arm_insn, INSN_S_L_BIT_NUM))
>      {
> -      /* LDM  (1,2,3) where LDM  (3) changes CPSR too.  */
> +      /* LDM/LDMIA/LDMFD, LDMDA/LDMFA, LDMDB and LDMIB.  */
>  
> -      if (bit (arm_insn_r->arm_insn, 20) && !bit (arm_insn_r->arm_insn, 22))
> -        {
> -          register_bits = bits (arm_insn_r->arm_insn, 0, 15);
> -          no_of_regs = 15;
> -        }
> -      else
> -        {
> -          register_bits = bits (arm_insn_r->arm_insn, 0, 14);
> -          no_of_regs = 14;
> -        }
> -      /* Get Rn.  */
> -      reg_src1 = bits (arm_insn_r->arm_insn, 16, 19);
> +      /* Find out which registers are going to be loaded from memory.  */
>        while (register_bits)
> -      {
> -        if (register_bits & 0x00000001)
> -          record_buf[index++] = register_count;
> -        register_bits = register_bits >> 1;
> -        register_count++;
> -      }
> +	{
> +	  if (register_bits & 0x00000001)
> +	    record_buf[arm_insn_r->reg_rec_count++] = register_count;
> +	  register_bits = register_bits >> 1;
> +	  register_count++;
> +	}
>  
> -        /* Extra space for Base Register and CPSR; wihtout optimization.  */
> -        record_buf[index++] = reg_src1;
> -        record_buf[index++] = ARM_PS_REGNUM;
> -        arm_insn_r->reg_rec_count = index;
> +      /* Extra space for Base Register and CPSR; wihtout optimization.  */

s/wihtout/without/
  
Yao Qi Oct. 21, 2014, 1:03 p.m. UTC | #3
Luis Machado <lgustavo@codesourcery.com> writes:

> +      /* Handle bit P.  */
> +      if (bit (arm_insn_r->arm_insn, 24))
> +	{
> +	  record_buf_mem[1] = tgt_mem_addr;
> +	}
> +      else
> +	{
> +	   record_buf_mem[1] = (uint32_t) u_regval;
> +	}

These braces are not necessary.
  

Patch

2014-10-21  Luis Machado  <lgustavo@codesourcery.com>

	* arm-tdep.c (INSN_S_L_BIT_NUM): Document.
	(arm_record_ld_st_imm_offset): Reimplement to cover all
	load/store cases for ARM opcode 010.
	(arm_record_ld_st_multiple): Reimplement to cover all
	load/store cases for ARM opcode 100.

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index e2559ec..3a75d3f 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -10676,6 +10676,8 @@  vfp - VFP co-processor."),
 #define THUMB2_INSN_SIZE_BYTES 4
 
 
+/* Position of the bit within a 32-bit ARM instruction
+   that defines whether the instruction is a load or store.  */
 #define INSN_S_L_BIT_NUM 20
 
 #define REG_ALLOC(REGS, LENGTH, RECORD_BUF) \
@@ -11473,110 +11475,94 @@  arm_record_data_proc_imm (insn_decode_record *arm_insn_r)
   return 0;
 }
 
-/* Handling opcode 010 insns.  */
+/* Handle ARM mode instructions with opcode 010.  */
 
 static int
 arm_record_ld_st_imm_offset (insn_decode_record *arm_insn_r)
 {
   struct regcache *reg_cache = arm_insn_r->regcache;
 
-  uint32_t reg_src1 = 0 , reg_dest = 0;
-  uint32_t offset_12 = 0, tgt_mem_addr = 0;
+  uint32_t reg_base , reg_dest;
+  uint32_t offset_12, tgt_mem_addr;
   uint32_t record_buf[8], record_buf_mem[8];
+  unsigned char wback;
+  ULONGEST u_regval;
 
-  ULONGEST u_regval = 0;
+  /* Calculate wback.  */
+  wback = (bit (arm_insn_r->arm_insn, 24) == 0)
+	  || (bit (arm_insn_r->arm_insn, 21) == 1);
 
-  arm_insn_r->opcode = bits (arm_insn_r->arm_insn, 21, 24);
-  arm_insn_r->decode = bits (arm_insn_r->arm_insn, 4, 7);
+  arm_insn_r->reg_rec_count = 0;
+  reg_base = bits (arm_insn_r->arm_insn, 16, 19);
 
   if (bit (arm_insn_r->arm_insn, INSN_S_L_BIT_NUM))
     {
+      /* LDR (immediate), LDR (literal), LDRB (immediate), LDRB (literal), LDRBT
+	 and LDRT.  */
+
       reg_dest = bits (arm_insn_r->arm_insn, 12, 15);
-      /* LDR insn has a capability to do branching, if
-         MOV LR, PC is precedded by LDR insn having Rn as R15
-         in that case, it emulates branch and link insn, and hence we
-         need to save CSPR and PC as well.  */
-      if (ARM_PC_REGNUM != reg_dest)
-        {
-          record_buf[0] = bits (arm_insn_r->arm_insn, 12, 15);
-          arm_insn_r->reg_rec_count = 1;
-        }
-      else
-        {
-          record_buf[0] = reg_dest;
-          record_buf[1] = ARM_PS_REGNUM;
-          arm_insn_r->reg_rec_count = 2;
-        }
+      record_buf[arm_insn_r->reg_rec_count++] = reg_dest;
+
+      /* The LDR instruction is capable of doing branching.  If MOV LR, PC
+	 preceeds a LDR instruction having R15 as reg_base, it
+	 emulates a branch and link instruction, and hence we need to save
+	 CPSR and PC as well.  */
+      if (ARM_PC_REGNUM == reg_dest)
+	record_buf[arm_insn_r->reg_rec_count++] = ARM_PS_REGNUM;
+
+      /* If wback is true, also save the base register, which is going to be
+	 written to.  */
+      if (wback)
+	record_buf[arm_insn_r->reg_rec_count++] = reg_base;
     }
   else
     {
-      /* Store, immediate offset, immediate pre-indexed,
-         immediate post-indexed.  */
-      reg_src1 = bits (arm_insn_r->arm_insn, 16, 19);
+      /* STR (immediate), STRB (immediate), STRBT and STRT.  */
+
       offset_12 = bits (arm_insn_r->arm_insn, 0, 11);
-      regcache_raw_read_unsigned (reg_cache, reg_src1, &u_regval);
-      /* U == 1 */
+      regcache_raw_read_unsigned (reg_cache, reg_base, &u_regval);
+
+      /* Handle bit U.  */
       if (bit (arm_insn_r->arm_insn, 23))
-        {
-          tgt_mem_addr = u_regval + offset_12;
-        }
+	{
+	  /* U == 1: Add the offset. */
+	  tgt_mem_addr = (uint32_t) u_regval + offset_12;
+	}
       else
-        {
-          tgt_mem_addr = u_regval - offset_12;
-        }
+	{
+	  /* U == 0: subtract the offset. */
+	  tgt_mem_addr = (uint32_t) u_regval - offset_12;
+	}
+
+      /* Bit 22 tells us whether the store instruction writes 1 byte or 4
+	 bytes.  */
+      if (bit (arm_insn_r->arm_insn, 22))
+	{
+	  /* STRB and STRBT: 1 byte.  */
+	  record_buf_mem[0] = 1;
+	}
+      else
+	{
+	  /* STR and STRT: 4 bytes.  */
+	  record_buf_mem[0] = 4;
+	}
+
+      /* Handle bit P.  */
+      if (bit (arm_insn_r->arm_insn, 24))
+	{
+	  record_buf_mem[1] = tgt_mem_addr;
+	}
+      else
+	{
+	   record_buf_mem[1] = (uint32_t) u_regval;
+	}
 
-      switch (arm_insn_r->opcode)
-        {
-          /* STR.  */
-          case 8:
-          case 12:
-          /* STR.  */
-          case 9:
-          case 13:
-          /* STRT.  */    
-          case 1:
-          case 5:
-          /* STR.  */    
-          case 4:
-          case 0:
-            record_buf_mem[0] = 4;
-          break;
-
-          /* STRB.  */
-          case 10:
-          case 14:
-          /* STRB.  */    
-          case 11:
-          case 15:
-          /* STRBT.  */    
-          case 3:
-          case 7:
-          /* STRB.  */    
-          case 2:
-          case 6:
-            record_buf_mem[0] = 1;
-          break;
-
-          default:
-            gdb_assert_not_reached ("no decoding pattern found");
-          break;
-        }
-      record_buf_mem[1] = tgt_mem_addr;
       arm_insn_r->mem_rec_count = 1;
 
-      if (9 == arm_insn_r->opcode || 11 == arm_insn_r->opcode
-          || 13 == arm_insn_r->opcode || 15 == arm_insn_r->opcode
-          || 0 == arm_insn_r->opcode || 2 == arm_insn_r->opcode
-          || 4 == arm_insn_r->opcode || 6 == arm_insn_r->opcode
-          || 1 == arm_insn_r->opcode || 3 == arm_insn_r->opcode
-          || 5 == arm_insn_r->opcode || 7 == arm_insn_r->opcode
-         )
-        {
-          /* We are handling pre-indexed mode; post-indexed mode;
-             where Rn is going to be changed.  */
-          record_buf[0] = reg_src1;
-          arm_insn_r->reg_rec_count = 1;
-        }
+      /* If wback is true, also save the base register, which is going to be
+	 written to.  */
+      if (wback)
+	record_buf[arm_insn_r->reg_rec_count++] = reg_base;
     }
 
   REG_ALLOC (arm_insn_r->arm_regs, arm_insn_r->reg_rec_count, record_buf);
@@ -11847,134 +11833,95 @@  arm_record_ld_st_reg_offset (insn_decode_record *arm_insn_r)
   return 0;
 }
 
-/* Handling opcode 100 insns.  */
+/* Handle ARM mode instructions with opcode 100.  */
 
 static int
 arm_record_ld_st_multiple (insn_decode_record *arm_insn_r)
 {
   struct regcache *reg_cache = arm_insn_r->regcache;
-
-  uint32_t register_list[16] = {0}, register_count = 0, register_bits = 0;
-  uint32_t reg_src1 = 0, addr_mode = 0, no_of_regs = 0;
-  uint32_t start_address = 0, index = 0;
+  uint32_t register_count = 0, register_bits;
+  uint32_t reg_base, addr_mode;
   uint32_t record_buf[24], record_buf_mem[48];
+  uint32_t wback;
+  ULONGEST u_regval;
 
-  ULONGEST u_regval[2] = {0};
+  /* Fetch the list of registers.  */
+  register_bits = bits (arm_insn_r->arm_insn, 0, 15);
+  arm_insn_r->reg_rec_count = 0;
+
+  /* Fetch the base register that contains the address we are loading data
+     to.  */
+  reg_base = bits (arm_insn_r->arm_insn, 16, 19);
 
-  /* This mode is exclusively for load and store multiple.  */
-  /* Handle incremenrt after/before and decrment after.before mode;
-     Rn is changing depending on W bit, but as of now we store Rn too
-     without optimization.  */
+  /* Calculate wback.  */
+  wback = (bit (arm_insn_r->arm_insn, 24) == 0)
+	  || (bit (arm_insn_r->arm_insn, 21) == 1);
 
   if (bit (arm_insn_r->arm_insn, INSN_S_L_BIT_NUM))
     {
-      /* LDM  (1,2,3) where LDM  (3) changes CPSR too.  */
+      /* LDM/LDMIA/LDMFD, LDMDA/LDMFA, LDMDB and LDMIB.  */
 
-      if (bit (arm_insn_r->arm_insn, 20) && !bit (arm_insn_r->arm_insn, 22))
-        {
-          register_bits = bits (arm_insn_r->arm_insn, 0, 15);
-          no_of_regs = 15;
-        }
-      else
-        {
-          register_bits = bits (arm_insn_r->arm_insn, 0, 14);
-          no_of_regs = 14;
-        }
-      /* Get Rn.  */
-      reg_src1 = bits (arm_insn_r->arm_insn, 16, 19);
+      /* Find out which registers are going to be loaded from memory.  */
       while (register_bits)
-      {
-        if (register_bits & 0x00000001)
-          record_buf[index++] = register_count;
-        register_bits = register_bits >> 1;
-        register_count++;
-      }
+	{
+	  if (register_bits & 0x00000001)
+	    record_buf[arm_insn_r->reg_rec_count++] = register_count;
+	  register_bits = register_bits >> 1;
+	  register_count++;
+	}
 
-        /* Extra space for Base Register and CPSR; wihtout optimization.  */
-        record_buf[index++] = reg_src1;
-        record_buf[index++] = ARM_PS_REGNUM;
-        arm_insn_r->reg_rec_count = index;
+      /* Extra space for Base Register and CPSR; wihtout optimization.  */
+      record_buf[arm_insn_r->reg_rec_count++] = reg_base;
+      record_buf[arm_insn_r->reg_rec_count++] = ARM_PS_REGNUM;
     }
   else
     {
-      /* It handles both STM(1) and STM(2).  */
-      addr_mode = bits (arm_insn_r->arm_insn, 23, 24);    
+      /* STM (STMIA, STMEA), STMDA (STMED), STMDB (STMFD) and STMIB (STMFA).  */
 
-      register_bits = bits (arm_insn_r->arm_insn, 0, 15);
-      /* Get Rn.  */
-      reg_src1 = bits (arm_insn_r->arm_insn, 16, 19);
-      regcache_raw_read_unsigned (reg_cache, reg_src1, &u_regval[0]);
+      addr_mode = bits (arm_insn_r->arm_insn, 23, 24); 
+
+      regcache_raw_read_unsigned (reg_cache, reg_base, &u_regval);
+
+      /* Find out how many registers are going to be stored to memory.  */
       while (register_bits)
-        {
-          if (register_bits & 0x00000001)
-            register_count++;
-          register_bits = register_bits >> 1;
-        }
+	{
+	  if (register_bits & 0x00000001)
+	    register_count++;
+	  register_bits = register_bits >> 1;
+	}
 
       switch (addr_mode)
-        {
-          /* Decrement after.  */
-          case 0:                          
-            start_address = (u_regval[0]) - (register_count * 4) + 4;
-            arm_insn_r->mem_rec_count = register_count;
-            while (register_count)
-              {
-                record_buf_mem[(register_count * 2) - 1] = start_address;
-                record_buf_mem[(register_count * 2) - 2] = 4;
-                start_address = start_address + 4;
-                register_count--;
-              }
-          break;    
-
-          /* Increment after.  */
-          case 1:
-            start_address = u_regval[0];
-            arm_insn_r->mem_rec_count = register_count;
-            while (register_count)
-              {
-                record_buf_mem[(register_count * 2) - 1] = start_address;
-                record_buf_mem[(register_count * 2) - 2] = 4;
-                start_address = start_address + 4;
-                register_count--;
-              }
-          break;    
-
-          /* Decrement before.  */
-          case 2:
-
-            start_address = (u_regval[0]) - (register_count * 4);
-            arm_insn_r->mem_rec_count = register_count;
-            while (register_count)
-              {
-                record_buf_mem[(register_count * 2) - 1] = start_address;
-                record_buf_mem[(register_count * 2) - 2] = 4;
-                start_address = start_address + 4;
-                register_count--;
-              }
-          break;    
-
-          /* Increment before.  */
-          case 3:
-            start_address = u_regval[0] + 4;
-            arm_insn_r->mem_rec_count = register_count;
-            while (register_count)
-              {
-                record_buf_mem[(register_count * 2) - 1] = start_address;
-                record_buf_mem[(register_count * 2) - 2] = 4;
-                start_address = start_address + 4;
-                register_count--;
-              }
-          break;    
-
-          default:
-            gdb_assert_not_reached ("no decoding pattern found");
-          break;    
-        }
+	{
+	  /* STMDA (STMED): Decrement after.  */
+	  case 0:
+	  record_buf_mem[1] = (uint32_t) u_regval
+			      - register_count * INT_REGISTER_SIZE + 4;
+	  break;
+	  /* STM (STMIA, STMEA): Increment after.  */
+	  case 1:
+	  record_buf_mem[1] = (uint32_t) u_regval;
+	  break;
+	  /* STMDB (STMFD): Decrement before.  */
+	  case 2:
+	  record_buf_mem[1] = (uint32_t) u_regval
+			      - register_count * INT_REGISTER_SIZE;
+	  break;
+	  /* STMIB (STMFA): Increment before.  */
+	  case 3:
+	  record_buf_mem[1] = (uint32_t) u_regval + INT_REGISTER_SIZE;
+	  break;
+	  default:
+	    gdb_assert_not_reached ("no decoding pattern found");
+	  break;
+	}
 
-      /* Base register also changes; based on condition and W bit.  */
-      /* We save it anyway without optimization.  */
-      record_buf[0] = reg_src1;
-      arm_insn_r->reg_rec_count = 1;
+      record_buf_mem[0] = register_count * INT_REGISTER_SIZE;
+      arm_insn_r->mem_rec_count = 1;
+
+      /* If wback is true, also save the base register, which is going to be
+	 written to.  */
+      if (wback)
+	record_buf[arm_insn_r->reg_rec_count++] = reg_base;
     }
 
   REG_ALLOC (arm_insn_r->arm_regs, arm_insn_r->reg_rec_count, record_buf);