From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24571 invoked by alias); 8 Oct 2008 12:26:25 -0000 Received: (qmail 24562 invoked by uid 22791); 8 Oct 2008 12:26:24 -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, 08 Oct 2008 12:25:48 +0000 Received: (qmail 5379 invoked from network); 8 Oct 2008 12:25:46 -0000 Received: from unknown (HELO orlando.local) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 8 Oct 2008 12:25:46 -0000 From: Pedro Alves To: teawater , Joel Brobecker Subject: Re: [reverse] PATCH: Several interface changes Date: Wed, 08 Oct 2008 12:26:00 -0000 User-Agent: KMail/1.9.9 Cc: gdb-patches@sourceware.org, "Michael Snyder" References: <200810071709.48346.pedro@codesourcery.com> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200810081325.44852.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-10/txt/msg00247.txt.bz2 On Wednesday 08 October 2008 08:28:05, teawater wrote: > I think the idea of this patch is good. Thanks for taking a look! > But maybe process record still not need it now because p record still > not support multi-thread. > Of course, p record will need it in the feature. The basic idea and motivation of the patch was to show that we don't need a target_set_execution_direction method in the target ops, or have the target_ops implementation query an infrun global --- the request to go backwards is a property of the command the user requested. Much like a single-step range property is a per-command/per-thread property. I believe that a single per-target_ops setting isn't quite the right abstraction. If a remote stub doesn't need a special indication that 'all operations from now on apply to reverse debugging', and can sort itself out from the stateless resume requests, a native target also shouldn't need it. If we find that a native target reverse debugging implementation *does require* a "set_execution_direction" command to work propertly and can't get it from the resume interface request, then the remote protocol may prove itself insuficient. E.g., if "target record" needs it, then implementing target record in gdbserver would be impossible with the current remote protocol --- there's always extensions and new packets, of course. As I said, this is just a thought, a proof-of-concept, only for consideration. It's *not* meant to hold reverse debugging from going into mainline. I'm more worried with the remote protocol being done right (and I manifested my opinion about that already), than about this, which is all internal implementation detail that we can change at any time. > > Michael, how about your target? > > > On Wed, Oct 8, 2008 at 00:09, Pedro Alves wrote: > > On Monday 06 October 2008 23:11:14, Michael Snyder wrote: > >> Pedro Alves wrote: > >> > A per-target property may seems to make sense on > >> > single-threaded,single-inferior targets, but when you add support > >> > for multi-inferiors per target (e.g., extended-remote has some of it now, > >> > and I'm going to push more of it), or multi-threaded support, the > >> > per-target setting may not make sense anymore --- explicit requests > >> > at the target resume interface (just like your new packets) may make > >> > more sense. Imagine forward execution non-stop debugging in all threads > >> > but one, which the user is examining in reverse. What's the target > >> > direction in this case? > >> > >> Yakkk!!! > > > > :-) Here's an alternative interface I was considering and envisioning > > when I mentioned the above. Consider this just a suggestion. If it > > looks bad, let's quickly forget about it. > > > >> > The question to me is --- when/why does the target (as in, the debug > >> > API abstraction) ever need to know about the current direction that > >> > it couldn't get from the core's request? > > > > So, after last night's discussion, I came up with the attached to > > see how it would look like. The major change is that I consider the > > reverse/forward-direction thing a property or the command the user > > requested, and as such, belongs together with the other thread > > stepping state we keep in struct thread_info, and the > > target_ops implementation, adjusts itself to the direction GDB > > requests with target_resume. I've extended target_resume's interface > > to accept this instead of a `step' boolean: > > > > enum target_resume_kind > > { > > /* Continue forward. */ > > rk_continue, > > > > /* Step forward. */ > > rk_step, > > > > /* Continue in the reverse direction. */ > > rk_continue_reverse, > > > > /* Step in the reverse direction. */ > > rk_step_reverse, > > }; > > > > (notice that the order of the things in the enum allows me to > > miss some conversions --- I'm lazy). > > > > I can't say if I like this new target_resume interface or > > not. I just tried doing it to see how it would look. > > > > (I can imagine that we're in the future going to extend the > > target_resume interface to be more like gdbserver's, but, well, > > that's another issue.) > > > > So, the interface at the command level implementation is just > > like it was before: > > > > 1) call clear_proceed_status (); > > > > 2) /* construct the step/continue request */ > > > > 3) call proceed (...); > > > > Where in #2, you can set the thread to go backwards by > > doing: > > > > thread->reverse = 1; > > > > The attached patch applies against the reverse-20080930-branch. > > > > Other things I've done in the patch: > > > > * Added support for a "Tnn nohistory" stop reply that translates > > to TARGET_WAITKIND_NO_HISTORY. When going multi-threaded, or > > multi-process this will allow things like "T05;thread:pPID.TID;nohistory" > > for free. I absolutelly didn't test this. I've no reverse aware target > > at hand. > > > > * At places, error out if async + reverse or non-stop + reverse > > was requested. > > > > * Added a target_can_reverse_p method, so infcmd.c can check if the > > target supports reverse execution before calling into the target. Not > > strictly necessary, though, but I thought this was nicer this way. > > > > I checked that I can use the record target on x86 (actually x86_64 > > with -m32) as good as without the patch, but it's quite possible I > > broke things badly. -- Pedro Alves