From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8638 invoked by alias); 2 Sep 2008 15:37:05 -0000 Received: (qmail 8628 invoked by uid 22791); 2 Sep 2008 15:37:03 -0000 X-Spam-Check-By: sourceware.org Received: from qnxmail.qnx.com (HELO qnxmail.qnx.com) (209.226.137.76) by sourceware.org (qpsmtpd/0.31) with ESMTP; Tue, 02 Sep 2008 15:36:10 +0000 Received: from Nebula.ott.qnx.com (nebula.ott.qnx.com [10.42.3.30]) by hub.ott.qnx.com (8.9.3/8.9.3) with ESMTP id LAA19820; Tue, 2 Sep 2008 11:36:00 -0400 Received: from [10.42.100.129] ([10.42.100.129]) by Nebula.ott.qnx.com with Microsoft SMTPSVC(6.0.3790.3959); Tue, 2 Sep 2008 11:42:29 -0400 Message-ID: <48BD5D64.1050708@qnx.com> Date: Tue, 02 Sep 2008 15:37:00 -0000 From: Aleksandar Ristovski User-Agent: Thunderbird 2.0.0.16 (Windows/20080708) MIME-Version: 1.0 To: Joel Brobecker CC: gdb-patches@sources.redhat.com Subject: Re: [patch] step over permanent breakpoint References: <20080818110709.GD16894@adacore.com> <48AAE8D7.7040608@qnx.com> <20080825163516.GA31707@adacore.com> In-Reply-To: <20080825163516.GA31707@adacore.com> Content-Type: multipart/mixed; boundary="------------070904060906060805030906" X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2008-09/txt/msg00019.txt.bz2 This is a multi-part message in MIME format. --------------070904060906060805030906 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-length: 1081 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 --------------070904060906060805030906 Content-Type: text/plain; name="i386_permanent_breakpoints-20080902.diff.ChangeLog" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="i386_permanent_breakpoints-20080902.diff.ChangeLog" Content-length: 640 2008-09-02 Aleksandar Ristovski * 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. --------------070904060906060805030906 Content-Type: text/plain; name="i386_permanent_breakpoints-20080902.diff" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="i386_permanent_breakpoints-20080902.diff" Content-length: 3610 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; } --------------070904060906060805030906 Content-Type: text/plain; name="i386-bp_permanent.exp.20080902.ChangeLog" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="i386-bp_permanent.exp.20080902.ChangeLog" Content-length: 101 2008-09-02 Aleksandar Ristovski * gdb.arch/i386-bp_permanent.exp: New test. --------------070904060906060805030906 Content-Type: text/plain; name="i386-bp_permanent.exp.20080902" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="i386-bp_permanent.exp.20080902" Content-length: 3116 # 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 . # 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." --------------070904060906060805030906--