From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 56265 invoked by alias); 13 Feb 2020 22:52:37 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 55728 invoked by uid 89); 13 Feb 2020 22:52:36 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-5.3 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.1 spammy=child's, closure, childs, owns X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 13 Feb 2020 22:52:34 +0000 Received: from [172.16.0.95] (192-222-181-218.qc.cable.ebox.net [192.222.181.218]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 07BC01E5F8; Thu, 13 Feb 2020 17:52:32 -0500 (EST) Subject: Re: [PATCH 1/2] gdb: cleanup of displaced_step_inferior_state::reset/displaced_step_clear To: Simon Marchi , gdb-patches@sourceware.org References: <20200122151410.30012-1-simon.marchi@efficios.com> From: Simon Marchi Message-ID: <627719a0-6ae6-40b9-a6c4-6123dca89dc7@simark.ca> Date: Thu, 13 Feb 2020 22:52:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 MIME-Version: 1.0 In-Reply-To: <20200122151410.30012-1-simon.marchi@efficios.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2020-02/txt/msg00545.txt.bz2 On 2020-01-22 10:14 a.m., Simon Marchi wrote: > displaced_step_inferior_state::reset and displaced_step_clear appear to > have the same goal, but they don't do the same thing. > displaced_step_inferior_state::reset clears more things than > displaced_step_clear, but it misses free'ing the closure, which > displaced_step_clear does. > > This patch replaces displaced_step_clear's implementation with just a call to > displaced_step_inferior_state::reset. It then changes > displaced_step_inferior_state::step_closure to be a unique_ptr, to indicate the > fact that displaced_step_inferior_state owns the closure (and so that it is > automatically freed when the field is reset). > > It should be possible to get rid of displaced_step_clear entirely, but I'm not > sure what the best way, give that it's used in scope exit macros. > > The test gdb.base/step-over-syscall.exp caught a problem when doing this, which > I consider to be a latent bug which my cleanup exposes. In > handle_inferior_event, in the TARGET_WAITKIND_FORKED case, if we displaced-step > over a fork syscall, we make sure to restore the memory that we used as a > displaced-stepping buffer in the child. We do so using the > displaced_step_inferior_state of the parent. However, we do it after calling > displaced_step_fixup for the parent, which clears the information in the > parent's displaced_step_inferior_state. It worked fine before, because > displaced_step_clear didn't completely clear the displaced_step_inferior_state > structure, so the required information (in this case the gdbarch) was > still available after clearing. > > I fixed it by making GDB restore the child's memory before calling the > displaced_step_fixup on the parent. This way, the data in the > displaced_step_inferior_state structure is still valid when we use it for the > child. This is the error you would get in > gdb.base/step-over-syscall.exp without this fix: > > /home/smarchi/src/binutils-gdb/gdb/gdbarch.c:3911: internal-error: ULONGEST gdbarch_max_insn_length(gdbarch*): Assertion `gdbarch != NULL' failed. If there's no objection, I would push these two patches next week. Simon