Re: Patch to propagate GDB's knowledge of the executing state to frontend

[ali_anwar <ali_anwar@codesourcery.com>]

[-- Attachment #1: Type: text/plain, Size: 1766 bytes --]

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

[-- Attachment #2: target_state.patch --]
[-- Type: text/x-patch, Size: 2853 bytes --]

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);
     }
 
   if (stop_stack_dummy == STOP_STACK_DUMMY)
@@ -6154,6 +6164,20 @@
 }
 
 static int
+handle_inferior_event_stub (void *ecs)
+{
+  handle_inferior_event (ecs);
+  return (0);
+}
+
+static int
+regcache_dup_stub (void *arg)
+{
+  stop_registers = regcache_dup (get_current_regcache ());
+  return (0);
+}
+
+static int
 hook_stop_stub (void *cmd)
 {
   execute_cmd_pre_hook ((struct cmd_list_element *) cmd);