From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14419 invoked by alias); 6 Oct 2008 21:57:15 -0000 Received: (qmail 14410 invoked by uid 22791); 6 Oct 2008 21:57:15 -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, 06 Oct 2008 21:56:39 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id CA9062A967B; Mon, 6 Oct 2008 17:56:37 -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 Avm03M3h8YU1; Mon, 6 Oct 2008 17:56:37 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 873F02A9666; Mon, 6 Oct 2008 17:56:37 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 5DBAEE7ACD; Mon, 6 Oct 2008 14:56:37 -0700 (PDT) Date: Mon, 06 Oct 2008 21:57:00 -0000 From: Joel Brobecker To: Michael Snyder Cc: "gdb-patches@sourceware.org" , Daniel Jacobowitz , Pedro Alves , teawater Subject: Re: [RFA] Reverse Debugging, 4/5 Message-ID: <20081006215637.GE21853@adacore.com> References: <48E3CD40.3070206@vmware.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <48E3CD40.3070206@vmware.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-10/txt/msg00181.txt.bz2 > * breakpoint.c (breakpoint_silence): New function. > * infcmd.c (finish_command): Check for reverse exec direction. You're going to hate me, now :). This is another instance where I think we can break the code a little differently: 1. finish_command_backwards (I would have prefered "reverse_finish_command but :-P) 2. finish_command_forward 3. finish_command: { [do all the stuff about checking for args, etc] if (target_get_exec_dir () == EXEC_REVERSE) finish_command_backwards () else finish_command_forward () } That way, we just split the finish_command code into two parts without moving some of the code, and it's clear that the two paths are completely distinct. The "branch-off" approach (that we used for Ada but that I'm trying to avoid like the plague now) does obscure the structure of your program. > +void > +breakpoint_silence (struct breakpoint *b) > +{ > + /* Silence the breakpoint. */ > + b->silent = 1; Minor nit: This name brings little meaning when I see it being called. Can we change it to "make_breakpoint_silent"? That way, the comment in the body becomes useless and can be removed. > + if (find_pc_partial_function (get_frame_pc (get_current_frame ()), > + NULL, &func_addr, NULL) == 0) > + internal_error (__FILE__, __LINE__, > + "Finish: couldn't find function."); Internal error? I understand that it should probably never happen in this context, but how about making it a simple error instead. If we trip this check, it's true that something went wrong, but let's just abort the command and let the user try to continue rather than asking the user whether we should abort the whole session. > + /* TODO: Let's not worry about async until later. */ Should we add a check now, though, and error out if async was requested? > + /* (Kludgy way of letting wait_for_inferior know...) */ > + tp->step_range_start = tp->step_range_end = 1; AARGH! More special meaning to these addresses. We really ought to clean these up and put some specific flags in the structure, one day. I don't know why we're trying so hard to resume these fields. -- Joel