From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id 8IeGC8pWA2E5VAAAWB0awg (envelope-from ) for ; Thu, 29 Jul 2021 21:32:58 -0400 Received: by simark.ca (Postfix, from userid 112) id 2D6B71EDFB; Thu, 29 Jul 2021 21:32:58 -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=unavailable 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 5231F1E813 for ; Thu, 29 Jul 2021 21:32:54 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id A36013886C7F for ; Fri, 30 Jul 2021 01:32:53 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org A36013886C7F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1627608773; bh=FzQZ0Yhf1nHxPOF0k9DLjaXxAH3UkKxc8yPORajaw/M=; 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=Em8ELbojX/dU3R4AYwh39fhCvPOVSKJk4/qA2P6n20AMHis27OK8SgoRNcg11r43e O7lxl5ZVIfnzfloiEF8NNnG7/XTYbe6zD2vwK1o1hpgWkvK8gqB1Od5svudsi6rumG oT9x8cVsmEY68VWD45EK8I6wRrfdMWRKKpwzBqYg= Received: from mail-pl1-x630.google.com (mail-pl1-x630.google.com [IPv6:2607:f8b0:4864:20::630]) by sourceware.org (Postfix) with ESMTPS id 5A14C3857407 for ; Fri, 30 Jul 2021 01:32:33 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5A14C3857407 Received: by mail-pl1-x630.google.com with SMTP id e21so9128067pla.5 for ; Thu, 29 Jul 2021 18:32:33 -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=FzQZ0Yhf1nHxPOF0k9DLjaXxAH3UkKxc8yPORajaw/M=; b=hPxGnXx7UpDJr1EkyIpzOwX/nRWQMM6Ug9XM2rvTGppd3wZPD0WIPrts1Ohk+E/qA3 li4Ov54K36bbD2kv4fFhjubge65DG1mC/RlHns8U1Wvr/chy0+kzR4rE7tq8yOl5WfHG +XnFmNeZLoJljzegs1JgzinK7aaQNRdsD2nzNSb2kcL8crLOtGm7zyae7WsGCcU7iYCg XnMpcYnyJiszuH1r7d9G/CkXYhxZ3/R4tummk6frkUA6yM20h+29HzHJsd5GOvwVycSZ SV02cinS8/JTdFSX7VtxD9CMPJ9EKQfsX5lLFIJrauhTbkZeV14Pl8dHjWvKFk9xKX5k E/0Q== X-Gm-Message-State: AOAM530pHoRcoRbOs/DaNv+Mgzlj6xrqSrNo4d0qUac4Omh+pbjaPQ4I DeTUBqt66ULeSuJ4EpAvLfWMkbqlDGxmRQ== X-Google-Smtp-Source: ABdhPJxaV1/NPO2dHvlAVWZpMp9O8RKXsAztN9eGcLGjg7rXrQUqp0itj3pU8pjpJNQ9aG7bRX5huw== X-Received: by 2002:a17:90a:c87:: with SMTP id v7mr392830pja.106.1627608752404; Thu, 29 Jul 2021 18:32:32 -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 z14sm77708pfn.156.2021.07.29.18.32.30 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 29 Jul 2021 18:32:32 -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> <9c697f35-1059-5ea8-83c3-ea75fcc95b32@polymtl.ca> Message-ID: <0ef1ceef-f6b4-7310-bec0-d54d4b646fea@linaro.org> Date: Thu, 29 Jul 2021 22:32:28 -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: <9c697f35-1059-5ea8-83c3-ea75fcc95b32@polymtl.ca> 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" On 7/29/21 9:59 PM, Simon Marchi wrote: > > > On 2021-07-29 4:17 p.m., Luis Machado wrote: >> 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). > > What does "need to step over that hardware watchpoint" means, > concretely? After the watchpoint has triggered, are the side effects of > the instruction committed to memory and registers? Or are we in a state > as if the instruction didn't complete? How is that step over done? No side effects of the instruction are committed in this case. Memory and registers will have their old values as if the instruction didn't execute. From reading the code, most architectures have non-steppable hardware watchpoints. GDB just disables the hardware watchpoints, single-steps past that instruction and then enables the hardware watchpoints again. GDB just disables all hardware watchpoints for the sake of simplicity. You can see this logic in infrun.c:handle_signal_stop, around this comment: /* If necessary, step over this watchpoint. We'll be back to display it in a moment. */ if (stopped_by_watchpoint && (target_have_steppable_watchpoint () || gdbarch_have_nonsteppable_watchpoint (gdbarch))) It is actually best to disable all hardware watchpoints. If we end up disabling just one hardware watchpoint, and then we happen to have another hardware watchpoint that is active and also getting triggered, we might be stuck in an endless loop as well. > >>> >>> 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. > > Ok, but that patch you linked isn't merged yet? So it sounds strange to > say "from now on", it sounds like there's a dependency between the two Absolutely. There is a dependency. My plan is to merge this fix first, and then merge the AArch64 hardware watchpoint detection fixes. I just didn't group those together, but that's the right order. I'll make sure to point out the dependency in the other patch. > patches. Let's say the current patch is merged before the other one, > maybe it should say "but AArch64 will need to do it it an upcoming > patch", and then you can given the link. > >> 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? > > Yes, this help. Please feel free to include in the commit message any > additional detail that you gave here, since it might help somebody else > in the future. I'll make the commit message more detailed. > > The code change is fine, so once you send a revised version it should go > fairly quickly. Sorry for the delay. No worries. Thanks for the feedback! > > Thanks! > > Simon >