From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29411 invoked by alias); 7 Dec 2008 17:12:20 -0000 Received: (qmail 29402 invoked by uid 22791); 7 Dec 2008 17:12:18 -0000 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 07 Dec 2008 17:11:42 +0000 Received: (qmail 10899 invoked from network); 7 Dec 2008 17:11:40 -0000 Received: from unknown (HELO orlando.local) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 7 Dec 2008 17:11:40 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: [rfc] [0/7] infrun cleanup Date: Sun, 07 Dec 2008 17:12:00 -0000 User-Agent: KMail/1.9.10 Cc: "Ulrich Weigand" , drow@false.org References: <200812070015.mB70FKfE017783@d12av02.megacenter.de.ibm.com> <200812070128.39176.pedro@codesourcery.com> In-Reply-To: <200812070128.39176.pedro@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200812071711.47727.pedro@codesourcery.com> 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-12/txt/msg00130.txt.bz2 On Sunday 07 December 2008 01:28:38, Pedro Alves wrote: > On Sunday 07 December 2008 00:15:20, Ulrich Weigand wrote: > > > I'd appreciate any feedback on this approach (in particular if > > you've done things differently in your attempt) ... > > Eh, no, a lot of dejavu here. :-) > > > Overall patch set tested on amd64-linux with no regressions. > > Thanks much for doing this. I've commented on one. I'll > look at the rest tomorrow. > It all looks good to me. The only bit that concerns me, is: > Within handle_inferior_event (and its subroutines), every path that > ends in > > stop_stepping (ecs); > return; > > is replaced by > > return 0; > > and likewise every path that ends in > > prepare_to_wait (ecs); > return > > is replaced by > > return 1; I'm not so sure this makes things clearer than what's there currently. One now has to remember what "return 0" or "return 1" means, while previously, calls to prepare_to_wait/stop_stepping made it quite explicit. We also lost the debug message that hinted us that we're going to need to wait for another target event ("infrun: prepare_to_wait"), or that we're done ("infrun: stop_stepping"). Perhaps leave the stop_stopping/prepare_to_wait functions, for the debug output, and for clarity? Say, you could make it like so: - prepare_to_wait (ecs); - return; + return prepare_to_wait (); - stop_stepping (ecs); - return; + return stop_stepping (); And something like: -static void -stop_stepping (struct execution_control_state *ecs) +static int +stop_stepping (void) { if (debug_infrun) fprintf_unfiltered (gdb_stdlog, "infrun: stop_stepping\n"); /* Let callers know we don't want to wait for the inferior anymore. */ - ecs->wait_some_more = 0; + return 0; } Maybe it's just me, though, it's not a strong opinion. There are a couple of comments left behind that should be cleaned up, if we remove those functions, e.g., /* Print why the inferior has stopped. We always print something when the inferior exits, or receives a signal. The rest of the cases are dealt with later on in normal_stop() and print_it_typical(). Ideally there should be a call to this function from handle_inferior_event() each time stop_stepping() is called.*/ static void print_stop_reason (enum inferior_stop_reason stop_reason, int stop_info) Or, /* Refresh prev_pc value just prior to resuming. This used to be done in stop_stepping, however, setting prev_pc there did not handle scenarios such as inferior function calls or returning from a function via the return command. In those cases, the prev_pc ... -- Pedro Alves