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);
@@ -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);