Re: [PATCH, v6] Fix reverse stepping multiple contiguous PC ranges over the line table

[Ulrich Weigand <Ulrich.Weigand@de.ibm.com>]

Luis Machado <luis.machado@arm.com> wrote:

Carl asked me to look into this as well, so a couple of comments:

>When stepping forward from line 40, we skip both of these ranges and
>land on line 42. When stepping backward from line 42, we stop at the
>start PC of the second (or first, going backwards) range of line 40.

I've been wondering why the forward-stepping case works.  It looks
like this is because of the code at the end of process_event_stop_test,
which verifies whether we have stepped to a different line, and if not,
just continues stepping.

It seems it would be preferable to have the same mechanism for both
forward- and reverse-stepping, but that doesn't work because of
this piece of code you point out:

>This happens because we have this check in
>infrun.c:process_event_stop_test:
>
>      /* When stepping backward, stop at beginning of line range
>	 (unless it's the function entry point, in which case
>	 keep going back to the call point).  */
>      CORE_ADDR stop_pc = ecs->event_thread->stop_pc ();
>      if (stop_pc == ecs->event_thread->control.step_range_start
>	  && stop_pc != ecs->stop_func_start
>	  && execution_direction == EXEC_REVERSE)
>	end_stepping_range (ecs);
>      else
>	keep_going (ecs);

In my view, this code is already questionable.  This is under the
overall control of "pc_in_thread_step_range", and the point of
this is to check that we're still supposed to step!  But even
though the test returns true, we now add a special case where
we stop stepping after all ...

Maybe we should not hard-code the end_stepping_range call here, but
rath
er just fall through to the rest of process_event_step_test
to handle
whatever other case we may encounter?  Or equivalently,
have
pc_in_thread_step_range do a
  start < pc <= end
comparison in the
EXEC_REVERSE case, instead of
  start <= pc < end 

[ But maybe this is also better done as independent cleanup,
I'm not saying we necessarily have to do it this way ... ]


>Another solution I thought about is to merge the contiguous ranges
>when we are reading the line tables. Though I'm not sure if we
>really want to process that data as opposed to keeping it as the
>compiler created, and then working around that.

Maybe not immediately when reading the line tables; we might
want to actually use the column granularity at some point.

But at the point where we set up the stepping range in
prepare_one_step, we could increase the range *there*.
This might not only be somewhat simpler, but also improve
performance since the check is only done once and not every
time we run through process_event_stop_test (and it might
even also improve the performance of forward-stepping in 
those cases if we increase the range downwards too).


>+      CORE_ADDR range_start = ecs->event_thread-
>>control.step_range_start;
>+      if (execution_direction == EXEC_REVERSE)
>+	{
>+	  gdb::optional<CORE_ADDR> real_range_start
>+	    = find_line_range_start (ecs->event_thread->stop_pc ());
>+
>+	  if (real_range_start.has_value ())
>+	    range_start = *real_range_start;
>+	}
>+
>       /* When stepping backward, stop at beginning of line range
> 	 (unless it's the function entry point, in which case
> 	 keep going back to the call point).  */
>       CORE_ADDR stop_pc = ecs->event_thread->stop_pc ();
>-      if (stop_pc == ecs->event_thread->control.step_range_start
>+      if (stop_pc == range_start
> 	  && stop_pc != ecs->stop_func_start
> 	  && execution_direction == EXEC_REVERSE)
> 	end_stepping_range (ecs);

As an aside, if we keep this approach, it would be cleaner
to have the whole logic under a single "if EXEC_REVERSE".
(There is no point in choosing a value for the "range_start"
variable in EXEC_FORWARD if it is never used.)


Bye,
Ulrich