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

[Daniel Jacobowitz <drow at false dot org>]

On Thu, Jun 22, 2006 at 12:51:34PM -0700, PAUL GILLIAM wrote:
> This fixes a problem noticed on ppc64-linux: automatically stepping out
> of the 'puts' library function (because it had no line number
> information) would cause an endless loop.  This happened because of the
> following sequence of instructions:
> 
>         L1:   lwarx   r11,0,r3
>               cmpw    r11,r9
>               bne-    L2
>               stwcx.  r0,0,r3
>               bne-    L1
>         L2:   isync

Since no one else did I guess I get to look at these :-)

Paul, do you know if these patches still work?  There's a report that
they cause internal errors.  One great way to test would be adding a
testcase to gdb.arch, and also running the GDB testsuite.  I imagine
this is caused by single stepping over anything _but_ an atomic op,
since insert_single_step_breakpoint was called conditionally and
remove_single_step_breakpoints unconditionally.  I imagine we got there
because you always return 1, even if you didn't do anything.

I'm still not thrilled with the approach but no one came up with an
alternative so we'll do it your way - implemented is better than not.

> I have attached two patches.  'change-software-single-step.diff' makes
> the generic changes to all the existing software single step routines:
> changing their type from void to int and always returning a 1.

Two things.  First of all, the patch won't compile; I saw at least one
"return 1" without a semicolon.  Secondly, here's some suggestions for
the ChangeLog:

> 	* gdbarch.sh: Change the return type of software_single_step from
> 	void to int and reformatted some comments to <= 80 columns.
> 	* gdbarch.c, gdbarch.h: Regenerated.
> 	* alpha-tdep.c (alpha_software_single_step): Change the return type
> 	from void to int and always return 1.
> 	* alpha-tdep.h: Change the return type of alpha_software_single_step
> 	from void to int.

	* gdbarch.sh (software_single_step): Return int.  Doc fixes.
	* gdbarch.c, gdbarch.h: Regenerated.
	* alpha-tdep.c (alpha_software_single_step): Return 1.
	* alpha-tdep.h (alpha_software_single_step): Update.

Or, for big mechanical changes like this one, I think it's reasonable
to summarize:

	* alpha-tdep.c, alpha-tdep.h, arm-tdep.c, [list all the
	files]: Update software_single_step methods.

> 'ppc-atomic-single-step.diff' adds the new ppc_atomic_single_step
> routine to ppc-linux-tdep.c and updates the ppc_linux_init_abi routine
> to use set_gdbarch_software_single_step() to plug the new routine into
> the architecture vector.

> +  /* Enable software_single_step in case someone tries to sngle step a
> +     sequence of instructions that should be atomic. */

Two spaces after a period.

> +      for (i= 1; i < 5; ++i)

Spaces around an operator.

> +	error (_("Tried to step over an atomic sequence of instructions but could not find the end of the sequence."));

Line too long.  Either wrap the string or add newlines to the error
message, for instance:

	error (_("Tried to step over an atomic sequence of instructions but "
		 "could not find the end of the sequence."));


-- 
Daniel Jacobowitz
CodeSourcery