This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 2/4] Support up to 3 conditional branches in an atomic sequence
- From: Pedro Alves <palves at redhat dot com>
- To: Anton Blanchard <anton at samba dot org>
- Cc: Joel Brobecker <brobecker at adacore dot com>, gdb-patches at sourceware dot org, emachado at linux dot vnet dot ibm dot com, luis_gustavo at mentor dot com, ulrich dot weigand at de dot ibm dot com
- Date: Fri, 28 Mar 2014 17:22:44 +0000
- Subject: Re: [PATCH 2/4] Support up to 3 conditional branches in an atomic sequence
- Authentication-results: sourceware.org; auth=none
- References: <1395978111-30706-1-git-send-email-anton at samba dot org> <1395978111-30706-2-git-send-email-anton at samba dot org> <20140328131230 dot GE4030 at adacore dot com> <5335AD94 dot 4030701 at redhat dot com>
On 03/28/2014 05:12 PM, Pedro Alves wrote:
>>> --- a/gdb/rs6000-tdep.c
>>> +++ b/gdb/rs6000-tdep.c
>>> @@ -1088,7 +1088,7 @@ ppc_deal_with_atomic_sequence (struct frame_info *frame)
>>> struct address_space *aspace = get_frame_address_space (frame);
>>> enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>>> CORE_ADDR pc = get_frame_pc (frame);
>>> - CORE_ADDR breaks[2] = {-1, -1};
>>> + CORE_ADDR breaks[MAX_SINGLE_STEP_BREAKPOINTS];
>
> If we ever bump MAX_SINGLE_STEP_BREAKPOINTS further,
> you'd still only want 4 here.
>
> I think it'd be better if this was:
>
> /* 3 conditional branches + 1 terminating branch. */
> CORE_ADDR breaks[4];
>
> Followed by:
>
> gdb_static_assert (MAX_SINGLE_STEP_BREAKPOINTS >= ARRAY_SIZE (breaks));
>
> This way clearly documents that we need to support 4 sss breakpoints.
> As it is, nothing in your patch leaves any indication in the source to
> that effect, so the poor soul trying to revamp software single-step
> breakpoints could miss this.
Sorry, I wrote this before realizing that there could even be more
condition branches in the region and writing the "is there a hard limit",
and then pushed send too soon. So assuming there's no limit and we're
just trying to be practical in what we support, there's really no harm
in leaving this as:
CORE_ADDR breaks[MAX_SINGLE_STEP_BREAKPOINTS];
but I still think a comment (+ the assert would be nice) is
missing. Something like:
/* The ppc64 Linux kernel has regions with 3 conditional branches.
(plus 1 for the terminating branch). */
gdb_static_assert (MAX_SINGLE_STEP_BREAKPOINTS >= 4);
?
BTW, shouldn't GDB warn or even error out if too many
conditional branches are found? I think that'd be better
than silently getting stuck forever.
--
Pedro Alves