From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id PYSaNuwMA2HyTQAAWB0awg (envelope-from ) for ; Thu, 29 Jul 2021 16:17:48 -0400 Received: by simark.ca (Postfix, from userid 112) id CE24D1EDFB; Thu, 29 Jul 2021 16:17:48 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-0.7 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,RDNS_DYNAMIC,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (ip-8-43-85-97.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id E5BF71E4A3 for ; Thu, 29 Jul 2021 16:17:47 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 8A950385481A for ; Thu, 29 Jul 2021 20:17:47 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 8A950385481A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1627589867; bh=tGAhnwTfCfVqvtO9CUdmQw+MBTDM5dqqeDTpHPqmK3U=; h=Subject:To:References:Date:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=AxMFd215KREfvcouYOz+7exojmhAjLiFfknDsOCGQvddwQaUW4ulsxPSWOpf3Dfz2 Cf/pivtCC4beMk0Jhpg3/Pb1Sh9J7ifJu7yz+s/P2jrLHxAZFwSY2EFj3s2rKeR0Af yaCwIXZQGrYEDR7j60XY+HV6crVcUjfIS4ZHIrUA= Received: from mail-pl1-x634.google.com (mail-pl1-x634.google.com [IPv6:2607:f8b0:4864:20::634]) by sourceware.org (Postfix) with ESMTPS id 14F18385800F for ; Thu, 29 Jul 2021 20:17:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 14F18385800F Received: by mail-pl1-x634.google.com with SMTP id e5so8299643pld.6 for ; Thu, 29 Jul 2021 13:17:26 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=tGAhnwTfCfVqvtO9CUdmQw+MBTDM5dqqeDTpHPqmK3U=; b=SjSD1iOAPao/NePT5KRz4H2f2OTz9l99h2P6sK66MAg6DHfIBLr4pGLsv7UJQ5oGwS 6Rr7+i1RMRCyxd9RX/BT0+EQOyEUGxzKL8Fp+7+PlbOmjGAeKqCT/QTIdyvA5VXgZ7nK 4juJ9plwaBjdxZOttJzt6YFY4i91unKGTO52ALHjSwivNFyabsIWYCFNqlDqOcJMQp6N Kgb1QaNfqXPJB0gu0doC0SwvfGSTpQgfy03UwO0Jb28k7lL1BcvQU6gtjX5EOSi9TRGu mZ59GsxwUBba1dR2hTv2u2PNznvp/3KONpWnOz5Gaax7B9lP4CvqJsdD+gipjGvuMmPY ijSQ== X-Gm-Message-State: AOAM533lGXbL4bYYyPYJHsfpKA8PQ9CMybHAfJkAWV5hb0USgpJOC9Om JQ+lsLpZtzR2LZ0ExGnJ/eGagBGPOhu5uQ== X-Google-Smtp-Source: ABdhPJyf28reSYVeVJ5cgadBxWDwWBOAvLqkxn2ZkhD788x4BqvsM4+KBYrmtumyoj/NXA+tLmE/dQ== X-Received: by 2002:a05:6a00:884:b029:346:8678:ce15 with SMTP id q4-20020a056a000884b02903468678ce15mr6871355pfj.75.1627589844887; Thu, 29 Jul 2021 13:17:24 -0700 (PDT) Received: from ?IPv6:2804:7f0:4841:25bc:391a:6fe6:3de7:130a? ([2804:7f0:4841:25bc:391a:6fe6:3de7:130a]) by smtp.gmail.com with ESMTPSA id d22sm4559456pfo.88.2021.07.29.13.17.23 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 29 Jul 2021 13:17:24 -0700 (PDT) Subject: Re: [PATCH] Fix displaced stepping watchpoint check order To: Simon Marchi , gdb-patches@sourceware.org References: <20210608154230.354202-1-luis.machado@linaro.org> Message-ID: Date: Thu, 29 Jul 2021 17:17:21 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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: , From: Luis Machado via Gdb-patches Reply-To: Luis Machado Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" Hi, On 7/29/21 4:36 PM, Simon Marchi wrote: > I think this is ok, but in all honestly I don't completely understand > how the interaction between watchpoints and displaced stepping is > expected to work. Let me try to clarify. When we attempt to execute an instruction in the scratch space (displaced stepping), we may potentially trigger a hardware watchpoint. For AArch64, hardware watchpoints are non-steppable, so that means GDB will need to step over that hardware watchpoint so the instruction's execution completes (if there is no hardware watchpoint trigger, the instruction gets executed just fine). Now, if the hardware watchpoint trigger did happen (and GDB detects that properly), then displaced_step_instruction_executed_successfully (...) will return false. The above check happens after we restore the displaced stepping buffer contents. So the original instruction that caused the hardware watchpoint trigger is gone. That is fine if we don't have to look at the instruction being stepped-over. > > Just some nits: > > On 2021-06-08 11:42 a.m., Luis Machado via Gdb-patches wrote: >> When checking the stopped data address, I noticed, under some circumstances, >> that the instruction at PC wasn't the expected one. This happens because the >> displaced stepping machinery restores the buffer before checking if the >> instruction executed successfully, which in turn calls the watchpoint check. >> >> I guess this was never noticed because stopped data address checks usually >> don't need to fetch the instruction at PC, but AArch64 needs to do it from >> now on. > > Can you clarify what you mean by "from now on"? Can you indicate what > change you are referring to? > From the following change (https://sourceware.org/pipermail/gdb-patches/2021-July/181095.html) onwards, we need to look at the load/store instruction to figure out the memory access size so we can reliably tell if a hardware watchpoint has triggered. This is due to how AArch64's spec defines how to provide a stopped data address, and the valid ranges. With the old code, if we try to fetch the instruction at PC, we will get a bogus value that is not the real instruction that caused the hardware watchpoint trigger. Hence why the patch moves the call to displaced_step_instruction_executed_successfully (...) up and before we restore the displaced stepping buffer. If a hardware watchpoint trigger takes place and GDB doesn't recognize it, then displaced_step_instruction_executed_successfully (...) will return true and GDB will move on and will attempt to execute the same instruction again, only to be halted due to the same hardware watchpoint trigger that it can't detect. So GDB gets into an infinite loop. More generally, if we ever fail to acknowledge a hardware watchpoint trigger on an architecture with non-steppable watchpoints and displaced stepping support, we will run into this infinite loop (as far as I can tell). Does that make sense? >> >> We should check if the instruction executed successfully before we restore the >> scratchpad contents. >> >> Regression tested on aarch64-linux/Ubuntu 20.04. >> >> gdb/ChangeLog: >> >> YYYY-MM-DD Luis Machado >> >> * displaced-stepping.c (displaced_step_buffers::finish): Move check >> upwards. >> --- >> gdb/displaced-stepping.c | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/gdb/displaced-stepping.c b/gdb/displaced-stepping.c >> index 59b78c22f6a..06324d523d8 100644 >> --- a/gdb/displaced-stepping.c >> +++ b/gdb/displaced-stepping.c >> @@ -227,6 +227,11 @@ displaced_step_buffers::finish (gdbarch *arch, thread_info *thread, >> >> ULONGEST len = gdbarch_max_insn_length (arch); >> >> + /* Check if the execution was successful before restoring the buffer >> + contents. */ >> + bool instruction_executed_successfully >> + = displaced_step_instruction_executed_successfully (arch, sig); > > Maybe extend the comment to say "why". Right now I think it just states > what is in plain sight when looking at the code, I think it would be > more useful if it said why it's important to do that. I can expand it to make it more clear. > > Simon >