Hi Keith,

On Wed, Aug 05, 2015 at 04:30:04PM -0700, Keith Seitz wrote:
> *This patch has previously been approved.*
> 
> This patch adds support for address locations, of the form "*ADDR".
> [Support for address linespecs has been removed/replaced by this "new"
> location type.] This patch also converts any existing address locations
> from its previous linespec type.
> 
> gdb/ChangeLog:
> 
> 	* breakpoint.c (create_thread_event_breakpoint, init_breakpoint_sal):
> 	Convert linespec to address location.
> 	* linespec.c (canonicalize_linespec): Do not handle address
> 	locations here.
> 	(convert_address_location_to_sals): New function; contents moved
> 	from ...
> 	(convert_linespc_to_sals): ... here.
> 	(parse_linespec): Remove address locations from linespec grammar.
> 	Remove handling of address locations.
> 	(linespec_lex_to_end): Remove handling of address linespecs.
> 	(event_location_to_sals): Handle ADDRESS_LOCATION.
> 	(linespec_expression_to_pc): Export.
> 	* linespec.h (linespec_expression_to_pc): Add declaration.
> 	* location.c (struct event_location.u) <address>: New member.
> 	(new_address_location, get_address_location): New functions.
> 	(copy_event_location, delete_event_location, event_location_to_string)
> 	(string_to_event_location, event_location_empty_p): Handle address
> 	locations.
> 	* location.h (enum event_location_type): Add ADDRESS_LOCATION.
> 	(new_address_location, get_address_location): Declare.
> 	* python/py-finishbreakpoint.c (bpfinishpy_init): Convert linespec
> 	to address location.
> 	* spu-tdep.c (spu_catch_start): Likewise.

We just found an issue with this patch.  For instance, if you try
to debug a program (any program) which has PIE enabled, you'll see:

    (gdb) b *main
    Breakpoint 1 at 0x51a: file hello.c, line 3.
    (gdb) run
    Starting program: /[...]/hello
    Error in re-setting breakpoint 1: Warning:
    Cannot insert breakpoint 1.
    Cannot access memory at address 0x51a

    Warning:
    Cannot insert breakpoint 1.
    Cannot access memory at address 0x51a

What happens is that the patch makes the implicit assumption that
the address computed the first time is static, as if it was designed
to only support litteral expressions (Eg. "*0x1234"). This allows
the shortcut of not re-computing the breakpoint location's address
when re-setting breakpoints.

However, this does not work in general, as demonstrated in the example
above. As a side note, an interesting side effect is that, before
running the program, "info break" shows (correctly):

    (gdb) info break
    Num     Type           Disp Enb Address    What
    1       breakpoint     keep y   0x0000051a in main at hello.c:3

But after running the program, we now get ("What" is empty):

    (gdb) info break
    Num     Type           Disp Enb Address    What
    1       breakpoint     keep y   0x0000051a

For my initial attempt at fixing this, I tried to extend what you did
to handle *EXPR as an ADDRESS_LOCATION event_location, and the attached
patch is an attempt to do just that. One of the holes I had to plug was
the fact that the original expression was not stored anywhere (which
makes sense, given that we explicitly skipping the re-evaluation).
I re-used the EL_STRING part of the location rather than making the
address field in the event_location union a struct of its own holding
both address and expression.

In any case, the patch attached seems to work, at least for the case
above. It's currently only lightly tested, and also only a prototype
where documentation and memory management are missing. But while working
on this approach, I had a growing feeling that we needed to step back
and re-evaluate whether using an ADDRESS_LOCATION for handling those
was the right approach at all. For instance, in light of the above,
would it make better sense to just handle "*EXPR" the same way we handle
non-explicit locations? We could keep ADDRESS_LOCATION event locations
for cases where we know the breakpoint's address is indeed static
(Eg: thread-event breakpoints), but I'm wondering if the gain is
really worth the extra code...

WDYT? I admit I'm a little lost still between the various layers
of locations, event_locations, etc. Do you want to take it from there?

Thanks!
-- 
Joel