From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10606 invoked by alias); 9 Nov 2012 23:02:28 -0000 Received: (qmail 10598 invoked by uid 22791); 9 Nov 2012 23:02:27 -0000 X-SWARE-Spam-Status: No, hits=-4.1 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,MISSING_HEADERS,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:02:22 +0000 Received: from svr-orw-fem-01.mgc.mentorg.com ([147.34.98.93]) by relay1.mentorg.com with esmtp id 1TWxaf-0005JD-QS from Luis_Gustavo@mentor.com ; Fri, 09 Nov 2012 15:02:21 -0800 Received: from NA1-MAIL.mgc.mentorg.com ([147.34.98.181]) by svr-orw-fem-01.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Fri, 9 Nov 2012 15:02:21 -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:02:20 -0800 Message-ID: <509D8B85.6000803@codesourcery.com> Date: Fri, 09 Nov 2012 23:02: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 CC: Pedro Alves , 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> <509D8B21.2040804@codesourcery.com> In-Reply-To: <509D8B21.2040804@codesourcery.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/msg00270.txt.bz2 On 11/09/2012 09:00 PM, Luis Machado wrote: > 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. *midway through* that is...