From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20407 invoked by alias); 27 Sep 2009 21:47:25 -0000 Received: (qmail 20390 invoked by uid 22791); 27 Sep 2009 21:47:23 -0000 X-SWARE-Spam-Status: No, hits=-1.4 required=5.0 tests=AWL,BAYES_00,HK_OBFDOM,MSGID_FROM_MTA_HEADER,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mtagate4.de.ibm.com (HELO mtagate4.de.ibm.com) (195.212.17.164) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 27 Sep 2009 21:47:18 +0000 Received: from d12nrmr1607.megacenter.de.ibm.com (d12nrmr1607.megacenter.de.ibm.com [9.149.167.49]) by mtagate4.de.ibm.com (8.13.1/8.13.1) with ESMTP id n8RLlECu000552 for ; Sun, 27 Sep 2009 21:47:14 GMT Received: from d12av02.megacenter.de.ibm.com (d12av02.megacenter.de.ibm.com [9.149.165.228]) by d12nrmr1607.megacenter.de.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id n8RLlEIT3280944 for ; Sun, 27 Sep 2009 23:47:14 +0200 Received: from d12av02.megacenter.de.ibm.com (loopback [127.0.0.1]) by d12av02.megacenter.de.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id n8RLlEHW031816 for ; Sun, 27 Sep 2009 23:47:14 +0200 Received: from tuxmaker.boeblingen.de.ibm.com (tuxmaker.boeblingen.de.ibm.com [9.152.85.9]) by d12av02.megacenter.de.ibm.com (8.12.11.20060308/8.12.11) with SMTP id n8RLlDCU031811; Sun, 27 Sep 2009 23:47:13 +0200 Message-Id: <200909272147.n8RLlDCU031811@d12av02.megacenter.de.ibm.com> Received: by tuxmaker.boeblingen.de.ibm.com (sSMTP sendmail emulation); Sun, 27 Sep 2009 23:47:13 +0200 Subject: [rfc] Fix PowerPC displaced stepping regression To: gdb-patches@sourceware.org Date: Sun, 27 Sep 2009 21:47:00 -0000 From: "Ulrich Weigand" Cc: julian@codesourcery.com (Julian Brown), pedro@codesourcery.com (Pedro Alves), drow@false.org (Daniel Jacobowitz) In-Reply-To: <200909241935.n8OJZ6dR028352@d12av02.megacenter.de.ibm.com> from "Ulrich Weigand" at Sep 24, 2009 09:35:06 PM MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit 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 X-SW-Source: 2009-09/txt/msg00855.txt.bz2 I wrote: > It seems this change broke displaced stepping on PowerPC. > > I'm not sure I understand the rationale behind these changes to the > displaced stepping logic in infrun.c in the first place. Why is > everything conditioned on gdbarch_software_single_step_p, which just > says whether or not the architecture has installed a single-stepping > routine -- but this alone doesn't say whether software stepping is > actually needed in any given situation ... OK, it seems there are two separate changes: - In non-stop mode, we never want to use software single-step as common code does not support this in multiple threads at once. - On platforms with no hardware single-step available, GDB common code should not use "step" but "continue" to run displaced copies. The first change does make sense, also on PowerPC. It is in fact the second change that is problematic, as it would force PowerPC to implement a much more complex displaced stepping logic just to avoid using hardware single-stepping the displaced copies .. which there is no need for in the first place. The following patch keeps the first change, but makes the second change conditional on a new gdbarch callback instead of simply checking for gdbarch_software_single_step_p. This allows PowerPC to say that even though it has installed a SW single-step routine to handle some specific corner cases, it still wants to use HW stepping for displaced copies. The default is such that everything should be unchanged for the ARM case. Tested on s390(x)-linux and ppc(64)-linux with no regressions, fixes all non-stop related test case failures. Does this look reasonable? Bye, Ulrich ChangeLog: * gdbarch.sh (displaced_step_hw_singlestep): New callback. * gdbarch.c, gdbarch.h: Regenerate. * arch-utils.c (default_displaced_step_hw_singlestep): New function. * arch-utils.h (default_displaced_step_hw_singlestep): Add prototype. * ppc-linux-tdep.c (ppc_displaced_step_hw_singlestep): New function. (rs6000_gdbarch_init): Install it. * infrun.c (displaced_step_fixup): Use new callback to determine whether to "step" or "continue" displaced copy. (resume): Likewise. Do not call maybe_software_singlestep for displaced stepping. (maybe_software_singlestep): Do not handle displaced stepping. Index: gdb/arch-utils.c =================================================================== RCS file: /cvs/src/src/gdb/arch-utils.c,v retrieving revision 1.182 diff -c -p -r1.182 arch-utils.c *** gdb/arch-utils.c 31 Jul 2009 14:39:11 -0000 1.182 --- gdb/arch-utils.c 27 Sep 2009 21:06:05 -0000 *************** simple_displaced_step_free_closure (stru *** 67,72 **** --- 67,78 ---- xfree (closure); } + int + default_displaced_step_hw_singlestep (struct gdbarch *gdbarch, + struct displaced_step_closure *closure) + { + return !gdbarch_software_single_step_p (gdbarch); + } CORE_ADDR displaced_step_at_entry_point (struct gdbarch *gdbarch) Index: gdb/arch-utils.h =================================================================== RCS file: /cvs/src/src/gdb/arch-utils.h,v retrieving revision 1.104 diff -c -p -r1.104 arch-utils.h *** gdb/arch-utils.h 2 Jul 2009 17:25:52 -0000 1.104 --- gdb/arch-utils.h 27 Sep 2009 21:06:05 -0000 *************** extern void *** 49,54 **** --- 49,59 ---- simple_displaced_step_free_closure (struct gdbarch *gdbarch, struct displaced_step_closure *closure); + /* Default implementation of gdbarch_displaced_hw_singlestep. */ + extern int + default_displaced_step_hw_singlestep (struct gdbarch *gdbarch, + struct displaced_step_closure *closure); + /* Possible value for gdbarch_displaced_step_location: Place displaced instructions at the program's entry point, leaving space for inferior function call return breakpoints. */ Index: gdb/gdbarch.c =================================================================== RCS file: /cvs/src/src/gdb/gdbarch.c,v retrieving revision 1.454 diff -c -p -r1.454 gdbarch.c *** gdb/gdbarch.c 21 Sep 2009 05:52:05 -0000 1.454 --- gdb/gdbarch.c 27 Sep 2009 21:06:06 -0000 *************** struct gdbarch *** 232,237 **** --- 232,238 ---- gdbarch_skip_permanent_breakpoint_ftype *skip_permanent_breakpoint; ULONGEST max_insn_length; gdbarch_displaced_step_copy_insn_ftype *displaced_step_copy_insn; + gdbarch_displaced_step_hw_singlestep_ftype *displaced_step_hw_singlestep; gdbarch_displaced_step_fixup_ftype *displaced_step_fixup; gdbarch_displaced_step_free_closure_ftype *displaced_step_free_closure; gdbarch_displaced_step_location_ftype *displaced_step_location; *************** struct gdbarch startup_gdbarch = *** 371,376 **** --- 372,378 ---- 0, /* skip_permanent_breakpoint */ 0, /* max_insn_length */ 0, /* displaced_step_copy_insn */ + default_displaced_step_hw_singlestep, /* displaced_step_hw_singlestep */ 0, /* displaced_step_fixup */ NULL, /* displaced_step_free_closure */ NULL, /* displaced_step_location */ *************** gdbarch_alloc (const struct gdbarch_info *** 464,469 **** --- 466,472 ---- gdbarch->elf_make_msymbol_special = default_elf_make_msymbol_special; gdbarch->coff_make_msymbol_special = default_coff_make_msymbol_special; gdbarch->register_reggroup_p = default_register_reggroup_p; + gdbarch->displaced_step_hw_singlestep = default_displaced_step_hw_singlestep; gdbarch->displaced_step_fixup = NULL; gdbarch->displaced_step_free_closure = NULL; gdbarch->displaced_step_location = NULL; *************** verify_gdbarch (struct gdbarch *gdbarch) *** 627,632 **** --- 630,636 ---- /* Skip verify of skip_permanent_breakpoint, has predicate */ /* Skip verify of max_insn_length, has predicate */ /* Skip verify of displaced_step_copy_insn, has predicate */ + /* Skip verify of displaced_step_hw_singlestep, invalid_p == 0 */ /* Skip verify of displaced_step_fixup, has predicate */ if ((! gdbarch->displaced_step_free_closure) != (! gdbarch->displaced_step_copy_insn)) fprintf_unfiltered (log, "\n\tdisplaced_step_free_closure"); *************** gdbarch_dump (struct gdbarch *gdbarch, s *** 791,796 **** --- 795,803 ---- "gdbarch_dump: displaced_step_free_closure = <%s>\n", host_address_to_string (gdbarch->displaced_step_free_closure)); fprintf_unfiltered (file, + "gdbarch_dump: displaced_step_hw_singlestep = <%s>\n", + host_address_to_string (gdbarch->displaced_step_hw_singlestep)); + fprintf_unfiltered (file, "gdbarch_dump: displaced_step_location = <%s>\n", host_address_to_string (gdbarch->displaced_step_location)); fprintf_unfiltered (file, *************** set_gdbarch_displaced_step_copy_insn (st *** 3145,3150 **** --- 3152,3174 ---- } int + gdbarch_displaced_step_hw_singlestep (struct gdbarch *gdbarch, struct displaced_step_closure *closure) + { + gdb_assert (gdbarch != NULL); + gdb_assert (gdbarch->displaced_step_hw_singlestep != NULL); + if (gdbarch_debug >= 2) + fprintf_unfiltered (gdb_stdlog, "gdbarch_displaced_step_hw_singlestep called\n"); + return gdbarch->displaced_step_hw_singlestep (gdbarch, closure); + } + + void + set_gdbarch_displaced_step_hw_singlestep (struct gdbarch *gdbarch, + gdbarch_displaced_step_hw_singlestep_ftype displaced_step_hw_singlestep) + { + gdbarch->displaced_step_hw_singlestep = displaced_step_hw_singlestep; + } + + int gdbarch_displaced_step_fixup_p (struct gdbarch *gdbarch) { gdb_assert (gdbarch != NULL); Index: gdb/gdbarch.h =================================================================== RCS file: /cvs/src/src/gdb/gdbarch.h,v retrieving revision 1.404 diff -c -p -r1.404 gdbarch.h *** gdb/gdbarch.h 21 Sep 2009 05:52:06 -0000 1.404 --- gdb/gdbarch.h 27 Sep 2009 21:06:06 -0000 *************** typedef struct displaced_step_closure * *** 734,739 **** --- 734,753 ---- extern struct displaced_step_closure * gdbarch_displaced_step_copy_insn (struct gdbarch *gdbarch, CORE_ADDR from, CORE_ADDR to, struct regcache *regs); extern void set_gdbarch_displaced_step_copy_insn (struct gdbarch *gdbarch, gdbarch_displaced_step_copy_insn_ftype *displaced_step_copy_insn); + /* Return true if GDB should use hardware single-stepping to execute + the the displaced instruction identified by CLOSURE. If false, + GDB will simply restart execution at the displaced instruction + location, and it is up to the target to ensure GDB will receive + control again (e.g. by placing a software breakpoint instruction + into the displaced instruction buffer). + + The default implementation returns false on all targets that + provide a gdbarch_software_single_step routine, and true otherwise. */ + + typedef int (gdbarch_displaced_step_hw_singlestep_ftype) (struct gdbarch *gdbarch, struct displaced_step_closure *closure); + extern int gdbarch_displaced_step_hw_singlestep (struct gdbarch *gdbarch, struct displaced_step_closure *closure); + 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. Index: gdb/gdbarch.sh =================================================================== RCS file: /cvs/src/src/gdb/gdbarch.sh,v retrieving revision 1.497 diff -c -p -r1.497 gdbarch.sh *** gdb/gdbarch.sh 21 Sep 2009 05:52:05 -0000 1.497 --- gdb/gdbarch.sh 27 Sep 2009 21:06:07 -0000 *************** V:ULONGEST:max_insn_length:::0:0 *** 654,659 **** --- 654,670 ---- # here. M:struct displaced_step_closure *:displaced_step_copy_insn:CORE_ADDR from, CORE_ADDR to, struct regcache *regs:from, to, regs + # Return true if GDB should use hardware single-stepping to execute + # the the displaced instruction identified by CLOSURE. If false, + # GDB will simply restart execution at the displaced instruction + # location, and it is up to the target to ensure GDB will receive + # control again (e.g. by placing a software breakpoint instruction + # into the displaced instruction buffer). + # + # The default implementation returns false on all targets that + # provide a gdbarch_software_single_step routine, and true otherwise. + m:int:displaced_step_hw_singlestep:struct displaced_step_closure *closure:closure::default_displaced_step_hw_singlestep::0 + # 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. Index: gdb/infrun.c =================================================================== RCS file: /cvs/src/src/gdb/infrun.c,v retrieving revision 1.409 diff -c -p -r1.409 infrun.c *** gdb/infrun.c 15 Sep 2009 03:30:06 -0000 1.409 --- gdb/infrun.c 27 Sep 2009 21:06:07 -0000 *************** displaced_step_fixup (ptid_t event_ptid, *** 1002,1011 **** displaced_step_dump_bytes (gdb_stdlog, buf, sizeof (buf)); } ! if (gdbarch_software_single_step_p (gdbarch)) ! target_resume (ptid, 0, TARGET_SIGNAL_0); ! else target_resume (ptid, 1, TARGET_SIGNAL_0); /* Done, we're stepping a thread. */ break; --- 1002,1012 ---- displaced_step_dump_bytes (gdb_stdlog, buf, sizeof (buf)); } ! if (gdbarch_displaced_step_hw_singlestep ! (gdbarch, displaced_step_closure)) target_resume (ptid, 1, TARGET_SIGNAL_0); + else + target_resume (ptid, 0, TARGET_SIGNAL_0); /* Done, we're stepping a thread. */ break; *************** maybe_software_singlestep (struct gdbarc *** 1114,1132 **** { int hw_step = 1; ! if (gdbarch_software_single_step_p (gdbarch)) { ! if (use_displaced_stepping (gdbarch)) ! hw_step = 0; ! else if (gdbarch_software_single_step (gdbarch, get_current_frame ())) ! { ! hw_step = 0; ! /* Do not pull these breakpoints until after a `wait' in ! `wait_for_inferior' */ ! singlestep_breakpoints_inserted_p = 1; ! singlestep_ptid = inferior_ptid; ! singlestep_pc = pc; ! } } return hw_step; } --- 1115,1129 ---- { int hw_step = 1; ! if (gdbarch_software_single_step_p (gdbarch) ! && gdbarch_software_single_step (gdbarch, get_current_frame ())) { ! hw_step = 0; ! /* Do not pull these breakpoints until after a `wait' in ! `wait_for_inferior' */ ! singlestep_breakpoints_inserted_p = 1; ! singlestep_ptid = inferior_ptid; ! singlestep_pc = pc; } return hw_step; } *************** a command like `return' or `jump' to con *** 1208,1217 **** discard_cleanups (old_cleanups); return; } } /* Do we need to do it the hard way, w/temp breakpoints? */ ! if (step) step = maybe_software_singlestep (gdbarch, pc); if (should_resume) --- 1205,1217 ---- discard_cleanups (old_cleanups); return; } + + step = gdbarch_displaced_step_hw_singlestep + (gdbarch, displaced_step_closure); } /* Do we need to do it the hard way, w/temp breakpoints? */ ! else if (step) step = maybe_software_singlestep (gdbarch, pc); if (should_resume) Index: gdb/rs6000-tdep.c =================================================================== RCS file: /cvs/src/src/gdb/rs6000-tdep.c,v retrieving revision 1.337 diff -c -p -r1.337 rs6000-tdep.c *** gdb/rs6000-tdep.c 18 Sep 2009 15:48:23 -0000 1.337 --- gdb/rs6000-tdep.c 27 Sep 2009 21:06:08 -0000 *************** ppc_displaced_step_fixup (struct gdbarch *** 1058,1063 **** --- 1058,1072 ---- from + offset); } + /* Always use hardware single-stepping to execute the + displaced instruction. */ + static int + ppc_displaced_step_hw_singlestep (struct gdbarch *gdbarch, + struct displaced_step_closure *closure) + { + return 1; + } + /* Instruction masks used during single-stepping of atomic sequences. */ #define LWARX_MASK 0xfc0007fe #define LWARX_INSTRUCTION 0x7c000028 *************** rs6000_gdbarch_init (struct gdbarch_info *** 3898,3903 **** --- 3907,3914 ---- /* Setup displaced stepping. */ set_gdbarch_displaced_step_copy_insn (gdbarch, simple_displaced_step_copy_insn); + set_gdbarch_displaced_step_hw_singlestep (gdbarch, + ppc_displaced_step_hw_singlestep); set_gdbarch_displaced_step_fixup (gdbarch, ppc_displaced_step_fixup); set_gdbarch_displaced_step_free_closure (gdbarch, simple_displaced_step_free_closure); -- Dr. Ulrich Weigand GNU Toolchain for Linux on System z and Cell BE Ulrich.Weigand@de.ibm.com