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 038B238582A1 for ; Wed, 7 Feb 2024 17:10:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 038B238582A1 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 038B238582A1 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=158.69.221.121 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1707325826; cv=none; b=QUc7OdzUDX2LzqCSgz2eFtnezY7AcjWRIli9VwpNjJrGB/hRcAF2Z4MzKCipmSwoDvYpO3pqGD+azQi2Zl0bpO1Z08ddlYsZz+xd6vBAG6LGZFZhm6bwnGnXmyQQXshBD/iaFNAiHzC7oHME97BnZvtpApmWwEAx7yHCERE2P5I= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1707325826; c=relaxed/simple; bh=1cV1lbRdzb9WPnwsm+uONy3RT+/zda8h0y8zsXdbUUA=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=SQA87uSfMvjeQ0uO3Yw2I94enfLcwwVNHBYThu6iiHPT2KPXg2pXgeK1syFGhRyzTfTdRosS21+awoRCsH/UvTOjLTNDoclEJH5bXZ11Ib/rFxEkqCnugF/TUG+qhJUGYML9UCyEWrHQZjjqEHwHn8BNPwHdPXDQ0Muc+EmfhS4= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1707325823; bh=1cV1lbRdzb9WPnwsm+uONy3RT+/zda8h0y8zsXdbUUA=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=wlir6CkSjA+daH2oZQq33QdJmVqbI0eYL9qGIbsqon0LkBy7go82+PNoUJrMmxOKD 7r/RR9XmtTgRASI6RMAy/gWrBBOZ8P4p0we22x79nTWgyubshTggUyDwigbpQ09Kog zhG2kS4nPlVBxpuSD5CfYCnoxEWO0n+E8vuAWUNM= Received: from [172.16.0.192] (192-222-143-198.qc.cable.ebox.net [192.222.143.198]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 3165E1E098; Wed, 7 Feb 2024 12:10:23 -0500 (EST) Message-ID: Date: Wed, 7 Feb 2024 12:10:22 -0500 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [FYI/pushed v4 08/25] Thread options & clone events (Linux GDBserver) Content-Language: fr To: Tom Tromey , Luis Machado Cc: Pedro Alves , gdb-patches@sourceware.org, Andrew Burgess References: <20231113150427.477431-1-pedro@palves.net> <20231113150427.477431-9-pedro@palves.net> <87eddpej3r.fsf@tromey.com> <2e61811c-ff0f-44d0-bf6f-e98a9de87c33@arm.com> <87mssccmb3.fsf@tromey.com> From: Simon Marchi In-Reply-To: <87mssccmb3.fsf@tromey.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,SPF_HELO_PASS,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 2/7/24 10:43, Tom Tromey wrote: >>>>>> "Luis" == Luis Machado writes: > > Luis> But find_process_pid returns nullptr. I wonder if it is one of those cases > Luis> where we have to deal with the tid rather than the pid. > > Luis> Does this look like the same case you were chasing? > > Yes. The issue is that the new inferior isn't created until after the > new thread -- but the order can't really be reversed in the caller. > > I've appended the patch. I put off sending it because for internal > reasons it hasn't been through the AdaCore automated testing yet. > However, I did test it (using the AdaCore test suite -- not gdb's) > myself. > > Let me know what you think. > > Tom > > commit 5464152cb1145bc1df108eb6904a642d8bc73b8c > Author: Tom Tromey > Date: Mon Feb 5 13:18:51 2024 -0700 > > Fix crash in aarch64-linux gdbserver > > We noticed that aarch64-linux gdbserver will crash when the inferior > vforks. This happens in aarch64_get_debug_reg_state: > > struct process_info *proc = find_process_pid (pid); > > return &proc->priv->arch_private->debug_reg_state; > > Here, find_process_pid returns nullptr -- the new inferior hasn't yet > been created in linux_process_target::handle_extended_wait. > > This patch fixes the problem by having aarch64_get_debug_reg_state > return nullptr in this case, and then updating > aarch64_linux_new_thread to check for this. > > diff --git a/gdb/nat/aarch64-linux.c b/gdb/nat/aarch64-linux.c > index 5ebbc9b81f8..894de8aa3eb 100644 > --- a/gdb/nat/aarch64-linux.c > +++ b/gdb/nat/aarch64-linux.c > @@ -81,9 +81,9 @@ aarch64_linux_new_thread (struct lwp_info *lwp) > /* If there are hardware breakpoints/watchpoints in the process then mark that > all the hardware breakpoint/watchpoint register pairs for this thread need > to be initialized (with data from aarch_process_info.debug_reg_state). */ > - if (aarch64_any_set_debug_regs_state (state, false)) > + if (state == nullptr || aarch64_any_set_debug_regs_state (state, false)) > DR_MARK_ALL_CHANGED (info->dr_changed_bp, aarch64_num_bp_regs); > - if (aarch64_any_set_debug_regs_state (state, true)) > + if (state == nullptr || aarch64_any_set_debug_regs_state (state, true)) > DR_MARK_ALL_CHANGED (info->dr_changed_wp, aarch64_num_wp_regs); I don't really understand all of this, but I'm wondering if the condition should be: if (state != nullptr && aarch64_any_set_debug_regs_state (state, ...)) If we have no existing aarch64_debug_reg_state, do we really need to mark the breakpoints as needing to be updated? > lwp_set_arch_private_info (lwp, info); > diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc > index 28d75d035dc..2a4f01a54da 100644 > --- a/gdbserver/linux-aarch64-low.cc > +++ b/gdbserver/linux-aarch64-low.cc > @@ -403,7 +403,8 @@ struct aarch64_debug_reg_state * > aarch64_get_debug_reg_state (pid_t pid) > { > struct process_info *proc = find_process_pid (pid); > - > + if (proc == nullptr) > + return nullptr; > return &proc->priv->arch_private->debug_reg_state; > } I was wondering if the GDB version of this function needed to get updated too. It works differently: /* See aarch64-nat.h. */ struct aarch64_debug_reg_state * aarch64_get_debug_reg_state (pid_t pid) { return &aarch64_debug_process_state[pid]; } Here, aarch64_debug_process_state is an unordered_map, meaning that if pid isn't currently in the map, a default aarch64_debug_reg_state will be constructed (is it going to be initialized properly?). So we end up with two different semantics for the two versions of the function, which might become a source of confusion later. Simon