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

[Luis Machado <lgustavo@codesourcery.com>]

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