Re: [PATCH 2/2] new tracepoint downloaded MI notification.

[Pedro Alves <palves at redhat dot com>]

On 09/29/2012 03:13 PM, Yao Qi wrote:
> On 09/29/2012 01:46 AM, Pedro Alves wrote:
>>> Took me a second to recall, but the reason the address is
>>> >necessary is multi-location tracepoints -- a tracepoint on the
>>> >target is identified by the { number, address } tuple.  We don't
>>> >send over the location's sub number (like 1.1, 1.2, etc.).
>>> >
>>> >Should we mention this somewhere (other than at the tracepoint
>>> >packets description), so frontend people don't wonder whether they
>>> >can ignore the address field, and why aren't the other fields of
>>> >the tracepoint (like spec string) included?
>> And I guess the related question is, are frontends interested
>> in { number, address }, which is target side detail, or on
>> { number, location number }, which is how other breakpoints are
>> presented to the frontend?  I would think the latter?
> 
> From the frontend's point of view, {number, location number} is better, and the schema "number.location_number" has been used in "=breakpoint-modified" notification.  However, if we want to use {number, location number} here, we have to guarantee that the location number is an attribute of bp_location, because:

This is not a particular issue of tracepoint locations, so, if it was a problem, it
would be a problem for the existing notifications and MI commands as well.

IOW, this would need to be fixed for all those other cases that expose location
numbers, not just come up with an ad hoc solution.

IOW, there's no good justification for deviating this notification from
existing practice.

> 
> Nowadays, location number is generated by incrementing a counter during iterating 
> a list of bp_location of breakpoint (in breakpoint.c:print_one_breakpoint), so
> I am wondering that the bp_location object may have the different number,
> if the list of bp_locations of a breakpoint is removed due to some reasons.
> Looks bp_location list of breakpoint is *not* removed except in
> breakpoint_program_space_exit, after examine the source code.


> 
> For example,
> 
> Originally we have a tracepoint of 3 locations,
> 
> 4       tracepoint     keep y   <MULTIPLE>
>         collect $eip^M
> 4.1                         y     0x0804859c in func4 inf 1
> 4.2                         y     0xb7ffc480 in func4 inf 2
> 4.3                         y     0xb7ffc488 in func4 inf 1
> 
> due to some reason, bp_location on address 0xb7ffc480 is removed (for example, inferior 2 is removed), and original bp_location 4.3 becomes 4.2.

(BTW, I tried it and looks to be broken - the location of the removed
inferior is left behind, but with "inf 2" removed...)

Locations also come and go with shared libraries being loaded and unloaded,
along with other similar cases of loading and unloading symbols from GDB.

In the case above, the frontend gets notified of the inferior exiting.  It
feels like it should be notified that the breakpoint has been modified, but
that doesn't appear to trigger presently.

> In short, if we can make location number persistent (unchanged for a given bp_location object), then {number, location number} is fine, otherwise, I'd prefer {number, address}.

{number, address} may looks stabler, but not even that is completely stable.
Change the program's code a little, recompile, reload symbols, and you'll see the
address change.  Whatever smarts you'd want the frontend to have to track
locations, GDB could do instead, once for all frontends, and better, because
it may use properties other than the address to decided whether a new instance
of a location is logically the same as the old one (think of stap probes or static
tracepoints, where the address is really a very low target side detail; or just
consider line number instead of address).  I think all this goes hand in hand with
redesigning how GDB handles breakpoint re-setting (or rather, get rid of re-setting as is),
which will make this whole stabler locations issue better.  Keith will be going to
work on that very soon, IIUC.

-- 
Pedro Alves