This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFC] "single step" atomic instruction sequences as a whole on PPC
- From: uweigand at de dot ibm dot com
- To: luisgpm at linux dot vnet dot ibm dot com
- Cc: uweigand at de dot ibm dot com (Ulrich Weigand), gdb-patches at sourceware dot org
- Date: Wed, 2 May 2007 16:08:26 +0200 (CEST)
- Subject: Re: [RFC] "single step" atomic instruction sequences as a whole on PPC
Luis Machado wrote:
> Patch updated to reflect the inclusion of the regcache parameter.
Thanks! A number of additional comments:
> +static int
> +deal_with_atomic_sequence ()
Parameter is missing here.
> +{
> + CORE_ADDR pc = read_pc ();
> + CORE_ADDR breaks[2] = {-1, -1};
> + CORE_ADDR loc = pc;
> + int insn = read_memory_integer (loc, PPC_INSN_SIZE);
> + int last_break = 0;
> + int i;
> +
> + /* Assume all atomic sequences start with an lwarx or ldarx instruction. */
Should have two spaces after '.' (multiple instances of this throughout the patch)
> + if ((insn & LWARX_MASK) != LWARX_INSTRUCTION
> + && (insn & LWARX_MASK) != LDARX_INSTRUCTION)
> + return 0;
> +
> + /* 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);
> +
> + /* Assume at most one conditional branch instruction between
> + the lwarx and stwcx instructions.*/
Well, what happens if there *is* more than one? You should at least
detect that case ...
> + if ((insn & BC_MASK) == BC_INSTRUCTION)
> + {
> + last_break = 1;
> + breaks[1] = IMMEDIATE_PART (insn);
> + if ( ! ABSOLUTE_P(insn))
Space before '('; no spaces around '!'.
> + breaks[1] += loc;
> + continue;
> + }
Why don't you use the existing branch_dest routine instead of
re-implementing (parts of) it here?
> + 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 at %s\n \
> + but could not find the end of the sequence."),
> + core_addr_to_string(pc));
Shouldn't you return to the default single-step handling if you do not
understand the sequence you find?
> +
> + 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 at %s\n \
> + but the instruction sequence ended in an unexpected way."),
> + core_addr_to_string(pc));
I'm wondering why this test is necessary. So what if there is no conditional
branch immediately after the stwcx? That shouldn't really matter ...
> +
> + breaks[0] = loc;
> +
> + /* This should never happen, but make sure we don't put
> + two breakpoints on the same address. */
> + if (last_break && breaks[1] == breaks[0])
> + last_break = 0;
> +
> + 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"),
> + core_addr_to_string (pc));
Should we really output this message unconditionally? If you're automatically
single-stepping (say, due to software watch points), this message could potentially
show up very frequently ...
Bye,
Ulrich
--
Dr. Ulrich Weigand
Linux on zSeries Development
Ulrich.Weigand@de.ibm.com