Re: [patch] step over permanent breakpoint

[Joel Brobecker <brobecker at adacore dot com>]

> 2008-08-05  Aleksandar Ristovski  <aristovski@qnx.com>
> 
> 	* breakpoint.c (breakpoint_init_inferior): Mark as not inserted only
> 	non-permanent breakpoints.
> 	(create_breakpoint): Check if the location points to a permanent
> 	breakpoint.
> 	(update_breakpoint_locations): Make sure new locations of permanent
> 	breakpoints are properly initialized.
> 	* i386-tdep.c (i386_skip_permanent_breakpoint): New function.
> 	(i386_gdbarch_init): Set gdbarch_skip_permanent_breakpoint.

Overall, this looks reasonable. I have a few comments, though. See below.
BTW: I just approved a patch for HP/UX that also deals with permanent
breakpoints. But fortunately, they don't seem to collide at all. See:
http://www.sourceware.org/ml/gdb-patches/2008-08/msg00466.html.

>    ALL_BP_LOCATIONS (bpt)
> -    bpt->inserted = 0;
> +    {
> +      if (bpt->owner->enable_state != bp_permanent)
> +	bpt->inserted = 0;
> +    }

I think the usual style is to avoid the curly braces in this case:

  ALL_BP_LOCATIONS (bpt)
    if (bpt->owner->enable_state != bp_permanent)
      bpt->inserted = 0;

> +      /* Check if the location points to permanent breakpoint.  */
> +      if (loc != NULL)

Can loc actually be NULL? It looks like it is always initialized
in the code above...

> +	{
> +	  int len;
> +	  CORE_ADDR addr = loc->address;
> +	  const gdb_byte *brk = gdbarch_breakpoint_from_pc (current_gdbarch,
> +							    &addr, &len);
> +	  gdb_byte target_mem[32];
> +	  if (!target_read_memory (loc->address, target_mem, len))

I would prefer if you compared against zero rather than checking
the result of !target_read_memory - intuitively, it looks as if
the read was not succesful (I did notice that others did the same
before).

> +	    {
> +	      /* We have the target memory here.  */

I think the comment will be useless once you change the way you check
the result of target_read_memory above.

> +/* On i386, breakpoint is exactly 1 byte long, so we just
> +   adjust the PC in the regcache.  */

I think this comment should be moved inside the function body.
If you add a comment at the beginning of a function, it's best to
describe WHAT the function does without going into the implementation
details. For instance:

/* Assuming that the PC points to a breakpoint instruction, update it
   to point to the instruction following it.  */

For functions implementing gdbarch methods whose name also match
the name of that method, comments do not always bring much value,
since they just repeat the comment associated to that method.
Bottom-line: Just move your comment inside the function :).

> 2008-08-05  Aleksandar Ristovski  <aristovski@qnx.com>
> 
> 	* i386-bp_permanent.exp: New test.

You're missing part of the path to the testcase. Since the ChangeLog
is inside gdb/testsuite whereas your testcase lives in
gdb/testsuite/gdb.arch, the above should be:

        * gdb.arch/i386-bp_permanent.exp: New test.

> # Copyright (C) 2003, 2004, 2006, 2007, 2008 Free Software Foundation, Inc.

Should only be (c) 2008.

> # Please email any bugs, comments, and/or additions to this file to:
> # bug-gdb@gnu.org

Please remove this part from the header.  IIRC, we removed all
references to this email address.

> # Test i386 prologue analyzer.

Copy/paste error? We're not testing the prologue analyzer as far as
I can tell, but rather the ability to step past a permanent
breakpoint...

> if ![istarget "i?86-*-*"] then {
>     verbose "Skipping i386 prologue tests."
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Needs to be fixed as well.

> if ![runto_main] then {
>     gdb_suppress_tests

We're trying to get rid of this. Do a "return -1" instead.

> # Testcase for standard prologue.

Copy/paste error again...

> send_gdb "disassemble standard\n";
> gdb_expect 60 {
>     -re ".*($hex) <standard\\+0>.*($hex) <standard\\+4>.*($hex) <standard\\+5>.*($hex) <standard\\+6>.*" {
>       set standard_start $expect_out(1,string);
>       set address $expect_out(2,string);
>       set address1 $expect_out(3,string);
>       set address2 $expect_out(4,string);
>     }
>     default {
>       send_user "Oops, can't find address\n"
>       gdb_supress_tests
>     }
> }

Is it possible to use gdb_test_multiple, here? This routine is our
standard testing routine when gdb_test is not sufficient, and handles
all common error situations.


> # We want to fetch esp at the start of 'standard' function to make sure
> # skip_permanent_breakpoint implementation really skips only the perm. 
> # breakpoint. If, for whatever reason, 'leave' instruction doesn't get
> # executed, esp will not have this value.
> send_gdb "print \$esp\n"
> gdb_expect 60 {
>     -re ".1.*($hex).*" {
>       set start_esp $expect_out(1,string);
>     }
>     default {
>       gdb_fail "Fetching esp failed."
>     }
> }

Same here (use gdb_test_multiple).

-- 
Joel