Re: [PATCH 17/17] infrun: scheduler-locking reverse

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

Hello Markus,

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.
If I put there:
	static const char *scheduler_mode = schedlock_reverse;
then during forward "step" there will be no locking which would be
a regression for Fedora.


> It could be seen as bug-fix that record targets did not honour scheduler-locking.

I agree.


> 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".


> > My guess is there could be a new setting "set reverse-scheduler-locking" also
> > with "off/step/on" as it seems to me "step" also makes sense for the
> > scheduler locking in reverse mode.
> 
> This would allow us to set different scheduler-locking modes for reverse/replay
> execution and for normal debugging.  While this adds flexibility, I'm not sure it is
> actually needed.

I agree.  It allows all the combinations to be set although some of the
combinations do not make much sense.


> 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?

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;
+    }

Here I feel just some misunderstanding.

The patch seems to me needing some update so that both schedlock_step and
schedlock_reverse can be the default at once.


Thanks,
Jan