This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 4/7] range stepping: gdb
- From: Pedro Alves <palves at redhat dot com>
- To: Yao Qi <yao at codesourcery dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Tue, 14 May 2013 19:31:31 +0100
- Subject: Re: [PATCH 4/7] range stepping: gdb
- References: <1363006291-13334-1-git-send-email-yao at codesourcery dot com> <1363006291-13334-5-git-send-email-yao at codesourcery dot com>
On 03/11/2013 12:51 PM, Yao Qi wrote:
> @@ -4689,7 +4694,36 @@ append_resumption (char *p, char *endp,
> if (step && siggnal != GDB_SIGNAL_0)
> p += xsnprintf (p, endp - p, ";S%02x", siggnal);
> else if (step)
> - p += xsnprintf (p, endp - p, ";s");
> + {
> + struct thread_info *tp = NULL;
> + CORE_ADDR pc;
> +
> + gdb_assert (!ptid_equal (ptid, minus_one_ptid));
I think you'll trip on this in case the target supports
vCont but not actually listing thread ids (magic_null_ptid).
> +
> + if (ptid_is_pid (ptid))
> + tp = find_thread_ptid (inferior_ptid);
> + else
> + tp = find_thread_ptid (ptid);
> + gdb_assert (tp != NULL);
> +
> + pc = regcache_read_pc (get_thread_regcache (ptid));
> + if (rs->support_vCont.r /* Target supports step range. */
> + /* Can't do range stepping for all threads of a process
> + 'pPID.-1'. */
> + && !(remote_multi_process_p (rs) && ptid_is_pid (ptid))
> + /* Not step over a breakpoint. */
> + && !tp->control.trap_expected
> + /* We have a range to single step. */
> + && THREAD_WITHIN_SINGLE_STEP_RANGE (tp, pc))
This is problematic. It's better to _not_ have target itself decide
when to range step or to single step, and peeking at "infrun-owned"
variables. For example, with software watchpoints, GDB needs to have control
of single-steps, in order to evaluate the watchpoints at after each
instruction is executed, so trap_expected isn't enough. (My v3 adds a test for
that, that v2 fails.)
I dislike the design of using PC checks here too :-/. That
seems fragile, and potentially inefficient (considering GDB ever
sending more than one range action per packet, that might end up
fetching registers for threads unnecessarily). IMO, it's better to have
the core run control (infrun.c/infcmd.c) decide when to range step or
single-step.
> + {
> + p += xsnprintf (p, endp - p, ";r");
> + p += hexnumstr (p, tp->control.step_range_start);
> + p += xsnprintf (p, endp - p, ",");
> + p += hexnumstr (p, tp->control.step_range_end);
> + }
> + else
> + p += xsnprintf (p, endp - p, ";s");
> + }
> else if (siggnal != GDB_SIGNAL_0)
> p += xsnprintf (p, endp - p, ";C%02x", siggnal);
> else
>
Thanks,
--
Pedro Alves