From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-f42.google.com (mail-wm1-f42.google.com [209.85.128.42]) by sourceware.org (Postfix) with ESMTPS id E5AC1384E78B for ; Mon, 12 Dec 2022 20:15:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org E5AC1384E78B Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wm1-f42.google.com with SMTP id v124-20020a1cac82000000b003cf7a4ea2caso6200205wme.5 for ; Mon, 12 Dec 2022 12:15:46 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:content-language:in-reply-to:mime-version :user-agent:date:message-id:references:to:subject:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=vSNzRl1Qt4UqpKxlSRLj1HKHMd4vQe4o4akxQ2S50/g=; b=H52df5s33OfcHWNkWyygNl9oXpAB4fEqmZGNN/snFf+7ViMTxwu8Ep6DCiWRuZpxvl XXFOgHMYwNNIMAaPjMLW60FY+DIrigHPmfYtr3vtIefVnyp1V+Zyzxa0bKMUsPBJIu57 aYcgX4v6fj9XglWLSkQDjofBN93SJaYMdRldEyk5CcPF7c/4hL+QfTL+sP5rK+N+b47w wlSt93vlQrSzfp79RuavLsmmmh8n9xOWFO31Hwzya8YK0uqoZxAcqd1SoC+spZeTv44O 0zSokgca6Pp15i3Cr9e4NEsjlA54YZ/jbgEKSEmvyQ0gsUf7WKN4wo+KHWNhuKvIRW8e 4Ysw== X-Gm-Message-State: ANoB5pklX21OzctfGyHVWhmiRmxMBnhkhZFGHPe4CRYEag4NwgUw3AC+ oF5lrEo7Dl9/qrSUwPFkwAC5ztdoK3cUSQ== X-Google-Smtp-Source: AA0mqf6SLeFoJJvaLGrbU95+sSIScZ9s2svf99MD6A85P9wAv0wpQQJIc9Nc2S+Pl0wsz8AoS9iL0g== X-Received: by 2002:a05:600c:1d02:b0:3cf:6d9a:7b1f with SMTP id l2-20020a05600c1d0200b003cf6d9a7b1fmr13894665wms.32.1670876145731; Mon, 12 Dec 2022 12:15:45 -0800 (PST) Received: from ?IPv6:2001:8a0:f912:6700:afd9:8b6d:223f:6170? ([2001:8a0:f912:6700:afd9:8b6d:223f:6170]) by smtp.gmail.com with ESMTPSA id j7-20020a05600c190700b003b4cba4ef71sm11689255wmq.41.2022.12.12.12.15.44 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 12 Dec 2022 12:15:45 -0800 (PST) From: Pedro Alves Subject: Re: [PATCH v2 07/29] Thread options & clone events (core + remote) To: Simon Marchi , gdb-patches@sourceware.org References: <20220713222433.374898-1-pedro@palves.net> <20220713222433.374898-8-pedro@palves.net> <5d68cd36-e8d6-e8ad-5428-863e79742062@simark.ca> Message-ID: Date: Mon, 12 Dec 2022 20:15:50 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: <5d68cd36-e8d6-e8ad-5428-863e79742062@simark.ca> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.5 required=5.0 tests=BAYES_00,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,GIT_PATCH_0,HEADER_FROM_DIFFERENT_DOMAINS,KAM_DMARC_STATUS,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,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 List-Id: On 2022-07-21 4:14 a.m., Simon Marchi wrote: > > > 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. Done, in all affected patches, including commit logs / subjects. > >> 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. If child threads inherit parent options, and then we apply that strategy, then we have no way to clear the options of all threads, even those GDB doesn't know about yet. If threads don't inherit options, then their options start cleared, and "QThreadOptions;0" thus results in all threads with options cleared, whether we apply that strategy or not. > >> @@ -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). True. Tweaked. > >> + 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. Done. > > 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". Done, in a separate patch. > >> 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? I got rid of the boolean 'supports/doesn't support' distinction instead, it's not really useful. There's no good use for gdbserver supporting the packet but then supporting no option. Instead, we'll have this: + /* Returns the set of supported thread options. */ + virtual gdb_thread_options supported_thread_options (); and the target can just return 0 to indicate no support. The callers end up changing like this: diff --git c/gdbserver/server.cc w/gdbserver/server.cc index 307290ed7de..871cfccd74e 100644 --- c/gdbserver/server.cc +++ w/gdbserver/server.cc @@ -908,11 +908,11 @@ handle_general_set (char *own_buf) { const char *p = own_buf + strlen ("QThreadOptions"); - gdb_thread_options supported_options; - if (!target_supports_set_thread_options (&supported_options)) + gdb_thread_options supported_options = target_supported_thread_options (); + if (supported_options == 0) { - /* Something went wrong -- we don't support options, but GDB - sent the packet anyway. */ + /* Something went wrong -- we don't support any option, but + GDB sent the packet anyway. */ write_enn (own_buf); return; } @@ -2611,8 +2611,8 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p) strcat (own_buf, ";vContSupported+"); - gdb_thread_options supported_options; - if (target_supports_set_thread_options (&supported_options)) + gdb_thread_options supported_options = target_supported_thread_options (); + if (supported_options != 0) { char *end_buf = own_buf + strlen (own_buf); sprintf (end_buf, ";QThreadOptions=%s",