From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31814 invoked by alias); 5 Dec 2007 01:18:01 -0000 Received: (qmail 31801 invoked by uid 22791); 5 Dec 2007 01:18:01 -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.31) with ESMTP; Wed, 05 Dec 2007 01:17:55 +0000 Received: (qmail 14048 invoked from network); 5 Dec 2007 01:17:53 -0000 Received: from unknown (HELO localhost) (jimb@127.0.0.2) by mail.codesourcery.com with ESMTPA; 5 Dec 2007 01:17:53 -0000 To: Vladimir Prus Cc: gdb-patches@sources.redhat.com Subject: Re: [RFA] Clarify infrun variable naming. References: <200711231623.04823.vladimir@codesourcery.com> From: Jim Blandy Date: Wed, 05 Dec 2007 01:18:00 -0000 In-Reply-To: <200711231623.04823.vladimir@codesourcery.com> (Vladimir Prus's message of "Fri, 23 Nov 2007 16:23:04 +0300") Message-ID: User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.0.50 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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: 2007-12/txt/msg00070.txt.bz2 Vladimir Prus writes: > The infrun.c file has a variable named trap_expected, which > is a bit misleading -- after all, most times when we resume > inferior, we get SIGTRAP. As it turns out, that variable > is set when we're stepping over breakpoints, so a better > name would be stepping_over_breakpoint. Likewise, > ecs->another_trap also indicates that keep_going should > be stepping over breakpoint. The attached patch clarifies > the naming and adds comments, and has no behaviour changes. > (The patch is on top of my previous breakpoints_inserted > removing patch). > OK? Vlad and I had a conversation about this on the CodeSourcery IRC channel, let me summarize here: stepping_over_breakpoint would be a better name for trap_expected, except that we already have a variable named stepping_past_breakpoint. What is the relationship between trap_expected and stepping_past_breakpoint? As Vlad explains in the comments in his patch, we set trap_expected when we do a single-thread, breakpoints-uninserted step to get a thread past a breakpoint it has hit. We set stepping_past_breakpoint when GDB reports a breakpoint hit, and then the user switches to a different thread and steps that thread. In this case, GDB needs to step the original thread across the breakpoint it hit (a single-thread, breakpoints-uninserted step), and then step of the thread the user selected. In effect, we set aside the user's command until we've dealt with the thread that hit the breakpoint. Note that stepping_past_breakpoint is only ever set when trap_expected is set. Thinking in terms of desired behavior, this makes sense because there's no need to put off stepping the user's selected thread if we can simply continue the thread the hit the breakpoint --- trap_expected is our indication that we can't. And looking at the code, we see this is true because prepare_to_proceed only sets stepping_past_breakpoint if it will return non-zero, and that proceed always sets trap_expected if prepare_to_proceed returns non-zero. Check. I think stepping_past_breakpoint is not a good name for this variable: it doesn't do anything to suggest the two-thread delayed-action situation at hand. It sounds like it'd be involved with an ordinary step past a breakpoint. I don't think there's any need for both stepping_past_breakpoint and stepping_past_breakpoint_ptid. We could have a single ptid_t variable, equal to null_ptid when no special handling is necessary. So, my suggestions were: - We should replace stepping_past_breakpoint and stepping_past_breakpoint_ptid with a single ptid_t variable, deferred_step_ptid. - We should rename trap_expected to stepped_over_breakpoint. The past tense 'stepped' suggests that we're talking about a step which has already happened (which is true by the time we reach handle_inferior_event). (And if I meditate carefully enough on the best possible names, it'll be quite some time before I have to look at Vlad's harder patches. :))