From patchwork Fri Feb 14 20:11:44 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Marchi X-Patchwork-Id: 38086 Received: (qmail 44047 invoked by alias); 14 Feb 2020 20:11:50 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 44036 invoked by uid 89); 14 Feb 2020 20:11:49 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-17.8 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.1 spammy=sig, highlights 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; Fri, 14 Feb 2020 20:11:47 +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)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 61A201E5F3; Fri, 14 Feb 2020 15:11:45 -0500 (EST) Subject: Re: [PATCH 1/2] gdb: cleanup of displaced_step_inferior_state::reset/displaced_step_clear To: Pedro Alves , Simon Marchi , gdb-patches@sourceware.org References: <20200122151410.30012-1-simon.marchi@efficios.com> From: Simon Marchi Message-ID: Date: Fri, 14 Feb 2020 15:11:44 -0500 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: On 2020-02-14 2:39 p.m., Pedro Alves wrote: > On 1/22/20 3:14 PM, 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 reason it needs to be wrapped in a cleanup instead of say SCOPE_EXIT, > is that it needs to be discardable with the "cleanup.release ();" call. > I'm not sure there's a better way. Ok, makes sense. > Renaming displaced_step_clear to displaced_step_reset and adding a comment > may result in clearer code, though. I did this. > >> diff --git a/gdb/infrun.h b/gdb/infrun.h >> index 8040b28f0172..c6329c844d9b 100644 >> --- a/gdb/infrun.h >> +++ b/gdb/infrun.h >> @@ -290,7 +290,7 @@ struct displaced_step_inferior_state >> failed_before = 0; >> step_thread = nullptr; >> step_gdbarch = nullptr; >> - step_closure = nullptr; >> + step_closure.reset (); > > I see people sometimes doing this change and I'm curious. > Is it for clarity? I never really thought about it, I thought it was just the typical way of clearing a unique_ptr. But now that I think about it, I prefer calling reset() over just assigning to nullptr, because it kind of highlights the fact that this is not a simple pointer. So it makes it more obvious to me that this line will free whatever the pointer points to. But otherwise I don't mind. > > Anyway, this LGTM. I'll push the updated patch below. From aecc33e5f041af66a3a7eae0c28455b9c9a93b12 Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Wed, 22 Jan 2020 10:14:09 -0500 Subject: [PATCH] gdb: cleanup of displaced_step_inferior_state::reset/displaced_step_clear 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). 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. gdb/ChangeLog: * infrun.c (get_displaced_step_closure_by_addr): Adjust to std::unique_ptr. (displaced_step_clear): Rename to... (displaced_step_reset): ... this. Just call displaced->reset (). (displaced_step_clear_cleanup): Rename to... (displaced_step_reset_cleanup): ... this. (displaced_step_prepare_throw): Adjust to std::unique_ptr. (displaced_step_fixup): Likewise. (resume_1): Likewise. (handle_inferior_event): Restore child's memory before calling displaced_step_fixup on the parent. * infrun.h (displaced_step_inferior_state) : Adjust to std::unique_ptr. : Change type to std::unique_ptr. --- gdb/infrun.c | 52 +++++++++++++++++++++++++--------------------------- gdb/infrun.h | 4 ++-- 2 files changed, 27 insertions(+), 29 deletions(-) diff --git a/gdb/infrun.c b/gdb/infrun.c index 3e846f8e6802..e3e4bdb9b847 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -1540,7 +1540,7 @@ get_displaced_step_closure_by_addr (CORE_ADDR addr) /* If checking the mode of displaced instruction in copy area. */ if (displaced->step_thread != nullptr && displaced->step_copy == addr) - return displaced->step_closure; + return displaced->step_closure.get (); return NULL; } @@ -1596,20 +1596,18 @@ use_displaced_stepping (struct thread_info *tp) && !displaced_state->failed_before); } -/* Clean out any stray displaced stepping state. */ +/* Simple function wrapper around displaced_step_inferior_state::reset. */ + static void -displaced_step_clear (struct displaced_step_inferior_state *displaced) +displaced_step_reset (displaced_step_inferior_state *displaced) { - /* Indicate that there is no cleanup pending. */ - displaced->step_thread = nullptr; - - delete displaced->step_closure; - displaced->step_closure = NULL; + displaced->reset (); } -/* A cleanup that wraps displaced_step_clear. */ -using displaced_step_clear_cleanup - = FORWARD_SCOPE_EXIT (displaced_step_clear); +/* A cleanup that wraps displaced_step_reset. We use this instead of, say, + SCOPE_EXIT, because it needs to be discardable with "cleanup.release ()". */ + +using displaced_step_reset_cleanup = FORWARD_SCOPE_EXIT (displaced_step_reset); /* Dump LEN bytes at BUF in hex to FILE, followed by a newline. */ void @@ -1691,7 +1689,7 @@ displaced_step_prepare_throw (thread_info *tp) target_pid_to_str (tp->ptid).c_str ()); } - displaced_step_clear (displaced); + displaced_step_reset (displaced); scoped_restore_current_thread restore_thread; @@ -1754,12 +1752,12 @@ displaced_step_prepare_throw (thread_info *tp) succeeds. */ displaced->step_thread = tp; displaced->step_gdbarch = gdbarch; - displaced->step_closure = closure; + displaced->step_closure.reset (closure); displaced->step_original = original; displaced->step_copy = copy; { - displaced_step_clear_cleanup cleanup (displaced); + displaced_step_reset_cleanup cleanup (displaced); /* Resume execution at the copy. */ regcache_write_pc (regcache, copy); @@ -1862,7 +1860,7 @@ displaced_step_fixup (thread_info *event_thread, enum gdb_signal signal) if (displaced->step_thread != event_thread) return 0; - displaced_step_clear_cleanup cleanup (displaced); + displaced_step_reset_cleanup cleanup (displaced); displaced_step_restore (displaced, displaced->step_thread->ptid); @@ -1879,7 +1877,7 @@ displaced_step_fixup (thread_info *event_thread, enum gdb_signal signal) { /* Fix up the resulting state. */ gdbarch_displaced_step_fixup (displaced->step_gdbarch, - displaced->step_closure, + displaced->step_closure.get (), displaced->step_original, displaced->step_copy, get_thread_regcache (displaced->step_thread)); @@ -2472,8 +2470,8 @@ resume_1 (enum gdb_signal sig) pc = regcache_read_pc (get_thread_regcache (tp)); displaced = get_displaced_stepping_state (tp->inf); - step = gdbarch_displaced_step_hw_singlestep (gdbarch, - displaced->step_closure); + step = gdbarch_displaced_step_hw_singlestep + (gdbarch, displaced->step_closure.get ()); } } @@ -5305,6 +5303,15 @@ Cannot fill $_exitsignal with the correct signal number.\n")); struct regcache *child_regcache; CORE_ADDR parent_pc; + if (ecs->ws.kind == TARGET_WAITKIND_FORKED) + { + struct displaced_step_inferior_state *displaced + = get_displaced_stepping_state (parent_inf); + + /* Restore scratch pad for child process. */ + displaced_step_restore (displaced, ecs->ws.value.related_pid); + } + /* GDB has got TARGET_WAITKIND_FORKED or TARGET_WAITKIND_VFORKED, indicating that the displaced stepping of syscall instruction has been done. Perform cleanup for parent process here. Note @@ -5315,15 +5322,6 @@ Cannot fill $_exitsignal with the correct signal number.\n")); that needs it. */ start_step_over (); - if (ecs->ws.kind == TARGET_WAITKIND_FORKED) - { - struct displaced_step_inferior_state *displaced - = get_displaced_stepping_state (parent_inf); - - /* Restore scratch pad for child process. */ - displaced_step_restore (displaced, ecs->ws.value.related_pid); - } - /* Since the vfork/fork syscall instruction was executed in the scratchpad, the child's PC is also within the scratchpad. Set the child's PC to the parent's PC value, which has already been fixed up. diff --git a/gdb/infrun.h b/gdb/infrun.h index 8040b28f0172..c6329c844d9b 100644 --- a/gdb/infrun.h +++ b/gdb/infrun.h @@ -290,7 +290,7 @@ struct displaced_step_inferior_state failed_before = 0; step_thread = nullptr; step_gdbarch = nullptr; - step_closure = nullptr; + step_closure.reset (); step_original = 0; step_copy = 0; step_saved_copy.clear (); @@ -310,7 +310,7 @@ struct displaced_step_inferior_state /* The closure provided gdbarch_displaced_step_copy_insn, to be used for post-step cleanup. */ - displaced_step_closure *step_closure; + std::unique_ptr step_closure; /* The address of the original instruction, and the copy we made. */