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

Pedro Alves palves@redhat.com
Tue Jun 16 22:05:07 GMT 2020


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.

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;
~~~~

Thanks,
Pedro Alves



More information about the Gdb-patches mailing list