Re: [RFA 4/9] Explicit locations v2 - Add address locations

[Doug Evans <dje at google dot com>]

On Fri, May 30, 2014 at 11:18 AM, Keith Seitz <keiths@redhat.com> wrote:
> This is an update of this patch based on changes from reviews of previous
> patches.
>
> Keith
>
> Changes since last version:
>   - remove SAVE_SPEC stuff
>   - canonicalize_linespec: remove expression stuff
>   - ditto convert_linespec_to_sals
>   - event_location_to_string_const: compute return value
>     (aka remove SAVE_SPEC)
>   - new_address_location
>
> ChangeLog
> 2014-05-29  Keith Seitz  <keiths@redhat.com>
>
>
>         * breakpoint.c (create_thread_event_breakpoint): Convert
>         linespec to address location.
>         (init_breakpoint_sal): Likewise.
>         * 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.
>         (event_location_to_sals): Handle ADDRESS_LOCATION.
>
>         (linespec_expression_to_pc): Export.
>         * linespec.h (linespec_expression_to_pc): Add declaration.
>         * location.c (copy_event_location): Handle address locations.
>         (delete_event_location): Likewise.
>         (event_location_to_string): Likewise.
>         (string_to_event_location): Likewise.
>         (event_location_empty_p): Likewise.
>         * location.h (enum event_location_type): Add EVENT_LOCATION_ADDRESS.
>         (struct event_location.u) <address>: New member.
>         (ADDRESS_LOCATION): Define accessor macro.
>
>         * python/py-finishbreakpoint.c (bpfinishpy_init): Remove local
>         variable `finish_pc'.
>         Convert linespec to address location.
>         * spu-tdep.c (spu_catch_start): Convert linespec to address
>         location.
>

Hi.
I've given the series another read through.
Not as detailed a read through as I want to, but that'll have to wait for a bit.

I do have one comment that I thought I'd forward on now though ...

Maybe I'm asking for too much in a C-based world,
but IWBN if code like this was done differently.
It's not a blocker, but I would like your thoughts.

@@ -291,10 +290,8 @@ bpfinishpy_init (PyObject *self, PyObject *args,
PyObject *kwargs)
       struct event_location location;

       /* Set a breakpoint on the return address.  */
-      initialize_event_location (&location, LINESPEC_LOCATION);
-      finish_pc = get_frame_pc (prev_frame);
-      xsnprintf (small_buf, sizeof (small_buf), "*%s", hex_string (finish_pc));
-      EVENT_LOCATION_LINESPEC (&location) = small_buf;
+      initialize_event_location (&location, ADDRESS_LOCATION);
+      EVENT_LOCATION_ADDRESS (&location) = get_frame_pc (prev_frame);
       create_breakpoint (python_gdbarch,
                          &location, NULL, thread, NULL,
                          0

[apologies if there's a cut-n-paste error there - I hope that comes through ok]

There's still too much implementation detail exposed here for my
taste, but this is C so we have to compromise.  In this case,
bpfinishpy_init has to take on the responsibility of knowing what
needs to be done to properly initialize an ADDRESS_LOCATION - we can't
call a constructor because the object is on the stack.
I think there are a few cases in the patch set like this, so IWBN, if
possible, to do something better here.

I can think of two solutions.  Maybe there's another even better one.

1) Have a "placement new"-like constructor akin to the existing
new_foo_location constructors we have that takes a pointer to the
object to initialize.
2) Always use the existing new_foo_location constructors and free the
object when done.

Thoughts?