This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: Patch to propagate GDB's knowledge of the executing state to frontend
- From: Pedro Alves <palves at redhat dot com>
- To: ali_anwar <ali_anwar at codesourcery dot com>
- Cc: dje at google dot com, Yao Qi <yao at codesourcery dot com>, gdb-patches at sourceware dot org
- Date: Fri, 09 Nov 2012 19:48:41 +0000
- 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>
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<ali_anwar@codesourcery.com>
>> > > +
>> > > + * 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 <ali_anwar@codesourcery.com>
> +
> + * 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 <gingold@adacore.com>
>
> * 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