Re: [PATCH] Fix that different function breakpoints are set at same pc address (PR gdb/12703)

[Yao Qi <yao at codesourcery dot com>]

On 06/24/2011 04:59 PM, Pedro Alves wrote:
> On Friday 24 June 2011 04:55:06, Yao Qi wrote:
>> On 06/24/2011 10:30 AM, Terry Guo wrote:
> 
>>
>> IMO, this is a target-specific bug, so this PR's component should be
>> tdept, so it should be "PR tdept/12703" instead of "PR gdb/12703".
>>
>> I'd move your test cases break-function.{c,exp} to gdb.arch/ dir,
>> because it is target-dependent fix.  I am sure this case is useful to
>> other ports.
> 
> The testcase might help catch the same issue in other archs.
> IMO, it should stay generic if possible.
> 

OK.  Let us leave it in gdb.base.  I suggest that test case can be
renamed to reflect what we want to test here, such as
"break-outside-function.exp".

> I agree with Yao when he says in the PR that there seems to be
> some other root cause for the bug.  Shouldn't
> thumb_instruction_changes_pc have caught that "b.n" ?
> 
> 00008160 <fault_isr>:
>     8160:    e7fe          b.n    8160 <fault_isr>
>     ...
> 
> 00008164 <reset_isr>:
>     8164:    4a05          ldr    r2, [pc, #20]    ; (817c <reset_isr+0x18>)
> 

thumb_instruction_changes_pc can handle "b.n".  AFAICS, the problem is
in thumb_analyze_prologue.  In thumb_analyze_prologue, there are a lot
if/else branches, like below,

      else if ((insn & 0xe000) == 0xe000)  // <-- [1]
	{
          ....
	  else if (thumb2_instruction_changes_pc (insn, inst2))
	    {
	      /* Don't scan past anything that might change control flow.  */
	      break;
	    }
	  else
	    {
	      /* The optimizer might shove anything into the prologue,
		 so we just skip what we don't recognize.  */
	      unrecognized_pc = start;
	    }

	  start += 2;
	}
      else if (thumb_instruction_changes_pc (insn))
	{
	  /* Don't scan past anything that might change control flow.  */
	  break;
	}

The instruction "b.n 8160" is 0xe7fe, so condition check [1] is true,
and thumb_instruction_changes_pc is unreachable.  This is cause of this
problem, I doubt.


The line of code [1] is discussed in this patch

  [rfa] ARM prologue parsing support for Thumb-2 instructions
  http://sourceware.org/ml/gdb-patches/2010-10/msg00132.html

IIUC, condition check [1] is for 32-bit Thumb-2 instructions (I may be
wrong, of course).  I have an untested patch.

>>> +void foo(void)
>>
>> This doesn't comply to GNU coding standard.  Please move "foo ()" to
>> next line.
> 
> Note that test code does not strictly _need_ to follow the
> coding standards.  Though it's indeed nice when it does.
> GDB should be able to debug non-GNU code too.  :-)
> 

Oh, I don't know that.  Sorry about the noise I made here.

-- 
Yao (éå)

	gdb/
	* arm-tdep.c (thumb_analyze_prologue): Check condition for 32-bit
	Thumb-2 instructions. 

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 2dd8c9e..7f5a0e1 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -832,8 +832,9 @@ thumb_analyze_prologue (struct gdbarch *gdbarch,
 	  constant = read_memory_unsigned_integer (loc, 4, byte_order);
 	  regs[bits (insn, 8, 10)] = pv_constant (constant);
 	}
-      else if ((insn & 0xe000) == 0xe000)
+      else if ((insn & 0xe000) == 0xe000 && (insn & 0x1800) != 0)
 	{
+	  /* 32-bit Thumb-2 instructions.  */
 	  unsigned short inst2;
 
 	  inst2 = read_memory_unsigned_integer (start + 2, 2,