This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
RE: [PATCH 17/17] infrun: scheduler-locking reverse
- From: "Metzger, Markus T" <markus dot t dot metzger at intel dot com>
- To: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- Cc: Pedro Alves <palves at redhat dot com>, "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Wed, 16 Sep 2015 07:59:23 +0000
- Subject: RE: [PATCH 17/17] infrun: scheduler-locking reverse
- Authentication-results: sourceware.org; auth=none
- References: <1441794909-32718-1-git-send-email-markus dot t dot metzger at intel dot com> <1441794909-32718-18-git-send-email-markus dot t dot metzger at intel dot com> <55F03A12 dot 80307 at redhat dot com> <20150912194344 dot GA7575 at host1 dot jankratochvil dot net> <A78C989F6D9628469189715575E55B23331AE05D at IRSMSX104 dot ger dot corp dot intel dot com> <20150915171931 dot GA2056 at host1 dot jankratochvil dot net>
> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Jan Kratochvil
> Sent: Tuesday, September 15, 2015 7:20 PM
Hello Jan,
> On Tue, 15 Sep 2015 11:28:36 +0200, Metzger, Markus T wrote:
> > > Thanks for bringing it up. I would have hard time mergin this patch's:
> > > -static const char *scheduler_mode = schedlock_off;
> > > +static const char *scheduler_mode = schedlock_reverse;
> > > with
> > > http://pkgs.fedoraproject.org/cgit/gdb.git/tree/gdb-6.6-
> scheduler_locking-
> > > step-is-default.patch
> > > -static const char *scheduler_mode = schedlock_off;
> > > +static const char *scheduler_mode = schedlock_step;
> >
> > I don't believe that we need to preserve the current behaviour. If Fedora
> > defaults to schedlock_step, why not also for record targets?
>
> Yes, I would find logical that during "reverse-step" on Fedora the scheduler
> locking is also turned on. But I do not see how to achieve it with your
> patch. If I put there:
> static const char *scheduler_mode = schedlock_step;
> then during "reverse-*" commands there will be no scheduler locking.
That's a bug.
Record btrace still has some code that implements an implicit scheduler-
locking on during reverse/replay execution for all-stop targets. With Pedro's
ASNS patch, scheduler-locking should be honoured.
We could also make record btrace honour scheduler-locking for (soon legacy) all-
stop targets. The current implementation applies the same stepping command
to all threads matching the argument PTID:
ALL_NON_EXITED_THREADS (tp)
if (ptid_match (tp->ptid, ptid))
record_btrace_resume_thread (tp, flag);
I think what we want instead is to apply the requested stepping command to
INFERIOR_PTID and the corresponding continue command to all other threads
matching PTID. Is that correct?
We wouldn't even need to check for non-stop mode or distinguish between
non-stop and all-stop targets. Infrun should take care of all that.
What if INFERIOR_PTID does not match the argument PTID? Is that an internal
error? Or would we only resume the argument PTID in this case? What if it is
a process and not a single thread?
Record full doesn't really support multi-threading so scheduler-locking doesn't
apply.
> > The same holds for mainline, of course. But the new mode is much closer
> to
> > schedlock_step than it is to schedlock_off.
>
> Maybe. But when I make the default "schedlock_reverse" then it will
> regress
> Fedora's forward "step".
Mainline will default to schedlock_reverse to maintain the current behaviour.
Fedora will continue to default to schedlock_step and record btrace will now
behave correctly on (reverse-)continue commands (the current series doesn't
since the underlying target is still all-stop). It should already do the right thing
for (reverse-)step/next/finish commands.
> > I really only added the new mode because I did not want to change the
> default
> > behaviour completely. Over time, I think we may want to deprecate it,
> even
> > though schedlock_off isn't very useful with the usually quite short
> execution
> > history. Maybe mainline will default to schedlock_step one day, as well?
>
> But how to make the schedlock_step default while still benefiting from your
> patch?
The goal is to have record btrace honour scheduler-locking. This patch is trying
to do this without changing the current default behaviour by making the current
behaviour a new scheduler-locking mode.
> If I/we make schedlock_step the default then this code of your patch gets
> disabled:
>
> + else if ((scheduler_mode == schedlock_reverse)
> + && ((execution_direction == EXEC_REVERSE)
> + || target_record_is_replaying (minus_one_ptid)))
> + {
> + /* User-settable 'scheduler' mode requires solo thread resume during
> + reverse/replay stepping. */
> + resume_ptid = inferior_ptid;
> + }
Yes. But the code above this in infrun.c then applies:
else if ((scheduler_mode == schedlock_on)
|| (scheduler_mode == schedlock_step && step))
{
/* User-settable 'scheduler' mode requires solo thread
resume. */
resume_ptid = inferior_ptid;
}
Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Prof. Dr. Hermann Eul
Chairperson of the Supervisory Board: Tiffany Doon Silva
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928