From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15045 invoked by alias); 6 Oct 2008 21:22:10 -0000 Received: (qmail 15037 invoked by uid 22791); 6 Oct 2008 21:22:10 -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:21:35 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 6040C2A962D; Mon, 6 Oct 2008 17:21:33 -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 bfZ9yKbap+Ap; Mon, 6 Oct 2008 17:21:33 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 435132A961F; Mon, 6 Oct 2008 17:21:33 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 11102E7ACD; Mon, 6 Oct 2008 14:21:33 -0700 (PDT) Date: Mon, 06 Oct 2008 21:22:00 -0000 From: Joel Brobecker To: Michael Snyder Cc: "gdb-patches@sourceware.org" , Daniel Jacobowitz , Pedro Alves , teawater Subject: Re: [RFA] Reverse Debugging, 3/5 Message-ID: <20081006212132.GB21853@adacore.com> References: <48E3CD0B.8020003@vmware.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <48E3CD0B.8020003@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/msg00167.txt.bz2 I had some comments, too, but Pedro made most of them. Here are mine that don't overlap with his... > + /* OK, we're just gonna keep stepping here. */ I would prefer if we kept slang out of the comments. Can we use "going to" instead? > + if (stop_func_sal.pc == stop_pc) > + { > + /* We're there already. Just stop stepping now. */ > + ecs->event_thread->stop_step = 1; > + print_stop_reason (END_STEPPING_RANGE, 0); > + stop_stepping (ecs); > + return; > + } > + /* Else just reset the step range and keep going. > + No step-resume breakpoint, they don't work for > + epilogues, which can have multiple entry paths. */ > + ecs->event_thread->step_range_start = stop_func_sal.pc; > + ecs->event_thread->step_range_end = stop_func_sal.end; > + keep_going (ecs); > + return; > + } Regarding this hunk, it really took me a long time to understand what we're supposed to do when execution is reverse. I think it was a combination of the function name together with the fact that we're not trying to undo what the function does, but instead implement whatever command the user has issued which caused us to call this function. I am thinking, for the sake of clarity, to implement this in a separate function and call the appropriate function from handle_inferior_event. The function name can then be used to explain at a glance what it's supposed to do... -- Joel