From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9336 invoked by alias); 9 Nov 2012 23:00:52 -0000 Received: (qmail 9327 invoked by uid 22791); 9 Nov 2012 23:00:51 -0000 X-SWARE-Spam-Status: No, hits=-4.6 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,TW_EG X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 09 Nov 2012 23:00:44 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1TWxZ3-00058r-Sk from Luis_Gustavo@mentor.com ; Fri, 09 Nov 2012 15:00:41 -0800 Received: from NA1-MAIL.mgc.mentorg.com ([147.34.98.181]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Fri, 9 Nov 2012 15:00:41 -0800 Received: from [0.0.0.0] ([172.16.63.104]) by NA1-MAIL.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.3959); Fri, 9 Nov 2012 15:00:40 -0800 Message-ID: <509D8B21.2040804@codesourcery.com> Date: Fri, 09 Nov 2012 23:00:00 -0000 From: Luis Machado Reply-To: lgustavo@codesourcery.com User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.28) Gecko/20120313 Lightning/1.0b2 Thunderbird/3.1.20 MIME-Version: 1.0 To: Pedro Alves CC: ali_anwar , dje@google.com, Yao Qi , gdb-patches@sourceware.org Subject: Re: Patch to propagate GDB's knowledge of the executing state to frontend References: <50891E05.7050509@codesourcery.com> <508F719C.2080409@codesourcery.com> <20627.61842.606081.697743@ruffy2.mtv.corp.google.com> <50941519.6010005@codesourcery.com> <509D5E19.5060409@redhat.com> In-Reply-To: <509D5E19.5060409@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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: 2012-11/txt/msg00269.txt.bz2 On 11/09/2012 05:48 PM, Pedro Alves wrote: > On 11/02/2012 06:46 PM, ali_anwar wrote: >> On 11/02/2012 09:15 PM, dje@google.com wrote: >>> Yao Qi writes: >>> > On 10/25/2012 07:09 PM, ali_anwar wrote: >>> > > [...] >>> > > @@ -1,3 +1,13 @@ >>> > > +2012-10-25 Ali Anwar >>> > > + >>> > > + * infrun.c (handle_inferior_event_stub, regcache_dup_stub): >>> > > + New functions. >>> > > + (normal_stop): Change to propagate GDB's knowledge of the >>> > > + executing state to frontend when not able to fetch registers. >>> > > + (wait_for_inferior): Chnage to propagate GDB's knowledge of >>> > ^^^^^^ typo >>> > >>> > >>> > > + the executing state if not able to fetch backtrace once the >>> > > + step has already occured. >>> > ^^^^^^^ typo. >>> > >>> > In each changelog entry, we'll put 'what do we change' instead of 'why >>> > do we change in this way'. So this entry can be simplified. >>> >>> Hi. >>> >>> I agree with your first sentence, and would add that if such an >>> explanation is needed, it belongs in the code not the changelog. >>> [We don't have enough comments in the code explaining *why* things >>> are the way they are.] >>> >>> But I'd say that's not the case here, at least for the changelog entries. >>> Instead, I would remove the leading "Change to", and just say "Propagate ...". >>> >>> Also, I would add a comment to the code explaining *why* the calls are wrapped >>> in catch_error (and I would have the comment live at the call to catch_error, >>> not in the definition of the two new stubs). >>> >>> One could also say the two new functions also require comments, >>> but they're pretty simple and hook_stop_stub doesn't have a comment, >>> so I'd be ok with leaving them out. >> >> Thanks for the review. Please find attached the modified patch. >> >> -Ali >> >> target_state.patch >> >> >> Index: gdb/ChangeLog >> =================================================================== >> RCS file: /cvs/src/src/gdb/ChangeLog,v >> retrieving revision 1.14760 >> diff -u -r1.14760 ChangeLog >> --- gdb/ChangeLog 24 Oct 2012 19:08:15 -0000 1.14760 >> +++ gdb/ChangeLog 2 Nov 2012 18:41:48 -0000 >> @@ -1,3 +1,11 @@ >> +2012-10-25 Ali Anwar >> + >> + * infrun.c (handle_inferior_event_stub, regcache_dup_stub): >> + New functions. >> + (normal_stop): Propagate GDB's knowledge of the executing >> + state to frontend. >> + (wait_for_inferior): Likewise. >> + >> 2012-10-24 Tristan Gingold >> >> * ravenscar-sparc-thread.c (ravenscar_sparc_fetch_registers): >> Index: gdb/infrun.c >> =================================================================== >> RCS file: /cvs/src/src/gdb/infrun.c,v >> retrieving revision 1.559 >> diff -u -r1.559 infrun.c >> --- gdb/infrun.c 17 Sep 2012 07:26:55 -0000 1.559 >> +++ gdb/infrun.c 2 Nov 2012 18:41:49 -0000 >> @@ -73,6 +73,10 @@ >> >> static int hook_stop_stub (void *); >> >> +static int regcache_dup_stub (void *); >> + >> +static int handle_inferior_event_stub (void *); >> + >> static int restore_selected_frame (void *); >> >> static int follow_fork (void); >> @@ -2700,8 +2704,11 @@ >> state. */ >> old_chain = make_cleanup (finish_thread_state_cleanup,&minus_one_ptid); >> >> - /* Now figure out what to do with the result of the result. */ >> - handle_inferior_event (ecs); >> + /* Now figure out what to do with the result of the result. If an >> + error happens while handling the event, catch it to propagate >> + GDB's knowledge of the executing state. */ >> + catch_errors (handle_inferior_event_stub, ecs, >> + "Error while handling inferior event:\n", RETURN_MASK_ALL); >> >> /* No error, don't finish the state yet. */ >> discard_cleanups (old_chain); > > I don't understand this. The point of the finish_thread_state_cleanup > is doing exactly what you say is missing. If you swallow errors, then the > cleanup doesn't run at all (it's discarded immediately afterwards). > >> @@ -6080,9 +6087,12 @@ >> if (stop_registers) >> regcache_xfree (stop_registers); >> >> - /* NB: The copy goes through to the target picking up the value of >> - all the registers. */ >> - stop_registers = regcache_dup (get_current_regcache ()); >> + /* NB: The copy goes through to the target picking up the value >> + of all the registers. Catch error to propagate GDB's knowledge >> + of the executing state to frontend even when not able to fetch >> + registers. */ >> + catch_errors (regcache_dup_stub, NULL, >> + "Error while running regcache_dup:\n", RETURN_MASK_ALL); > > normal_stop has the finish_thread_state_cleanup installed too at the > top, and it has been run already above: > > /* Let the user/frontend see the threads as stopped. */ > do_cleanups (old_chain); > > So I'm afraid I don't understand exactly what this is fixing, and _how_ > this is fixing it. On an error, frontends should just no longer trust > any of their state, and issue a full refresh. Hey, Should frontends relying on MI information treat ^error specially and not look for any *stopped records? Suppose a step command was issued and we failed middleway through that command, at a point where gdb ran the inferior, noticed it stop but could not fetch enough data to produce a backtrace. Say, register access error or memory access error. At this point, the inferior is already stopped, but all the frontend is left with is the ^error output, which does not propagate the state of the inferior very clearly. For the frontend, the inferior is still running given the lack of a *stopped record. The MI specification gives room for slightly different interpretations unfortunately. Luis