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

Yao Qi
2011-06-24T03:55:29Z

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