From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 76323 invoked by alias); 22 Jan 2020 15:14:29 -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 76311 invoked by uid 89); 22 Jan 2020 15:14:29 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-23.2 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_PASS autolearn=ham version=3.3.1 spammy=child's, childs, 3911, latent X-HELO: mail.efficios.com Received: from mail.efficios.com (HELO mail.efficios.com) (167.114.26.124) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 22 Jan 2020 15:14:27 +0000 Received: from localhost (localhost [127.0.0.1]) by mail.efficios.com (Postfix) with ESMTP id 1A37A25A1A8 for ; Wed, 22 Jan 2020 10:14:25 -0500 (EST) Received: from mail.efficios.com ([127.0.0.1]) by localhost (mail03.efficios.com [127.0.0.1]) (amavisd-new, port 10032) with ESMTP id jE0g0-t7F0Un; Wed, 22 Jan 2020 10:14:24 -0500 (EST) Received: from localhost (localhost [127.0.0.1]) by mail.efficios.com (Postfix) with ESMTP id 9F46525A1A6; Wed, 22 Jan 2020 10:14:24 -0500 (EST) DKIM-Filter: OpenDKIM Filter v2.10.3 mail.efficios.com 9F46525A1A6 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=efficios.com; s=default; t=1579706064; bh=XaIb/fOGBxVMLRxnN5Lh7nG4dZf/c2ZLETPoU1EMrmU=; h=From:To:Date:Message-Id:MIME-Version; b=NlFzOL4k+RobHUrOPf7dIpxynFRjOEkKwxNRyKgNGRNmXP57p9Hfz+/Ie2DBIdmaH o+/6wcDbLEdYXyxugA1SGx84L+nGexI15nEB84B4JtGFFRpGCpC9cQvJz1ahfwKGr9 2iE9p1FOSRGQ8yHrGfeRjDDm61dG22yOlP3KlrvgJ8j1IyPOVIWl3RyDcKRbis1mzN a2TakvFXb4WsXQDUQfc3YsRA8AfFS3Yes9ycIOcpP+2FQ5joHwK6Vy7ubf7Bcm0Wne GE6JtgQGQs26oo7K0s0ipMp33GIIeH7DxeEdWvcYNCPZlzvTB0bHtCAXaWTx2Wv8gw kLAWfQXRqnCPQ== Received: from mail.efficios.com ([127.0.0.1]) by localhost (mail03.efficios.com [127.0.0.1]) (amavisd-new, port 10026) with ESMTP id O_evLmsFjaYZ; Wed, 22 Jan 2020 10:14:24 -0500 (EST) Received: from smarchi-efficios.internal.efficios.com (192-222-181-218.qc.cable.ebox.net [192.222.181.218]) by mail.efficios.com (Postfix) with ESMTPSA id 7AECD259EE5; Wed, 22 Jan 2020 10:14:24 -0500 (EST) From: Simon Marchi To: gdb-patches@sourceware.org Cc: Simon Marchi Subject: [PATCH 1/2] gdb: cleanup of displaced_step_inferior_state::reset/displaced_step_clear Date: Wed, 22 Jan 2020 15:14:00 -0000 Message-Id: <20200122151410.30012-1-simon.marchi@efficios.com> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-SW-Source: 2020-01/txt/msg00676.txt.bz2 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, w= hich 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 calli= ng 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_st= ate 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 t= he 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: ULON= GEST gdbarch_max_insn_length(gdbarch*): Assertion `gdbarch !=3D NULL' faile= d. gdb/ChangeLog: * infrun.c (get_displaced_step_closure_by_addr): Adjust to std::unique_ptr. (displaced_step_clear): Just call displaced->reset (). (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 | 36 ++++++++++++++++-------------------- gdb/infrun.h | 4 ++-- 2 files changed, 18 insertions(+), 22 deletions(-) diff --git a/gdb/infrun.c b/gdb/infrun.c index 9c4a07daba97..1fee65730dbc 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -1548,7 +1548,7 @@ get_displaced_step_closure_by_addr (CORE_ADDR addr) /* If checking the mode of displaced instruction in copy area. */ if (displaced->step_thread !=3D nullptr && displaced->step_copy =3D=3D addr) - return displaced->step_closure; + return displaced->step_closure.get (); =20 return NULL; } @@ -1606,13 +1606,9 @@ use_displaced_stepping (struct thread_info *tp) =20 /* Clean out any stray displaced stepping state. */ static void -displaced_step_clear (struct displaced_step_inferior_state *displaced) +displaced_step_clear (displaced_step_inferior_state *displaced) { - /* Indicate that there is no cleanup pending. */ - displaced->step_thread =3D nullptr; - - delete displaced->step_closure; - displaced->step_closure =3D NULL; + displaced->reset (); } =20 /* A cleanup that wraps displaced_step_clear. */ @@ -1762,7 +1758,7 @@ displaced_step_prepare_throw (thread_info *tp) succeeds. */ displaced->step_thread =3D tp; displaced->step_gdbarch =3D gdbarch; - displaced->step_closure =3D closure; + displaced->step_closure.reset (closure); displaced->step_original =3D original; displaced->step_copy =3D copy; =20 @@ -1887,7 +1883,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_t= hread)); @@ -2480,8 +2476,8 @@ resume_1 (enum gdb_signal sig) pc =3D regcache_read_pc (get_thread_regcache (tp)); =20 displaced =3D get_displaced_stepping_state (tp->inf); - step =3D gdbarch_displaced_step_hw_singlestep (gdbarch, - displaced->step_closure); + step =3D gdbarch_displaced_step_hw_singlestep + (gdbarch, displaced->step_closure.get ()); } } =20 @@ -5313,6 +5309,15 @@ Cannot fill $_exitsignal with the correct signal num= ber.\n")); struct regcache *child_regcache; CORE_ADDR parent_pc; =20 + if (ecs->ws.kind =3D=3D TARGET_WAITKIND_FORKED) + { + struct displaced_step_inferior_state *displaced + =3D 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 @@ -5323,15 +5328,6 @@ Cannot fill $_exitsignal with the correct signal num= ber.\n")); that needs it. */ start_step_over (); =20 - if (ecs->ws.kind =3D=3D TARGET_WAITKIND_FORKED) - { - struct displaced_step_inferior_state *displaced - =3D 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 scrat= chpad, 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 =3D 0; step_thread =3D nullptr; step_gdbarch =3D nullptr; - step_closure =3D nullptr; + step_closure.reset (); step_original =3D 0; step_copy =3D 0; step_saved_copy.clear (); @@ -310,7 +310,7 @@ struct displaced_step_inferior_state =20 /* 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; =20 /* The address of the original instruction, and the copy we made. */ --=20 2.25.0