From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7292 invoked by alias); 9 Nov 2012 19:48:52 -0000 Received: (qmail 7282 invoked by uid 22791); 9 Nov 2012 19:48:50 -0000 X-SWARE-Spam-Status: No, hits=-7.8 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_EG X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 09 Nov 2012 19:48:46 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id qA9JmhvP022406 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 9 Nov 2012 14:48:43 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id qA9JmfAx003021; Fri, 9 Nov 2012 14:48:42 -0500 Message-ID: <509D5E19.5060409@redhat.com> Date: Fri, 09 Nov 2012 19:48:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:16.0) Gecko/20121016 Thunderbird/16.0.1 MIME-Version: 1.0 To: ali_anwar CC: 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> In-Reply-To: <50941519.6010005@codesourcery.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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/msg00267.txt.bz2 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. > + catch_errors (regcache_dup_stub, NULL, > + "Error while running regcache_dup:\n", RETURN_MASK_ALL); So this also not okay for the related reason that it would swallow all kinds of errors (even ctrl-c), so that the error is no longer propagated to the user/frontend. -- Pedro Alves