Re: [patch] step over permanent breakpoint

[Aleksandar Ristovski <aristovski at qnx dot com>]

Joel Brobecker wrote:
> 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.

Done.

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

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

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

> Again, I find the "$pattern_matched" confusing. I'd prefer it if you compared
> directly against litteral 0.
Done.
> 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 $".
It was a shortcut for "\\\$1". 
Changed.
>



Attached revised patches.

Thanks,

Aleksandar Ristovski
QNX Software Systems

2008-09-02  Aleksandar Ristovski  <aristovski@qnx.com>

	* breakpoint.c (breakpoint_init_inferior): Mark as not inserted only
	non-permanent breakpoints.
	(bpstat_stop_status): Change enable_state to bp_disabled only for
	non-permanent breakpoints.
	(bp_loc_is_permanent): New function.
	(create_breakpoint): Check if the location points to a permanent
	breakpoint and if it does, make breakpoint permanent.
	(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.

Index: gdb/breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.345
diff -u -p -r1.345 breakpoint.c
--- gdb/breakpoint.c	26 Aug 2008 17:36:44 -0000	1.345
+++ gdb/breakpoint.c	2 Sep 2008 15:17:15 -0000
@@ -1750,7 +1750,8 @@ breakpoint_init_inferior (enum inf_conte
   struct bp_location *bpt;
 
   ALL_BP_LOCATIONS (bpt)
-    bpt->inserted = 0;
+    if (bpt->owner->enable_state != bp_permanent)
+      bpt->inserted = 0;
 
   ALL_BREAKPOINTS_SAFE (b, temp)
   {
@@ -3088,7 +3089,8 @@ bpstat_stop_status (CORE_ADDR bp_addr, p
 	/* We will stop here */
 	if (b->disposition == disp_disable)
 	  {
-	    b->enable_state = bp_disabled;
+	    if (b->enable_state != bp_permanent)
+	      b->enable_state = bp_disabled;
 	    update_global_location_list (0);
 	  }
 	if (b->silent)
@@ -5081,6 +5083,34 @@ add_location_to_breakpoint (struct break
   set_breakpoint_location_function (loc);
   return loc;
 }
+
+
+/* Return 1 if LOC is pointing to a permanent breakpoint, 
+   return 0 otherwise.  */
+
+static int
+bp_loc_is_permanent (struct bp_location *loc)
+{
+  int len;
+  CORE_ADDR addr;
+  const gdb_byte *brk;
+  gdb_byte *target_mem;
+
+  gdb_assert (loc != NULL);
+
+  addr = loc->address;
+  brk = gdbarch_breakpoint_from_pc (current_gdbarch, &addr, &len);
+
+  target_mem = alloca (len);
+
+  if (target_read_memory (loc->address, target_mem, len) == 0
+      && memcmp (target_mem, brk, len) == 0)
+    return 1;
+
+  return 0;
+}
+
+
 
 /* Create a breakpoint with SAL as location.  Use ADDR_STRING
    as textual description of the location, and COND_STRING
@@ -5135,6 +5165,9 @@ create_breakpoint (struct symtabs_and_li
 	  loc = add_location_to_breakpoint (b, type, &sal);
 	}
 
+      if (bp_loc_is_permanent (loc))
+	make_breakpoint_permanent (b);
+
       if (b->cond_string)
 	{
 	  char *arg = b->cond_string;
@@ -7389,6 +7422,10 @@ update_breakpoint_locations (struct brea
 	b->line_number = sals.sals[i].line;
     }
 
+  /* Update locationos of permanent breakpoints.  */
+  if (b->enable_state == bp_permanent)
+    make_breakpoint_permanent (b);
+
   /* If possible, carry over 'disable' status from existing breakpoints.  */
   {
     struct bp_location *e = existing_locations;
@@ -8152,6 +8189,7 @@ single_step_breakpoint_inserted_here_p (
   return 0;
 }
 
+
 
 /* This help string is used for the break, hbreak, tbreak and thbreak commands.
    It is defined as a macro to prevent duplication.
Index: gdb/i386-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-tdep.c,v
retrieving revision 1.264
diff -u -p -r1.264 i386-tdep.c
--- gdb/i386-tdep.c	26 Aug 2008 17:40:24 -0000	1.264
+++ gdb/i386-tdep.c	2 Sep 2008 15:17:16 -0000
@@ -2624,6 +2624,18 @@ i386_fetch_pointer_argument (struct fram
   return read_memory_unsigned_integer (sp + (4 * (argi + 1)), 4);
 }
 
+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.  */
+  current_pc += 1;
+  regcache_write_pc (regcache, current_pc);
+}
+
+
 
 static struct gdbarch *
 i386_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
@@ -2812,6 +2824,9 @@ i386_gdbarch_init (struct gdbarch_info i
   if (tdep->mm0_regnum == 0)
     tdep->mm0_regnum = gdbarch_num_regs (gdbarch);
 
+  set_gdbarch_skip_permanent_breakpoint (gdbarch,
+					 i386_skip_permanent_breakpoint);
+
   return gdbarch;
 }
 

2008-09-02  Aleksandar Ristovski  <aristovski@qnx.com>

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

# Copyright (C) 2008 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/>.


# This file is part of the gdb testsuite.

if $tracelevel {
    strace $tracelevel
}

# Test stepping over permanent breakpoints on i386.

if ![istarget "i?86-*-*"] then {
    verbose "Skipping skip over permanent breakpoint on i386 tests."
    return
}

set testfile "i386-prologue"
set srcfile ${testfile}.c
set binfile ${objdir}/${subdir}/${testfile}

# some targets have leading underscores on assembly symbols.
# TODO: detect this automatically
set additional_flags ""
if [istarget "i?86-*-cygwin*"] then {
    set additional_flags "additional_flags=-DSYMBOL_PREFIX=\"_\""
}

# Don't use "debug", so that we don't have line information for the assembly
# fragments.
if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable [list $additional_flags]] != "" } {
    untested i386-prologue.exp
    return -1
}


gdb_exit
gdb_start
gdb_reinitialize_dir $srcdir/$subdir
gdb_load ${binfile}

#
# Run to `main' where we begin our tests.
#

if ![runto_main] then {
  return -1
}

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>.*$gdb_prompt $" {
      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 != 0} {
  fail "Disassemble failed, skipping entire test."
  return -1
}

gdb_breakpoint "*$function_start"

gdb_breakpoint "*$address"

gdb_test "continue" "Breakpoint .*, $function_start in $function.*" \
	 "Stop at the '$function' start breakpoint (fetching esp)."

# 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
# executed, esp will not have this value.
set start_esp 0
gdb_test_multiple "print \$esp" "Fetch esp value." {
    -re "\\\$1.*($hex).*$gdb_prompt $" {
      set start_esp $expect_out(1,string);
    }
}

gdb_test "continue" "Breakpoint .*, $address in $function.*" \
	 "Stop at permanent breakpoint."

gdb_test "stepi" "$address1|$address2 in $function.*" \
	 "Single stepping past permanent breakpoint."

gdb_test "print \$esp" ".*$start_esp.*" \
	 "ESP value does not match - step_permanent_breakpoint wrong."