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 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 (éå)