Re: [PATCH 2/3] Support up to 3 conditional branches in an atomic sequence

[Joel Brobecker <brobecker@adacore.com>]

Anton,

Thanks for the patch. I am not a real specialist of the software
single-step implementation, even though I worked on it a really
long time ago. But your changes mostly make sense to me (and seem
to be relatively mechanical in nature).

> 2012-06-05  Anton Blanchard  <anton@samba.org>
> 
> 	* gdb/breakpoint.h: Define NR_SINGLE_STEP_BREAKPOINTS
> 	* rs6000-tdep.c (ppc_deal_with_atomic_sequence): Allow for more
> 	than two breakpoints.
> 	* gdb/breakpoint.c (insert_single_step_breakpoint): Likewise
> 	(insert_single_step_breakpoint): Likewise
> 	(single_step_breakpoints_inserted): Likewise
> 	(cancel_single_step_breakpoints): Likewise
> 	(detach_single_step_breakpoints): Likewise
> 	(single_step_breakpoint_inserted_here_p): Likewise

Mostly OK. I would like you to change the name of the macro to
MAX_SINGLE_STEP_BREAKPOINTS, though. Can you, please?

Other comments and questions inline.

> -  int bc_insn_count = 0; /* Conditional branch instruction count.  */

Thanks for deleting that variable that seems redundant with
last_breakpoint...

> +          if (last_breakpoint >= (NR_SINGLE_STEP_BREAKPOINTS-1))
> +            return 0; /* too many conditional branches found, fallback

Can you remove the extra parens which are useless in this case?
And binary operators should have a space before and after. Thus:

        if (last_breakpoint > MAX_SINGLE_STEP_BREAKPOINTS - 1)

> Index: gdb/gdb/breakpoint.h
[...]
> -/* Manage a software single step breakpoint (or two).  Insert may be
> -   called twice before remove is called.  */
> +/* Manage software single step breakpoints.  */
> +#define NR_SINGLE_STEP_BREAKPOINTS 4
> +

Just curious: Why did you remove the second sentence from the comment?
Is it no longer true? Maybe it is the "twice" that should be changed
into "multiple times"?

Thanks,
-- 
Joel