From patchwork Thu Feb 23 15:46:54 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 65523 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id DAC81385B537 for ; Thu, 23 Feb 2023 15:47:40 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org DAC81385B537 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1677167260; bh=btS8O4o0UNPRkdT/F2BWRLTgW3Z0VkmSZZU+8rODYpA=; h=To:Cc:Subject:Date:In-Reply-To:References:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=QqN5ABHB+57sfm9eRIsvAR1hA/fjer4knFdy3lS+lAYazaW9okoqhXKJC+pxB0s6g EMa2GzNAvqg2D8RSv3gUDPRPJcWSO4OhtE049/9epLm9QLObm/yXXMECCtUWNrEoHN 7NO3hohf+v2OGWtMLb6T6lcFKh1c1U3ISM0p2k8E= X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id EE2123858020 for ; Thu, 23 Feb 2023 15:47:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org EE2123858020 Received: from mail-wr1-f69.google.com (mail-wr1-f69.google.com [209.85.221.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-564-oj0aUrDcPNGvHh-plF3Irw-1; Thu, 23 Feb 2023 10:47:06 -0500 X-MC-Unique: oj0aUrDcPNGvHh-plF3Irw-1 Received: by mail-wr1-f69.google.com with SMTP id o15-20020a05600002cf00b002c54a27803cso2540898wry.22 for ; Thu, 23 Feb 2023 07:47:06 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=btS8O4o0UNPRkdT/F2BWRLTgW3Z0VkmSZZU+8rODYpA=; b=tI4LBhMm8BtLEXL7rJvXhm2VMjXG4x5ixNzYtoEc2QYJTFg/8HgOQ/ZUC3h91ypSfo V18mdk+qfeg3n5eax2RtxqzAIdNUXyPxrp+kS2Edgr1jkQFSd2EEskvauxLoloUxBSVU k3yElYrnWjXLhLPNm6M1Xdt2OdW3rcCAlz9pNEul2Kjyub99p2tUUllkOvqJO6bfNetq W2o8EJVrre3md9Ukg1o4Qoarp7ru+vS5dJIa/YLDrkyqdB++TV48NskGU77aSrVCGmNr LbjpJ8V9hjcUKcy/PAht+RJTqIjKzXc7K1wJrnO8zUx0vxGEKkwTYfVWPdTR8CvP++Pm 8CaQ== X-Gm-Message-State: AO0yUKXSCFrI02Pb/2sN4hDCy5QyVq3e0xaSbba8czaDbYLs2ngKkG9p cOaxNmjvrgMLalyTQ2JJPhMmNgpMDdiFuSd5aVPG9vl5Hn1xry6xd4TohO/0XPs2kUumz1JCFry WdvIprNPXHai1yAoM6YEefq5rAmUjhaDN6DCsCQwf4R9finT+urfMAdsHk4nrT+6cCsP0cOMrpx xYJqI= X-Received: by 2002:a05:600c:755:b0:3da:fd07:1e3 with SMTP id j21-20020a05600c075500b003dafd0701e3mr3841806wmn.22.1677167224550; Thu, 23 Feb 2023 07:47:04 -0800 (PST) X-Google-Smtp-Source: AK7set+NVEAyR6yd6CE0kU9qhbAx5FYTbU08P5IVKnkFc8O7YadUjn1Sj9/JTldyL7tsKczdJCY3Og== X-Received: by 2002:a05:600c:755:b0:3da:fd07:1e3 with SMTP id j21-20020a05600c075500b003dafd0701e3mr3841768wmn.22.1677167223694; Thu, 23 Feb 2023 07:47:03 -0800 (PST) Received: from localhost (95.72.115.87.dyn.plus.net. [87.115.72.95]) by smtp.gmail.com with ESMTPSA id h13-20020a05600c314d00b003e2059c7978sm12252412wmo.36.2023.02.23.07.47.03 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 23 Feb 2023 07:47:03 -0800 (PST) To: gdb-patches@sourceware.org Cc: Andrew Burgess Subject: [PATCH 3/3] gdb: fix reg corruption from displaced stepping on amd64 Date: Thu, 23 Feb 2023 15:46:54 +0000 Message-Id: <8c62ce4df41c468775b88af3110999d08871843e.1677167018.git.aburgess@redhat.com> X-Mailer: git-send-email 2.25.4 In-Reply-To: References: MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-11.4 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_NUMSUBJECT, KAM_SHORT, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org 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: , X-Patchwork-Original-From: Andrew Burgess via Gdb-patches From: Andrew Burgess Reply-To: Andrew Burgess Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Sender: "Gdb-patches" This commit aims to address a problem that exists with the current approach to displaced stepping, and was identified in PR gdb/22921. Displaced stepping is currently supported on AArch64, ARM, amd64, i386, rs6000 (ppc), and s390. Of these, I believe there is a problem with the current approach which will impact amd64 and ARM, and can lead to random register corruption when the inferior makes use of asynchronous signals and GDB is using displaced stepping. The problem can be found in displaced_step_buffers::finish in displaced-stepping.c, and is this; after GDB tries to perform a displaced step, and the inferior stops, GDB classifies the stop into one of two states, either the displaced step succeeded, or the displaced step failed. If the displaced step succeeded then gdbarch_displaced_step_fixup is called, which has the job of fixing up the state of the current inferior as if the step had not been performed in a displaced manor. This all seems just fine. However, if the displaced step is considered to have not completed then GDB doesn't call gdbarch_displaced_step_fixup, instead GDB remains in displaced_step_buffers::finish and just performs a minimal fixup which involves adjusting the program counter back to its original value. The problem here is that for amd64 and ARM setting up for a displaced step can involve changing the values in some temporary registers. If the displaced step succeeds then this is fine; after the step the temporary registers are restored to their original values in the architecture specific code. But if the displaced step does not succeed then the temporary registers are never restored, and they retain their modified values. In this context a temporary register is simply any register that is not otherwise used by the instruction being stepped that the architecture specific code considers safe to borrow for the lifetime of the instruction being stepped. In the bug PR gdb/22921, the amd64 instruction being stepped is an rip-relative instruction like this: jmp *0x2fe2(%rip) When we displaced step this instruction we borrow a register, and modify the instruction to something like: jmp *0x2fe2(%rcx) with %rcx having its value adjusted to contain the original %rip value. Now if the displaced step does not succeed then %rcx will be left with a corrupted value. Obviously corrupting any register is bad; in the bug report this problem was spotted because %rcx is used as a function argument register. And finally, why might a displaced step not succeed? Asynchronous signals provides one reason. GDB sets up for the displaced step and, at that precise moment, the OS delivers a signal (SIGALRM in the test), the signal stops the inferior at the address of the displaced instruction. GDB cancels the displaced instruction, handles the signal, and then tries again with the displaced step. But it is that first cancellation of the displaced step that causes the problem; in that case GDB (correctly) sees the displaced step as having not completed, and so does not perform the architecture specific fixup, leaving the register corrupted. The reason why I think AArch64, rs600, i386, and s390 are not effected by this problem is that I don't believe these architectures make use of any temporary registers, so when a displaced step is not completed successfully, the minimal fix up is sufficient. On amd64 we use at most one temporary register. On ARM, looking at arm_displaced_step_copy_insn_closure, we could modify up to 16 temporary registers, and the instruction being displaced stepped could be expanded to multiple replacement instructions, which increases the chances of this bug triggering. This commit only aims to address the issue on amd64 for now, though I believe that the approach I'm proposing here might be applicable for ARM too. What I propose is that we always call gdbarch_displaced_step_fixup. We will now pass an extra argument to gdbarch_displaced_step_fixup, this is the program-counter value at which the inferior stopped. Using this program-counter value GDB can determine if the displaced stepped instruction completed or not. Or (in the future) for ARM, how far through the displaced step sequence the inferior got before being interrupted. It is then possible for the architecture specific code in GDB to conditionally roll back some or all of the instructions effects, i.e. on amd64 we can roll back the temporary register value in all cases. In order to move all architectures to this new API, I have moved the minimal roll-back version of the code inside the architecture specific fixup functions for AArch64, rs600, s390, and ARM. For all of these except ARM I think this is good enough, as no temporaries are used all that's needed is the program counter restore anyway. For ARM the minimal code is no worse than what we had before, though I do consider this architectures displaced-stepping broken. For amd64 and i386 I make use of the program counter to conditionally roll back the inferior state. For i386 this is not strictly necessary, as no temporaries are used. However, the structure of i386_displaced_step_fixup is so similar to amd64_displaced_step_fixup that is seems a shame to take a different approach in these two functions. One thing to note is that previously we determined if the displaced step had completed based on how the inferior stopped. Stopping with a SIGTRAP we assumed meant that we had hit the breakpoint at the end of the displaced instruction. Anything else indicated the displaced instruction had not completed successfully. After this patch GDB now relies entirely on the program-counter value to make this decision. If the program-counter points that the start of the displaced instruction buffer (i.e. it hasn't changed) then the displaced step didn't complete, otherwise (for amd64 and i386, which always replace with a single instruction), we assume the displaced step did complete. I've updated the gdb.arch/amd64-disp-step.exp test to cover the 'jmpq*' instruction that was causing problems in the original bug, and also added support for testing the displaced step in the presence of asynchronous signal delivery. Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=22921 --- gdb/aarch64-tdep.c | 19 +++- gdb/aarch64-tdep.h | 3 +- gdb/amd64-tdep.c | 28 ++--- gdb/amd64-tdep.h | 2 +- gdb/arm-tdep.c | 27 ++++- gdb/arm-tdep.h | 3 +- gdb/displaced-stepping.c | 41 +------ gdb/gdbarch-components.py | 22 ++-- gdb/gdbarch-gen.h | 25 +++-- gdb/gdbarch.c | 4 +- gdb/i386-tdep.c | 28 ++--- gdb/i386-tdep.h | 2 +- gdb/rs6000-tdep.c | 15 ++- gdb/s390-tdep.c | 16 ++- .../gdb.arch/amd64-disp-step-signal.c | 30 +++++ gdb/testsuite/gdb.arch/amd64-disp-step.S | 15 +++ gdb/testsuite/gdb.arch/amd64-disp-step.exp | 106 +++++++++++++++--- 17 files changed, 270 insertions(+), 116 deletions(-) create mode 100644 gdb/testsuite/gdb.arch/amd64-disp-step-signal.c diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c index 5b1b9921f87..46c977140a7 100644 --- a/gdb/aarch64-tdep.c +++ b/gdb/aarch64-tdep.c @@ -3367,16 +3367,23 @@ aarch64_displaced_step_copy_insn (struct gdbarch *gdbarch, void aarch64_displaced_step_fixup (struct gdbarch *gdbarch, struct displaced_step_copy_insn_closure *dsc_, - CORE_ADDR from, CORE_ADDR to, - struct regcache *regs) + struct regcache *regs, + CORE_ADDR from, CORE_ADDR to, CORE_ADDR pc) { + /* If the PC at which the inferior stopped is still the TO address then + the displaced instruction was never executed. All we need to do in + this case is restore the program-counter to its original location and + we're done. */ + if (pc == to) + { + pc = from + (pc - to); + regcache_write_pc (regs, pc); + return; + } + aarch64_displaced_step_copy_insn_closure *dsc = (aarch64_displaced_step_copy_insn_closure *) dsc_; - ULONGEST pc; - - regcache_cooked_read_unsigned (regs, AARCH64_PC_REGNUM, &pc); - displaced_debug_printf ("PC after stepping: %s (was %s).", paddress (gdbarch, pc), paddress (gdbarch, to)); diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h index ae38327ffab..801a151913b 100644 --- a/gdb/aarch64-tdep.h +++ b/gdb/aarch64-tdep.h @@ -141,8 +141,9 @@ displaced_step_copy_insn_closure_up void aarch64_displaced_step_fixup (struct gdbarch *gdbarch, displaced_step_copy_insn_closure *dsc, + struct regcache *regs, CORE_ADDR from, CORE_ADDR to, - struct regcache *regs); + CORE_ADDR pc); bool aarch64_displaced_step_hw_singlestep (struct gdbarch *gdbarch); diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c index 0ae09dc7bfb..c2aad2f7161 100644 --- a/gdb/amd64-tdep.c +++ b/gdb/amd64-tdep.c @@ -1692,9 +1692,11 @@ amd64_insn_is_jump (struct gdbarch *gdbarch, CORE_ADDR addr) void amd64_displaced_step_fixup (struct gdbarch *gdbarch, struct displaced_step_copy_insn_closure *dsc_, - CORE_ADDR from, CORE_ADDR to, - struct regcache *regs) + struct regcache *regs, + CORE_ADDR from, CORE_ADDR to, CORE_ADDR pc) { + bool displaced_step_completed = pc != to; + amd64_displaced_step_copy_insn_closure *dsc = (amd64_displaced_step_copy_insn_closure *) dsc_; enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); @@ -1728,15 +1730,13 @@ amd64_displaced_step_fixup (struct gdbarch *gdbarch, the displaced instruction; make it relative to the original insn. Well, signal handler returns don't need relocation either, but we use the value of %rip to recognize those; see below. */ - if (! amd64_absolute_jmp_p (insn_details) - && ! amd64_absolute_call_p (insn_details) - && ! amd64_ret_p (insn_details)) + if (!displaced_step_completed + || (!amd64_absolute_jmp_p (insn_details) + && !amd64_absolute_call_p (insn_details) + && !amd64_ret_p (insn_details))) { - ULONGEST orig_rip; int insn_len; - regcache_cooked_read_unsigned (regs, AMD64_RIP_REGNUM, &orig_rip); - /* A signal trampoline system call changes the %rip, resuming execution of the main program after the signal handler has returned. That makes them like 'return' instructions; we @@ -1752,24 +1752,24 @@ amd64_displaced_step_fixup (struct gdbarch *gdbarch, it unrelocated. Goodness help us if there are PC-relative system calls. */ if (amd64_syscall_p (insn_details, &insn_len) - && orig_rip != to + insn_len + && pc != to + insn_len /* GDB can get control back after the insn after the syscall. Presumably this is a kernel bug. Fixup ensures its a nop, we add one to the length for it. */ - && orig_rip != to + insn_len + 1) + && pc != to + insn_len + 1) displaced_debug_printf ("syscall changed %%rip; not relocating"); else { - ULONGEST rip = orig_rip - insn_offset; + CORE_ADDR rip = pc - insn_offset; /* If we just stepped over a breakpoint insn, we don't backup the pc on purpose; this is to match behaviour without stepping. */ - regcache_cooked_write_unsigned (regs, AMD64_RIP_REGNUM, rip); + regcache_write_pc (regs, rip); displaced_debug_printf ("relocated %%rip from %s to %s", - paddress (gdbarch, orig_rip), + paddress (gdbarch, pc), paddress (gdbarch, rip)); } } @@ -1782,7 +1782,7 @@ amd64_displaced_step_fixup (struct gdbarch *gdbarch, /* If the instruction was a call, the return address now atop the stack is the address following the copied instruction. We need to make it the address following the original instruction. */ - if (amd64_call_p (insn_details)) + if (displaced_step_completed && amd64_call_p (insn_details)) { ULONGEST rsp; ULONGEST retaddr; diff --git a/gdb/amd64-tdep.h b/gdb/amd64-tdep.h index 929b4b8bdc5..58953259765 100644 --- a/gdb/amd64-tdep.h +++ b/gdb/amd64-tdep.h @@ -93,7 +93,7 @@ extern displaced_step_copy_insn_closure_up amd64_displaced_step_copy_insn struct regcache *regs); extern void amd64_displaced_step_fixup (struct gdbarch *gdbarch, displaced_step_copy_insn_closure *closure, - CORE_ADDR from, CORE_ADDR to, struct regcache *regs); + struct regcache *regs, CORE_ADDR from, CORE_ADDR to, CORE_ADDR pc); /* Initialize the ABI for amd64. Uses DEFAULT_TDESC as fallback tdesc, if INFO does not specify one. */ diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c index 03df41a64b3..70c3e94f322 100644 --- a/gdb/arm-tdep.c +++ b/gdb/arm-tdep.c @@ -8649,9 +8649,32 @@ arm_displaced_init_closure (struct gdbarch *gdbarch, CORE_ADDR from, void arm_displaced_step_fixup (struct gdbarch *gdbarch, struct displaced_step_copy_insn_closure *dsc_, - CORE_ADDR from, CORE_ADDR to, - struct regcache *regs) + struct regcache *regs, + CORE_ADDR from, CORE_ADDR to, CORE_ADDR pc) { + /* The following block exists as a temporary measure while displaced + stepping is fixed architecture at a time within GDB. + + In an earlier implementation of displaced stepping, if GDB thought the + displaced instruction had not been executed then this fix up function + was never called. As a consequence, state that should be fixed by + this function was left in a corrupted state. + + However, it's not as simple as always calling this function; this + function needs to be updated to decide what should be fixed up based + on whether the function executed or not, which requires each + architecture to be considered individually. + + Until that is done for this architecture, this block replicates the + code that used to exist in the caller; if the displaced instruction + was not executed, fix up the program counter only. */ + if (pc == to) + { + pc = from + (pc - to); + regcache_write_pc (regs, pc); + return; + } + arm_displaced_step_copy_insn_closure *dsc = (arm_displaced_step_copy_insn_closure *) dsc_; diff --git a/gdb/arm-tdep.h b/gdb/arm-tdep.h index a8d21c44ba4..ab31d3e2e9c 100644 --- a/gdb/arm-tdep.h +++ b/gdb/arm-tdep.h @@ -296,7 +296,8 @@ int arm_frame_is_thumb (frame_info_ptr frame); extern void arm_displaced_step_fixup (struct gdbarch *, displaced_step_copy_insn_closure *, - CORE_ADDR, CORE_ADDR, struct regcache *); + struct regcache *, + CORE_ADDR, CORE_ADDR, CORE_ADDR); /* Return the bit mask in ARM_PS_REGNUM that indicates Thumb mode. */ extern int arm_psr_thumb_bit (struct gdbarch *); diff --git a/gdb/displaced-stepping.c b/gdb/displaced-stepping.c index 7c9610ee728..d4128485d3f 100644 --- a/gdb/displaced-stepping.c +++ b/gdb/displaced-stepping.c @@ -263,23 +263,6 @@ write_memory_ptid (ptid_t ptid, CORE_ADDR memaddr, write_memory (memaddr, myaddr, len); } -static bool -displaced_step_instruction_executed_successfully (gdbarch *arch, - gdb_signal signal) -{ - if (signal != GDB_SIGNAL_TRAP) - return false; - - if (target_stopped_by_watchpoint ()) - { - if (gdbarch_have_nonsteppable_watchpoint (arch) - || target_have_steppable_watchpoint ()) - return false; - } - - return true; -} - displaced_step_finish_status displaced_step_buffers::finish (gdbarch *arch, thread_info *thread, gdb_signal sig) @@ -327,25 +310,13 @@ displaced_step_buffers::finish (gdbarch *arch, thread_info *thread, regcache *rc = get_thread_regcache (thread); - bool instruction_executed_successfully - = displaced_step_instruction_executed_successfully (arch, sig); + CORE_ADDR pc = regcache_read_pc (rc); + gdbarch_displaced_step_fixup (arch, copy_insn_closure.get (), rc, + buffer->original_pc, buffer->addr, pc); - if (instruction_executed_successfully) - { - gdbarch_displaced_step_fixup (arch, copy_insn_closure.get (), - buffer->original_pc, - buffer->addr, rc); - return DISPLACED_STEP_FINISH_STATUS_OK; - } - else - { - /* Since the instruction didn't complete, all we can do is relocate the - PC. */ - CORE_ADDR pc = regcache_read_pc (rc); - pc = buffer->original_pc + (pc - buffer->addr); - regcache_write_pc (rc, pc); - return DISPLACED_STEP_FINISH_STATUS_NOT_EXECUTED; - } + return (pc == buffer->addr + ? DISPLACED_STEP_FINISH_STATUS_NOT_EXECUTED + : DISPLACED_STEP_FINISH_STATUS_OK); } const displaced_step_copy_insn_closure * diff --git a/gdb/gdbarch-components.py b/gdb/gdbarch-components.py index 06be4b57051..bb36220774b 100644 --- a/gdb/gdbarch-components.py +++ b/gdb/gdbarch-components.py @@ -1826,9 +1826,9 @@ gdbarch_software_single_step routine, and true otherwise. Method( comment=""" -Fix up the state resulting from successfully single-stepping a -displaced instruction, to give the result we would have gotten from -stepping the instruction in its original location. +Fix up the state after attempting to single-step a displaced +instruction, to give the result we would have gotten from stepping the +instruction in its original location. REGS is the register state resulting from single-stepping the displaced instruction. @@ -1836,20 +1836,28 @@ displaced instruction. CLOSURE is the result from the matching call to gdbarch_displaced_step_copy_insn. -If you provide gdbarch_displaced_step_copy_insn.but not this -function, then GDB assumes that no fixup is needed after -single-stepping the instruction. +FROM is the address where the instruction was original located, TO is +the address of the displaced buffer where the instruction was copied +to for stepping, and PC is the address at which the inferior stopped +after stepping. For a general explanation of displaced stepping and how GDB uses it, see the comments in infrun.c. + +This function will be called both when the single-step succeeded, and +in the case where the single-step didn't succeed, for example, if the +inferior was interrupted by a signal. Within the function it is +possible to use PC and TO to determine if the instruction was stepped +or not. """, type="void", name="displaced_step_fixup", params=[ ("struct displaced_step_copy_insn_closure *", "closure"), + ("struct regcache *", "regs"), ("CORE_ADDR", "from"), ("CORE_ADDR", "to"), - ("struct regcache *", "regs"), + ("CORE_ADDR", "pc"), ], predicate=False, predefault="NULL", diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h index ac63e84b191..e5d33924e90 100644 --- a/gdb/gdbarch-gen.h +++ b/gdb/gdbarch-gen.h @@ -1068,9 +1068,9 @@ typedef bool (gdbarch_displaced_step_hw_singlestep_ftype) (struct gdbarch *gdbar extern bool gdbarch_displaced_step_hw_singlestep (struct gdbarch *gdbarch); extern void set_gdbarch_displaced_step_hw_singlestep (struct gdbarch *gdbarch, gdbarch_displaced_step_hw_singlestep_ftype *displaced_step_hw_singlestep); -/* Fix up the state resulting from successfully single-stepping a - displaced instruction, to give the result we would have gotten from - stepping the instruction in its original location. +/* Fix up the state after attempting to single-step a displaced + instruction, to give the result we would have gotten from stepping the + instruction in its original location. REGS is the register state resulting from single-stepping the displaced instruction. @@ -1078,15 +1078,22 @@ extern void set_gdbarch_displaced_step_hw_singlestep (struct gdbarch *gdbarch, g CLOSURE is the result from the matching call to gdbarch_displaced_step_copy_insn. - If you provide gdbarch_displaced_step_copy_insn.but not this - function, then GDB assumes that no fixup is needed after - single-stepping the instruction. + FROM is the address where the instruction was original located, TO is + the address of the displaced buffer where the instruction was copied + to for stepping, and PC is the address at which the inferior stopped + after stepping. For a general explanation of displaced stepping and how GDB uses it, - see the comments in infrun.c. */ + see the comments in infrun.c. + + This function will be called both when the single-step succeeded, and + in the case where the single-step didn't succeed, for example, if the + inferior was interrupted by a signal. Within the function it is + possible to use PC and TO to determine if the instruction was stepped + or not. */ -typedef void (gdbarch_displaced_step_fixup_ftype) (struct gdbarch *gdbarch, struct displaced_step_copy_insn_closure *closure, CORE_ADDR from, CORE_ADDR to, struct regcache *regs); -extern void gdbarch_displaced_step_fixup (struct gdbarch *gdbarch, struct displaced_step_copy_insn_closure *closure, CORE_ADDR from, CORE_ADDR to, struct regcache *regs); +typedef void (gdbarch_displaced_step_fixup_ftype) (struct gdbarch *gdbarch, struct displaced_step_copy_insn_closure *closure, struct regcache *regs, CORE_ADDR from, CORE_ADDR to, CORE_ADDR pc); +extern void gdbarch_displaced_step_fixup (struct gdbarch *gdbarch, struct displaced_step_copy_insn_closure *closure, struct regcache *regs, CORE_ADDR from, CORE_ADDR to, CORE_ADDR pc); extern void set_gdbarch_displaced_step_fixup (struct gdbarch *gdbarch, gdbarch_displaced_step_fixup_ftype *displaced_step_fixup); /* Prepare THREAD for it to displaced step the instruction at its current PC. diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c index 4958d3c5fc6..c1cbf1cbb74 100644 --- a/gdb/gdbarch.c +++ b/gdb/gdbarch.c @@ -4045,13 +4045,13 @@ set_gdbarch_displaced_step_hw_singlestep (struct gdbarch *gdbarch, } void -gdbarch_displaced_step_fixup (struct gdbarch *gdbarch, struct displaced_step_copy_insn_closure *closure, CORE_ADDR from, CORE_ADDR to, struct regcache *regs) +gdbarch_displaced_step_fixup (struct gdbarch *gdbarch, struct displaced_step_copy_insn_closure *closure, struct regcache *regs, CORE_ADDR from, CORE_ADDR to, CORE_ADDR pc) { gdb_assert (gdbarch != NULL); gdb_assert (gdbarch->displaced_step_fixup != NULL); if (gdbarch_debug >= 2) gdb_printf (gdb_stdlog, "gdbarch_displaced_step_fixup called\n"); - gdbarch->displaced_step_fixup (gdbarch, closure, from, to, regs); + gdbarch->displaced_step_fixup (gdbarch, closure, regs, from, to, pc); } void diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c index 476d17f1efd..2caf24daac2 100644 --- a/gdb/i386-tdep.c +++ b/gdb/i386-tdep.c @@ -842,9 +842,11 @@ i386_displaced_step_copy_insn (struct gdbarch *gdbarch, void i386_displaced_step_fixup (struct gdbarch *gdbarch, struct displaced_step_copy_insn_closure *closure_, - CORE_ADDR from, CORE_ADDR to, - struct regcache *regs) + struct regcache *regs, + CORE_ADDR from, CORE_ADDR to, CORE_ADDR pc) { + bool displaced_step_completed = pc != to; + enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); /* The offset we applied to the instruction's address. @@ -886,15 +888,13 @@ i386_displaced_step_fixup (struct gdbarch *gdbarch, the displaced instruction; make it relative. Well, signal handler returns don't need relocation either, but we use the value of %eip to recognize those; see below. */ - if (! i386_absolute_jmp_p (insn) - && ! i386_absolute_call_p (insn) - && ! i386_ret_p (insn)) + if (!displaced_step_completed + || (!i386_absolute_jmp_p (insn) + && !i386_absolute_call_p (insn) + && !i386_ret_p (insn))) { - ULONGEST orig_eip; int insn_len; - regcache_cooked_read_unsigned (regs, I386_EIP_REGNUM, &orig_eip); - /* A signal trampoline system call changes the %eip, resuming execution of the main program after the signal handler has returned. That makes them like 'return' instructions; we @@ -910,25 +910,25 @@ i386_displaced_step_fixup (struct gdbarch *gdbarch, it unrelocated. Goodness help us if there are PC-relative system calls. */ if (i386_syscall_p (insn, &insn_len) - && orig_eip != to + (insn - insn_start) + insn_len + && pc != to + (insn - insn_start) + insn_len /* GDB can get control back after the insn after the syscall. Presumably this is a kernel bug. i386_displaced_step_copy_insn ensures its a nop, we add one to the length for it. */ - && orig_eip != to + (insn - insn_start) + insn_len + 1) + && pc != to + (insn - insn_start) + insn_len + 1) displaced_debug_printf ("syscall changed %%eip; not relocating"); else { - ULONGEST eip = (orig_eip - insn_offset) & 0xffffffffUL; + ULONGEST eip = (pc - insn_offset) & 0xffffffffUL; /* If we just stepped over a breakpoint insn, we don't backup the pc on purpose; this is to match behaviour without stepping. */ - regcache_cooked_write_unsigned (regs, I386_EIP_REGNUM, eip); + regcache_write_pc (regs, eip); displaced_debug_printf ("relocated %%eip from %s to %s", - paddress (gdbarch, orig_eip), + paddress (gdbarch, pc), paddress (gdbarch, eip)); } } @@ -941,7 +941,7 @@ i386_displaced_step_fixup (struct gdbarch *gdbarch, /* If the instruction was a call, the return address now atop the stack is the address following the copied instruction. We need to make it the address following the original instruction. */ - if (i386_call_p (insn)) + if (displaced_step_completed && i386_call_p (insn)) { ULONGEST esp; ULONGEST retaddr; diff --git a/gdb/i386-tdep.h b/gdb/i386-tdep.h index 371bce72369..1244b098a21 100644 --- a/gdb/i386-tdep.h +++ b/gdb/i386-tdep.h @@ -448,7 +448,7 @@ extern displaced_step_copy_insn_closure_up i386_displaced_step_copy_insn struct regcache *regs); extern void i386_displaced_step_fixup (struct gdbarch *gdbarch, displaced_step_copy_insn_closure *closure, - CORE_ADDR from, CORE_ADDR to, regcache *regs); + regcache *regs, CORE_ADDR from, CORE_ADDR to, CORE_ADDR pc); /* Initialize a basic ELF architecture variant. */ extern void i386_elf_init_abi (struct gdbarch_info, struct gdbarch *); diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c index 592b447948e..560955644aa 100644 --- a/gdb/rs6000-tdep.c +++ b/gdb/rs6000-tdep.c @@ -950,9 +950,20 @@ ppc_displaced_step_copy_insn (struct gdbarch *gdbarch, static void ppc_displaced_step_fixup (struct gdbarch *gdbarch, struct displaced_step_copy_insn_closure *closure_, - CORE_ADDR from, CORE_ADDR to, - struct regcache *regs) + struct regcache *regs, + CORE_ADDR from, CORE_ADDR to, CORE_ADDR pc) { + /* If the PC at which the inferior stopped is still the TO address then + the displaced instruction was never executed. All we need to do in + this case is restore the program-counter to its original location and + we're done. */ + if (pc == to) + { + pc = from + (pc - to); + regcache_write_pc (regs, pc); + return; + } + enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); /* Our closure is a copy of the instruction. */ ppc_displaced_step_copy_insn_closure *closure diff --git a/gdb/s390-tdep.c b/gdb/s390-tdep.c index cab1757c5ab..f1ed69ee0a4 100644 --- a/gdb/s390-tdep.c +++ b/gdb/s390-tdep.c @@ -481,9 +481,20 @@ s390_displaced_step_copy_insn (struct gdbarch *gdbarch, static void s390_displaced_step_fixup (struct gdbarch *gdbarch, displaced_step_copy_insn_closure *closure_, - CORE_ADDR from, CORE_ADDR to, - struct regcache *regs) + struct regcache *regs, + CORE_ADDR from, CORE_ADDR to, CORE_ADDR pc) { + /* If the PC at which the inferior stopped is still the TO address then + the displaced instruction was never executed. All we need to do in + this case is restore the program-counter to its original location and + we're done. */ + if (pc == to) + { + pc = from + (pc - to); + regcache_write_pc (regs, pc); + return; + } + /* Our closure is a copy of the instruction. */ s390_displaced_step_copy_insn_closure *closure = (s390_displaced_step_copy_insn_closure *) closure_; @@ -496,7 +507,6 @@ s390_displaced_step_fixup (struct gdbarch *gdbarch, int i2, d2; /* Get current PC and addressing mode bit. */ - CORE_ADDR pc = regcache_read_pc (regs); ULONGEST amode = 0; if (register_size (gdbarch, S390_PSWA_REGNUM) == 4) diff --git a/gdb/testsuite/gdb.arch/amd64-disp-step-signal.c b/gdb/testsuite/gdb.arch/amd64-disp-step-signal.c new file mode 100644 index 00000000000..c968146624a --- /dev/null +++ b/gdb/testsuite/gdb.arch/amd64-disp-step-signal.c @@ -0,0 +1,30 @@ +/* This file is part of GDB, the GNU debugger. + + Copyright 2023 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see . */ + +#include +#include + +static void +sigalrm_handler (int sig) +{ +} + +void +setup_signal_handler () +{ + signal(SIGALRM, sigalrm_handler); +} diff --git a/gdb/testsuite/gdb.arch/amd64-disp-step.S b/gdb/testsuite/gdb.arch/amd64-disp-step.S index b25e292bdf0..bf73778cf43 100644 --- a/gdb/testsuite/gdb.arch/amd64-disp-step.S +++ b/gdb/testsuite/gdb.arch/amd64-disp-step.S @@ -23,6 +23,10 @@ main: nop + callq setup_signal_handler + + nop + /***********************************************/ /* test call/ret */ @@ -135,6 +139,14 @@ test_rip_rdi: test_rip_rdi_end: nop + .global test_jmp +test_jmp: + jmpq *jmp_dest(%rip) + nop + .global test_jmp_end +test_jmp_end: + nop + /* skip over test data */ jmp done @@ -142,6 +154,9 @@ test_rip_rdi_end: answer: .8byte 42 +jmp_dest: + .8byte test_jmp_end + /***********************************************/ /* all done */ diff --git a/gdb/testsuite/gdb.arch/amd64-disp-step.exp b/gdb/testsuite/gdb.arch/amd64-disp-step.exp index 2aee1e05774..09ba56e490e 100644 --- a/gdb/testsuite/gdb.arch/amd64-disp-step.exp +++ b/gdb/testsuite/gdb.arch/amd64-disp-step.exp @@ -17,15 +17,16 @@ # Test amd64 displaced stepping. +load_lib gdb-python.exp require is_x86_64_m64_target set newline "\[\r\n\]*" set opts {debug nopie} -standard_testfile .S +standard_testfile .S -signal.c -if { [prepare_for_testing "failed to prepare" $testfile $srcfile $opts] } { +if { [prepare_for_testing "failed to prepare" $testfile "$srcfile $srcfile2" $opts] } { return -1 } @@ -154,9 +155,13 @@ proc set_regs { regs val } { } } -# Verify all REGS equal VAL, except REG which equals REG_VAL. +# Verify all REGS equal VAL, except EXCEPT_REG which equals +# EXCEPT_REG_VAL. +# +# It is fine for EXCEPT_REG to be the empty string, in which case no +# register will be checked for EXCEPT_REG_VAL. -proc verify_regs { test_name regs val except_reg except_reg_val } { +proc_with_prefix verify_regs { regs val except_reg except_reg_val } { global newline foreach reg ${regs} { @@ -165,36 +170,101 @@ proc verify_regs { test_name regs val except_reg except_reg_val } { set expected ${except_reg_val} } # The cast to (int) is because RBP is printed as a pointer. - gdb_test "p (int) \$${reg}" " = ${expected}${newline}" "${test_name} ${reg} expected value" + gdb_test "p (int) \$${reg}" " = ${expected}${newline}" "${reg} expected value" } } -proc rip_test { reg } { +# Run the rip-relative tests. +# +# TEST_START_LABEL and TEST_END_LABEL are two labels that delimit the +# test in the srcfile. +# +# REG is either the name of a register which is the destiation +# location (when testing the add instruction), otherwise REG should be +# the empty string, when testing the 'jmpq*' instruction. +# +# SIGNAL_MODES is a list which always contains 'off' and optionally +# might also contain 'on'. The 'on' value is only included if GDB +# supports Python. The test is repeated for each signal mode. With +# signal mode 'on' GDB uses Python to have a signal sent to the +# inferior. +proc rip_test { reg test_start_label test_end_label signal_modes } { global srcfile rip_regs - set test_start_label "test_rip_${reg}" - set test_end_label "test_rip_${reg}_end" - gdb_test "break ${test_start_label}" \ "Breakpoint.*at.* file .*$srcfile, line.*" gdb_test "break ${test_end_label}" \ "Breakpoint.*at.* file .*$srcfile, line.*" - gdb_test "continue" \ - "Continuing.*Breakpoint.*, ${test_start_label} ().*" \ - "continue to ${test_start_label}" + foreach_with_prefix send_signal $signal_modes { + if {$send_signal eq [lindex $signal_modes 0]} { + # The first time through we can just continue to the + # breakpoint. + gdb_test "continue" \ + "Continuing.*Breakpoint.*, ${test_start_label} ().*" \ + "continue to ${test_start_label}" + } else { + # For the second time through the test we need to jump + # back to the beginning. + gdb_test "jump ${test_start_label}" \ + "Breakpoint.*, ${test_start_label} ().*" \ + "jump back to ${test_start_label}" + } + + set_regs ${rip_regs} 0 - set_regs ${rip_regs} 0 + if {$send_signal} { + gdb_test_no_output "python signal_inferior()" \ + "send signal" + } + + gdb_test "continue" \ + "Continuing.*Breakpoint.*, ${test_end_label} ().*" \ + "continue to ${test_end_label}" - gdb_test "continue" \ - "Continuing.*Breakpoint.*, ${test_end_label} ().*" \ - "continue to ${test_end_label}" + verify_regs ${rip_regs} 0 ${reg} 42 + } +} - verify_regs "test rip w/${reg}" ${rip_regs} 0 ${reg} 42 +if {[allow_python_tests] && ![is_remote target]} { + # The signal sending tests require that the signal appear to + # arrive from an outside source, i.e. we can't use GDB's 'signal' + # command to deliver it. + # + # The signal must arrive while GDB is processing the displaced + # step instruction. + # + # If we use 'signal' to send the signal GDB doesn't actually do + # the displaced step, but instead just delivers the signal. + # + # By having Python ask the OS to deliver us a signal we will + # (hopefully) see the signal while processing the displaced step + # instruction. + # + # Obviously non of this will work if the target is remote. + gdb_test_multiline "Create function to send SIGALRM" \ + "python" "" \ + "import os, signal" "" \ + "def signal_inferior():" "" \ + " os.kill(gdb.selected_inferior().pid, signal.SIGALRM)" "" \ + "end" "" + + set signal_modes { off on } +} else { + set signal_modes { off } } +# The the rip-relative add instructions. There's a test writing to +# each register in RIP_REGS in turn. foreach reg ${rip_regs} { - rip_test $reg + with_test_prefix "add into ${reg}" { + rip_test $reg "test_rip_${reg}" "test_rip_${reg}_end" $signal_modes + } +} + +# Now test the rip-relative 'jmpq*' instruction. +with_test_prefix "rip-relative jmpq*" { + rip_test "" "test_jmp" "test_jmp_end" $signal_modes } ##########################################