Re: [RFC stub-side break conditions 3/5] GDB-side changes

[Pedro Alves <palves at redhat dot com>]

On 01/10/2012 10:39 AM, Luis Machado wrote:
> 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.

Ah, that makes more sense.

> 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.

Right.

> 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.

Might that be confused with a function call, like?

     stop only if i==foo (stub)

What does the output look like in MI ?  The new MI field should be documented in the manual.

> 
>>
>> > 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.

Indeed they are.  Good then.

> 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.

Yes.

> 
> 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?

Yes, though at this level, we think about program and address spaces instead of processes.

>> > @@ -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.

See http://sourceware.org/ml/gdb-patches/2008-07/msg00026.html .

> 
>>
>> > +  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.

Looking forward.