This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
[rfc] Fix PowerPC displaced stepping regression
- From: "Ulrich Weigand" <uweigand at de dot ibm dot com>
- To: gdb-patches at sourceware dot org
- Cc: julian at codesourcery dot com (Julian Brown), pedro at codesourcery dot com (Pedro Alves), drow at false dot org (Daniel Jacobowitz)
- Date: Sun, 27 Sep 2009 23:47:13 +0200 (CEST)
- Subject: [rfc] Fix PowerPC displaced stepping regression
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