From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19752 invoked by alias); 7 Dec 2008 18:20:37 -0000 Received: (qmail 19741 invoked by uid 22791); 7 Dec 2008 18:20:36 -0000 X-Spam-Check-By: sourceware.org Received: from mtagate2.de.ibm.com (HELO mtagate2.de.ibm.com) (195.212.17.162) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 07 Dec 2008 18:19:52 +0000 Received: from d12nrmr1607.megacenter.de.ibm.com (d12nrmr1607.megacenter.de.ibm.com [9.149.167.49]) by mtagate2.de.ibm.com (8.13.1/8.13.1) with ESMTP id mB7IJosG012351 for ; Sun, 7 Dec 2008 18:19:50 GMT Received: from d12av02.megacenter.de.ibm.com (d12av02.megacenter.de.ibm.com [9.149.165.228]) by d12nrmr1607.megacenter.de.ibm.com (8.13.8/8.13.8/NCO v9.1) with ESMTP id mB7IJodc3784830 for ; Sun, 7 Dec 2008 19:19:50 +0100 Received: from d12av02.megacenter.de.ibm.com (loopback [127.0.0.1]) by d12av02.megacenter.de.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id mB7IJnrM004722 for ; Sun, 7 Dec 2008 19:19:49 +0100 Received: from tuxmaker.boeblingen.de.ibm.com (tuxmaker.boeblingen.de.ibm.com [9.152.85.9]) by d12av02.megacenter.de.ibm.com (8.12.11.20060308/8.12.11) with SMTP id mB7IJnUh004717; Sun, 7 Dec 2008 19:19:49 +0100 Message-Id: <200812071819.mB7IJnUh004717@d12av02.megacenter.de.ibm.com> Received: by tuxmaker.boeblingen.de.ibm.com (sSMTP sendmail emulation); Sun, 7 Dec 2008 19:19:49 +0100 Subject: Re: [rfc] [0/7] infrun cleanup To: pedro@codesourcery.com (Pedro Alves) Date: Sun, 07 Dec 2008 18:20:00 -0000 From: "Ulrich Weigand" Cc: gdb-patches@sourceware.org, drow@false.org In-Reply-To: <200812071711.47727.pedro@codesourcery.com> from "Pedro Alves" at Dec 07, 2008 05:11:47 PM MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit 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/msg00132.txt.bz2 Pedro Alves wrote: > 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. Thanks for your feedback! > 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? I agree that the return 0/1 is not quite optimal. On the other hand, I feel it would be nice to get rid of those nearly-empty functions (the debug messages seem to me mostly redundant, as the callers of stop_stepping / prepare_to_wait typically already have their own, more specific debug message ...). In the context of some further cleanup and splitting handle_inferior_event into multiple more independent parts, I had been wondering whether it might be a good idea to use a enum (like enum inferior_stop_reason) instead of the boolean: handle_inferior_event (and its hypothetical subroutines) would return enum values to indicate *why* the inferior stopped, including a new STILL_RUNNING value to indicate that it in fact hasn't yet stopped. In this set-up you'd have statements like return STILL_RUNNING; or return END_STEPPING_RANGE; or (another potential new value) return HIT_BREAKPOINT; within handle_inferior_event; and its caller would be rewritten like do { ptid = target_wait (...); stop_reason = handle_inferior_event (ptid, ...); } while (stop_reason == STILL_RUNNING); It might be feasible to use the stop_reason in the future to merge some of the print_stop_reason stuff into normal_stop and reduce the amount of duplicate checks; or even to replace some of the "global" output variables like stopped_by_random_signal or tp->stop_step. What do you think? > 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.*/ I thought I had updated that one? > /* 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 > ... I left that because it specifically refers to how things were done in the past, when we still had that function ... Maybe it could be marked as "the former stop_stepping" or so. Bye, Ulrich -- Dr. Ulrich Weigand GNU Toolchain for Linux on System z and Cell BE Ulrich.Weigand@de.ibm.com