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 87EF13858D32 for ; Fri, 29 Sep 2023 17:59:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 87EF13858D32 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1696010383; bh=SnLfoAL5/E8m5diAfnDrqybntA87ZeaZbrupu0E9eHE=; h=Date:Subject:To:References:From:In-Reply-To:From; b=tw+DFqecvs+OKvN5Qp8t4H5zETYdrwjFzS1qUmGo4A7VIEtUeQxPWANg/3L+46LMY Cl87QG9Pn54eEj1E5j0UtpZFV2Fk9ehOnGCdjwSPC+9vO/7uop+NNaqAkjNyi+1uJT p5JKcZXi49/EfFgFMNJHkfcP0++kHiy8Fppr0QsA= Received: from [172.20.35.29] (unknown [207.253.217.242]) (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 AB9B41E092; Fri, 29 Sep 2023 13:59:43 -0400 (EDT) Message-ID: Date: Fri, 29 Sep 2023 13:59:43 -0400 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] [gdb/arm] Only allow closure lookup by address if there are threads displaced-stepping To: Luis Machado , gdb-patches@sourceware.org References: <20230929081503.4014732-1-luis.machado@arm.com> Content-Language: fr From: Simon Marchi In-Reply-To: <20230929081503.4014732-1-luis.machado@arm.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,SPF_HELO_PASS,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 9/29/23 04:15, Luis Machado via Gdb-patches wrote: > Since commit 1e5ccb9c5ff4fd8ade4a8694676f99f4abf2d679, we have an assertion in > displaced_step_buffers::copy_insn_closure_by_addr that makes sure a closure > is available whenever we have a match between the provided address argument and > the buffer address. > > That is fine, but the report in PR30872 shows this assertion triggering when > it really shouldn't. After some investigation, here's what I found out. > > The 32-bit Arm architecture is the only one that calls > gdbarch_displaced_step_copy_insn_closure_by_addr directly, and that's because > 32-bit Arm needs to figure out the thumb state of the original instruction > that we displaced-stepped through the displaced-step buffer. > > Before the assertion was put in place by commit > 1e5ccb9c5ff4fd8ade4a8694676f99f4abf2d679, there was the possibility of > getting nullptr back, which meant we were not doing a displaced-stepping > operation. > > Now, with the assertion in place, this is running into issues. > > It looks like displaced_step_buffers::copy_insn_closure_by_addr is > being used to return a couple different answers depending on the > state we're in: > > 1 - If we are actively displaced-stepping, then copy_insn_closure_by_addr > is supposed to return a valid closure for us, so we can determine the > thumb mode. > > 2 - If we are not actively displaced-stepping, then copy_insn_closure_by_addr > should return nullptr to signal that there isn't any displaced-step buffers > in use, because we don't have a valid closure (but we should always have > this). > > Since the displaced-step buffers are always allocated, but not always used, > that means the buffers will always contain data. In particular, the buffer > addr field cannot be used to determine if the buffer is active or not. > > For instance, we cannot set the buffer addr field to 0x0, as that can be a > valid PC in some cases. > > My understanding is that the current_thread field should be a good candidate > to signal that a particular displaced-step buffer is active or not. If it is > nullptr, we have no threads using that buffer to displaced-step. Otherwise, > it is an active buffer in use by a particular thread. > > The following fix modifies the displaced_step_buffers::copy_insn_closure_by_addr > function so we only attempt to return a closure if the buffer has an assigned > current_thread and if the buffer address matches the address argument. > > Alternatively, I think we could use a function to answer the question of > whether we're actively displaced-stepping (so we have an active buffer) or > not. > > I've also added a testcase that exercises the problem, restricted to 32-bit > Arm, as that is the only architecture that faces this problem. > > Regression-tested on Ubuntu 20.04. OK? > > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30872 > --- > gdb/displaced-stepping.c | 3 +- > .../gdb.arch/arm-displaced-step-closure.c | 5 +++ > .../gdb.arch/arm-displaced-step-closure.exp | 41 +++++++++++++++++++ > 3 files changed, 48 insertions(+), 1 deletion(-) > create mode 100644 gdb/testsuite/gdb.arch/arm-displaced-step-closure.c > create mode 100644 gdb/testsuite/gdb.arch/arm-displaced-step-closure.exp > > diff --git a/gdb/displaced-stepping.c b/gdb/displaced-stepping.c > index bc59ef01478..41c3c999d1e 100644 > --- a/gdb/displaced-stepping.c > +++ b/gdb/displaced-stepping.c > @@ -277,7 +277,8 @@ displaced_step_buffers::copy_insn_closure_by_addr (CORE_ADDR addr) > { > for (const displaced_step_buffer &buffer : m_buffers) > { > - if (addr == buffer.addr) > + /* Make sure we have active buffers to compare to. */ > + if (buffer.current_thread != nullptr && addr == buffer.addr) > { > /* The closure information should always be available. */ > gdb_assert (buffer.copy_insn_closure.get () != nullptr); This code change looks correct to me. We indeed only have a copy_insn_closure when the buffer is being used by a thread. > diff --git a/gdb/testsuite/gdb.arch/arm-displaced-step-closure.c b/gdb/testsuite/gdb.arch/arm-displaced-step-closure.c > new file mode 100644 > index 00000000000..085e682be50 > --- /dev/null > +++ b/gdb/testsuite/gdb.arch/arm-displaced-step-closure.c > @@ -0,0 +1,5 @@ > +int main (int argc, char **argv) > + > +{ > + return 0; > +} Missing license, and apply GNU formatting. > diff --git a/gdb/testsuite/gdb.arch/arm-displaced-step-closure.exp b/gdb/testsuite/gdb.arch/arm-displaced-step-closure.exp > new file mode 100644 > index 00000000000..ddac04ebe76 > --- /dev/null > +++ b/gdb/testsuite/gdb.arch/arm-displaced-step-closure.exp > @@ -0,0 +1,41 @@ > +# Copyright 2023 Free Software Foundation, Inc. > +# > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 3 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see . > +# > +# This file is part of the gdb testsuite. > +# > +# Test a displaced stepping closure management bug, where a closure lookup > +# by address returns a match even if no displaced stepping is currently > +# taking place. > + > +require is_aarch32_target > + > +standard_testfile > +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } { > + return -1 > +} > + > +if ![runto_main] { > + return -1 > +} > + > +# We have a breakpoint at the current pc (from stopping at main). Step over > +# the breakpoint. > +gdb_test "stepi" ".*" "step-over breakpoint" > + > +# Now attempt to disassemble the entry point function, where the displaced > +# stepping buffer is. With the bug, gdb will crash when we attempt to list > +# the PC that was used to displaced-step the previous instruction. > +gdb_test "disassemble _start" ".*End of assembler dump\." \ > + "disassemble through displaced-step buffer" I don't see anything inherently ARM-specific with the test. Can we have it as a general test? Simon