From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-f52.google.com (mail-wm1-f52.google.com [209.85.128.52]) by sourceware.org (Postfix) with ESMTPS id 445693858407 for ; Mon, 11 Jul 2022 15:20:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 445693858407 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-f52.google.com with SMTP id l68so3227802wml.3 for ; Mon, 11 Jul 2022 08:20:02 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=/f0x/NEQWVK/yk2lq4o2hV/KJrkSYAyoDM7nJwG7GSw=; b=6la5FD4s5C7MAXq6wbu1NyiMohGC2YQoO4TWSFJriqeEchV+AOUM93O/H4zr7SllIP q9lc5u4K5KmF0ElLBhlTKLDk19mf6pbqcrAD35CxZ/IYI8B48x09gG5i8kABUVfHDOI3 zCXNEnJkPD1+9zdEWcAXsNbkduEZ5QutxE4sM/ygvKmCw9i+7V42wZwFd3roSGgK4fCf Ffn61cw8I/npnkOR/WkbZIlSClf7wdBxndsj1hQs+hhMQuDJIK1xpJdwLK6nlwNzsRw7 mwxN7yqRl0cXDQwb4O7QkKEfZ/DruDnnBRfC1kmdR8xOV7GL/dHuvfR78P6kjG6PdMcn bMjQ== X-Gm-Message-State: AJIora9J0AB2HjlqkxybVBjPj1vBmDFQk4GufoMJNfudi9bpS6rl96qJ Uqv6Bw7FRAOy+Z/q2SaMUv/cn+CNSnA= X-Google-Smtp-Source: AGRyM1uRQnxvE7eeJeZ4kddE+X1JJDB8a2HzATsuzdou3AAXsm2AsMMcSOF7JCKJEluSUxtp6BVRXA== X-Received: by 2002:a05:600c:1c04:b0:3a2:c5fb:503b with SMTP id j4-20020a05600c1c0400b003a2c5fb503bmr16453397wms.126.1657552800054; Mon, 11 Jul 2022 08:20:00 -0700 (PDT) Received: from ?IPv6:2001:8a0:f924:2600:209d:85e2:409e:8726? ([2001:8a0:f924:2600:209d:85e2:409e:8726]) by smtp.gmail.com with ESMTPSA id y12-20020a5d620c000000b0021d63fe0f03sm5969183wru.12.2022.07.11.08.19.58 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 11 Jul 2022 08:19:58 -0700 (PDT) Subject: Re: [PATCH 25/25] Document remote clone events, and QThreadOptions packet To: Eli Zaretskii Cc: gdb-patches@sourceware.org References: <20220620225419.382221-1-pedro@palves.net> <20220620225419.382221-26-pedro@palves.net> <838rpqkyof.fsf@gnu.org> From: Pedro Alves Message-ID: <8ca45d20-55f3-f3b0-82c3-971d54f2e0a3@palves.net> Date: Mon, 11 Jul 2022 16:19:57 +0100 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: <838rpqkyof.fsf@gnu.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.1 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, 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 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: Mon, 11 Jul 2022 15:20:04 -0000 Hi! On 2022-06-21 1:07 p.m., Eli Zaretskii wrote: >> From: Pedro Alves >> Date: Mon, 20 Jun 2022 23:54:19 +0100 >> >> 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. > > Thanks. > >> ** GDBserver is now supported on LoongArch GNU/Linux. >> >> +* New remote packets >> + >> +clone stop reason >> + Indicates that a clone system call was executed. > > I'm confused: what is the relation between the "stop reason" part and > the description saying that "a clone system call was executed"? The > gdb.texinfo description only mentions "clone" as the packet name, > without the other 2 words. What am I missing? A "stop reason" is a remote-protocol defined term, which should be familiar to remote stub maintainers. See here: @item If @var{n} is a recognized @dfn{stop reason}, it describes a more specific event that stopped the target. The currently defined stop reasons are listed below. The @var{aa} should be @samp{05}, the trap signal. At most one stop reason should be present. So that new line in NEWS is saying that we're added a new "stop reason", called "clone". When the remote target reports it, it means that the program being debugged execute a clone system call. This is exactly how e.g. the "fork" and "vfork" stop reasons (and others) were mentioned in NEWS back when they were introduced: ~~~ fork stop reason Indicates that a fork system call was executed. vfork stop reason Indicates that a vfork system call was executed. ~~~ > >> +@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. Refer to @ref{thread-id syntax} >> +for the format of the @var{thread-id} field. This packet is only >> +applicable to targets that support clone events. > > The text refers to @var{r} and @var{thread-id}, but they are not > present on the @item line that describes the packet itself. Where are > those parameters used in this case. Hmm. @var{r} is described further up. This is all within the description of the T stop reply, which mentions: @item T @var{AA} @var{n1}:@var{r1};@var{n2}:@var{r2};@dots{} ... Each @samp{@var{n}:@var{r}} pair is interpreted as follows: ... @item If @var{n} is a recognized @dfn{stop reason}, it describes a more specific event that stopped the target. The currently defined stop reasons are listed below. ... So for the "clone" case in question, "clone" is the recognized stop reason, and in the n:r pair, r is the thread ID of the child. However, I agree that @var{thread-id} appears a bit out of the blue. Note this is the exact same wording as in the existing fork and vfork stop reasons, which I just copied. E.g.: @cindex fork events, remote reply @item fork The packet indicates that @code{fork} was called, and @var{r} is the thread ID of the new child process. Refer to @ref{thread-id syntax} for the format of the @var{thread-id} field. (...) A bit further above, we say: @item If @var{n} is @samp{thread}, then @var{r} is the @var{thread-id} of the stopped thread, as specified in @ref{thread-id syntax}. so how about we use the same wording for fork/vfork? Like so (against current master): ~~~~~~~~~~~~~~~ diff --git c/gdb/doc/gdb.texinfo w/gdb/doc/gdb.texinfo index 7a4e337d15b..79b2fccb501 100644 --- c/gdb/doc/gdb.texinfo +++ w/gdb/doc/gdb.texinfo @@ -41655,11 +41655,10 @@ apply. @cindex fork events, remote reply @item fork -The packet indicates that @code{fork} was called, and @var{r} -is the thread ID of the new child process. Refer to -@ref{thread-id syntax} for the format of the @var{thread-id} -field. This packet is only applicable to targets that support -fork events. +The packet indicates that @code{fork} was called, and @var{r} is the +@var{thread-id} of the new child process, as specified in +@ref{thread-id syntax}. This packet is only applicable to targets +that support fork events. This packet should not be sent by default; older @value{GDBN} versions did not support it. @value{GDBN} requests it, by supplying an @@ -41669,11 +41668,10 @@ indicating support. @cindex vfork events, remote reply @item vfork -The packet indicates that @code{vfork} was called, and @var{r} -is the thread ID of the new child process. Refer to -@ref{thread-id syntax} for the format of the @var{thread-id} -field. This packet is only applicable to targets that support -vfork events. +The packet indicates that @code{vfork} was called, and @var{r} is the +@var{thread-id} of the new child process, as specified in +@ref{thread-id syntax}. This packet is only applicable to targets +that support vfork events. This packet should not be sent by default; older @value{GDBN} versions did not support it. @value{GDBN} requests it, by supplying an ~~~~~~~~~~~~~~~ I'd push this as a separate patch right away, if OK. > >> +@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 rightmost options with a matching >> +@var{thread-id} are applied. > > "Rightmost" means here "the last in the list"? If so, perhaps it's > worth saying that explicitly to avoid possible confusion. I can change it, but I'm curious what the confusion could be? I used rightmost here to contrast with the vCont description, which uses "leftmost". > >> +@item >> +Without clone events (on systems where threads are spawned via a clone >> +system call), if the single-stepped thread spawns a new clone child >> +(i.e., it executes a clone system call), and: >> + >> +@itemize @minus >> +@item >> +the breakpoint is stepped-over in-line, the spawned thread incorrectly >> +runs free while the breakpoint being stepped over is not inserted, >> +thus the spawned thread may potentially run past the breakpoint >> +without stopping for it. By enabling @code{GDB_TO_CLONE}, the new >> +cloned thread is halted before it executes any instruction; >> + >> +@item >> +if alternativelly displaced (out-of-line) stepping is used, the >> +spawned thread starts running at the out-of-line PC, leading to >> +undefined behavior, usually crashes or data corruption. By enabling >> +@code{GDB_TO_CLONE}, the new cloned thread is halted before it >> +executes any instruction, and @value{GDBN} adjusts its PC before >> +resuming its execution. >> +@end itemize > > This reads awkwardly, because you say "and:", and start each @item of > the following itemized list with a lower-case letter, but each @item > includes more than one sentence. How about the following rephrasing: > > For example, @value{GDBN} enables the @code{GDB_TO_EXIT} and > @code{GDB_TO_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_TO_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_TO_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; > > @item > if displaced (out-of-line) stepping is used, the cloned thread starts > running at the out-of-line PC, leading to undefined behavior, usually > crashing or corrupting data. > @end itemize > > @end itemize > Thank you, that is perfect, I'll take it.