Re: [PATCH v6 4/9] Explicit locations: introduce address locations

[Keith Seitz <keiths@redhat.com>]

Hi, Joel,

Thank you for bringing this to my attention. I was wondering when a bug
about this mega-patch would surface!

[Aside: Bah humbug. There is no test covering this simple/common use case!]

On 12/13/2015 11:11 PM, Joel Brobecker wrote:

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

Ha. Yeah. That would be a bug. :-)

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

That looks like the correct approach at fixing the bug. Or at least, I'm
fairly confident your patch is (darn close if not identical) to what I
would have done.

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

Obviously, this is a decision which needs to be made before I can commit
to any course of action.

> For instance, in light of the above, would it make better sense to
> just handle "*EXPR" the same way we handle non-explicit locations?

I've given this some thought in an attempt to convincingly steer the
direction in favor of removing address locations or keeping them.

More on this after I offer this opinion (all $0.000000000002 of it):

> We could keep ADDRESS_LOCATION event locations for cases where we
> know the breakpoint's address is indeed static.

Regardless of how address locations are specified via the UI, I have a
pretty strong preference that they are all treated alike, and I think
you'd probably wouldn't (completely) disagree.

So this boils down to whether address locations are linespecs or are
their own class of location.

This question is a little difficult for me to definitely answer.

Certainly address locations are treated differently than true linespecs.
No colons are permitted; after the expression is parsed, any further
linespec-y options are ignored (actually error). Likewise on the
explicit side, -address precludes the use of any other
(location-specific) option.

If we later added some sort of address-y specific feature, it could be
introduced in a linespec-y way using a ':' (break *EXPR:option / break
option:*EXPR).

On the explicit side, we'd probably have a new option name (or share an
existing one).

Regardless of how we chose to implement this expansion, though, it is
(quite likely) incompatible with generic source:func:label:line
linespecs. So once again, we have an all-or-nothing representation.

Use this set of rules for "linespec locations" and this set of rules for
"address locations." [I have tried to linguistically distinguish between
our historical linespecs (which include *EXPR) and "linespec locations"
(which currently exclude *EXPR).]

One could argue that this either-or behavior supports maintaining
separate location types.

One could also argue that this behavior is degenerate. It's a special
case. [Wow, am I running for political office or what?!? Nothing like a
simple, straightforward answer to a question!]

I guess when it boils down to it, I don't really have a super strong
preference for either case, but I do have a preference for maintaining
addresses as a separate location type. What can I say? I hate special
cases! :-P

> 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?

I am more than happy to fix anything related to this code in any
appropriate manner dictated by maintainers. [Of course, if this involves
a massive rewrite, I will have to clear with my management!]

Let me know how you and other maintainers would like me to proceed.

Keith