On 02/07/2012 08:08 PM, Stan Shebs wrote:
> On 1/27/12 12:33 PM, Luis Gustavo wrote:
>> A few more changes on this patch. It addresses all the previous
>> comments and suggestions.
>>
>>
>> I've changed the "info break" output a little. We now display "<mode>
>> evaluates conditions" next to the location description, and we also
>> output that same information via MI. Though it works, i'm not entirely
>> happy with the way it is displayed.
>>
>> Previously i displayed such information next to the condition field,
>> but it doesn't work right for breakpoints with multiple locations
>> since the conditions are printed for the breakpoint instead of the
>> locations. Suggestions?
>
> Can it actually happen that some locations of a breakpoint are evaluated
> host-side, and other target-side? It makes my head hurt trying to think
> of an example. :-)
>

Yes and no. It can happen when, for example, you have two inferiors with 
the same function, like "main".  Inserting a breakpoint in "main" will 
create a breakpoint with two locations (one for each inferior).

Suppose we start one them, but not the other. If we have a condition, 
the condition from the location of the inferior that has been started 
can be evaluated on the target, whereas the one from the inferior that 
did not start can't. It's more of a corner case.


> In any case, the note should follow the condition, and I suggest the
> wording "(host evals)" and "(target evals)" if all locations are going
> to be handled the same way. If locations are varying, then the condition
> follower can say "(host or target evals)" then each location description
> can say just "(host)" or "(target)". It's a little messier to calculate,
> but users will appreciate the clarity.
>
> (I'm preferring "host" to "gdb", but I suppose that might be confusing
> if GDB was talking to a sprite also running on the host and controlling
> target via JTAG.)

I switched to using "host" and "target" now. And i did the change to the 
output of "info break".

>
> +/* Mark locations as "conditions have changed" in case
> + the target supports evaluating conditions on
> + its side. */
>
>
> This is kind of petty, but shouldn't the lines be filled? I think Emacs
> would make this a two-line comment.

Fixed.

>
>
> + warning (_("Target does not support condition evaluation.\n"
> + "Using GDB evaluation mode instead."));
>
>
> I tend to think this should say "breakpoint condition", so it's clear
> that it's not referring to tracepoint conditions, or command list
> conditions, or Python conditions, etc.

Fixed.

>
> + /* If we got here, it means the condition could not be parse to a valid
>
>
> be parsed

Fixed.

>
> (Eventually we should be nicer and say *why* the condition didn't make
> the cut. Users would be happy to replace a convenience var with a
> literal value if they knew that's all that was wrong.)

I'll handle this in a different patch as it involves translating cryptic 
internal error messages to something the users can actually read and 
undestand.

>
> - update_global_location_list (0);
> + if (is_breakpoint (bpt))
> + update_global_location_list (1);
> + else
> + update_global_location_list (0);
>
>
> update_global_location_list (is_breakpoint (bpt))

Fixed. Thanks.

Luis