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: Joel Brobecker <brobecker at adacore dot com>
- Cc: Anton Blanchard <anton at samba dot org>, 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:57:57 +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> <20140328173251 dot GJ4030 at adacore dot 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