From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 53B823857BBB for ; Thu, 21 Jul 2022 03:14:37 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 53B823857BBB Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 06C481E13B; Wed, 20 Jul 2022 23:14:36 -0400 (EDT) Message-ID: <5d68cd36-e8d6-e8ad-5428-863e79742062@simark.ca> Date: Wed, 20 Jul 2022 23:14:36 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH v2 07/29] Thread options & clone events (core + remote) Content-Language: en-US To: Pedro Alves , gdb-patches@sourceware.org References: <20220713222433.374898-1-pedro@palves.net> <20220713222433.374898-8-pedro@palves.net> From: Simon Marchi In-Reply-To: <20220713222433.374898-8-pedro@palves.net> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 21 Jul 2022 03:14:39 -0000 On 2022-07-13 18:24, Pedro Alves wrote: > A previous patch taught GDB about a new TARGET_WAITKIND_THREAD_CLONED > event kind, and made the Linux target report clone events. > > A following patch will teach Linux GDBserver to do the same thing. > > However, for remote debugging, it wouldn't be ideal for GDBserver to > report every clone event to GDB, when GDB only cares about such events > in some specific situations. Reporting clone events all the time > would be potentially chatty. We don't enable thread create/exit > events all the time for the same reason. Instead we have the > QThreadEvents packet. QThreadEvents is target-wide, though. > > This patch makes GDB instead explicitly request that the target > reports clone events or not, on a per-thread basis. > > In order to be able to do that with GDBserver, we need a new remote > protocol feature. Since a following patch will want to enable thread > exit events on per-thread basis too, the packet introduced here is > more generic than just for clone events. It lets you enable/disable a > set of options at once, modelled on Linux ptrace's PTRACE_SETOPTIONS. > > IOW, this commit introduces a new QThreadOptions packet, that lets you > specify a set of per-thread event options you want to enable. The > packet accepts a list of options/thread-id pairs, similarly to vCont, > processed left to right, with the options field being a number > interpreted as a bit mask of options. The only option defined in this > commit is GDB_TO_CLONE (0x1), which ask the remote target to report It took me a while to understand that "TO" means "Thread Options". Since this ends up in the documentation, I think it wouldn't hurt to be clear and use GDB_THREAD_OPTION_CLONE. > clone events. Another patch later in the series will introduce > another option. > > For example, this packet sets option "1" (clone events) on thread > p1000.2345: > > QThreadOptions;1:p1000.2345 > > and this clears options for all threads of process 1000, and then sets > option "1" (clone events) on thread p1000.2345: > > QThreadOptions;0:p1000.-1;1:p1000.2345 > > This clears options of all threads of all processes: > > QThreadOptions;0 > > The target reports the set of supported options by including > "QThreadOptions=" in its qSupported response. > > infrun is then tweaked to enable GDB_TO_CLONE when stepping over a > breakpoint. > > Unlike PTRACE_SETOPTIONS, fork/vfork/clone children do NOT inherit > their parent's thread options. This is so that GDB can send e.g., > "QThreadOptions;0;1:TID" without worrying about threads it doesn't > know about yet. Just wondering (the behavior you chose is fine with me), couldn't we use the same strategy as we use for resumption, where each layer is responsible of "hiding" processes and threads it has not yet reported? This would mean that even is a wildcard selector is used, GDBserver would not apply the options to an unreported child thread. > @@ -892,6 +904,114 @@ handle_general_set (char *own_buf) > return; > } > > + if (startswith (own_buf, "QThreadOptions;")) > + { > + const char *p = own_buf + strlen ("QThreadOptions"); > + > + gdb_thread_options supported_options; > + if (!target_supports_set_thread_options (&supported_options)) > + { > + /* Something went wrong -- we don't support options, but GDB > + sent the packet anyway. */ > + write_enn (own_buf); > + return; > + } > + > + /* We could store the options directly in thread->thread_options > + without this map, but that would mean that a QThreadOptions > + packet with a wildcard like "QThreadOptions;0;3:TID" would > + result in the debug logs showing: > + > + [options for TID are now 0x0] > + [options for TID are now 0x3] > + > + It's nicer if we only print the final options for each TID, > + and if we only print about it if the options changed compared > + to the options that were previously set on the thread. */ > + std::unordered_map set_options; > + > + while (*p != '\0') > + { > + if (p[0] != ';') > + { > + write_enn (own_buf); > + return; > + } > + p++; > + > + /* Read the options. */ > + > + gdb_thread_options options = parse_gdb_thread_options (&p); > + > + if ((options & ~supported_options) != 0) > + { > + /* GDB asked for an unknown or unsupported option, so > + error out. */ > + std::string err > + = string_printf ("E.Unknown option requested: %s\n", > + hex_string (options)); > + strcpy (own_buf, err.c_str ()); > + return; > + } > + > + ptid_t ptid; > + > + if (p[0] == ';' || p[0] == '\0') > + ptid = minus_one_ptid; > + else if (p[0] == ':') > + { > + const char *q; > + > + ptid = read_ptid (p + 1, &q); > + > + if (p == q) > + { > + write_enn (own_buf); > + return; > + } > + p = q; > + if (p[0] != ';' && p[0] != '\0') > + { > + write_enn (own_buf); > + return; > + } > + } > + else > + { > + write_enn (own_buf); > + return; > + } > + > + /* Convert PID.-1 => PID.0 for ptid.matches. */ > + if (ptid != minus_one_ptid && ptid.lwp () == -1) The "ptid != minus_one_ptid" part seems unnecessary. If the second part of the condition is going to match, then for sure ptid is not going to be equal to (-1,0,0). > + ptid = ptid_t (ptid.pid ()); > + > + for_each_thread ([&] (thread_info *thread) > + { > + if (ptid_of (thread).matches (ptid)) > + set_options[thread] = options; > + }); > + } > + > + for (const auto &iter : set_options) > + { > + thread_info *thread = iter.first; > + gdb_thread_options options = iter.second; > + > + if (debug_threads && thread->thread_options != options) > + { > + debug_printf ("[options for %s are now 0x%x]\n", > + target_pid_to_str (ptid_of (thread)).c_str (), > + (unsigned) options); This should probably use threads_debug_printf. IWBN to have a small function that formats `options` as an "or" of the enabled bits, it would make logs easier to read. For example if the two options are enabled, it would show up as "GDB_TO_CLONE | GDB_TO_EXIT". > diff --git a/gdbserver/target.h b/gdbserver/target.h > index 6c536a30778..33142363a02 100644 > --- a/gdbserver/target.h > +++ b/gdbserver/target.h > @@ -277,6 +277,12 @@ class process_stratum_target > /* Returns true if vfork events are supported. */ > virtual bool supports_vfork_events (); > > + /* Returns true if the target supports setting thread options. If > + options are supported, write into SUPPORTED_OPTIONS the set of > + supported options. */ > + virtual bool supports_set_thread_options > + (gdb_thread_options *supported_options); I know I'm the one who used to dislike non-const references, but I have accepted it now. Therefore, this should probably be a reference, if we don't intend to be able to pass nullptr. Or, have the target return an optional? Simon