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

Emi SUZUKI
2007-03-02T12:47:48Z

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

Hello Luis,

From: Luis Machado <luisgpm at linux dot vnet dot ibm dot com>
Subject: Re: [patch] "single step" atomic instruction sequences as a whole.
Date: Wed, 28 Feb 2007 13:09:00 -0300

> Also, i haven't been able to reproduce the issue related at this post:
> (http://sourceware.org/ml/gdb-patches/2006-09/msg00060.html)

I think one of the reasons why is that you didn't build the target as
Emre did; 'atomic_dec' seemed to be inline expanded in that target.
I could get it with -O3 option of gcc-4.1.0 on my machine running FC5.

And I could reproduce the issue with your patch by setting a
breakpoint at where 'printf' preceded by 'atomic_dec' is before
running the target.

But the fix is fairly easy:
===================================================================
diff -u rs6000-tdep.c.old rs6000-tdep.c
--- rs6000-tdep.c.old 2007-03-02 19:33:12.000000000 +0900
+++ rs6000-tdep.c 2007-03-02 19:34:07.000000000 +0900
@@ -764,7 +764,7 @@
if (last_break && breaks[1] == breaks[0])
last_break = 0;

- for (i= 0; i < last_break; ++i)
+ for (i= 0; i <= last_break; ++i)
insert_single_step_breakpoint (breaks[i]);

printf_unfiltered (_("Stepping over an atomic sequence of instructions beginning at %s\n"),
diff -u ppc-linux-tdep.c.old ppc-linux-tdep.c
--- ppc-linux-tdep.c.old 2007-03-02 19:33:33.000000000 +0900
+++ ppc-linux-tdep.c 2007-03-02 19:33:54.000000000 +0900
@@ -996,7 +996,7 @@
if (last_break && breaks[1] == breaks[0])
last_break = 0;

- for (i= 0; i < last_break; ++i)
+ for (i= 0; i <= last_break; ++i)
insert_single_step_breakpoint (breaks[i]);

printf_unfiltered (_("Stepping over an atomic sequence of instructions beginning at %s\n"),
===================================================================

Another thing I want to notice you about Paul's patch is that the
patch for ppc-linux-tdep.c is not needed at all: they do exactly the
same and RS6000 is the superset of PowerPC.
And I think that the load or store instructions which deal with
doublewords can be appeared as a part of the atomic sequence; Paul's
patch doesn't check it. But maybe not needed for now.

BTW, I had another mistake on my patch send last time...
Please replace the attached one.

My best regards,
--
Emi SUZUKI

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

diff -uBbEw -ruN -x CVS 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-02 21:17:31.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 -uBbEw -ruN -x CVS 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,7 +556,9 @@
if (breakpoint_here_p (read_pc ()) == permanent_breakpoint_here)
SKIP_PERMANENT_BREAKPOINT ();

- if (SOFTWARE_SINGLE_STEP_P () && step)
+ if (step)
+ {
+ if (SOFTWARE_SINGLE_STEP_P ())
{
/* Do it the hard way, w/temp breakpoints */
SOFTWARE_SINGLE_STEP (sig, 1 /*insert-breakpoints */ );
@@ -565,6 +567,8 @@
/* 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");
+
+ 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 -uBbEw -ruN -x CVS 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-02 21:15:39.000000000 +0900
@@ -696,6 +696,79 @@
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)
+ error (_("Tried to step over an atomic sequence of instructions but could not find the end of the sequence."));
+
+ loc += PPC_INSN_SIZE;
+ insn = read_memory_integer (loc, PPC_INSN_SIZE);
+
+ if ((insn & BC_MASK) != BC_INSTRUCTION)
+ error (_("Tried to step over an atomic sequence of instructions but it did not end as expected."));
+
+ /* 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,8 +788,20 @@
{
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]);

+ 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]);
@@ -724,6 +809,7 @@
/* Don't put two breakpoints on the same address. */
if (breaks[1] == breaks[0])
breaks[1] = -1;
+ }

for (ii = 0; ii < 2; ++ii)
{
@@ -3442,6 +3529,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