From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-f54.google.com (mail-wm1-f54.google.com [209.85.128.54]) by sourceware.org (Postfix) with ESMTPS id 601B73856962 for ; Mon, 13 Nov 2023 14:15:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 601B73856962 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 601B73856962 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=209.85.128.54 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699884925; cv=none; b=bSEAfxQL1HUvR8blGpLxetoEbgWBvrJJIVDKGqJcEM5QSOAJ3ucOIvne7q8l8hOJ4Gm8T0jOOvQykEODGTy58Kh+SEnxAqGEsV6Zm+xpdyQU+2jZBdzwlEK6L+7iXjAOTaS6kepG74aoO1CnLWGxQffk9VEzvGhRAKxty1xvAk0= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699884925; c=relaxed/simple; bh=6HHWKQCsoQuKgHwGQAnEcMJfDapl5v8Sn3/ZSkpoLjM=; h=Message-ID:Date:MIME-Version:Subject:To:From; b=H3RHdKuCpzXLbq/h50B0I3Vjz4UQuIFlYBD1vO+TB15jFz5/qI2pj6OkSzKyk6tsLDnJba8sLgIDcXKH4UHXlO2r9tXYEnHUMdXsHyzogLuI1Y4m4QO4eM/GWXA5kJZmTfcpgYs7rEhklTBEZeyDuXjoN4x5eVXTv2hleMDMDt0= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-wm1-f54.google.com with SMTP id 5b1f17b1804b1-407da05f05aso33300025e9.3 for ; Mon, 13 Nov 2023 06:15:23 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1699884922; x=1700489722; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=vldfFcpuydApLNF+H6WiBdjyVFWu/qHegID9G1rg2kI=; b=rbwCCiloZqURoA/gfvGl1hV4vy1Mba/v4QumrCXtxMaYkpQ700RRkRzt4oRFHnIAPU yHroNQBt9GOwUg2zLiEZQ5XQgmb7cbBZI8lqw6LbeS066ChICIOR+ZysGQMqck0U1zWS WXrlVGOYQbQ4C4IKW3pwmI7gSEQ5HujBU96fsvSk0Kr23In8+y09IEI6XwV542zK6iK3 5TC3qwAzo8v3JK72nJaM8OrniywT3y0HuenZexUZ48HHc4ejnaR+Wh08hjD43QhqHeaI kYoaOkwH1XtF96blfeP/sVz80g24xzOcS3kGGIookSW61GSAAlYWoVCIZ/OiSz1eLvtu Wmnw== X-Gm-Message-State: AOJu0Yw7oFZLk4JuaD2/3wZ2IO2WfqfMuOdIcgfFcsQql1sreI3n4gFl qgUZISamfqFlb2KPG4tycUc= X-Google-Smtp-Source: AGHT+IEmVWF/x8f/3ZPr18ToEFFQ+CkHaR2wTk0kvTnCixoQIGPzNZQTYF1ddC+9oCrlyyKxlaRuhA== X-Received: by 2002:a1c:6a18:0:b0:408:4cf7:4f91 with SMTP id f24-20020a1c6a18000000b004084cf74f91mr5467965wmc.16.1699884921865; Mon, 13 Nov 2023 06:15:21 -0800 (PST) Received: from ?IPV6:2001:8a0:f91e:1a00:8060:1e54:fb28:9635? ([2001:8a0:f91e:1a00:8060:1e54:fb28:9635]) by smtp.gmail.com with ESMTPSA id he9-20020a05600c540900b0040a48430837sm9830701wmb.13.2023.11.13.06.15.20 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 13 Nov 2023 06:15:21 -0800 (PST) Message-ID: Date: Mon, 13 Nov 2023 14:15:20 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 28/31] Document remote clone events, and QThreadOptions packet Content-Language: en-US To: Andrew Burgess , gdb-patches@sourceware.org Cc: Eli Zaretskii References: <20221212203101.1034916-1-pedro@palves.net> <20221212203101.1034916-29-pedro@palves.net> <87a5x4yjds.fsf@redhat.com> From: Pedro Alves In-Reply-To: <87a5x4yjds.fsf@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-9.7 required=5.0 tests=BAYES_00,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,GIT_PATCH_0,HEADER_FROM_DIFFERENT_DOMAINS,KAM_DMARC_STATUS,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE 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 2023-06-12 13:06, Andrew Burgess via Gdb-patches wrote: > Pedro Alves writes: > >> This commit documents in both manual and NEWS: >> >> - the new remote clone event stop reply, >> - the new QThreadOptions packet and its current defined options, >> - the associated "set/show remote thread-events-packet" command, >> - and the associated QThreadOptions qSupported feature. >> >> Approved-By: Eli Zaretskii >> Change-Id: Ic1c8de1fefba95729bbd242969284265de42427e >> --- >> gdb/NEWS | 20 +++++++ >> gdb/doc/gdb.texinfo | 127 ++++++++++++++++++++++++++++++++++++++++++-- >> 2 files changed, 144 insertions(+), 3 deletions(-) >> >> diff --git a/gdb/NEWS b/gdb/NEWS >> index 0aa153b83da..b1d3dd7e7d9 100644 >> --- a/gdb/NEWS >> +++ b/gdb/NEWS >> @@ -160,6 +160,10 @@ set style tui-current-position [on|off] >> Whether to style the source and assembly code highlighted by the >> TUI's current position indicator. The default is off. >> >> +set remote thread-options-packet >> +show remote thread-options-packet >> + Set/show the use of the thread options packet. >> + >> * Changed commands >> >> document user-defined >> @@ -285,6 +289,22 @@ GNU/Linux/CSKY (gdbserver) csky*-*linux* >> >> GDB now supports floating-point on LoongArch GNU/Linux. >> >> +* New remote packets >> + >> +New stop reason: clone >> + Indicates that a clone system call was executed. >> + >> +QThreadOptions >> + Enable/disable optional event reporting, on a per-thread basis. >> + Currently supported options are GDB_THREAD_OPTION_CLONE, to enable >> + clone event reporting, and GDB_THREAD_OPTION_EXIT to enable thread >> + exit event reporting. >> + >> +QThreadOptions in qSupported >> + The qSupported packet allows GDB to inform the stub it supports the >> + QThreadOptions packet, and the qSupported response can contain the >> + set of thread options the remote stub supports. >> + >> *** Changes in GDB 12 >> >> * DBX mode is deprecated, and will be removed in GDB 13 >> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo >> index 5e75f32e0cd..ef62fac366f 100644 >> --- a/gdb/doc/gdb.texinfo >> +++ b/gdb/doc/gdb.texinfo >> @@ -24079,6 +24079,10 @@ are: >> @tab @code{QThreadEvents} >> @tab Tracking thread lifetime. >> >> +@item @code{thread-options} >> +@tab @code{QThreadOptions} >> +@tab Set thread event reporting options. >> + >> @item @code{no-resumed-stop-reply} >> @tab @code{no resumed thread left stop reply} >> @tab Tracking thread lifetime. >> @@ -42110,6 +42114,17 @@ appropriate @samp{qSupported} feature (@pxref{qSupported}). The >> remote stub must also supply the appropriate @samp{qSupported} feature >> indicating support. >> >> +@cindex thread clone events, remote reply >> +@anchor{thread clone event} >> +@item clone >> +The packet indicates that @code{clone} was called, and @var{r} is the >> +thread ID of the new child thread, as specified in @ref{thread-id >> +syntax}. This packet is only applicable to targets that support clone >> +events. >> + >> +This packet should not be sent by default; @value{GDBN} requests it >> +with the @ref{QThreadOptions} packet. >> + >> @cindex thread create event, remote reply >> @anchor{thread create event} >> @item create >> @@ -42148,9 +42163,10 @@ hex strings. >> @item w @var{AA} ; @var{tid} >> >> The thread exited, and @var{AA} is the exit status. This response >> -should not be sent by default; @value{GDBN} requests it with the >> -@ref{QThreadEvents} packet. See also @ref{thread create event} above. >> -@var{AA} is formatted as a big-endian hex string. >> +should not be sent by default; @value{GDBN} requests it with either >> +the @ref{QThreadEvents} or @ref{QThreadOptions} packets. See also >> +@ref{thread create event} above. @var{AA} is formatted as a >> +big-endian hex string. >> >> @item N >> There are no resumed threads left in the target. In other words, even >> @@ -42875,6 +42891,11 @@ same thread. @value{GDBN} does not enable this feature unless the >> stub reports that it supports it by including @samp{QThreadEvents+} in >> its @samp{qSupported} reply. >> >> +This packet always enables/disables event reporting for all threads of >> +all processes under control of the remote stub. For per-thread >> +control of optional event reporting, see the @ref{QThreadOptions} >> +packet. >> + >> Reply: >> @table @samp >> @item OK >> @@ -42891,6 +42912,94 @@ the stub. >> Use of this packet is controlled by the @code{set remote thread-events} >> command (@pxref{Remote Configuration, set remote thread-events}). >> >> +@anchor{QThreadOptions} >> +@item QThreadOptions@r{[};@var{options}@r{[}:@var{thread-id}@r{]]}@dots{} >> +@cindex thread options, remote request >> +@cindex @samp{QThreadOptions} packet >> + >> +For each inferior thread, the last @var{options} in the list with a >> +matching @var{thread-id} are applied. Any options previously set on a >> +thread are discarded and replaced by the new options specified. >> +Threads that do not match any @var{thread-id} retain their >> +previously-set options. Thread IDs are specified using the syntax >> +described in @ref{thread-id syntax}. If multiprocess extensions >> +(@pxref{multiprocess extensions}) are supported, options can be >> +specified to apply to all threads of a process by using the >> +@samp{p@var{pid}.-1} form of @var{thread-id}. Options with no >> +@var{thread-id} apply to all threads. Specifying no options is an >> +error. >> + >> +@var{options} is the bitwise @code{OR} of the following values. All >> +values are given in hexadecimal representation. > > Hi Pedro, > > As I said in my other email, I think we should document here that 0 is a > valid value, and that this clears all the options. > > Also, I think we should say: 'given in big-endian hexadecimal > representation'. > > We are clear on the byte-ordering for other encoded values. I know we > don't currently (and are probably unlikely to ever) need more than one > byte, but I figure it doesn't hurt to future proof the spec. Note that sentence was just trying to talk about the numeric value of each option listed below, the "0x1" in "GDB_THREAD_OPTION_CLONE (0x1)". If there ever was an option like "GDB_THREAD_OPTION_FOO (0x8000)", it wouldn't make sense to talk about that number 0x8000 being represented in big or little endian, just like it doesn't make sense to say that 1234235345345 is in big or little endian. It's just a number. How integers are encoded in the protocol is a separate question, and I don't think we should make a local decision about that here, because hex integers appears in many places, like e.g.: ‘vAttach;pid’ Attach to a new process with the specified process ID pid. The process ID is a hexadecimal integer identifying the process. By "local decision" I mean, specifying how hex integers are encoded for a specific packet. That seems to me something that should be described once in a central place. I actually thought we talked about it in the overview section: https://sourceware.org/gdb/current/onlinedocs/gdb.html/Overview.html .. since we use unpack_varlen_hex pretty consistently. Looks like we don't after all. I could swear I had seen it mentioned somewhere before. We should fix that. We should say that we encode hex integers as strings with no 0x prefix. I don't know what compelled me back then to explicitly mention that the values are in hexadecimal, when it's totally obvious (they start with 0x). I do think I was missing something in this patch, and thanks for poking about this, which is to explicitly state that "options" is encoded as an hex integer. So I did this (IMO obvious) change: -@var{options} is the bitwise @code{OR} of the following values. All -values are given in hexadecimal representation. +@var{options} is an hexadecimal integer specifying the enabled thread +options, and is the bitwise @code{OR} of the following values. > >> + >> +@table @code >> +@item GDB_THREAD_OPTION_CLONE (0x1) >> +Report thread clone events (@pxref{thread clone event}). This is only >> +meaningful for targets that support clone events (e.g., GNU/Linux >> +systems). > > It was only while reading this that a question occurred to me. Is CLONE > the best name for this option? Or are we at risk of making the remote > protocol too Linux specific here? > > Would it be better if we called the option GDB_THREAD_OPTION_CREATED, or > GDB_THREAD_OPTION_START for example? > > I guess the name itself isn't really important, maybe what's important > is how we talk about the option, e.g. maybe we could say: > > Report thread creation events (@pxref{...}). This is only meaningful > for target that support creating new threads (e.g., GNU/Linux systems). > > The reason I think we should ensure we don't focus on "thread clone" but > instead "thread creation" is .... (see below) No, I don't think so. It's important that this is distinct from "thread create". We end up with two different events "cloned" and "created": /* The thread was cloned. The event's ptid corresponds to the cloned parent. The cloned child is held stopped at its entry point, and its ptid is in the event's m_child_ptid. The target must not add the cloned child to GDB's thread list until target_ops::follow_clone() is called. */ TARGET_WAITKIND_THREAD_CLONED, /* The thread was created. */ TARGET_WAITKIND_THREAD_CREATED, And likewise in the RSP: @cindex thread clone events, remote reply @anchor{thread clone event} @item clone The packet indicates that @code{clone} was called, and @var{r} is the thread ID of the new child thread, as specified in @ref{thread-id syntax}. This packet is only applicable to targets that support clone events. This packet should not be sent by default; @value{GDBN} requests it with the @ref{QThreadOptions} packet. @cindex thread create event, remote reply @anchor{thread create event} @item create The packet indicates that the thread was just created. The new thread is stopped until @value{GDBN} sets it running with a resumption packet (@pxref{vCont packet}). This packet should not be sent by default; @value{GDBN} requests it with the @ref{QThreadEvents} packet. See also the @samp{w} (@pxref{thread exit event}) remote reply below. The @var{r} part is ignored. "created" events are reported by the thread that was created. There is no way to ask a thread to enable reporting "thread created" events for itself, because, well, the thread doesn't exist yet before the event is reported. Hence, there's no corresponding GDB_THREAD_OPTION_CREATED option. Clone events, OTOH are reported by the parent thread, the one that clones itself to create the child. The option exists to ask a specific thread to report TARGET_WAITKIND_THREAD_CLONED / T05:clone events, hence the matching GDB_THREAD_OPTION_CLONE name. "clone" seems like a pretty accurate name for what happens, I don't think we need to try to come up with a different name. That it matches the Linux/ptrace event name (not a coincidence, of course) is a bonus in my view, one less thing to learn. >> + >> +@item GDB_THREAD_OPTION_EXIT (0x2) >> +Report thread exit events (@pxref{thread exit event}). >> +@end table >> + >> +@noindent >> + >> +For example, @value{GDBN} enables the @code{GDB_THREAD_OPTION_EXIT} >> +and @code{GDB_THREAD_OPTION_CLONE} options when single-stepping a >> +thread past a breakpoint, for the following reasons: >> + >> +@itemize @bullet >> +@item >> +If the single-stepped thread exits (e.g., it executes a thread exit >> +system call), enabling @code{GDB_THREAD_OPTION_EXIT} prevents >> +@value{GDBN} from waiting forever, not knowing that it should no >> +longer expect a stop for that same thread, and blocking other threads >> +from progressing. >> + >> +@item >> +If the single-stepped thread spawns a new clone child (i.e., it >> +executes a clone system call), enabling @code{GDB_THREAD_OPTION_CLONE} >> +halts the cloned thread before it executes any instructions, and thus >> +prevents the following problematic situations: >> + >> +@itemize @minus >> +@item >> +If the breakpoint is stepped-over in-line, the spawned thread would >> +incorrectly run free while the breakpoint being stepped over is not >> +inserted, and thus the cloned thread may potentially run past the >> +breakpoint without stopping for it; > > .... this situation. Even if we imagine an OS where thread creation > isn't done as a "clone", but instead spawns the thread into existence > with its own unique $pc value, this case would still be a problem, and > would still justify being notified about the new thread, right? We don't need to imagine it, it's how e.g., Windows works. Yes, we still want to be notified about new threads in such systems, but there we don't have thread-grained control of such events. All we can do is ask to be notified of all thread creations. That's what this series does, with code like: gdb_thread_options options = GDB_THREAD_OPTION_CLONE | GDB_THREAD_OPTION_EXIT; if (target_supports_set_thread_options (options)) tp->set_thread_options (options); else target_thread_events (true); Pedro Alves