Re: [RFA] Clarify infrun variable naming.

[Jim Blandy <jimb at codesourcery dot com>]

Vladimir Prus <vladimir at codesourcery.com> writes:
> The infrun.c file has a variable named trap_expected, which
> is a bit misleading -- after all, most times when we resume
> inferior, we get SIGTRAP. As it turns out, that variable
> is set when we're stepping over breakpoints, so a better
> name would be stepping_over_breakpoint. Likewise,
> ecs->another_trap also indicates that keep_going should
> be stepping over breakpoint. The attached patch clarifies
> the naming and adds comments, and has no behaviour changes.
> (The patch is on top of my previous breakpoints_inserted
> removing patch).
> OK?

Vlad and I had a conversation about this on the CodeSourcery IRC
channel, let me summarize here:

stepping_over_breakpoint would be a better name for trap_expected,
except that we already have a variable named stepping_past_breakpoint.
What is the relationship between trap_expected and
stepping_past_breakpoint?

As Vlad explains in the comments in his patch, we set trap_expected
when we do a single-thread, breakpoints-uninserted step to get a
thread past a breakpoint it has hit.

We set stepping_past_breakpoint when GDB reports a breakpoint hit, and
then the user switches to a different thread and steps that thread.
In this case, GDB needs to step the original thread across the
breakpoint it hit (a single-thread, breakpoints-uninserted step), and
then step of the thread the user selected.  In effect, we set aside
the user's command until we've dealt with the thread that hit the
breakpoint.

Note that stepping_past_breakpoint is only ever set when trap_expected
is set.  Thinking in terms of desired behavior, this makes sense
because there's no need to put off stepping the user's selected thread
if we can simply continue the thread the hit the breakpoint ---
trap_expected is our indication that we can't.  And looking at the
code, we see this is true because prepare_to_proceed only sets
stepping_past_breakpoint if it will return non-zero, and that proceed
always sets trap_expected if prepare_to_proceed returns non-zero.
Check.

I think stepping_past_breakpoint is not a good name for this variable:
it doesn't do anything to suggest the two-thread delayed-action
situation at hand.  It sounds like it'd be involved with an ordinary
step past a breakpoint.

I don't think there's any need for both stepping_past_breakpoint and
stepping_past_breakpoint_ptid.  We could have a single ptid_t
variable, equal to null_ptid when no special handling is necessary.

So, my suggestions were:

- We should replace stepping_past_breakpoint and
  stepping_past_breakpoint_ptid with a single ptid_t variable,
  deferred_step_ptid.

- We should rename trap_expected to stepped_over_breakpoint.  The past
  tense 'stepped' suggests that we're talking about a step which has
  already happened (which is true by the time we reach
  handle_inferior_event).

(And if I meditate carefully enough on the best possible names, it'll
be quite some time before I have to look at Vlad's harder patches.  :))