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

[Pedro Alves <palves@redhat.com>]

On 03/28/2014 05:32 PM, Joel Brobecker wrote:
>>> IIUC, it looks like MAX_SINGLE_STEP_BREAKPOINTS is actually not
>>> the max, but MAX - 1. I was a little confused by that. Why not
>>> change MAX to 3, and then adjust the array definition and code
>>> accordingly? I think things will be slightly simpler to understand.
>>
>> IMO that would be more confusing.  I read MAX_SINGLE_STEP_BREAKPOINTS
>> as meaning the "maximum number of of single-step breakpoints we
>> can create simultaneously".  I think you're reading it as
>> "the highest index possible into the single-step breakpoints
>> array" ?
> 
> Here is how I read the patch: MAX_SINGLE_STEP_BREAKPOINTS is the size
> of the array, and we rely on the last element always being NULL
> to determine the number of "live" elements we actually have.

But we can fill the whole array, there's no sentinel.  E.g.:

+  for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++)
+    if (single_step_breakpoints[i] == NULL)
+        break;
+
+  gdb_assert (i < MAX_SINGLE_STEP_BREAKPOINTS);
+

This just looks for the first empty slot.

> Hence, to me, the maximum number of SS breakpoints we can handle
> in practice is not MAX_SINGLE_STEP_BREAKPOINTS but 1 less.

Nope.  We can handle MAX_SINGLE_STEP_BREAKPOINTS.

> For
> instance, I think the patch is trying to increase the number of
> SS breakpoints to 3, and yet defines MAX_SINGLE_STEP_BREAKPOINTS
> to 4.

Anton is making the port handle 3 conditional branches + 1
terminating branch, so that's 4.  I guess it's either the
subject that's confusing you, or this, perhaps:

> -          if (bc_insn_count >= 1)
> -            return 0; /* More than one conditional branch found, fallback
> +          if (last_breakpoint >= MAX_SINGLE_STEP_BREAKPOINTS - 1)
> +            return 0; /* too many conditional branches found, fallback
>                           to the standard single-step code.  */

This is "- 1" here because that's leaving space for the terminating
branch.  At least, that's what I understood.

I blame lack of comments in the patch.  :-)

> Perhaps it's time to just use a vec? That way, we don't have
> a limit at all anymore...

Yeah...

-- 
Pedro Alves