From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id BIJyDdMbYmooiykAWB0awg (envelope-from ) for ; Thu, 23 Jul 2026 09:49:07 -0400 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=Hdto0cHQ; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 220721E099; Thu, 23 Jul 2026 09:49:07 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-25) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-6.4 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED autolearn=ham autolearn_force=no version=4.0.1 Received: from vm01.sourceware.org (vm01.sourceware.org [38.145.34.32]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange x25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 3E8F91E099 for ; Thu, 23 Jul 2026 09:49:05 -0400 (EDT) Received: from vm01.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 8E31E4BA2E3F for ; Thu, 23 Jul 2026 13:49:04 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 8E31E4BA2E3F Authentication-Results: sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=Hdto0cHQ Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTP id E5A454BA2E12 for ; Thu, 23 Jul 2026 13:48:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org E5A454BA2E12 Authentication-Results: sourceware.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org E5A454BA2E12 Authentication-Results: sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1784814516; cv=none; b=QCp/nI+r5Sa1fPlMMwdZU0godg+qrCmdFxb/kNOyNIO9mi+6AeH6NTPNBpllsN+zH3Zg74GeH+8DkhZUHbUir8eZb+YnsSvmqa5E8RizYm8uAzDin4SD5Np8jIdd1OISq8Wed/hxddsIVAu5valTVEse+un0wuxY9vZH4CQm7Qk= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1784814516; c=relaxed/simple; bh=efLvADYO4hS73lGPK4sm0m+I69UELnihB+bob5iZzmg=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=IWNM8FGj3aToPIHvEo9iVYmvUNQZ9Ij62R8eqjhkLoREALDxmC4Fsm2VdPtzX0po5ScR1eY9pArNm3Vn86oK+AhEvfbDw8P1EQ2Ecmq0cBJEQ/SjxsS8sqgh0yP/AQmv/8UVVQ+IL+q3VZzoA707Pecb3VU9oBcAd6uLEVDjFAk= ARC-Authentication-Results: i=1; sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=Hdto0cHQ DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org E5A454BA2E12 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1784814515; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=uvinum5Jyp9qdboelwemLi7OZYevtQ+6ZgV4asn4tbw=; b=Hdto0cHQ+pJ+JY8jkQKIKccMALsPS6N2TVNKNFogvKa4bkBX+KUqtCzuvUb8z6Dp9MAX71 o3za0xWDZhNH8qhonSCUY9r/HLeD10AWPOsLu+mxES/IX5MmRB0dRbAkij7BlbjVAEbQ8D KtELzbNsa3q6/IKO0h7V563mBZ2iCuw= Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-671-m_9oYBkrPlKzhPDBYl_XFw-1; Thu, 23 Jul 2026 09:48:33 -0400 X-MC-Unique: m_9oYBkrPlKzhPDBYl_XFw-1 X-Mimecast-MFC-AGG-ID: m_9oYBkrPlKzhPDBYl_XFw_1784814512 Received: by mail-wm1-f72.google.com with SMTP id 5b1f17b1804b1-4954a93c565so3122455e9.2 for ; Thu, 23 Jul 2026 06:48:33 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1784814512; x=1785419312; h=content-transfer-encoding:content-type:mime-version:message-id:date :references:in-reply-to:subject:cc:to:from:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to :content-type; bh=Islevk6nQaPTw5mHuDBm6IF59v+55B5BHChkTzfCRI4=; b=oXFaQpt8TZnqVEvjXrXZCUKdsxUSDUzJzOlbc9ywh2+3uE1+ZOOiFdguOLJymlnDYE 2IWIVO4KGFMlP2kTDcBnBwYPDH9MlGCEuM1TNs8yVLj2fEzttmn596QSEyrMZ9QwEkBN LhplidfP9a+KDUpUEH+pGn0vUwQqcDlXvsSstNxG/5AxiAr2fSYPbH2xHjei66Yrd+DH uPneDCK1m5JowLjOr/N7Gdxc+C/t/jWddJ0sC6Zbuk+xdySW+1/JE4tCbY101/I6dEBj pR4GApvHHPb0EfNeOb30FO/t3rTGNu8pKVMajgwkRjN4jznoryJTkzJjy9/9h3XFKmC6 X6HQ== X-Forwarded-Encrypted: i=1; AHgh+RqgLS7FxDUfwuDMZhY0jDsP9bxngqFwoNECmXobxV2X/q0cqRagGkkp/3DNpw7t+wnQNUGB08t0JO+9gA==@sourceware.org X-Gm-Message-State: AOJu0YypEnS8Dmfm+a9gfqknqYgXfDYRjDOkv80ghESdGhbFQNSn+5fL iU1it5zYeDqao1en9syoiKiJPzRL82NkoWmmWjMTdUCdPF8GrKsxf5PpG2hyKUCLhAZUPalt6+M +VHuE8E/I0MRTkylySlnINKO+lXQCmmynBWuJnNRbdob6Ce1c9eSot89gluCmdBb6EFPyqrc= X-Gm-Gg: AR+sD12b5QD3+sdkO7k2Aa7mFoxCIaFxQ2+bRl/6qXLuQmg3osQFvvmQ1FzWBMRFdci euUE7mY8gxVFwuvQ5KBuJhyS0EaGak0KVBICrxRI6JWPuK1hyXaxdwJZlUMYc+14kiOneOeBCtq EyMJYjI0P5o1fH/+zwF1YvojPy4v+ol56s8qpxcjyV8uvV05Wo/Ysx3LkOztd+nlood+MTHEZEe G4tRTBLdFj34G4F7Lau34wDQnylB5X3Xf9bnkE9OyfvCphqDepMe0W+rdKu+TEscz2EuKDh12Es PS3efjCkKcUIpumIB7U0do6UCSZbcIl/uWb2Kn5F1V7H6iDJv6TeKqpK63Pp9q413QuWwAEn X-Received: by 2002:a05:600c:470a:b0:493:c8a6:b517 with SMTP id 5b1f17b1804b1-49573d28ea9mr32827695e9.38.1784814511818; Thu, 23 Jul 2026 06:48:31 -0700 (PDT) X-Received: by 2002:a05:600c:470a:b0:493:c8a6:b517 with SMTP id 5b1f17b1804b1-49573d28ea9mr32827345e9.38.1784814511232; Thu, 23 Jul 2026 06:48:31 -0700 (PDT) Received: from localhost ([31.111.209.233]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4956a6354a3sm239430395e9.10.2026.07.23.06.48.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 23 Jul 2026 06:48:30 -0700 (PDT) From: Andrew Burgess To: Klaus Gerlicher , gdb-patches@sourceware.org Cc: tom@tromey.com, guinevere@redhat.com, eliz@gnu.org Subject: Re: [PATCH v8 4/6] gdb: change the internal representation of scheduler locking. In-Reply-To: <20260722102746.131536-5-klaus.gerlicher@intel.com> References: <20260722102746.131536-1-klaus.gerlicher@intel.com> <20260722102746.131536-5-klaus.gerlicher@intel.com> Date: Thu, 23 Jul 2026 14:48:29 +0100 Message-ID: <87tsppolzm.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: YVc4kFyXf5qiNmnmu-v9yMzu9gz4l7HE6r0RNsx5tr4_1784814512 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces~public-inbox=simark.ca@sourceware.org Klaus Gerlicher writes: > From: Natalia Saiapova > > Introduce a new structure to manage different options of the scheduler > locking. The options can coexist together and be set individually. > In the next patch > > gdb: refine commands to control scheduler locking. > > we introduce the commands to control these options. In this patch we do > not introduce new commands and keep the previous API. > > New scheduler locking options are: > replay continue -- control continuing commands during replay mode. > replay step -- control stepping commands during replay mode. > continue -- control continuing commands during normal execution. > step -- control stepping commands during normal execution. > > Internally they hold a bool value, when true the locking is enabled. > > Mapping to the old settings > > Old Settings | New settings > ----------------------------------- > off | all are false > | > replay | continue =3D false, step =3D false, > | replay continue =3D true, replay step =3D true > | > step | continue =3D false, step =3D true, > | replay continue =3D false, replay step =3D true > | > on | all are true > --- > gdb/infrun.c | 147 ++++++++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 134 insertions(+), 13 deletions(-) > > diff --git a/gdb/infrun.c b/gdb/infrun.c > index ace34507cfe..0eb890bbbc1 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -107,8 +107,12 @@ static bool start_step_over (void); > =20 > static bool step_over_info_valid_p (void); > =20 > -static bool schedlock_applies (struct thread_info *tp); > -static bool schedlock_applies (bool step, bool record_will_replay); > +struct schedlock_options; > +static bool schedlock_applies (thread_info *tp); > +static bool schedlock_applies (bool step, > +=09=09=09 bool record_will_replay, > +=09=09=09 thread_info *tp =3D nullptr); > +static bool schedlock_applies_to_opts (const schedlock_options &, bool s= tep); > =20 > static void handle_process_exited (struct execution_control_state *ecs); > =20 > @@ -2373,7 +2377,69 @@ infrun_thread_ptid_changed (process_stratum_target= *target, > inferior_ptid =3D new_ptid; > } > =20 > -=0C > +/* A structure to hold scheduler locking settings for > + a mode, replay or normal. */ > +struct schedlock_options > +{ > + struct option { The opening '{' needs to be on the next line. Personally, I'm a fan of nesting classes like this, but in a couple of my recent patches I've been specifically asked not to nest classes like this, so I think the GDB style is to avoid such nesting. > + const std::string name; > + bool value; It would be better if these were made private. The NAME is easy to do, it's only used within member functions. VALUE is a little harder, but I had a play, and I added these member functions to the schedlock_options::option class: void make_cli_option (const char *name, const char *set_doc, =09=09=09const char *show_doc, const char *help_doc, =09=09=09cmd_func_ftype *set_func, =09=09=09cmd_list_element **set_list, =09=09=09cmd_list_element **show_list) { add_setshow_boolean_cmd (name, class_run, &m_value, =09=09=09 set_doc, show_doc, help_doc, =09=09=09 set_func, =09=09=09 show_schedlock_option, =09=09=09 set_list, =09=09=09 show_list); } void make_cli_option (const char *name, const char *set_doc, =09=09=09 const char *show_doc, const char *help_doc, =09=09=09 cmd_list_element **set_list, =09=09=09 cmd_list_element **show_list) { make_cli_option (name, set_doc, show_doc, help_doc, =09=09 set_schedlock_callback, set_list, show_list); } These can then be used instead of direct calls to add_setshow_boolean_cmd in the 'INIT_GDB_FILE (infrun)' function. > + > + option () =3D delete; > + option (std::string name, bool value) > + : name (std::move (name)), value (value) > + {} > + > + /* Forbid accidential copying. */ > + option (const option &) =3D delete; > + option operator=3D (const option &) =3D delete; Use: 'DISABLE_COPY_AND_ASSIGN (option);' instead. There's also a typo: s/accidential/accidental/, but I think the comment can go with the use of DISABLE_COPY_AND_ASSIGN. > + option (option &&) =3D default; > + option &operator=3D (option &&) =3D default; > + > + operator bool () const { return value; } > + const char *c_str () const { return value ? "on" : "off"; } > + /* Set new value. Return true, if the value has changed. */ > + bool set (bool new_value); > + }; > + > + schedlock_options () =3D delete; > + schedlock_options (option cont, option step) > + : cont (std::move (cont)), step (std::move (step)) > + {} > + > + /* Forbid accidential copying. */ > + schedlock_options (const schedlock_options &) =3D delete; > + schedlock_options operator=3D (const schedlock_options &) =3D delete; > + schedlock_options (schedlock_options &&) =3D default; > + schedlock_options &operator=3D (schedlock_options &&) =3D default; Same as above, use DISABLE_COPY_AND_ASSIGN and then remove the comment with typo. > + > + /* If true, the scheduler is locked during continuing. */ > + option cont; > + /* If true, the scheduler is locked during stepping. */ > + option step; > +}; > + > +bool > +schedlock_options::option::set (bool new_value) > +{ > + if (value !=3D new_value) > + { > + value =3D new_value; > + return true; > + } > + > + return false; > +} > + > +struct schedlock New types should get a header comment. > +{ > + schedlock (schedlock_options opt, schedlock_options replay_opt) > + : normal (std::move (opt)), replay (std::move (replay_opt)) > + {} > + > + schedlock_options normal; > + schedlock_options replay; > +}; > =20 > static const char schedlock_off[] =3D "off"; > static const char schedlock_on[] =3D "on"; > @@ -2386,7 +2452,42 @@ static const char *const scheduler_enums[] =3D { > schedlock_replay, > nullptr > }; > + > static const char *scheduler_mode =3D schedlock_replay; > + > +schedlock schedlock { Make this static maybe? Rather than overloading the name 'schedlock' for both the type and the variable, could we not find a better name for one or both of these? Actually, I'm not entirely convinced that the 'schedlock' type adds much value above just having two separate variables with better names, e.g.: schedlock_options schedlock_options_normal; schedlock_options schedlock_options_replay; This isn't a hard requirement though, if you are committed to the current structure then I'll not object. > + { > + {"cont", false}, > + {"step", false} > + }, > + { > + {"replay cont", true}, > + {"replay step", true} > + } > +}; > + > +/* A helper function to set scheduler locking shortcuts: > + set scheduler-locking on: all options are on. > + set scheduler-locking off: all options are off. > + set scheduler-locking replay: only replay options are on. > + set scheduler-locking step: only "step" and "replay step" are on. */ > + > +static void > +set_schedlock_shortcut_option (const char *shortcut) > +{ > + bool is_on =3D (shortcut =3D=3D schedlock_on); > + bool is_step =3D (shortcut =3D=3D schedlock_step); > + bool is_replay =3D (shortcut =3D=3D schedlock_replay); > + bool is_off =3D (shortcut =3D=3D schedlock_off); > + /* Check that we got a valid shortcut option. */ > + gdb_assert (is_on || is_step || is_replay || is_off); > + > + schedlock.normal.cont.set (is_on); > + schedlock.normal.step.set (is_on || is_step); > + schedlock.replay.cont.set (is_on || is_replay); > + schedlock.replay.step.set (is_on || is_replay || is_step); > +} > + > static void > show_scheduler_mode (struct ui_file *file, int from_tty, > =09=09 struct cmd_list_element *c, const char *value) > @@ -2403,9 +2504,13 @@ set_schedlock_func (const char *args, int from_tty= , struct cmd_list_element *c) > if (!target_can_lock_scheduler ()) > { > scheduler_mode =3D schedlock_off; > + /* Set scheduler locking off. */ > + set_schedlock_shortcut_option (schedlock_off); > error (_("Target '%s' cannot support this command."), > =09 target_shortname ()); > } > + > + set_schedlock_shortcut_option (scheduler_mode); > } > =20 > /* True if execution commands resume all threads of all processes by > @@ -2436,6 +2541,10 @@ ptid_t > user_visible_resume_ptid (int step) > { > ptid_t resume_ptid; > + thread_info *tp =3D nullptr; > + > + if (inferior_ptid !=3D null_ptid) > + tp =3D inferior_thread (); > =20 > if (non_stop) > { > @@ -2445,14 +2554,14 @@ user_visible_resume_ptid (int step) > } > else if (schedlock_applies (step, > =09=09=09 target_record_will_replay (inferior_ptid, > -=09=09=09=09=09=09=09 execution_direction))) > +=09=09=09=09=09=09=09 execution_direction), > +=09=09=09 tp)) > { > /* User-settable 'scheduler' mode requires solo thread > =09 resume. */ > resume_ptid =3D inferior_ptid; > } > - else if (inferior_ptid !=3D null_ptid > -=09 && inferior_thread ()->control.in_cond_eval) > + else if (tp !=3D nullptr && tp->control.in_cond_eval) > { > /* The inferior thread is evaluating a BP condition. Other thread= s > =09 might be stopped or running and we do not want to change their > @@ -3164,8 +3273,9 @@ clear_proceed_status (int step, bool about_to_proce= ed) > This is a convenience feature to not require the user to explicitly > stop replaying the other threads. We're assuming that the user's > intent is to resume tracing the recorded process. */ > - if (!non_stop && scheduler_mode =3D=3D schedlock_replay > - && !target_record_will_replay (inferior_ptid, execution_direction)= ) > + if (!non_stop && schedlock_applies_to_opts (schedlock.replay, step) > + && !target_record_will_replay (inferior_ptid, > +=09=09=09=09 execution_direction)) There's a change in behaviour here which isn't called out in the commit message. In fact the commit message gives the impression that this commit is a refactor and retains the existing behaviour. Previously we called `target_record_stop_replaying` only when in reply mode, but now we call it when in both replay mode and the more general 'on' mode. The comment above this `if` is definitely out of date. I had an LLM give a summary of what changed, here's what it said: Setup: Several threads have been recorded. The current thread (thread 1) has reached the end of its replay log and is no longer replaying. Threads 2 and 3 are still mid-replay. set scheduler-locking on is active. =20 Old behavior: target_record_stop_replaying() is not called because scheduler_mode !=3D schedlock_replay. Threads 2 and 3 remain in replay mode. They don't run (they're locked), but their replay state is preserved. If the user later does set scheduler-locking off and continues, threads 2 and 3 resume replaying from where they left off. New behavior: schedlock_applies_to_opts(schedlock.replay, step) is true (because schedlock_on sets replay.cont =3D true, replay.step =3D true), so target_record_stop_replaying() is called. Threads 2 and 3 are pulled out of replay mode. If the user later switches to set scheduler-locking off and continues, those threads execute live instead of replaying =E2=80=94 their replay st= ate has been silently lost. = =20 The same applies to schedlock_step when stepping: threads that were replaying get their replay state cleared even though they're locked and wouldn't have run during the step. This seems like it makes sense, but I don't claim to be an expert at the record/replay logic. Maybe this change is intentional? If it is then I think it should be called out and described in the commit message. The comment definitely needs to be updated. > target_record_stop_replaying (); > =20 > if (!non_stop && inferior_ptid !=3D null_ptid) > @@ -3241,6 +3351,17 @@ thread_still_needs_step_over (struct thread_info *= tp) > return what; > } > =20 > +/* Return true if OPTS lock the scheduler. > + STEP indicates whether a thread is about to step. > + Note, this does not take into the account the mode (replay or > + normal execution). */ Rephrase the last sentence to avoid 'Note, ' and remove an extra 'the': This function does not take into account the mode (replay or normal execution). Thanks, Andrew > + > +static bool > +schedlock_applies_to_opts (const schedlock_options &opts, bool step) > +{ > + return ((opts.cont && !step) || (opts.step && step)); > +} > + > /* Returns true if scheduler locking applies to TP. */ > =20 > static bool > @@ -3254,7 +3375,7 @@ schedlock_applies (thread_info *tp) > record_will_replay > =09=3D target_record_will_replay (tp->ptid, execution_direction); > } > - return schedlock_applies (step, record_will_replay); > + return schedlock_applies (step, record_will_replay, tp); > } > =20 > /* Returns true if scheduler locking applies. STEP indicates whether > @@ -3262,11 +3383,11 @@ schedlock_applies (thread_info *tp) > indicates whether we're about to replay. */ > =20 > static bool > -schedlock_applies (bool step, bool record_will_replay) > +schedlock_applies (bool step, bool record_will_replay, thread_info *tp) > { > - return (scheduler_mode =3D=3D schedlock_on > -=09 || (scheduler_mode =3D=3D schedlock_step && step) > -=09 || (scheduler_mode =3D=3D schedlock_replay && record_will_replay)); > + schedlock_options &opts > + =3D record_will_replay ? schedlock.replay : schedlock.normal; > + return schedlock_applies_to_opts (opts, step); > } > =20 > /* When FORCE_P is false, set process_stratum_target::COMMIT_RESUMED_STA= TE > --=20 > 2.34.1 > > Intel Deutschland GmbH > > Registered Address: Dornacher Strasse 1, 85622 Feldkirchen, Germany > Tel: +49 89 991 430, www.intel.de > Managing Directors: Harry Demas, Jeffrey Schneiderman, Yin Chong Sorrell > Chairperson of the Supervisory Board: Nicole Lau > Registered Seat: Munich > Commercial Register: Amtsgericht Muenchen HRB 186928