From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 96491 invoked by alias); 16 Sep 2015 07:59:39 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 96478 invoked by uid 89); 16 Sep 2015 07:59:38 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.5 required=5.0 tests=AWL,BAYES_00,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mga09.intel.com Received: from mga09.intel.com (HELO mga09.intel.com) (134.134.136.24) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 16 Sep 2015 07:59:36 +0000 Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga102.jf.intel.com with ESMTP; 16 Sep 2015 00:59:25 -0700 X-ExtLoop1: 1 Received: from irsmsx105.ger.corp.intel.com ([163.33.3.28]) by orsmga002.jf.intel.com with ESMTP; 16 Sep 2015 00:59:24 -0700 Received: from irsmsx104.ger.corp.intel.com ([169.254.5.201]) by irsmsx105.ger.corp.intel.com ([169.254.7.51]) with mapi id 14.03.0224.002; Wed, 16 Sep 2015 08:59:23 +0100 From: "Metzger, Markus T" To: Jan Kratochvil CC: Pedro Alves , "gdb-patches@sourceware.org" Subject: RE: [PATCH 17/17] infrun: scheduler-locking reverse Date: Wed, 16 Sep 2015 07:59:00 -0000 Message-ID: References: <1441794909-32718-1-git-send-email-markus.t.metzger@intel.com> <1441794909-32718-18-git-send-email-markus.t.metzger@intel.com> <55F03A12.80307@redhat.com> <20150912194344.GA7575@host1.jankratochvil.net> <20150915171931.GA2056@host1.jankratochvil.net> In-Reply-To: <20150915171931.GA2056@host1.jankratochvil.net> Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2015-09/txt/msg00363.txt.bz2 > -----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 =3D schedlock_off; > > > +static const char *scheduler_mode =3D 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 =3D schedlock_off; > > > +static const char *scheduler_mode =3D schedlock_step; > > > > I don't believe that we need to preserve the current behaviour. If Fed= ora > > defaults to schedlock_step, why not also for record targets? >=20 > Yes, I would find logical that during "reverse-step" on Fedora the schedu= ler > 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 =3D 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 Pedr= o'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 doe= sn'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. >=20 > 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 behaviou= r. 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 r= ight 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 wel= l? >=20 > But how to make the schedlock_step default while still benefiting from yo= ur > 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 cur= rent behaviour a new scheduler-locking mode. > If I/we make schedlock_step the default then this code of your patch gets > disabled: >=20 > + else if ((scheduler_mode =3D=3D schedlock_reverse) > + && ((execution_direction =3D=3D EXEC_REVERSE) > + || target_record_is_replaying (minus_one_ptid))) > + { > + /* User-settable 'scheduler' mode requires solo thread resume duri= ng > + reverse/replay stepping. */ > + resume_ptid =3D inferior_ptid; > + } Yes. But the code above this in infrun.c then applies: else if ((scheduler_mode =3D=3D schedlock_on) || (scheduler_mode =3D=3D schedlock_step && step)) { /* User-settable 'scheduler' mode requires solo thread resume. */ resume_ptid =3D 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