Re: [patch] "single step" atomic instruction sequences as a whole.

Emi SUZUKI
2007-03-08T08:50:12Z

[-- Attachment #1: Type: Text/Plain, Size: 1893 bytes --]

Andreas and Daniel,

Thank you for the comments.

From: Daniel Jacobowitz <drow at false.org>
Subject: Re: [patch] "single step" atomic instruction sequences as a whole.
Date: Tue, 06 Mar 2007 07:24:36 -0500

> On Tue, Mar 06, 2007 at 12:00:21PM +0100, Andreas Schwab wrote:
> > > + /* Assume that the atomic sequence ends with a stwcx instruction
> > > + followed by a conditional branch instruction. */
> > > + if ((insn & STWCX_MASK) != STWCX_INSTRUCTION)
> > > + error (_("Tried to step over an atomic sequence of instructions but could not find the end of the sequence."));
> >
> > I don't think error should be called here. It would probably be better to
> > just continue with the normal single-step here.
>
> Maybe a (once-only) warning? It would be nice to let the user know
> we're confused.

That's the last thing I have wondered what to do with respect to
Paul's patch.
Maybe any errors should not be called in that function, because the
function is just for checking if there is any sequence of instructions
that should be avoided running through. I have no idea about the
warning, though, it might be a help for debugging some of complecated
issues.

Anyway, I found that calling warning instead of error, and returning -1
(indecates that any atomic sequences are not found) after warning
fulfills both suggestions. I have done with the attached.

Meanwhile the things about RS6000-AIX came to me: it does not support
hardware single stepping, so SOFTWARE_SINGLE_STEP_P should always
return true. My patch has nothing to concern about it...

I have added a new file, tm-rs6000aix.h, to undef SOFTWARE_SINGLE_STEP_P
for only that target, but felt somewhat strange about the solution.
I feel like adding some trick for SOFTWARE_SINGLE_STEP_P to gdbarch.c
rather than undef'ing it, but no idea has come to mind for now.

My best regards,
--
Emi SUZUKI

[-- Attachment #2: rs6000-atomic-sequence_070308.diff --]
[-- Type: Text/Plain, Size: 10745 bytes --]

diff -ruN -x CVS -x '*~' src/gdb/config/powerpc/aix.mt gdb/gdb/config/powerpc/aix.mt
--- src/gdb/config/powerpc/aix.mt 2006-02-11 05:56:15.000000000 +0900
+++ gdb/gdb/config/powerpc/aix.mt 2007-03-08 16:18:37.000000000 +0900
@@ -1,4 +1,4 @@
# Target: PowerPC running AIX
TDEPFILES= rs6000-tdep.o rs6000-aix-tdep.o \
xcoffread.o ppc-sysv-tdep.o solib.o solib-svr4.o
-DEPRECATED_TM_FILE= config/rs6000/tm-rs6000.h
+DEPRECATED_TM_FILE= config/rs6000/tm-rs6000aix.h
diff -ruN -x CVS -x '*~' src/gdb/config/rs6000/tm-rs6000.h gdb/gdb/config/rs6000/tm-rs6000.h
--- src/gdb/config/rs6000/tm-rs6000.h 2007-03-02 21:42:44.000000000 +0900
+++ gdb/gdb/config/rs6000/tm-rs6000.h 2007-03-06 20:43:40.000000000 +0900
@@ -30,3 +30,9 @@
#define PROCESS_LINENUMBER_HOOK() aix_process_linenos ()
extern void aix_process_linenos (void);

+extern int rs6000_software_single_step_p (void);
+
+#ifdef SOFTWARE_SINGLE_STEP_P
+#undef SOFTWARE_SINGLE_STEP_P
+#endif
+#define SOFTWARE_SINGLE_STEP_P() rs6000_software_single_step_p()
diff -ruN -x CVS -x '*~' src/gdb/config/rs6000/tm-rs6000aix.h gdb/gdb/config/rs6000/tm-rs6000aix.h
--- src/gdb/config/rs6000/tm-rs6000aix.h 1970-01-01 09:00:00.000000000 +0900
+++ gdb/gdb/config/rs6000/tm-rs6000aix.h 2007-03-08 17:24:14.000000000 +0900
@@ -0,0 +1,33 @@
+/* Macro definitions for RS6000 running under AIX.
+ Copyright 2007 Free Software Foundation, Inc.
+
+ This file is part of GDB.
+
+ 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 2 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, write to the Free Software
+ Foundation, Inc., 59 Temple Place - Suite 330,
+ Boston, MA 02111-1307, USA. */
+
+#ifndef TM_RS6000AIX_H
+#define TM_RS6000AIX_H
+
+/* Use generic RS6000 definitions. */
+#include "rs6000/tm-rs6000.h"
+
+/* Almost all OS running on RS6000/PPC supports handling hardware single
+ step, but sometimes use software single step for skipping atomic
+ sequences of instructions. Only RS6000-AIX always use software single
+ step, so we use the default function for it. */
+#undef SOFTWARE_SINGLE_STEP_P
+
+#endif /* TM_RS6000AIX_H */
diff -ruN -x CVS -x '*~' src/gdb/infrun.c gdb/gdb/infrun.c
--- src/gdb/infrun.c 2007-03-02 21:42:43.000000000 +0900
+++ gdb/gdb/infrun.c 2007-03-02 21:28:25.000000000 +0900
@@ -556,15 +556,19 @@
if (breakpoint_here_p (read_pc ()) == permanent_breakpoint_here)
SKIP_PERMANENT_BREAKPOINT ();

- if (SOFTWARE_SINGLE_STEP_P () && step)
+ if (step)
{
- /* Do it the hard way, w/temp breakpoints */
- SOFTWARE_SINGLE_STEP (sig, 1 /*insert-breakpoints */ );
- /* ...and don't ask hardware to do it. */
- step = 0;
- /* and do not pull these breakpoints until after a `wait' in
- `wait_for_inferior' */
- singlestep_breakpoints_inserted_p = 1;
+ if (SOFTWARE_SINGLE_STEP_P ())
+ {
+ /* Do it the hard way, w/temp breakpoints */
+ SOFTWARE_SINGLE_STEP (sig, 1 /*insert-breakpoints */ );
+ /* ...and don't ask hardware to do it. */
+ step = 0;
+ /* and do not pull these breakpoints until after a `wait' in
+ `wait_for_inferior' */
+ singlestep_breakpoints_inserted_p = 1;
+ }
+
singlestep_ptid = inferior_ptid;
singlestep_pc = read_pc ();
}
@@ -1565,8 +1569,6 @@

if (stepping_past_singlestep_breakpoint)
{
- gdb_assert (SOFTWARE_SINGLE_STEP_P ()
- && singlestep_breakpoints_inserted_p);
gdb_assert (ptid_equal (singlestep_ptid, ecs->ptid));
gdb_assert (!ptid_equal (singlestep_ptid, saved_singlestep_ptid));

@@ -1579,9 +1581,13 @@
{
if (debug_infrun)
fprintf_unfiltered (gdb_stdlog, "infrun: stepping_past_singlestep_breakpoint\n");
- /* Pull the single step breakpoints out of the target. */
- SOFTWARE_SINGLE_STEP (0, 0);
- singlestep_breakpoints_inserted_p = 0;
+
+ if (singlestep_breakpoints_inserted_p)
+ {
+ /* Pull the single step breakpoints out of the target. */
+ SOFTWARE_SINGLE_STEP (0, 0);
+ singlestep_breakpoints_inserted_p = 0;
+ }

ecs->random_signal = 0;

@@ -1615,7 +1621,7 @@
if (!breakpoint_thread_match (stop_pc, ecs->ptid))
thread_hop_needed = 1;
}
- else if (SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p)
+ else if (singlestep_breakpoints_inserted_p)
{
/* We have not context switched yet, so this should be true
no matter which thread hit the singlestep breakpoint. */
@@ -1686,7 +1692,7 @@
/* Saw a breakpoint, but it was hit by the wrong thread.
Just continue. */

- if (SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p)
+ if (singlestep_breakpoints_inserted_p)
{
/* Pull the single step breakpoints out of the target. */
SOFTWARE_SINGLE_STEP (0, 0);
@@ -1735,7 +1741,7 @@
return;
}
}
- else if (SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p)
+ else if (singlestep_breakpoints_inserted_p)
{
sw_single_step_trap_p = 1;
ecs->random_signal = 0;
@@ -1757,7 +1763,7 @@
deprecated_context_hook (pid_to_thread_id (ecs->ptid));
}

- if (SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p)
+ if (singlestep_breakpoints_inserted_p)
{
/* Pull the single step breakpoints out of the target. */
SOFTWARE_SINGLE_STEP (0, 0);
diff -ruN -x CVS -x '*~' src/gdb/rs6000-aix-tdep.c gdb/gdb/rs6000-aix-tdep.c
--- src/gdb/rs6000-aix-tdep.c 2007-03-02 21:42:43.000000000 +0900
+++ gdb/gdb/rs6000-aix-tdep.c 2007-03-08 16:18:21.000000000 +0900
@@ -39,9 +39,6 @@
static void
rs6000_aix_init_osabi (struct gdbarch_info info, struct gdbarch *gdbarch)
{
- /* RS6000/AIX does not support PT_STEP. Has to be simulated. */
- set_gdbarch_software_single_step (gdbarch, rs6000_software_single_step);
-
/* Minimum possible text address in AIX. */
gdbarch_tdep (gdbarch)->text_segment_base = 0x10000000;
}
diff -ruN -x CVS -x '*~' src/gdb/rs6000-tdep.c gdb/gdb/rs6000-tdep.c
--- src/gdb/rs6000-tdep.c 2007-03-02 21:42:45.000000000 +0900
+++ gdb/gdb/rs6000-tdep.c 2007-03-07 18:01:01.000000000 +0900
@@ -696,6 +696,86 @@
return little_breakpoint;
}

+#define LWARX_MASK 0xfc0007fe
+#define LWARX_INSTRUCTION 0x7C000028
+#define LDARX_INSTRUCTION 0x7C000108
+#define STWCX_MASK 0xfc0007ff
+#define STWCX_INSTRUCTION 0x7c00012d
+#define STDCX_INSTRUCTION 0x7c0001ad
+#define BC_MASK 0xfc000000
+#define BC_INSTRUCTION 0x40000000
+#define IMMEDIATE_PART(insn) (((insn & ~3) << 16) >> 16)
+#define ABSOLUTE_P(insn) ((int) ((insn >> 1) & 1))
+
+CORE_ADDR
+rs6000_deal_with_atomic_sequence (CORE_ADDR pc, CORE_ADDR *branch)
+{
+ CORE_ADDR loc = pc;
+ CORE_ADDR bc = -1;
+ int insn = read_memory_integer (loc, PPC_INSN_SIZE);
+ int i;
+
+ /* Assume all atomic sequences start with an lwarx instruction. */
+ if ((insn & LWARX_MASK) != LWARX_INSTRUCTION
+ && (insn & LWARX_MASK) != LDARX_INSTRUCTION)
+ return -1;
+
+ /* Assume that no atomic sequence is longer than 6 instructions. */
+ for (i= 1; i < 5; ++i)
+ {
+ loc += PPC_INSN_SIZE;
+ insn = read_memory_integer (loc, PPC_INSN_SIZE);
+
+ /* At most one conditional branch instruction is between the lwarx
+ and stwcx. instructions. */
+ if ((insn & BC_MASK) == BC_INSTRUCTION)
+ {
+ bc = IMMEDIATE_PART (insn);
+ if (!ABSOLUTE_P (insn))
+ bc += loc;
+ continue;
+ }
+
+ if ((insn & STWCX_MASK) == STWCX_INSTRUCTION
+ || (insn & STWCX_MASK) == STDCX_INSTRUCTION)
+ break;
+ }
+
+ /* Assume that the atomic sequence ends with a stwcx instruction
+ followed by a conditional branch instruction. */
+ if ((insn & STWCX_MASK) == STWCX_INSTRUCTION
+ || (insn & STWCX_MASK) == STDCX_INSTRUCTION)
+ {
+ warning (_("Tried to step over an atomic sequence of instructions from %s but could not find the end of the sequence.", core_addr_to_string (pc)));
+ return -1;
+ }
+
+ loc += PPC_INSN_SIZE;
+ insn = read_memory_integer (loc, PPC_INSN_SIZE);
+
+ if ((insn & BC_MASK) != BC_INSTRUCTION)
+ {
+ warning (_("Tried to step over an atomic sequence of instructions from %s but it did not end as expected.", core_addr_to_string (pc)));
+ return -1;
+ }
+
+ /* set the location of conditional branch instruction between the lwarx
+ and stwcx. instruction if any. */
+ if (bc != loc)
+ *branch = bc;
+ else
+ *branch = -1;
+
+ return loc;
+}
+
+/* SOFTWARE_SINGLE_STEP_P */
+int
+rs6000_software_single_step_p (void)
+{
+ CORE_ADDR branch;
+ return (rs6000_deal_with_atomic_sequence (read_pc (), &branch) != -1);
+}

/* AIX does not support PT_STEP. Simulate it. */

@@ -715,21 +795,35 @@
{
loc = read_pc ();

- insn = read_memory_integer (loc, 4);
+ /* check if running on an atomic sequence of instructions */
+ breaks[0] = rs6000_deal_with_atomic_sequence (loc, &breaks[1]);

- breaks[0] = loc + breakp_sz;
- opcode = insn >> 26;
- breaks[1] = branch_dest (opcode, insn, loc, breaks[0]);
-
- /* Don't put two breakpoints on the same address. */
- if (breaks[1] == breaks[0])
- breaks[1] = -1;
+ if (breaks[0] != -1)
+ {
+ printf_unfiltered (_("Stepping over an atomic sequence of instructions. \n\
+Beginning at %s, break at %s next time.\n"),
+ core_addr_to_string (loc),
+ core_addr_to_string (breaks[0]));
+ gdb_flush (gdb_stdout);
+ }
+ else
+ {
+ insn = read_memory_integer (loc, PPC_INSN_SIZE);
+ breaks[0] = loc + breakp_sz;
+ opcode = insn >> 26;
+ breaks[1] = branch_dest (opcode, insn, loc, breaks[0]);
+
+ /* Don't put two breakpoints on the same address. */
+ if (breaks[1] == breaks[0])
+ breaks[1] = -1;
+ }

for (ii = 0; ii < 2; ++ii)
{
/* ignore invalid breakpoint. */
if (breaks[ii] == -1)
continue;
+
insert_single_step_breakpoint (breaks[ii]);
}
}
@@ -3442,6 +3536,7 @@

set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
set_gdbarch_breakpoint_from_pc (gdbarch, rs6000_breakpoint_from_pc);
+ set_gdbarch_software_single_step (gdbarch, rs6000_software_single_step);

/* Handle the 64-bit SVR4 minimal-symbol convention of using "FN"
for the descriptor and ".FN" for the entry-point -- a user