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

Pedro Alves
2014-03-28T17:58:12Z

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