Re: [patch] step over permanent breakpoint

[Joel Brobecker <brobecker@adacore.com>]

> 2008-08-19  Aleksandar Ristovski  <aristovski@qnx.com>
> 
> 	* breakpoint.c (bp_loc_permanent): New function.
> 	(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.

We're getting there. Still a few more comments...

> +static int bp_loc_permanent (struct bp_location *loc);

Moving the code to a function is a great idea.  I suggest to move
that function just before the function where you use it. That way,
you don't have to add this advance declaration here.

Also I suggest we change the name a bit. For instance:
  - bp_loc_is_permanent, or
  - permanent_bp_loc

> +  /* Update locationos of permanent breakpoints.  */
               ^^^^^^^^^^ locations

> +/* Determine if the location is pointing to a permanent breakpoint.  */

The description doesn't explain what the function does if the location
is pointing to a permanent breakpoint.  I suggest the following:

/* Return non-zero if LOC is pointing to a permanent breakpoint.  */

> +static int
> +bp_loc_permanent (struct bp_location *loc)
> +{
> +  const int READ_SUCCESS = 0;
     ^^^^^^^^^^^^^^^^^^^^^^
     Generally speaking, we usually do not use all caps on variables
     or constants.  In this particular case, if we had felt that a named
     constant was bringing something, we should declare it besides
     the function where it is used - here, I found this confusing.
     Let's just get rid of this constant and compare the result of
     target_read_memory against litteral 0.
     
> +  int len;
> +  CORE_ADDR addr;
> +  const gdb_byte *brk;
> +  gdb_byte target_mem[32];

The GNU Coding Standards explicitly warns against the use of arbitrary
sizes.  I would be very surprised that any breakpoint instruction be
larger than 32 bytes, but I still hate the idea of this ungarded limit.
Can you allocate the buffer with alloca instead?

> +static void
> +i386_skip_permanent_breakpoint (struct regcache *regcache)
> +{
> +  CORE_ADDR current_pc = regcache_read_pc (regcache);
> + /* On i386, breakpoint is exactly 1 byte long, so we just
> +    adjust the PC in the regcache.  */

Style: We usually add an empty line after the declarations, before
the first statement.

> set pattern_matched 0
> set function standard
> 
> set retcode [gdb_test_multiple "disassemble $function" "Disassemble function '$function'" {
>     -re ".*($hex) <$function\\+0>.*($hex) <$function\\+4>.*($hex) <$function\\+5>.*($hex) <$function\\+6>.*" {
>       set function_start $expect_out(1,string);
>       set address $expect_out(2,string);
>       set address1 $expect_out(3,string);
>       set address2 $expect_out(4,string);
>     }
> }]
> 
> if {$retcode != $pattern_matched} {

Again, I find the "$pattern_matched" confusing. I'd prefer it if you compared
directly against litteral 0.

> # We want to fetch esp at the start of '$function' function to make sure
> # skip_permanent_breakpoint implementation really skips only the perm. 
> # breakpoint. If, for whatever reason, 'leave' instruction doesn't get
                                        ^ the 'leave'
> # executed, esp will not have this value.
> set start_esp 0
> gdb_test_multiple "print \$esp\n" "Fetch esp value." {
>     -re ".1.*($hex).*" {

I am wondering whether you really need to have the "\n" at the end of
the command, and I'm worried that this might cause an unnecessary repeat
of your command during the test.

Also, I don't completely understand the ".1" part of your regexp. And
the gdb_test_multiple documentation also says that you should include
the final newline and prompt. So I suggest:

  -re ".*($hex).*$gdb_prompt $"


-- 
Joel