[PATCH] gdb: rename thread_control_state::trap_expected to stepping_over_breakpoint

[Simon Marchi]

On 2020-06-16 6:05 p.m., Pedro Alves wrote:
> On 6/16/20 7:03 PM, Simon Marchi wrote:
>> On 2020-05-28 4:00 p.m., Simon Marchi via Gdb-patches wrote:
>>> From: Simon Marchi <simon.marchi@efficios.com>
>>>
>>> While reading this code, I felt that the field `trap_expected` of
>>> `struct thread_control_state` did not have a very clear name.  It
>>> basically means that the thread is currently stepping over a breakpoint
>>> (that's what the comment above the field says anyway).  I know this is
>>> subjective, but I think that naming the field `stepping_over_breakpoint`
>>> instead would be more descriptive of its function.
>>>
>>> My only concern is that `struct thread_info` already contains a
>>> `stepping_over_breakpoint` field, so it might be a bit confusing, and I
>>> don't really understand the different between these two fields.
>>>
>>> Also, the comment above thread_control_state::stepping_over_breakpoint
>>> sounds like it's a bit outdated/incomplete.  It describes inline step
>>> overs, where we stop all other threads, but I think this variable is
>>> also set when the thread is doing a displaced step, and it doesn't
>>> mention that.  I don't feel apt to update the comment right now though.
>>>
>>> While at it, change the field to bool.
>>>
>>> gdb/ChangeLog:
>>>
>>> 	* gdbthread.h (struct thread_control_state)
>>> 	<trap_expected>: Rename to...
>>> 	<stepping_over_breakpoint>: ... this.  Update all users.
> 
> I really don't like this patch for making the two fields have the
> same name.  People can get used to funny names and then don't
> even think about it.  But the same name for two related, but
> different fields is just a recipe for confusion/pain.
> 
> stepping_over_breakpoint / stepping_over_watchpoint indicate
> whether we need to step over a breakpoint or a watchpoint the
> next time the thread is resumed.  So it indicates intention.
> 
> trap_expected indicates whether the step over action has
> already started, and now we're waiting for the trap.  The name
> makes more sense if you look at code like this:
> 
>   if (ecs->event_thread->suspend.stop_signal == GDB_SIGNAL_TRAP
>       && ecs->event_thread->control.trap_expected
>       && ecs->event_thread->stepping_over_watchpoint)
> ...
> 
> 	  && !(ecs->event_thread->suspend.stop_signal == GDB_SIGNAL_TRAP
> 	       && ecs->event_thread->control.trap_expected
> 	       && pc_at_non_inline_function (aspace,
> 
> ...
> 
>   if (ecs->event_thread->suspend.stop_signal == GDB_SIGNAL_TRAP
>       && ecs->event_thread->control.trap_expected
> 
> ...
>       if (ecs->event_thread->control.trap_expected
> 	  && ecs->event_thread->suspend.stop_signal != GDB_SIGNAL_TRAP)
> 
> ...
>       if (ecs->event_thread->prev_pc == ecs->event_thread->suspend.stop_pc
> 	  && ecs->event_thread->control.trap_expected
> 	  && ecs->event_thread->control.step_resume_breakpoint == NULL)
> 	{
> 	  /* We were just starting a new sequence, attempting to
> 	     single-step off of a breakpoint and expecting a SIGTRAP.
>                                                  ^^^^^^^^^^^^^^^^^^^
> ...
>       /* Otherwise, we no longer expect a trap in the current thread.
> 	 Clear the trap_expected flag before switching back -- this is
> 	 what keep_going does as well, if we call it.  */
>       ecs->event_thread->control.trap_expected = 0;
> ...
> 
> etc.

With this patch, I mostly wanted to raise the question, whether this name
still made sense.  With your explanation, it clarifies it (it's the
event we expect, rather than the action we are doing which leads to this
event) so it makes sense to keep it that way.

> I think we need to start by cleaning up the comments, making
> sure they match reality before we start thinking about better names.
> The documentation for trap_expected is way out of date.  It
> predates displaced stepping and non-stop mode ("keep other threads
> stopped").  It predates stepping over watchpoints with breakpoints
> inserted (keep_going_pass_signal).  Says the variable is cleared
> in normal_stop, when it isn't.
> 
> I.e., something like:
> 
> ~~~~
> diff --git c/gdb/gdbthread.h w/gdb/gdbthread.h
> index 1b4390f93f..dd3a23296d 100644
> --- c/gdb/gdbthread.h
> +++ w/gdb/gdbthread.h
> @@ -132,28 +132,10 @@ struct thread_control_state
>       any inlined frames).  */
>    struct frame_id step_stack_frame_id {};
>  
> -  /* Nonzero if we are presently stepping over a breakpoint.
> -
> -     If we hit a breakpoint or watchpoint, and then continue, we need
> -     to single step the current thread with breakpoints disabled, to
> -     avoid hitting the same breakpoint or watchpoint again.  And we
> -     should step just a single thread and keep other threads stopped,
> -     so that other threads don't miss breakpoints while they are
> -     removed.
> -
> -     So, this variable simultaneously means that we need to single
> -     step the current thread, keep other threads stopped, and that
> -     breakpoints should be removed while we step.
> -
> -     This variable is set either:
> -     - in proceed, when we resume inferior on user's explicit request
> -     - in keep_going, if handle_inferior_event decides we need to
> -     step over breakpoint.
> -
> -     The variable is cleared in normal_stop.  The proceed calls
> -     wait_for_inferior, which calls handle_inferior_event in a loop,
> -     and until wait_for_inferior exits, this variable is changed only
> -     by keep_going.  */
> +  /* True if the the thread is presently stepping over a breakpoint or
> +     a watchpoint, either with an inline step over or a displaced (out
> +     of line) step, and we're now expecting it to report a trap for
> +     the finished single step.  */
>    int trap_expected = 0;

Yep, that makes it much more straight to the point

Thanks for the explanation.

Simon