From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9903 invoked by alias); 18 Aug 2008 11:08:12 -0000 Received: (qmail 9894 invoked by uid 22791); 18 Aug 2008 11:08:11 -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, 18 Aug 2008 11:07:20 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 5DC3C2A972D; Mon, 18 Aug 2008 07:07:18 -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 7BT2B1G+t6Ou; Mon, 18 Aug 2008 07:07:18 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id C73462A96CD; Mon, 18 Aug 2008 07:07:16 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id B7EC1E7ACD; Mon, 18 Aug 2008 13:07:09 +0200 (CEST) Date: Mon, 18 Aug 2008 11:08:00 -0000 From: Joel Brobecker To: Aleksandar Ristovski Cc: gdb-patches@sources.redhat.com Subject: Re: [patch] step over permanent breakpoint Message-ID: <20080818110709.GD16894@adacore.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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/msg00468.txt.bz2 > 2008-08-05 Aleksandar Ristovski > > * 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 > > * 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) .*($hex) .*($hex) .*($hex) .*" { > 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