From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 1585) id 12E433858431; Fri, 11 Nov 2022 12:47:44 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 12E433858431 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1668170864; bh=uM+SnIwuW7trzLgfvCM7tk3NjTYJ0fsps9U9urlWT2o=; h=From:To:Subject:Date:From; b=EUskM22VzzGjfrdqK+xIxM8aN2GiKMB/0xlYLCZ9dETRBPD9LXMfKLlTXWo8scJbQ SAMiemDzkDxhMQBeDfHTODf1Uz4c6/xKN1Jh809VWlqrSgPrEaJqbcDb9yaSQaIm9m 9/eh3poO1y+OM8AGckajjKK7QzeV5FqUEMFBxM4Y= Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Luis Machado To: gdb-cvs@sourceware.org Subject: [binutils-gdb] Make sure a copy_insn_closure is available when we have a match in copy_insn_closure_by_addr X-Act-Checkin: binutils-gdb X-Git-Author: Luis Machado X-Git-Refname: refs/heads/master X-Git-Oldrev: 70b9d05b26e861524d70ee90dcd28cfd77032ddd X-Git-Newrev: 1e5ccb9c5ff4fd8ade4a8694676f99f4abf2d679 Message-Id: <20221111124744.12E433858431@sourceware.org> Date: Fri, 11 Nov 2022 12:47:44 +0000 (GMT) List-Id: https://sourceware.org/git/gitweb.cgi?p=3Dbinutils-gdb.git;h=3D1e5ccb9c5ff4= fd8ade4a8694676f99f4abf2d679 commit 1e5ccb9c5ff4fd8ade4a8694676f99f4abf2d679 Author: Luis Machado Date: Tue Oct 25 11:01:32 2022 +0100 Make sure a copy_insn_closure is available when we have a match in copy= _insn_closure_by_addr =20 PR gdb/29272 =20 Investigating PR29272, it was mentioned a particular test used to work = on GDB 10, but it started failing with GDB 11 onwards. I tracked it down to some displaced stepping improvements on commit 187b041e2514827b9d86190ed2471c4c7a352874. =20 In particular, one of the corner cases using copy_insn_closure_by_addr = got silently broken. It is hard to spot because it doesn't have any good te= sts for it, and the situation is quite specific to the Arm target. =20 Essentially, the change from the displaced stepping improvements made i= t so we could still invoke copy_insn_closure_by_addr correctly to return the pointer to a copy_insn_closure, but it always returned nullptr due to the order of the statements in displaced_step_buffer::prepare. =20 The way it is now, we first write the address of the displaced step buf= fer to PC and then save the copy_insn_closure pointer. =20 The problem is that writing to PC for the Arm target requires figuring out if the new PC is thumb mode or not. =20 With no copy_insn_closure data, the logic to determine the thumb mode during displaced stepping doesn't work, and gives random results that are difficult to track (SIGILL, SIGSEGV etc). =20 Fix this by reordering the PC write in displaced_step_buffer::prepare and, for safety, add an assertion to displaced_step_buffer::copy_insn_closure_by_addr so GDB stops right when it sees this invalid situation. If this gets broken again in the future, it will be easier to spot. =20 Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=3D29272 =20 Approved-By: Simon Marchi Diff: --- gdb/displaced-stepping.c | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/gdb/displaced-stepping.c b/gdb/displaced-stepping.c index eac2c5dab94..7dfd63d8716 100644 --- a/gdb/displaced-stepping.c +++ b/gdb/displaced-stepping.c @@ -139,15 +139,33 @@ displaced_step_buffers::prepare (thread_info *thread,= CORE_ADDR &displaced_pc) return DISPLACED_STEP_PREPARE_STATUS_CANT; } =20 - /* Resume execution at the copy. */ - regcache_write_pc (regcache, buffer->addr); - /* This marks the buffer as being in use. */ buffer->current_thread =3D thread; =20 /* Save this, now that we know everything went fine. */ buffer->copy_insn_closure =3D std::move (copy_insn_closure); =20 + /* Reset the displaced step buffer state if we failed to write PC. + Otherwise we will prevent this buffer from being used, as it will + always have a thread in buffer->current_thread. */ + auto reset_buffer =3D make_scope_exit + ([buffer] () + { + buffer->current_thread =3D nullptr; + buffer->copy_insn_closure.reset (); + }); + + /* Adjust the PC so it points to the displaced step buffer address that = will + be used. This needs to be done after we save the copy_insn_closure, = as + some architectures (Arm, for one) need that information so they can a= djust + other data as needed. In particular, Arm needs to know if the instru= ction + being executed in the displaced step buffer is thumb or not. Without= that + information, things will be very wrong in a random way. */ + regcache_write_pc (regcache, buffer->addr); + + /* PC update successful. Discard the displaced step state rollback. */ + reset_buffer.release (); + /* Tell infrun not to try preparing a displaced step again for this infe= rior if all buffers are taken. */ thread->inf->displaced_step_state.unavailable =3D true; @@ -264,7 +282,11 @@ displaced_step_buffers::copy_insn_closure_by_addr (COR= E_ADDR addr) for (const displaced_step_buffer &buffer : m_buffers) { if (addr =3D=3D buffer.addr) + { + /* The closure information should always be available. */ + gdb_assert (buffer.copy_insn_closure.get () !=3D nullptr); return buffer.copy_insn_closure.get (); + } } =20 return nullptr;