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

[Luis Machado <luis_gustavo at mentor dot com>]

Hi Tom,

On 01/06/2012 07:44 PM, Tom Tromey wrote:
> Luis>  I ran the testsuite and had no additional failures.
>
> I think the patches should include some tests for the new feature.

Yes. I still need to think about those.

I'll probably add a few GDB-side tests for safety, but mostly 
target-side ones like the tracepoint tests.
> Luis>  I'd like to hear about the change to "info break" and also the way we
> Luis>  should pass conditions down to the stub.
>
> I looked at this code and, while I think the output bits are fine, there
> is something I don't understand:
>
> +      /* 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)
>
> (First, there should be parens around that last subexpression.)
Fixed.
> If the user changes 'condition-evaluation', then the new setting might
> affect what "info break" prints.  But this seems weird, since what is
> relevant is what is actually going on under the hood.
>
> Or, on the other hand, changing 'condition-evaluation' should change the
> state of all breakpoints; but this requires additional code AFAICT.

This has been addressed according to Eli's and Pedro's comments.

If the user switches to a different mode, we will take appropriate 
action. Either we remove conditions or we add them to the target.
> Luis>  +int remote_supports_cond_breakpoints (void);
>
> Is it much uglier to rearrange things so no forward declaration is
> needed?

In this case, unfortunately, i think so.  I'd have to move the code that 
adds the conditions to the packet buffer to the far bottom of the file. 
If keeping similar functions apart is no issue, i can do this change.
> Luis>  +  /* List of conditions the target should evaluate if it supports target-side
> Luis>  +     breakpoint conditions.  */
> Luis>  +  struct condition_list *cond_list;
>
> It seems that the callee must free these.  I think that should be
> documented here.

This should be addressed now with the use of VEC's. We have a vector of 
agent expressions.
> Luis>  +  /* Conditional expression in agent expression
> Luis>  +     bytecode form.  This is used for stub-side breakpoint
> Luis>  +     condition evaluation.  */
> Luis>  +  struct agent_expr *cond_bytecode;
> Luis>  +
> Luis>  +  /* Signals that the condition has changed since the last time
> Luis>  +     we updated the global location list.  This means the condition
> Luis>  +     needs to be sent to the target again.  This is used together
> Luis>  +     with target-side breakpoint conditions.  */
> Luis>  +  int condition_changed;
> Luis>  +
> Luis>  +  /* Signals that breakpoint conditions need to be re-synched with the
> Luis>  +     target.  This has no use other than target-side breakpoints.  */
> Luis>  +  int needs_update;
>
> I still wish we had 'bool'.  Maybe this year.
>
> pahole says there are holes in bp_location on my x86-64 box.  I think it
> is somewhat preferable to fill the holes and to follow existing
> practice, that is, use 'char' for boolean fields here.
>
> My preference would be to use 'unsigned int : 1' for boolean fields, but
> I don't know how others feel about this.

I'm fine with either of those. I'll switch to char meanwhile, but will 
use bitfields if OK.

Thanks!

I'll send an updated patch soon, cc-ing everyone.

Luis
lgustavo@codesourcery.com