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

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

[Terry Guo]

Hello,

This patch addresses the bug in gdb/12703 which sets two different function
breakpoints at same pc address. In this patch I enhanced the way to analyze
the ARM thumb prologue to prevent the function breakpoint from being set
outside the function body.

Patch has been tested against arm-none-eabi with no regressions. OK for
commit?

Thanks,

Terry

gdb/ChangLog:
2011-06-16  Terry Guo  <terry.guo@arm.com>

        PR gdb/12703
        * arm-tdep.c (arm_skip_prologue): Don't scan beyond the end of
        the current function.

gdb/testsuite/ChangLog:
2011-06-16  Terry Guo  <terry.guo@arm.com>

        PR gdb/12703
        * gdb.base/break-function.c: New testcase.
        * gdb.base/break-function.exp: New script.

diff --git gdb/arm-tdep.c gdb/arm-tdep.c
index 2dd8c9e..c188576 100644
--- gdb/arm-tdep.c
+++ gdb/arm-tdep.c
@@ -1372,13 +1372,13 @@ arm_skip_prologue (struct gdbarch *gdbarch,
CORE_ADDR pc)
   enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code
(gdbarch);
   unsigned long inst;
   CORE_ADDR skip_pc;
-  CORE_ADDR func_addr, limit_pc;
+  CORE_ADDR func_addr, limit_pc, end_pc;
   struct symtab_and_line sal;
 
   /* See if we can determine the end of the prologue via the symbol table.
      If so, then return either PC, or the PC after the prologue, whichever
      is greater.  */
-  if (find_pc_partial_function (pc, NULL, &func_addr, NULL))
+  if (find_pc_partial_function (pc, NULL, &func_addr, &end_pc))
     {
       CORE_ADDR post_prologue_pc
 	= skip_prologue_using_sal (gdbarch, func_addr);
@@ -1439,6 +1439,9 @@ arm_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR
pc)
   if (limit_pc == 0)
     limit_pc = pc + 64;          /* Magic.  */
 
+  /* Don't scan beyond the end of the current function.  */
+  if (limit_pc > end_pc)
+    limit_pc = end_pc;
 
   /* Check if this is Thumb code.  */
   if (arm_pc_is_thumb (gdbarch, pc))
diff --git gdb/testsuite/gdb.base/break-function.c
gdb/testsuite/gdb.base/break-function.c
new file mode 100644
index 0000000..e24cf91
--- /dev/null
+++ gdb/testsuite/gdb.base/break-function.c
@@ -0,0 +1,47 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 1992, 1993, 1994, 1995, 1999, 2002, 2003, 2007, 2008, 2009,
2010,
+   2011 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
*/
+
+
+unsigned long _etext;
+unsigned long _data;
+unsigned long _edata;
+
+void foo(void)
+{
+  while(1)
+  {
+  }
+} /* End of function foo  */
+
+void bar(void)
+{
+    unsigned long *pulSrc, *pulDest;
+
+    pulSrc = &_etext;
+    for(pulDest = &_data; pulDest < &_edata; )
+    {
+        *pulDest++ = *pulSrc++;
+    }
+}
+
+
+int main()
+{
+  bar();
+  foo();  
+}
diff --git gdb/testsuite/gdb.base/break-function.exp
gdb/testsuite/gdb.base/break-function.exp
new file mode 100644
index 0000000..a42ba79
--- /dev/null
+++ gdb/testsuite/gdb.base/break-function.exp
@@ -0,0 +1,35 @@
+#   Copyright 1988, 1990, 1991, 1992, 1994, 1995, 1996, 1997, 1998, 1999,
+#   2000, 2002, 2003, 2007, 2008, 2009, 2010, 2011
+#   Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+set srcfile break-function.c
+
+if { [prepare_for_testing break-function.exp "break-function"
{break-function.c}] } {
+    return -1
+}
+
+set bp_location_boundary [gdb_get_line_number "End of function foo"]
+
+gdb_test_multiple "b foo" "Set breakpoint for function foo" {
+     -re "Breakpoint 1 at.*file.*, line (\[0-9\]+).*" {
+        set bp_pos $expect_out(1,string);       
+        if { $bp_pos > $bp_location_boundary } {
+            fail "The location of function breakpoint exceeds the body of
the function.\n"
+          } else {          
+            pass "PASS";
+          }
+     }
+}
-- 
1.7.1

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

[Yao Qi]

On 06/24/2011 10:30 AM, Terry Guo wrote:
> Hello,
> 
> This patch addresses the bug in gdb/12703 which sets two different function
> breakpoints at same pc address. In this patch I enhanced the way to analyze
> the ARM thumb prologue to prevent the function breakpoint from being set
> outside the function body.
> 
> Patch has been tested against arm-none-eabi with no regressions. OK for
> commit?
> 
> Thanks,
> 
> Terry
> 

I am not the people to approve or reject this patch.  My $0.2 here.

> gdb/ChangLog:
> 2011-06-16  Terry Guo  <terry.guo@arm.com>
> 
>         PR gdb/12703
>         * arm-tdep.c (arm_skip_prologue): Don't scan beyond the end of
>         the current function.
> 
> gdb/testsuite/ChangLog:
> 2011-06-16  Terry Guo  <terry.guo@arm.com>
> 
>         PR gdb/12703
>         * gdb.base/break-function.c: New testcase.
>         * gdb.base/break-function.exp: New script.

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.

> --- /dev/null
> +++ gdb/testsuite/gdb.base/break-function.c
> @@ -0,0 +1,47 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 1992, 1993, 1994, 1995, 1999, 2002, 2003, 2007, 2008, 2009,
> 2010,
> +   2011 Free Software Foundation, Inc.
> +

Your test case is a new one, so the year of copyright should be 2011.
Please remove the rest of them.

> +
> +
> +unsigned long _etext;
> +unsigned long _data;
> +unsigned long _edata;
> +
> +void foo(void)

This doesn't comply to GNU coding standard.  Please move "foo ()" to
next line.

> +{
> +  while(1)
          ^ need a space here.
> +  {
> +  }
> +} /* End of function foo  */

Put a "." at the end of comment with two spaces, or one space without ".".

> +
> +void bar(void)

"bar" should be move to next newline.  A space is needed ater "bar".
> +{
> +    unsigned long *pulSrc, *pulDest;
> +
> +    pulSrc = &_etext;
> +    for(pulDest = &_data; pulDest < &_edata; )
> +    {
> +        *pulDest++ = *pulSrc++;
> +    }
> +}

Indentation is not correct here.

> +#   Copyright 1988, 1990, 1991, 1992, 1994, 1995, 1996, 1997, 1998, 1999,
> +#   2000, 2002, 2003, 2007, 2008, 2009, 2010, 2011
> +#   Free Software Foundation, Inc.
> +

Only 2011 is needed here, because this case is created in 2011.

> +
> +set srcfile break-function.c
> +
> +if { [prepare_for_testing break-function.exp "break-function"
> {break-function.c}] } {
> +    return -1
> +}
> +
> +set bp_location_boundary [gdb_get_line_number "End of function foo"]
> +
> +gdb_test_multiple "b foo" "Set breakpoint for function foo" {
> +     -re "Breakpoint 1 at.*file.*, line (\[0-9\]+).*" {

When using gdb_test_multiple, "$gdb_prompt $" is needed at the end of
your regex.  Please reference gdb_test_multiple used in other places.

> +        set bp_pos $expect_out(1,string);       
> +        if { $bp_pos > $bp_location_boundary } {
> +            fail "The location of function breakpoint exceeds the body of
> the function.\n"
> +          } else {          
> +            pass "PASS";

The PASS message and FAIL message should be the same.  I had the same
problem in my patch yesterday. :)

-- 
Yao (齐尧)

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

[Pedro Alves]

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.

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>)

> > +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.  :-)

-- 
Pedro Alves

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

[Yao Qi]

[-- Attachment #1: Type: text/plain, Size: 2626 bytes --]

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 (齐尧)

[-- Attachment #2: thumb_skip_prologue.patch --]
[-- Type: text/x-patch, Size: 647 bytes --]

	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,

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

[Yao Qi]

On 06/24/2011 06:38 PM, Yao Qi wrote:
>> > 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.
> 

When talking with Terry Guo on HelloGCC workshop last Saturday in
Beijing, it reminds me that I still have a patch pending there, and
forget to ping it.

  http://sourceware.org/ml/gdb-patches/2011-06/msg00370.html

I regression tested this patch on armv7l-unknown-linux-gnueabi with
{-mthumb, -marm}, no new fails.  OK for mainline?

> 	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,

-- 
Yao (齐尧)

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

[Yao Qi]

On 09/27/2011 06:44 PM, Yao Qi wrote:
>   http://sourceware.org/ml/gdb-patches/2011-06/msg00370.html
> 
> I regression tested this patch on armv7l-unknown-linux-gnueabi with
> {-mthumb, -marm}, no new fails.  OK for mainline?
> 

Ping again.

>> > 	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,


-- 
Yao (齐尧)

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

[Ulrich Weigand]

Yao Qi wrote:

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

Huh.  That check is indeed wrong.  

> -      else if ((insn & 0xe000) == 0xe000)
> +      else if ((insn & 0xe000) == 0xe000 && (insn & 0x1800) != 0)
>  	{

Instead of open-coding the check, I think it would be preferable to
use the thumb_insn_size routine instead.

Note that there are a number of other places that either already open-
code the correct check, or -worse- use the same incorrect check as the
code above:

- thumb_in_function_epilogue_p
- thumb_get_next_pc_raw
- arm_breakpoint_from_pc

Would you mind converting them all to thumb_insn_size?

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

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

[Yao Qi]

On 10/10/2011 10:46 PM, Ulrich Weigand wrote:
>> > -      else if ((insn & 0xe000) == 0xe000)
>> > +      else if ((insn & 0xe000) == 0xe000 && (insn & 0x1800) != 0)
>> >  	{
> Instead of open-coding the check, I think it would be preferable to
> use the thumb_insn_size routine instead.
> 
> Note that there are a number of other places that either already open-
> code the correct check, or -worse- use the same incorrect check as the
> code above:
> 
> - thumb_in_function_epilogue_p
> - thumb_get_next_pc_raw
> - arm_breakpoint_from_pc
> 
> Would you mind converting them all to thumb_insn_size?

Yeah, we should replace these checks around many places with
thumb_insn_size, I agree.  Here is the patch for this purpose.
Regression tested on arm-linux-gnueabi with both -marm and -mthumb.
OK for mainline?

-- 
Yao (齐尧) 

	PR gdb/12703
	* arm-tdep.c (thumb_analyze_prologue): Call thumb_insn_size to check
	whether insn is a 32-bit Thumb-2 instruction.
	(thumb_in_function_epilogue_p): Likewise.
	(thumb_get_next_pc_raw): Likewise.
	(arm_breakpoint_from_pc): Likewise.

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 278e6e9..0db8b5f 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -231,6 +231,8 @@ static void arm_neon_quad_write (struct gdbarch *gdbarch,
 				 struct regcache *regcache,
 				 int regnum, const gdb_byte *buf);
 
+static int thumb_insn_size (unsigned short inst1);
+
 struct arm_prologue_cache
 {
   /* The stack pointer at the time this frame was created; i.e. the
@@ -836,7 +838,7 @@ 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 (thumb_insn_size (insn) == 4) /* 32-bit Thumb-2 instructions.  */
 	{
 	  unsigned short inst2;
 
@@ -3093,7 +3095,7 @@ thumb_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
 	  if (insn & 0x0100)  /* <registers> include PC.  */
 	    found_return = 1;
 	}
-      else if ((insn & 0xe000) == 0xe000)  /* 32-bit Thumb-2 instruction */
+      else if (thumb_insn_size (insn) == 4)  /* 32-bit Thumb-2 instruction */
 	{
 	  if (target_read_memory (scan_pc, buf, 2))
 	    break;
@@ -4335,14 +4337,9 @@ thumb_get_next_pc_raw (struct frame_info *frame, CORE_ADDR pc)
       int cond = itstate >> 4;
 
       if (! condition_true (cond, status))
-	{
-	  /* Advance to the next instruction.  All the 32-bit
-	     instructions share a common prefix.  */
-	  if ((inst1 & 0xe000) == 0xe000 && (inst1 & 0x1800) != 0)
-	    return MAKE_THUMB_ADDR (pc + 4);
-	  else
-	    return MAKE_THUMB_ADDR (pc + 2);
-	}
+	/* Advance to the next instruction.  All the 32-bit
+	   instructions share a common prefix.  */
+	return MAKE_THUMB_ADDR (pc + thumb_insn_size (inst1));
 
       /* Otherwise, handle the instruction normally.  */
     }
@@ -4376,7 +4373,7 @@ thumb_get_next_pc_raw (struct frame_info *frame, CORE_ADDR pc)
     {
       nextpc = pc_val + (sbits (inst1, 0, 10) << 1);
     }
-  else if ((inst1 & 0xe000) == 0xe000) /* 32-bit instruction */
+  else if (thumb_insn_size (inst1) == 4) /* 32-bit instruction */
     {
       unsigned short inst2;
       inst2 = read_memory_unsigned_integer (pc + 2, 2, byte_order_for_code);
@@ -8473,7 +8470,7 @@ arm_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr, int *lenptr)
 	    {
 	      unsigned short inst1;
 	      inst1 = extract_unsigned_integer (buf, 2, byte_order_for_code);
-	      if ((inst1 & 0xe000) == 0xe000 && (inst1 & 0x1800) != 0)
+	      if (thumb_insn_size (inst1) == 4)
 		{
 		  *lenptr = tdep->thumb2_breakpoint_size;
 		  return tdep->thumb2_breakpoint;

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

[Ulrich Weigand]

Yao Qi wrote:

> 	PR gdb/12703
> 	* arm-tdep.c (thumb_analyze_prologue): Call thumb_insn_size to check
> 	whether insn is a 32-bit Thumb-2 instruction.
> 	(thumb_in_function_epilogue_p): Likewise.
> 	(thumb_get_next_pc_raw): Likewise.
> 	(arm_breakpoint_from_pc): Likewise.

This is OK.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

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

[Yao Qi]

On 10/12/2011 07:58 PM, Ulrich Weigand wrote:
> Yao Qi wrote:
> 
>> 	PR gdb/12703
>> 	* arm-tdep.c (thumb_analyze_prologue): Call thumb_insn_size to check
>> 	whether insn is a 32-bit Thumb-2 instruction.
>> 	(thumb_in_function_epilogue_p): Likewise.
>> 	(thumb_get_next_pc_raw): Likewise.
>> 	(arm_breakpoint_from_pc): Likewise.
> 
> This is OK.
> 

Committed.  http://sourceware.org/ml/gdb-cvs/2011-10/msg00107.html

-- 
Yao (齐尧)