This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 2/3] Support up to 3 conditional branches in an atomic sequence
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Anton Blanchard <anton at samba dot org>
- Cc: gdb-patches at sourceware dot org
- Date: Wed, 13 Jun 2012 09:02:08 -0700
- Subject: Re: [PATCH 2/3] Support up to 3 conditional branches in an atomic sequence
- References: <20120606135557.7da37cbe@kryten> <20120606135655.57bd5b54@kryten>
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