From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17893 invoked by alias); 25 Aug 2008 16:36:02 -0000 Received: (qmail 17885 invoked by uid 22791); 25 Aug 2008 16:36:02 -0000 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.31) with ESMTP; Mon, 25 Aug 2008 16:35:22 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 7AE612A96A8; Mon, 25 Aug 2008 12:35:20 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id UwRpoYvp-m+g; Mon, 25 Aug 2008 12:35:20 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id AB4A72A9695; Mon, 25 Aug 2008 12:35:19 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 312ABE7ACD; Mon, 25 Aug 2008 18:35:17 +0200 (CEST) Date: Mon, 25 Aug 2008 16:36:00 -0000 From: Joel Brobecker To: Aleksandar Ristovski Cc: gdb-patches@sources.redhat.com Subject: Re: [patch] step over permanent breakpoint Message-ID: <20080825163516.GA31707@adacore.com> References: <20080818110709.GD16894@adacore.com> <48AAE8D7.7040608@qnx.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <48AAE8D7.7040608@qnx.com> User-Agent: Mutt/1.4.2.2i 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-08/txt/msg00612.txt.bz2 > 2008-08-19 Aleksandar Ristovski > > * 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