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

[Pedro Alves <palves@redhat.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