This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFC stub-side break conditions 3/5] GDB-side changes
- From: Luis Machado <luis_gustavo at mentor dot com>
- To: Pedro Alves <alves dot ped at gmail dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Tue, 10 Jan 2012 08:39:03 -0200
- Subject: Re: [RFC stub-side break conditions 3/5] GDB-side changes
- References: <4F05BA10.3090107@mentor.com> <4F071FE4.5020009@gmail.com>
- Reply-to: "Gustavo, Luis" <luis_gustavo at mentor dot com>
On 01/06/2012 02:23 PM, Pedro Alves wrote:
On 01/05/2012 02:56 PM, Luis Machado wrote:
Regarding always-inserted mode, if it is "on" we must send condition
changes right away, or else the target will potentially miss
breakpoint hits.
Could you elaborate a bit more on how will it miss breakpoint hits?
That is a bit misleading. Not actually the target, but GDB may not be
notified of breakpoint hits that it should've seen.
For example. Suppose we have a conditional breakpoint location inserted
at address foo. The inferior is running and stopping at the breakpoint
correctly when its condition is true.
Suppose we insert a new unconditional location at foo. If we don't synch
the breakpoint conditions of both those locations with the target, the
target will keep ignoring any breakpoint hits at foo for which the
condition of the first location is false. Of course we may miss a few
breakpoint hits during the condition synchronization operation, but GDB
will eventually synch the conditions with the target instead of waiting
for the next appropriate time to do so.
Another example is a set of 2 duplicate conditional breakpoint locations
inserted at address foo. If we make one of them unconditional while the
process is running (always-inserted mode on), the conditions need to be
synched with the target as well. If we don't, GDB may miss some
breakpoint triggers (since the target won't report every trigger, just
the conditional ones).
> I'd like to hear about the change to "info break"
As they say, a picture is worth a thousand words. How does the new
output look like? What about MI?
An example:
2 breakpoint keep y 0x080483dd in main at loop.c:12
stop only if i==5 (stub)
The change in the output is just a (stub) right after the conditional
expression.
This is being printed together with the conditional expression, so
frontends will possibly pick that output up without requiring any
additional changes.
> and also the way we should pass conditions down to the stub.
Currently i'm adding the agent expressions to a condition list in the
target info field of bp locations.
Sounds good at first sight.
I'm mostly wondering whether the assumption that we can re-insert
a breakpoint at the same address is solid.
Z packets should, in theory, be implemented in an idempotent way
according to the documentation. So adding the same breakpoint location
multiple times shouldn't be a problem.
I thought about potential issues with single-stepping breakpoints, but
they will be inserted with no conditions (making the target remove any
conditions that might be active on its end), thus i expect them to work.
> +/* Iterates through locations with address ADDRESS. BP_LOCP_TMP
points to
> + each object. BP_LOCP_START points to where the loop should
start from.
> + If BP_LOCP_START is a NULL pointer, the macro automatically
seeks the
> + appropriate location to start with. */
> +
> +#define ALL_BP_LOCATIONS_AT_ADDR(BP_LOCP_TMP, BP_LOCP_START,
ADDRESS) \
> + if (!BP_LOCP_START) \
> + BP_LOCP_START = get_first_locp_gte_addr (ADDRESS); \
> + for (BP_LOCP_TMP = BP_LOCP_START; \
> + (BP_LOCP_TMP< bp_location + bp_location_count \
> + && (*BP_LOCP_TMP)->address == ADDRESS); \
> + BP_LOCP_TMP++)
> +
The address alone is not enough when you consider multi-process.
The users of this macro aren't considering that. Several instances of
this problem.
IIUIC, we may have situations with breakpoint locations at the same
address but on different processes.
If a breakpoint spans multiple processes, we must resynch conditions for
both processes. If not, then we should only re-synch target conditions
for the process that contains such a breakpoint/location.
Does that make sense?
> +/* Based on location BL, create a list of breakpoint conditions to be
> + passed on to the target. If we have duplicated locations with
different
> + conditions, we will add such conditions to the list. The idea
is that the
> + target will evaluate the list of conditions and will only notify
GDB when
> + one of them is true. */
What does the return code mean?
> +
> +static int
> +build_target_condition_list (struct bp_location *bl)
> +{
Stale, it does not need one. Fixed.
> + /* Even if the target evaluated the condition on its end and
notified GDB, we
> + need to do so again since GDB does not know if we stopped due
to a
> + breakpoint or a single step.
> + This will only be done when we single step straight to a
conditional breakpoint. */
Where is the code that makes this true?
The second sentence is a bit stale. GDB re-evaluates conditions
everytime now, so we don't need to worry about what kind of event is
associated with the breakpoint trigger.
Fixed.
> +
> if (frame_id_p (b->frame_id)
> && !frame_id_eq (b->frame_id, get_stack_frame_id (get_current_frame
())))
> bs->stop = 0;
> @@ -4905,6 +5219,19 @@ print_one_breakpoint_location (struct br
> else
> ui_out_text (uiout, "\tstop only if ");
> ui_out_field_string (uiout, "cond", b->cond_string);
> +
> + /* Print whether the remote stub is doing the breakpoint's
condition
> + evaluation. If GDB is doing the evaluation, don't print
anything. */
> + if (loc&& is_breakpoint (b)&& loc->cond_bytecode
> + && breakpoint_condition_evaluation_mode ()
> + != condition_evaluation_gdb)
What if you set a conditional breakpoint, have it evaluate the in target,
and then, flip the setting to gdb-side, while the breakpoint's
condition is
still being evaluated on the target side? Shouldn't this always
print "target"/"stub" when loc->cond_bytecode is non-NULL?
I addressed this for Eli's comments as well. Now we prevent the user
from setting "target" evaluation mode if the target does not support it.
A warning is displayed.
The scenario you mentioned is taken care of now. Assume the target
supports condition evaluation, if the user switches from "gdb" to
"target" evaluation mode, GDB will send the breakpoint conditions to the
target.
Similarly, if the user switches from "target" to "gdb", those conditions
will be cleared.
> @@ -11387,7 +11810,10 @@ delete_breakpoint (struct breakpoint *bp
> itself, since remove_breakpoint looks at location's owner. It
> might be better design to have location completely
> self-contained, but it's not the case now. */
> - update_global_location_list (0);
> + if (is_breakpoint (bpt))
> + update_global_location_list (1);
The whole point of the update_global_location_list parameter is being
able
to say "don't insert locations new locations because I'm just deleting a
breakpoint". This was necessary for situations like following an
exec, IIRC.
I'll double check this since the testsuite didn't give me any visible
errors.
> + else
> + update_global_location_list (0);
>
> +/* List of conditions used to pass conditions down to the target. */
> +struct condition_list
> +{
> + /* Breakpoint condition. */
> + struct agent_expr *expr;
> + /* Pointer to the next condition. */
> + struct condition_list *next;
> +};
Can this be a VEC of `struct agent_expr *' instead?
I'm using VEC now.
Fixed all the other issues.
I'll send a new version of the patch soon.
Thanks for reviewing.
--
Luis
lgustavo@codesourcery.com