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

[Tom Tromey <tromey@redhat.com>]

>>>>> "Luis" == Luis Machado <luis_gustavo@mentor.com> writes:

Luis> This patch handles GDB-specific changes to support stub-side
Luis> breakpoint conditions.

Nice series, I quite like it.

Luis> I ran the testsuite and had no additional failures.

I think the patches should include some tests for the new feature.

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

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.

Luis> +int remote_supports_cond_breakpoints (void);

Is it much uglier to rearrange things so no forward declaration is
needed?

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.

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.

Tom