From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1741 invoked by alias); 6 May 2013 16:54:07 -0000 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 Received: (qmail 1728 invoked by uid 89); 6 May 2013 16:54:06 -0000 X-Spam-SWARE-Status: No, score=-8.3 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Mon, 06 May 2013 16:54:06 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r46Gs3Qp007031 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 6 May 2013 12:54:03 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r46Gs1di008806; Mon, 6 May 2013 12:54:02 -0400 Message-ID: <5187E029.8070008@redhat.com> Date: Mon, 06 May 2013 16:54:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130311 Thunderbird/17.0.4 MIME-Version: 1.0 To: Tom Tromey CC: Maxime Coste , gdb-patches@sourceware.org Subject: Re: [PATCH 1/3] Add a select_frame_reason enum parameter to select_frame References: <1367002961-12311-1-git-send-email-frrrwww@gmail.com> <1367002961-12311-2-git-send-email-frrrwww@gmail.com> <87r4htxeth.fsf@fleche.redhat.com> In-Reply-To: <87r4htxeth.fsf@fleche.redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-05/txt/msg00126.txt.bz2 On 04/29/2013 09:27 PM, Tom Tromey wrote: >>>>>> "Maxime" == Maxime Coste writes: > > Maxime> select_frame calls specify if the frame is selected due to > Maxime> a breakpoint/signal (REASON_STOP), a user command (REASON_USER) > Maxime> or just as an implementation detail (REASON_IMPL_DETAIL) which > Maxime> should restore the previous frame once finished. > > I don't mind this approach but I would like to hear from other > maintainers. I'm not sure I like the approach. I couldn't find any use for the REASON_STOP/REASON_USER distinction in the series. If there's a rationale for it, it'd be good to hear it, otherwise it just looks like over engineering. The (fi != selected_frame) check in the REASON_STOP looks suspicious. Was the intent to prevent a notification if the program stopped in the same function it was stopped before it was resumed? IMO, it'd be better to leave select_frame unaware of why it's being called, and just add a wrapper for user visible frame selection. It feels like different conceptual levels to me. IOW, something like: void select_user_frame (struct frame_info *fi) { int changed = (fi != selected_frame); select_frame (fi); if (changed) observer_notify_frame_changed (fi); } But, note that in non-stop mode, e.g., thread #2 may be running while you inspect thread #1. If thread #2 stops for any reason, say, a breakpoint, GDB switches temporarily to thread #2 to handle the event, and then that ends up calling normal_stop and select_frame. The patch makes that a user visible frame change. But, what happens is that GDB immediately afterwards changes back to thread #1 (and its frame), using a cleanup, giving the illusion that thread #1 was selected the whole time. This frame restore is an IMPL_DETAIL select_frame in this approach, so I think the frame change observer (python, in this case) ends up confused. Another problematic case I can think of off hand, is around errors. Say, the target is running, and while handling an event, we error out, and normal_stop isn't reached. I'm wondering whether we really need to call the frame change observers for stops. We have stop notifications/observers in both Python and MI, so it seems to be a script/frontend can already get the info it needs from those. If we limit frame change notifications to user frame changes (CLI up/down/frame and MI equivalents), then these issues with stops would be non-issues by design... -- Pedro Alves