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

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

[ali_anwar]

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

Hi,

Attached patch is to let GDB propagate the target state under following 
two scenarios:

1. Attached patch will enable GDB to inform about the state of the 
target when it was not able to fetch the non-general registers, when 
target is already stopped.

The reason behind this behavior was an error message which was caused 
when the GDB was not able to fetch the value of a certain register. The 
GDB should have told the front end about the current state of the 
target. The attached patch makes sure that it happens. This patch should 
be a safety measure in case some debugging stub behaves badly.

2. Attached patch will enable GDB to inform about the state of the 
target when it was not able to fetch the backtrace once the step has 
already occurred and target is in stopped state.

I have tested the patch for powerpc-eabi as well as i686-pc-linux. No 
regression was introduced by the patch and results were identical with 
and without the patch.

The patch has also been tested with both sync and async mode, no regression.

OK?

Thanks,
-Ali


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

Index: ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.14760
diff -u -r1.14760 ChangeLog
--- ChangeLog	24 Oct 2012 19:08:15 -0000	1.14760
+++ ChangeLog	25 Oct 2012 10:52:02 -0000
@@ -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
+	the executing state if not able to fetch backtrace once the
+	step has already occured.
+
 2012-10-24  Tristan Gingold  <gingold@adacore.com>
 
 	* ravenscar-sparc-thread.c (ravenscar_sparc_fetch_registers):
Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.559
diff -u -r1.559 infrun.c
--- infrun.c	17 Sep 2012 07:26:55 -0000	1.559
+++ infrun.c	25 Oct 2012 10:52:04 -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);
@@ -2701,7 +2705,8 @@
       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);
+      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);
@@ -6082,7 +6087,8 @@
 
       /* NB: The copy goes through to the target picking up the value of
 	 all the registers.  */
-      stop_registers = regcache_dup (get_current_regcache ());
+      catch_errors (regcache_dup_stub, NULL,
+                    "Error while running regcache_dup:\n", RETURN_MASK_ALL);
     }
 
   if (stop_stack_dummy == STOP_STACK_DUMMY)
@@ -6154,6 +6160,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);

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

[Yao Qi]

On 10/25/2012 07:09 PM, ali_anwar wrote:
> Hi,
>
> Attached patch is to let GDB propagate the target state under following
> two scenarios:
>
> 1. Attached patch will enable GDB to inform about the state of the
> target when it was not able to fetch the non-general registers, when
> target is already stopped.
>
> The reason behind this behavior was an error message which was caused
> when the GDB was not able to fetch the value of a certain register. The
> GDB should have told the front end about the current state of the
> target. The attached patch makes sure that it happens. This patch should
> be a safety measure in case some debugging stub behaves badly.
>
> 2. Attached patch will enable GDB to inform about the state of the
> target when it was not able to fetch the backtrace once the step has
> already occurred and target is in stopped state.
>

It is better to describe what will happen or what is wrong if this patch 
is not applied.

> Index: ChangeLog
> ===================================================================
> RCS file: /cvs/src/src/gdb/ChangeLog,v
> retrieving revision 1.14760
> diff -u -r1.14760 ChangeLog
> --- ChangeLog	24 Oct 2012 19:08:15 -0000	1.14760
> +++ ChangeLog	25 Oct 2012 10:52:02 -0000
> @@ -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.

> Index: infrun.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/infrun.c,v
> retrieving revision 1.559
> diff -u -r1.559 infrun.c
> --- infrun.c	17 Sep 2012 07:26:55 -0000	1.559
> +++ infrun.c	25 Oct 2012 10:52:04 -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);
> @@ -2701,7 +2705,8 @@
>         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);
> +      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);
> @@ -6082,7 +6087,8 @@
>
>         /* NB: The copy goes through to the target picking up the value of
>   	 all the registers.  */
> -      stop_registers = regcache_dup (get_current_regcache ());
> +      catch_errors (regcache_dup_stub, NULL,
> +                    "Error while running regcache_dup:\n", RETURN_MASK_ALL);
>       }
>
>     if (stop_stack_dummy == STOP_STACK_DUMMY)
> @@ -6154,6 +6160,20 @@
>   }
>
>   static int
> +handle_inferior_event_stub (void *ecs)
> +{
> +  handle_inferior_event (ecs);
> +  return (0);

parentheses are not needed.

> +}
> +
> +static int
> +regcache_dup_stub (void *arg)
> +{
> +  stop_registers = regcache_dup (get_current_regcache ());
> +  return (0);

Likewise.

-- 
Yao

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

[ali_anwar]

On 10/30/2012 11:20 AM, Yao Qi wrote:
> On 10/25/2012 07:09 PM, ali_anwar wrote:
>> Hi,
>>
>> Attached patch is to let GDB propagate the target state under following
>> two scenarios:
>>
>> 1. Attached patch will enable GDB to inform about the state of the
>> target when it was not able to fetch the non-general registers, when
>> target is already stopped.
>>
>> The reason behind this behavior was an error message which was caused
>> when the GDB was not able to fetch the value of a certain register. The
>> GDB should have told the front end about the current state of the
>> target. The attached patch makes sure that it happens. This patch should
>> be a safety measure in case some debugging stub behaves badly.
>>
>> 2. Attached patch will enable GDB to inform about the state of the
>> target when it was not able to fetch the backtrace once the step has
>> already occurred and target is in stopped state.
>>
>
> It is better to describe what will happen or what is wrong if this patch
> is not applied.
>

Thanks Yao for the review. Let me restate the actual problem:

Under certain scenarios, GDB is unable to specify the correct target 
state once the step/finish instruction is executed.

1. If you perform a step out (finish) and there is an error when GDB 
tries to fetch the register values.

2. If you perform a ste and there is an error when GDB tries to fetch 
the backtrace.

In both the cases the only output is an error message and nothing is 
printed as far as current target state is concerned.e.g.


(gdb)
-exec-finish
^running
*running,thread-id="all"
(gdb)
^error,msg="Could not fetch register \"\"; remote failure reply 'E22'"
(gdb)


In other words from MI's perspective, the step hasn't completed yet – 
the state is still "running".

The only concern is GDB not printing the state of the target. It does 
not matter why the error occurred.

>> + 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.
>

I will fix the both typos.

> 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.
>

I will look into it as well.

>> + handle_inferior_event (ecs);
>> + return (0);
>
> parentheses are not needed.
>
>> +}
>> +
>> + return (0);
>
> Likewise.
>

I will remove the parentheses.


Thanks,
-Ali

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

[ali_anwar]

On 10/30/2012 04:52 PM, ali_anwar wrote:
> On 10/30/2012 11:20 AM, Yao Qi wrote:
>> On 10/25/2012 07:09 PM, ali_anwar wrote:
>>> Hi,
>>>
>>> Attached patch is to let GDB propagate the target state under following
>>> two scenarios:
>>>
>>> 1. Attached patch will enable GDB to inform about the state of the
>>> target when it was not able to fetch the non-general registers, when
>>> target is already stopped.
>>>
>>> The reason behind this behavior was an error message which was caused
>>> when the GDB was not able to fetch the value of a certain register. The
>>> GDB should have told the front end about the current state of the
>>> target. The attached patch makes sure that it happens. This patch should
>>> be a safety measure in case some debugging stub behaves badly.
>>>
>>> 2. Attached patch will enable GDB to inform about the state of the
>>> target when it was not able to fetch the backtrace once the step has
>>> already occurred and target is in stopped state.
>>>
>>
>> It is better to describe what will happen or what is wrong if this patch
>> is not applied.
>>
>
> Thanks Yao for the review. Let me restate the actual problem:
>
> Under certain scenarios, GDB is unable to specify the correct target
> state once the step/finish instruction is executed.
>
> 1. If you perform a step out (finish) and there is an error when GDB
> tries to fetch the register values.
>
> 2. If you perform a ste and there is an error when GDB tries to fetch
> the backtrace.
>
> In both the cases the only output is an error message and nothing is
> printed as far as current target state is concerned.e.g.
>
>
> (gdb)
> -exec-finish
> ^running
> *running,thread-id="all"
> (gdb)
> ^error,msg="Could not fetch register \"\"; remote failure reply 'E22'"
> (gdb)
>
>
> In other words from MI's perspective, the step hasn't completed yet –
> the state is still "running".
>
> The only concern is GDB not printing the state of the target. It does
> not matter why the error occurred.
>
>>> + 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.
>>
>
> I will fix the both typos.
>
>> 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.
>>
>
> I will look into it as well.
>
>>> + handle_inferior_event (ecs);
>>> + return (0);
>>
>> parentheses are not needed.
>>
>>> +}
>>> +
>>> + return (0);
>>
>> Likewise.
>>
>
> I will remove the parentheses.
>
>

Ping.. Is it OK to commit?

-Ali

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

[Yao Qi]

On 11/02/2012 06:27 PM, ali_anwar wrote:
>
> Ping.. Is it OK to commit?

You need the review from global maintainers.  Could you post the latest 
patch in which my comments have been addressed, so that global 
maintainers (not me) can review it easily.

-- 
Yao

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

[ali_anwar]

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

On 10/30/2012 04:52 PM, ali_anwar wrote:
> On 10/30/2012 11:20 AM, Yao Qi wrote:
>> On 10/25/2012 07:09 PM, ali_anwar wrote:
>>> Hi,
>>>
>>> Attached patch is to let GDB propagate the target state under following
>>> two scenarios:
>>>
>>> 1. Attached patch will enable GDB to inform about the state of the
>>> target when it was not able to fetch the non-general registers, when
>>> target is already stopped.
>>>
>>> The reason behind this behavior was an error message which was caused
>>> when the GDB was not able to fetch the value of a certain register. The
>>> GDB should have told the front end about the current state of the
>>> target. The attached patch makes sure that it happens. This patch should
>>> be a safety measure in case some debugging stub behaves badly.
>>>
>>> 2. Attached patch will enable GDB to inform about the state of the
>>> target when it was not able to fetch the backtrace once the step has
>>> already occurred and target is in stopped state.
>>>
>>
>> It is better to describe what will happen or what is wrong if this patch
>> is not applied.
>>
>
> Thanks Yao for the review. Let me restate the actual problem:
>
> Under certain scenarios, GDB is unable to specify the correct target
> state once the step/finish instruction is executed.
>
> 1. If you perform a step out (finish) and there is an error when GDB
> tries to fetch the register values.
>
> 2. If you perform a ste and there is an error when GDB tries to fetch
> the backtrace.
>
> In both the cases the only output is an error message and nothing is
> printed as far as current target state is concerned.e.g.
>
>
> (gdb)
> -exec-finish
> ^running
> *running,thread-id="all"
> (gdb)
> ^error,msg="Could not fetch register \"\"; remote failure reply 'E22'"
> (gdb)
>
>
> In other words from MI's perspective, the step hasn't completed yet –
> the state is still "running".
>
> The only concern is GDB not printing the state of the target. It does
> not matter why the error occurred.
>
>>> + 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.
>>
>
> I will fix the both typos.
>
>> 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.
>>
>
> I will look into it as well.
>
>>> + handle_inferior_event (ecs);
>>> + return (0);
>>
>> parentheses are not needed.
>>
>>> +}
>>> +
>>> + return (0);
>>
>> Likewise.
>>
>
> I will remove the parentheses.
>

PFA the latest patch. OK to commit?

-Ali

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

Index: ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.14760
diff -u -r1.14760 ChangeLog
--- ChangeLog	24 Oct 2012 19:08:15 -0000	1.14760
+++ ChangeLog	25 Oct 2012 10:52:02 -0000
@@ -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.
+	(wait_for_inferior): Likewise.
+
 2012-10-24  Tristan Gingold  <gingold@adacore.com>
 
 	* ravenscar-sparc-thread.c (ravenscar_sparc_fetch_registers):
Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.559
diff -u -r1.559 infrun.c
--- infrun.c	17 Sep 2012 07:26:55 -0000	1.559
+++ infrun.c	25 Oct 2012 10:52:04 -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);
@@ -2701,7 +2705,8 @@
       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);
+      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);
@@ -6082,7 +6087,8 @@
 
       /* NB: The copy goes through to the target picking up the value of
 	 all the registers.  */
-      stop_registers = regcache_dup (get_current_regcache ());
+      catch_errors (regcache_dup_stub, NULL,
+                    "Error while running regcache_dup:\n", RETURN_MASK_ALL);
     }
 
   if (stop_stack_dummy == STOP_STACK_DUMMY)
@@ -6154,6 +6160,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);

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

[dje]

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.

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

[ali_anwar]

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

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

[Anwar, Ali]

Ping... OK to commit?

Regards,
-Ali
________________________________________
From: gdb-patches-owner@sourceware.org [gdb-patches-owner@sourceware.org] on behalf of Anwar, Ali
Sent: Friday, November 02, 2012 11:46 PM
To: dje@google.com
Cc: Qi, Yao; gdb-patches@sourceware.org
Subject: Re: Patch to propagate GDB's knowledge of the executing state to frontend

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

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

[Pedro Alves]

On 11/09/2012 07:30 PM, Anwar, Ali wrote:
> Ping... OK to commit?

Thanks.  I'm reviewing this.  Will send out another email in a few minutes.

-- 
Pedro Alves

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

[Pedro Alves]

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

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

[Luis Machado]

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<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.

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.

At this point, the inferior is already stopped, but all the frontend is 
left with is the ^error output, which does not propagate the state of 
the inferior very clearly.

For the frontend, the inferior is still running given the lack of a 
*stopped record.

The MI specification gives room for slightly different interpretations 
unfortunately.

Luis

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

[Luis Machado]

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<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.
>
> 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...

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

[Tom Tromey]

Luis> Should frontends relying on MI information treat ^error specially and
Luis> not look for any *stopped records?

I don't know the answer to this.  I did find this though:

http://sourceware.org/bugzilla/show_bug.cgi?id=7778

I tend to think it would be cleanest if gdb were to emit a *stopped in
case of error -- but only if it previously emitted *running.  I don't
know how feasible this is.

Luis> The MI specification gives room for slightly different interpretations
Luis> unfortunately.

For me, the text for "*running" is pretty clear:

     The frontend should assume that no interaction with a
     running thread is possible after this notification is produced.

I'm curious where the text is that gives room for another approach.

Tom

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

[Luis Machado]

On 11/13/2012 07:57 PM, Tom Tromey wrote:
> Luis>  Should frontends relying on MI information treat ^error specially and
> Luis>  not look for any *stopped records?
>
> I don't know the answer to this.  I did find this though:
>
> http://sourceware.org/bugzilla/show_bug.cgi?id=7778
>
> I tend to think it would be cleanest if gdb were to emit a *stopped in
> case of error -- but only if it previously emitted *running.  I don't
> know how feasible this is.

Right. That seems to be what Ali's patch is attempting to address, in 
order to improve the relationship with the frontends.

>
> Luis>  The MI specification gives room for slightly different interpretations
> Luis>  unfortunately.
>
> For me, the text for "*running" is pretty clear:
>
>       The frontend should assume that no interaction with a
>       running thread is possible after this notification is produced.
>
> I'm curious where the text is that gives room for another approach.

I wanted to know what is to be done once an ^error is thrown at the 
frontend and how it should behave.

There is this text:

"There's no guarantee that whenever an MI command reports an error, gdb 
or the target are in any specific state, and especially, the state is 
not reverted to the state before the MI command was processed. 
Therefore, whenever an MI command results in an error, we recommend that 
the frontend refreshes all the information shown in the user interface."

It is not clear how things need to be refreshed, specially when the 
frontend needs to assume a thread/inferior is still running after it saw 
^running but did not see a *stopped record, though it did see an ^error. 
But ^error does not imply the target has stopped.

So it needs to refresh information, but should it forget the target was 
running and attempt to fetch data anyway or should it do something else?

In the case of all-stop mode, we know the inferior is stopped. This may 
not be true for non-stop though.

Luis

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

[Pedro Alves]

On 11/13/2012 10:24 PM, Luis Machado wrote:
> On 11/13/2012 07:57 PM, Tom Tromey wrote:
>> Luis>  Should frontends relying on MI information treat ^error specially and
>> Luis>  not look for any *stopped records?
>>
>> I don't know the answer to this.  I did find this though:
>>
>> http://sourceware.org/bugzilla/show_bug.cgi?id=7778
>>
>> I tend to think it would be cleanest if gdb were to emit a *stopped in
>> case of error -- but only if it previously emitted *running.  I don't
>> know how feasible this is.
> 
> Right. That seems to be what Ali's patch is attempting to address, in order to improve the relationship with the frontends.

It might be a worthy goal (*), but IIUC that patch breaks things like

define foo
  step
  step
end

where an error happens while handling the first step, we'll go on
with the command list anyway, instead of canceling the whole canned
command.

(*) - though if the frontend is going to do a full refresh
I don't see that much point.

> 
>>
>> Luis>  The MI specification gives room for slightly different interpretations
>> Luis>  unfortunately.
>>
>> For me, the text for "*running" is pretty clear:
>>
>>       The frontend should assume that no interaction with a
>>       running thread is possible after this notification is produced.
>>
>> I'm curious where the text is that gives room for another approach.

Well, an "except when things go wrong" is missing here.  :-)

> I wanted to know what is to be done once an ^error is thrown at the frontend and how it should behave.
> 
> There is this text:
> 
> "There's no guarantee that whenever an MI command reports an error, gdb or the target are in any specific state, and especially, the state is not reverted to the state before the MI command was processed. Therefore, whenever an MI command results in an error, we recommend that the frontend refreshes all the information shown in the user interface."
> 
> It is not clear how things need to be refreshed, specially when the frontend needs to assume a thread/inferior is still running after it saw ^running but did not see a *stopped record, though it did see an ^error. But ^error does not imply the target has stopped.
> 
> So it needs to refresh information, but should it forget the target was running and attempt to fetch data anyway or should it do something else?
> 
> In the case of all-stop mode, we know the inferior is stopped. This may not be true for non-stop though.

Yes, this is what I was alluding to when I said that an error, frontends should
just no longer trust any of their state, and issue a full refresh.  I assumed
frontends to be already doing this.  -thread-info includes a "state" field for
each thread that says whether the thread is running or not.  The
finish_thread_state_cleanup cleanups are there to make sure the "state" field
ends up correct on errors:

-thread-info
^done,threads=[{id="1",target-id="Thread 0x7ffff7fce740 (LWP 26570)",name="gdb",frame={level="0",addr="0x0000000000457bcb",func="main",args=[{name="argc",value="1"},{name="argv",value="0x7fffffffdc78"}],file="../../src/gdb/gdb.c",fullname="/home/pedro/gdb/mygit/src/gdb/gdb.c",line="29"},state="stopped",core="0"}],current-thread-id="1"
(gdb)

I guess this means the frontend in question is either not issuing that
command, or not reading that "state" field.

-- 
Pedro Alves

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

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

[...]
Pedro> I guess this means the frontend in question is either not issuing that
Pedro> command, or not reading that "state" field.

I think it would be helpful to mention this explicitly in the text
related near ^error and *running.

Tom