From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id AA4843858D1E for ; Tue, 11 Jul 2023 18:43:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org AA4843858D1E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1689100981; 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=+iw8I1XiZXVgP0NxwpDJNor5z+8vV9ZBddxD32FWAQg=; b=Mw1AZjq1zgB3uW1wEOQVTUYG+qEC5rPNKIpdnPcLXbjY8Gw1d0QFmPFnYy8iU6LgdfmCCv SAwawZPRSXdb2U2rUdLCVpDeYpqJWoBu2hZ/lc2FinvaJpncIuKh/OyEjBQMHCM/DD8F75 OV3QUvpqjnYDPOKqMQHa0f/+SwTvOSc= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-54-9Pa6boBBOPyhS8GU5WBlTA-1; Tue, 11 Jul 2023 14:42:58 -0400 X-MC-Unique: 9Pa6boBBOPyhS8GU5WBlTA-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 64E068EBBA1; Tue, 11 Jul 2023 18:42:57 +0000 (UTC) Received: from f37-zws-nv (unknown [10.22.17.111]) by smtp.corp.redhat.com (Postfix) with ESMTPS id C56191121315; Tue, 11 Jul 2023 18:42:56 +0000 (UTC) Date: Tue, 11 Jul 2023 11:42:55 -0700 From: Kevin Buettner To: Cc: gdb-patches@sourceware.org, , luis.machado@arm.com Subject: Re: [PATCH] gdbserver: Install single-step breakpoint for a pending thread whose last_resume_kind is resume_step Message-ID: <20230711114034.5d2fe2cb@f37-zws-nv> In-Reply-To: <20230703030458.1192525-1-zhiyong.yan@windriver.com> References: <20230703030458.1192525-1-zhiyong.yan@windriver.com> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.3 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,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: Hi Zhiyong, See my comments below; there's still one formatting nit, but I also have a question about whether this is the right place to fix the bug that you're seeing. Also, I've added Luis to the CC list since he knows a lot more about the ARM architecture than I do. On Mon, 3 Jul 2023 11:04:58 +0800 wrote: > From: Zhiyong Yan > > Gdb should not assume pending threads always generate "a non-gdbserver trap event", for example "Signal 17" event could happen. Now that resume_stopped_resumed_lwps() -> may_hw_step() assumes that the break point must already exist, resume_one_thread() should ensure the software breaking point is installed although the thread is pending. > > Signed-off-by: Zhiyong Yan zhiyong.yan@windriver.com > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30387 > --- > gdbserver/linux-low.cc | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc > index e6a39202a98..7d8bbf71ddc 100644 > --- a/gdbserver/linux-low.cc > +++ b/gdbserver/linux-low.cc > @@ -4671,7 +4671,16 @@ linux_process_target::resume_one_thread (thread_info *thread, > proceed_one_lwp (thread, NULL); > } > else > - threads_debug_printf ("leaving LWP %ld stopped", lwpid_of (thread)); > + { > + threads_debug_printf ("leaving LWP %ld stopped", lwpid_of (thread)); > + if (thread->last_resume_kind == resume_step) > + { > + /* If resume_step is required by GDB, > + install single-step breakpoint. */ > + if (supports_software_single_step()) Formatting nit: you need a space between 'supports_software_single_step' and the left paren. > + install_software_single_step_breakpoints (lwp); > + } > + } > > thread->last_status.set_ignore (); > lwp->resume = NULL; With regard to the change itself... If the debugging printf is accurate, the LWP is being left in a stopped state. If that's the case, then why are software single step breakpoints being inserted? It seems to me that you'd only want to insert these breakpoints when the thread is actually about to resume. That said, I have no doubt that this change works for you, so clearly there's something going on which I do not understand. I'd like to understand the situation better before approving it. Also, once you have an explanation, please update the code comments and/or commit log message as appropriate. My personal preference is for a concise explanation to be placed in the code comments with any additional, longer winded explanations or examples being placed in the commit log message. Kevin