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

[Luis Machado <lgustavo@codesourcery.com>]

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