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

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

[Terry Guo]

Hi Yao and Pedro,

Thanks for your helpful comments. I did learnt a lot from them. After some
further investigation, I agree with Yao's patch and think it is better than
mine. Yao's patch also fixed another function break point issue which I will
show later. I performed "make check" for Yao's patch on QEMU and no
regression found. I am reworking the patch according to the comments.

I agree with the root cause is the function thumb_instruction_changes_pc
fails to catch the b.n instruction. And this is caused by check [1] as
pointed by Yao. Here is the disassembly code of function bar:

00008146 <bar>:
    8146:       b510            push    {r4, lr}
    8148:       2200            movs    r2, #0
    814a:       4905            ldr     r1, [pc, #20]   ; (8160 <bar+0x1a>)
    814c:       4c05            ldr     r4, [pc, #20]   ; (8164 <bar+0x1e>)
    814e:       4806            ldr     r0, [pc, #24]   ; (8168 <bar+0x22>)
    8150:       e002            b.n     8158 <bar+0x12>
    8152:       5813            ldr     r3, [r2, r0]
    8154:       5053            str     r3, [r2, r1]
    8156:       3204            adds    r2, #4
    8158:       1853            adds    r3, r2, r1
    815a:       42a3            cmp     r3, r4
    815c:       d3f9            bcc.n   8152 <bar+0xc>
    815e:       bd10            pop     {r4, pc}

With or without my previous patch, the command "b bar" will set break point
at location 0x8158. With Yao's patch, the break point will be set at
location 0x8150 because the thumb_instruction_changes_pc catches the b.n
instruction. That's also why I think Yao's patch is better.

Best regards,
Terry

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

[Terry Guo]

Hi,

I slightly modified the test case and got the below code:

00008142 <test>:
    8142:       250a            movs    r5, #10

00008144 <bar>:
    8144:       b510            push    {r4, lr}
    8146:       2300            movs    r3, #0
    8148:       4c07            ldr     r4, [pc, #28]   ; (8168 <bar+0x24>)
    814a:       4908            ldr     r1, [pc, #32]   ; (816c <bar+0x28>)
    814c:       4808            ldr     r0, [pc, #32]   ; (8170 <bar+0x2c>)
    814e:       e001            b.n     8154 <bar+0x10>
    8150:       505a            str     r2, [r3, r1]
    8152:       3304            adds    r3, #4
    8154:       185a            adds    r2, r3, r1
    8156:       4282            cmp     r2, r0
    8158:       591a            ldr     r2, [r3, r4]
    815a:       d3f9            bcc.n   8150 <bar+0xc>
    815c:       42aa            cmp     r2, r5
    815e:       bf8c            ite     hi
    8160:       2500            movhi   r5, #0
    8162:       2501            movls   r5, #1
    8164:       bd10            pop     {r4, pc}
    8166:       bf00            nop

Without Yao's patch, the "b bar" sets the break point at 0x8158. With Yao's
patch, the "b test" still sets the break point at 0x814e that is outside the
function test body. But the "b bar" sets the break point at 0x814e which is
better than 0x8158. IMHO, theoretically function test could have many other
different kinds of instruction patterns. So I think it is better to have two
enhancements one is Yao's patch which enhances the function
thumb_analyze_prologue and make it more accurate, and another one is my
patch which ensures the function break point to be always set inside the
function body in a more general way. Does this solution sound good?

Best regards,
Terry

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

[Yao Qi]

On 06/29/2011 09:26 AM, Terry Guo wrote:
> Hi,
> 
> I slightly modified the test case and got the below code:
> 
> 00008142 <test>:
>     8142:       250a            movs    r5, #10
> 

Could you show the source code of function "test"?  I can't figure out
how gcc generates such code.  Is it optimized by tail-call optimization?

> 
> Without Yao's patch, the "b bar" sets the break point at 0x8158. With Yao's
> patch, the "b test" still sets the break point at 0x814e that is outside the
> function test body. But the "b bar" sets the break point at 0x814e which is
> better than 0x8158. IMHO, theoretically function test could have many other
> different kinds of instruction patterns. So I think it is better to have two

There are many patterns of prologue, and that is why arm/thumb prologue
analyzer has long if/else statements to handle different cases.  My
understanding is, if GCC emit some prologue that GDB's prologue analyzer
doesn't understand, we have to improve prologue analyzer.

> enhancements one is Yao's patch which enhances the function
> thumb_analyze_prologue and make it more accurate, and another one is my
> patch which ensures the function break point to be always set inside the
> function body in a more general way. Does this solution sound good?

I am not against your approach, but prologue analyzer is still unable to
analyze some corner-case functions, so I am afraid stack backtrace on
these corner-case functions doesn't work, because stack bactrace need
the information provided from prologue analyzer.

In short, we should improve prologue analyzer to understand such
prologue as much as we can.  If it is difficult to understand some
strange prologue, such as your case, we have to resort to your patch.

-- 
Yao (齐尧)

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

[Terry Guo]

Hi,

Here is my case. As for stack backtrace, I wonder whether it works for the function that only contains one single instruction and function that doesn't even has a valid prologue. I also saw that current prologue analyzer cannot handle all cases, so I try to avoid this in a more general way.

unsigned long _etext;
unsigned long _data;
unsigned long _edata;
register unsigned long guard asm("r5");

void bar (void);
void test (void) __attribute__((naked));

void
foo (void)
{
  while(1)
    {
    }
} /* End of function foo.  */

void
test (void)
{
  guard = 10;
}

void
bar (void)
{
    unsigned long *pulSrc, *pulDest;

    pulSrc = &_etext;

    for (pulDest = &_data; pulDest < &_edata;)
      {
         *pulDest++ = *pulSrc++;
      }

    if (*pulSrc > guard)
      guard = 0;
    else
      guard = 1;
}

void
main (int argc, char **argv)
{
  test ();
}

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

[Yao Qi]

On 06/29/2011 03:00 PM, Terry Guo wrote:
> Hi,
> 
> Here is my case. As for stack backtrace, I wonder whether it works for the function that only contains one single instruction and function that doesn't even has a valid prologue. I also saw that current prologue analyzer cannot handle all cases, so I try to avoid this in a more general way.
> 

> register unsigned long guard asm("r5");
> 
> void bar (void);
> void test (void) __attribute__((naked));

attribute naked on the ARM, AVR, MCORE, RX and SPU ports indicates that
the specified function does not need prologue/epilogue sequences
generated by the compiler, so there is no prologue in function test.  Of
course, prologue analyzer can't help here.

However, this test case is not a correct program, because code after
"test" is unknown.  If you put correct epilogue in "test" to make it
either return to main, or jump to bar, prologue analyzer can handle it
correctly.

Looks this test case is invalid.  We have to find a valid/correct case
to show gdb's bug on setting breakpoint, and your patch could fix this bug.

-- 
Yao (齐尧)

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

[Terry Guo]

Hi,

First if you look at the generated binary code, I thought it is a valid case. After performing the function test, it will fall into function bar which is a normal function. At least my case can run correctly on QEMU. It is very likely for people to write program in assembly code like:

main:
    b test

test:
    movs r5, #10
bar:
    push.......
    .........

You cannot say this is a invalid program. If you have concern about attribute NAKED, I can rewrite it in assembly code.

Second, we all know that current prologue analyzer cannot handle all cases. My patch only intends to be a worthwhile supplement for cases that beyond the prologue analyzer capability.

Third, I also have strong interesting to enhance prologue analyze. I suggest we use another thread to discuss how to enhance prologue analyzer to handle all possible cases. And leave this one to discuss whether is it worthwhile to have my patch.

BR,
Terry

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

[Yao Qi]

On 06/29/2011 04:47 PM, Terry Guo wrote:
> Hi,
> 
> First if you look at the generated binary code, I thought it is a valid case. After performing the function test, it will fall into function bar which is a normal function. At least my case can run correctly on QEMU. It is very likely for people to write program in assembly code like:
> 
> main:
>     b test
> 
> test:
>     movs r5, #10
> bar:
>     push.......
>     .........
> 

We are lucky here GCC places bar next to function test physically, but
gcc may also place function test and bar in other layout, like

main:
	b test

bar:
	push ...

test:
	moves r5, #10

After test, processor will run some instructions that we don't know.
IMO, it is incorrect.

> You cannot say this is a invalid program. If you have concern about attribute NAKED, I can rewrite it in assembly code.
> 

I have no concern on attribute NAKED here.

> Second, we all know that current prologue analyzer cannot handle all cases. My patch only intends to be a worthwhile supplement for cases that beyond the prologue analyzer capability.
> 
> Third, I also have strong interesting to enhance prologue analyze. I suggest we use another thread to discuss how to enhance prologue analyzer to handle all possible cases. And leave this one to discuss whether is it worthwhile to have my patch.

OK, thanks for your clarification.  If you think your test case is
correct, please send the patch again, and the right people can
review/approve your patch.

-- 
Yao (齐尧)

P.S.  1. could you please set up your mail client to newline your mail
automatically?  It is hard to read one paragraph in one line.  2.  could
you reply mail with quoted context?

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

[Terry Guo]

> -----Original Message-----
> From: Yao Qi [mailto:yao@codesourcery.com]
> Sent: Wednesday, June 29, 2011 6:00 PM
> To: Terry Guo
> Cc: gdb-patches@sourceware.org; pedro@codesourcery.com
> Subject: Re: [PATCH] Fix that different function breakpoints are set at
> same pc address (PR gdb/12703)
> 
> On 06/29/2011 04:47 PM, Terry Guo wrote:
> > Hi,
> >
> > First if you look at the generated binary code, I thought it is a
> valid case. After performing the function test, it will fall into
> function bar which is a normal function. At least my case can run
> correctly on QEMU. It is very likely for people to write program in
> assembly code like:
> >
> > main:
> >     b test
> >
> > test:
> >     movs r5, #10
> > bar:
> >     push.......
> >     .........
> >
> 
> We are lucky here GCC places bar next to function test physically, but
> gcc may also place function test and bar in other layout, like
> 
> main:
> 	b test
> 
> bar:
> 	push ...
> 
> test:
> 	moves r5, #10
> 
> After test, processor will run some instructions that we don't know.
> IMO, it is incorrect.
> 
> > You cannot say this is a invalid program. If you have concern about
> attribute NAKED, I can rewrite it in assembly code.
> >
> 
> I have no concern on attribute NAKED here.

I know that GCC will "luckily" place code like that. That's why I just post
assembly code first. The key point here is the program can be written in 
assembly code and in this way the function position can be ensured without
the help of luck. is my understanding right? 
I use "naked" here in C code because I am lazy to write
assembly code. You can omit the "naked" in C and look at the case as a case
written in assembly code.

> 
> > Second, we all know that current prologue analyzer cannot handle all
> cases. My patch only intends to be a worthwhile supplement for cases
> that beyond the prologue analyzer capability.
> >
> > Third, I also have strong interesting to enhance prologue analyze. I
> suggest we use another thread to discuss how to enhance prologue
> analyzer to handle all possible cases. And leave this one to discuss
> whether is it worthwhile to have my patch.
> 
> OK, thanks for your clarification.  If you think your test case is
> correct, please send the patch again, and the right people can
> review/approve your patch.
>
> --
> Yao (齐尧)
> 
> P.S.  1. could you please set up your mail client to newline your mail
> automatically?  It is hard to read one paragraph in one line.  2.
> could
> you reply mail with quoted context?

Thanks for your reminding.

BR,
Terry 

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

[Richard Earnshaw]

On 29/06/11 11:00, Yao Qi wrote:
> On 06/29/2011 04:47 PM, Terry Guo wrote:
>> Hi,
>>
>> First if you look at the generated binary code, I thought it is a valid case. After performing the function test, it will fall into function bar which is a normal function. At least my case can run correctly on QEMU. It is very likely for people to write program in assembly code like:
>>
>> main:
>>     b test
>>
>> test:
>>     movs r5, #10
>> bar:
>>     push.......
>>     .........
>>
> 
> We are lucky here GCC places bar next to function test physically, but
> gcc may also place function test and bar in other layout, like
> 
> main:
> 	b test
> 
> bar:
> 	push ...
> 
> test:
> 	moves r5, #10
> 
> After test, processor will run some instructions that we don't know.
> IMO, it is incorrect.
> 

Which is one of the reasons why GCC has the option -fno-toplevel-reorder
to prevent such reordering.

R.

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

[Pedro Alves]

On Friday 01 July 2011 09:59:07, Richard Earnshaw wrote:
> On 29/06/11 11:00, Yao Qi wrote:
> > On 06/29/2011 04:47 PM, Terry Guo wrote:
> >> Hi,
> >>
> >> First if you look at the generated binary code, I thought it is a valid case. After performing the function test, it will fall into function bar which is a normal function. At least my case can run correctly on QEMU. It is very likely for people to write program in assembly code like:
> >>
> >> main:
> >>     b test
> >>
> >> test:
> >>     movs r5, #10
> >> bar:
> >>     push.......
> >>     .........
> >>
> > 
> > We are lucky here GCC places bar next to function test physically, but
> > gcc may also place function test and bar in other layout, like
> > 
> > main:
> > 	b test
> > 
> > bar:
> > 	push ...
> > 
> > test:
> > 	moves r5, #10
> > 
> > After test, processor will run some instructions that we don't know.
> > IMO, it is incorrect.
> > 
> 
> Which is one of the reasons why GCC has the option -fno-toplevel-reorder
> to prevent such reordering.

Indeed.  Well, GDB is not a compiler; it is supposed to be able to
debug buggy code.  :-)  Turning things upside down, the root question is
whether there are valid cases where when setting a breakpoint
at "foo", when GDB goes analysing the prologue, GDB should cross
function boundaries into the next function, which would get 
broken by Terry's proposal.  E.g., say, foo and bar get merged
by some smart compiler/linker:

foo:
  <nop>
bar:
  <regular prologue>
  <body code>

I guess setting a break on "foo" should set a breakpoint
on "<body code>".  Or (another user of prologue skipping,)
stepping into a foo() call should only stop on "<body code>".
Or, should GDB be conservative, and never cross the function
body when skipping the prologue the hard way, and only
cross it if debug info says so.  

-- 
Pedro Alves

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

[Terry Guo]

Hi,

Sorry for bringing this topic back after so long time.
Recently I met a case which I think may have relation with
this topic. Please see my embedded comments and if I am wrong
please correct me.

> Indeed.  Well, GDB is not a compiler; it is supposed to be able to
> debug buggy code.  :-)  Turning things upside down, the root question
> is
> whether there are valid cases where when setting a breakpoint
> at "foo", when GDB goes analysing the prologue, GDB should cross
> function boundaries into the next function, which would get
> broken by Terry's proposal.  E.g., say, foo and bar get merged
> by some smart compiler/linker:
> 
> foo:
>   <nop>
> bar:
>   <regular prologue>
>   <body code>
> 
> I guess setting a break on "foo" should set a breakpoint
> on "<body code>".  Or (another user of prologue skipping,)

I have a bunch of interrupt response functions in my source whose layout looks like:

ext_isr_100:
            nop
ext_isr_101:
            nop
ext_isr_102:
            nop
bar:
     <regular prologue>
     <body code>

If breakpoints of function ext_isr_100, ext_isr_101 and ext_isr_102 are all set on bar's
<body code>. Then I think I have no way to figure out which interrupt is trigged.
For this case, I still think it is better to set breakpoint inside the function body.


> stepping into a foo() call should only stop on "<body code>".
> Or, should GDB be conservative, and never cross the function
> body when skipping the prologue the hard way, and only
> cross it if debug info says so.
> 
> --
> Pedro Alves

Best regards,
Terry

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