diff mbox

Fix ARM machine state testcase failures

Message ID 543FF6DD.3070006@codesourcery.com
State New
Headers show

Commit Message

Luis Machado Oct. 16, 2014, 4:48 p.m. UTC
Hi,

When running GDB's reverse debugging testsuite against a few ARM 
multilibs, i noticed failures in the machinestate* testcases.

Further investigation showed that push and pop instruction encodings A1 
and A2 were not being handled properly, thus we missed saving important 
contents from registers and memory. When going backwards, such contents 
were not restored and thus we ended up with a corrupted state that did 
not correspond to the real values we had at a particular point in time.

Attached is a patch that fixes around 36 failures for both 
gdb.reverse/machinestate.exp and gdb.reverse/machinestate-precsave.exp 
testcases, making them fully pass. This is for both armv7 and armv4. I 
still see failures for armv4 thumb though, so it needs a bit more 
investigation.

I see no regressions due to this patch for armv7, armv7 thumb, armv4 and 
armv4 thumb.

Ok for trunk?

Regards,
Luis

Comments

Yao Qi Oct. 17, 2014, 8:35 a.m. UTC | #1
Luis Machado <lgustavo@codesourcery.com> writes:

> Further investigation showed that push and pop instruction encodings
> A1 and A2 were not being handled properly, thus we missed saving
> important contents from registers and memory. When going backwards,
> such contents were not restored and thus we ended up with a corrupted
> state that did not correspond to the real values we had at a
> particular point in time.

Luis,
I agree on your analysis, but I have a question to the code.  Do we
really need to handle PUSH and POP as a special case in LDR/STR
instructions?  AFAICS, PUSH/POP is just one kind of LDR/STR instruction
with post-indexed addressing.  Probably, we just need to tweak
arm_record_ld_st_imm_offset and arm_record_ld_st_multiple for
post-indexed addressing, at least LDR with post-indexed are not
handled in arm_record_ld_st_imm_offset.  What do you think?
Luis Machado Oct. 17, 2014, 1:44 p.m. UTC | #2
Hi Yao,

On 10/17/2014 05:35 AM, Yao Qi wrote:
> Luis Machado <lgustavo@codesourcery.com> writes:
>
>> Further investigation showed that push and pop instruction encodings
>> A1 and A2 were not being handled properly, thus we missed saving
>> important contents from registers and memory. When going backwards,
>> such contents were not restored and thus we ended up with a corrupted
>> state that did not correspond to the real values we had at a
>> particular point in time.
>
> Luis,
> I agree on your analysis, but I have a question to the code.  Do we
> really need to handle PUSH and POP as a special case in LDR/STR
> instructions?  AFAICS, PUSH/POP is just one kind of LDR/STR instruction
> with post-indexed addressing.  Probably, we just need to tweak
> arm_record_ld_st_imm_offset and arm_record_ld_st_multiple for
> post-indexed addressing, at least LDR with post-indexed are not
> handled in arm_record_ld_st_imm_offset.  What do you think?
>

Can PUSH/POP always be described as a LD/ST instruction though? The 
documentation seems to indicate this is not always the case.

For POP, we have two encodings: A1 and A2. For A1, the documentation [1] 
states that we should use LDM/LDMIA/LDFD if BitCount(register_list) < 2. 
I interpreted this as "Unless we are popping only a single register, we 
should use the POP instruction".

For PUSH, it seems to be the same case, but with STMDB/STMFD.

Disassembler output sometimes shows push/pop alongside the equivalent 
ld/st syntax, but other times only push/pop is shown.

Base on the above, there is indeed a little overlap between the handling 
of st/ld and push/pop instructions. For the sake of better code 
organization and readability, i went for special-casing push/pop and 
making it immediately obvious what we were handling, given the amount of 
ARM encodings we need to handle.

Does it make sense?

[1] ARM Architecture Reference Manual - ARMv7-A and ARMv7-R edition
Yao Qi Oct. 20, 2014, 3:56 a.m. UTC | #3
Luis Machado <lgustavo@codesourcery.com> writes:

> Can PUSH/POP always be described as a LD/ST instruction though? The

Yes, I think so.  PUSH/POP is the special case of LD/ST instruction, in
terms of instruction encoding, see details blow.

> documentation seems to indicate this is not always the case.
>
> For POP, we have two encodings: A1 and A2. For A1, the documentation
> [1] states that we should use LDM/LDMIA/LDFD if
> BitCount(register_list) < 2. I interpreted this as "Unless we are
> popping only a single register, we should use the POP instruction".

The doc suggests what instructions to use here, nothing to do with
instruction encoding.

 - POP A2 (POP instruction encoding A2) is a special case of LDR
   (immediate, ARM) A1.

   LDR (immediate, ARM), Encoding A1:
   cond  0 1 0 P U 0 W 1 Rn       Rt  imm12
   POP Encoding A2:
   cond  0 1 0 0 1 0 0 1 1 1 0 1  Rt 0 0 0 0 0 0 0 0 0 1 0 0

   we decode POP A2 as a LDR (immediate) A1, that means, P = 0, U = 1,
   W = 0, Rn = 13, imm12 = 4.  Then, index = false, add = true, wback =
   true,

   This is the post-indexed addressing.  IOW, "pop {r1}" is the same
   encoding as "ldr r1, [sp], #4".

 - POP A1 is a special case of LDMIA A1.

   LDMIA Encoding A1:
   cond 1 0 0 0 1 0 W 1 Rn        register_list
   POP Encoding A1:
   cond 1 0 0 0 1 0 1 1 (1 1 0 1) register_list

   we code POP A1 as a LDIMA, that means W = 1, Rn = 13.  IOW,
   "pop {r1, r2}" encoding is the same as "ldmia sp!, {r1, r2}".

 - PUSH A1 is a special case of STMDB A1.

   STMDB A1:
   cond 1 0 0 1 0 0 W 0 Rn      register_list

   PUSH A1:
   cond 1 0 0 1 0 0 1 0 1 1 0 1 register_list

   we decode PUSH A1 as STMDB A1, that means W = 1, Rn = 13.
   "push {r1, r2}" is the same encoding as "stmdb sp!, {r1, r2}".

 - Push A2 is a special case of STR (immediate ARM) A1.

   STR (immediate) A1:
   cond 0 1 0 P U 0 W 0 Rn      Rt imm12

   PUSH A2:
   cond 0 1 0 1 0 0 1 0 1 1 0 1 Rt 0 0 0 0 0 0 0 0 0 1 0 0

   we code PUSH A2 as STR (immediate) A1, then P = 1, U = 0, W = 1, Rn =
   13, imm12 = 4.  This is the STR with pre-indexed addressing mode.  Instruction
   "push {r1}" encoding is the same as "str r1, [sp, #-4]!".

IMO, we should fix gdb handling ld/st instructions in reverse debugging,
instead of handling only push/pop instructions.
diff mbox

Patch

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

	* arm-tdep.c (POP_A2, PUSH_A2): New #defines.
	(arm_record_ld_st_imm_offset): Handle single-register ARM
	mode push and pop instructions.
	(POP_A1, PUSH_A1, PUSH_POP_SP_REG_MASK): New #defines.
	(arm_record_ld_st_multiple): Handle multi-register ARM
	mode push and pop instructions.

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index e2559ec..c488906 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -11473,6 +11473,10 @@  arm_record_data_proc_imm (insn_decode_record *arm_insn_r)
   return 0;
 }
 
+/* Masks for PUSH and POP instructions that take a single register.  */
+#define POP_A2	0x049d0004 /* POP<c> <registers> */
+#define PUSH_A2 0x052d0004 /* PUSH<c> <registers> */
+
 /* Handling opcode 010 insns.  */
 
 static int
@@ -11489,7 +11493,39 @@  arm_record_ld_st_imm_offset (insn_decode_record *arm_insn_r)
   arm_insn_r->opcode = bits (arm_insn_r->arm_insn, 21, 24);
   arm_insn_r->decode = bits (arm_insn_r->arm_insn, 4, 7);
 
-  if (bit (arm_insn_r->arm_insn, INSN_S_L_BIT_NUM))
+  if ((arm_insn_r->arm_insn & POP_A2) == POP_A2)
+    {
+      /* Handle single-register pop.  */
+      int regno = bits (arm_insn_r->arm_insn, 12, 15);
+
+      record_buf[0] = regno;
+      arm_insn_r->reg_rec_count = 1;
+
+      /* The stack pointer register is always modified.  Check if we have
+	 already saved it though.  */
+      if (regno != ARM_SP_REGNUM)
+	{
+	  record_buf[1] = ARM_SP_REGNUM;
+	  arm_insn_r->reg_rec_count = 2;
+	}
+    }
+  else if ((arm_insn_r->arm_insn & PUSH_A2) == PUSH_A2)
+    {
+      /* Handle single-register push.  */
+
+      /* The stack pointer register is always updated.  */
+      record_buf[0] = ARM_SP_REGNUM;
+      arm_insn_r->reg_rec_count = 1;
+
+      /* The pushed register goes into the stack, so we need to record that
+	 chunk of memory as well.  */
+      regcache_raw_read_unsigned (reg_cache, ARM_SP_REGNUM, &u_regval);
+      tgt_mem_addr = (uint32_t) u_regval;
+      record_buf_mem[0] = INT_REGISTER_SIZE;
+      record_buf_mem[1] = tgt_mem_addr - INT_REGISTER_SIZE;
+      arm_insn_r->mem_rec_count = 1;
+    }
+  else if (bit (arm_insn_r->arm_insn, INSN_S_L_BIT_NUM))
     {
       reg_dest = bits (arm_insn_r->arm_insn, 12, 15);
       /* LDR insn has a capability to do branching, if
@@ -11847,6 +11883,11 @@  arm_record_ld_st_reg_offset (insn_decode_record *arm_insn_r)
   return 0;
 }
 
+/* Masks for PUSH and POP instructions that take multiple registers.  */
+#define POP_A1	0x08bd0000 /* POP<c> <registers> */
+#define PUSH_A1 0x092d0000 /* PUSH<c> <registers> */
+#define PUSH_POP_SP_REG_MASK 0x00002000
+
 /* Handling opcode 100 insns.  */
 
 static int
@@ -11861,12 +11902,66 @@  arm_record_ld_st_multiple (insn_decode_record *arm_insn_r)
 
   ULONGEST u_regval[2] = {0};
 
+  if ((arm_insn_r->arm_insn & POP_A1) == POP_A1)
+    {
+      /* Handle multi-register pop.  */
+      int regno = 0;
+
+      register_bits = bits (arm_insn_r->arm_insn, 0, 15);
+
+      /* Record which registers are going to be modified.  */
+      while (register_bits)
+	{
+	  if (register_bits & 0x00000001)
+	    record_buf[arm_insn_r->reg_rec_count++] = regno;
+	  register_bits = register_bits >> 1;
+	  regno++;
+	}
+
+      /* The stack pointer register is always modified.  Check if we have
+	 already saved it though.  */
+      if ((bits (arm_insn_r->arm_insn, 0, 15) & PUSH_POP_SP_REG_MASK) == 0)
+	{
+	  record_buf[arm_insn_r->reg_rec_count] = ARM_SP_REGNUM;
+	  arm_insn_r->reg_rec_count++;
+	}
+    }
+  else if ((arm_insn_r->arm_insn & PUSH_A1) == PUSH_A1)
+    {
+      /* Handle multi-register push.  */
+      int mem_chunks = 0;
+      uint32_t sp_addr;
+      ULONGEST u_regval;
+
+      register_bits = bits (arm_insn_r->arm_insn, 0, 15);
+      regcache_raw_read_unsigned (reg_cache, ARM_SP_REGNUM, &u_regval);
+      sp_addr = (uint32_t) u_regval;
+
+      /* The stack pointer register is always modified.  */
+      record_buf[0] = ARM_SP_REGNUM;
+      arm_insn_r->reg_rec_count = 1;
+
+      /* The pushed registers go into the stack, so we need to record those
+	 chunks of memory as well.  Count how many registers are going to be
+	 saved.  */
+      while (register_bits)
+	{
+	  if (register_bits & 0x00000001)
+	    mem_chunks++;
+
+	  register_bits = register_bits >> 1;
+	}
+
+      /* Record the chunks in one continuous block.  */
+      record_buf_mem[0] = mem_chunks * INT_REGISTER_SIZE;
+      record_buf_mem[1] = sp_addr - mem_chunks * INT_REGISTER_SIZE;
+      arm_insn_r->mem_rec_count = 1;
+    }
   /* 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.  */
-
-  if (bit (arm_insn_r->arm_insn, INSN_S_L_BIT_NUM))
+  else if (bit (arm_insn_r->arm_insn, INSN_S_L_BIT_NUM))
     {
       /* LDM  (1,2,3) where LDM  (3) changes CPSR too.  */