From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26962 invoked by alias); 8 Jun 2014 21:58:09 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 26952 invoked by uid 89); 8 Jun 2014 21:58:07 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.2 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,RP_MATCHES_RCVD,SPF_PASS,T_FILL_THIS_FORM_SHORT autolearn=ham version=3.3.2 X-HELO: mail-vc0-f174.google.com Received: from mail-vc0-f174.google.com (HELO mail-vc0-f174.google.com) (209.85.220.174) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Sun, 08 Jun 2014 21:58:05 +0000 Received: by mail-vc0-f174.google.com with SMTP id ik5so5492221vcb.33 for ; Sun, 08 Jun 2014 14:58:03 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=6fU+bPsJvHMObP4wKuONQVm1/gsESDYX93sMrdyDzDA=; b=NkIOs7T9iwRMF/oPMfQHyUniIAAdB+ZPbyLUUplvHQnb59ysw3zhh4F6aYG3O9n/Ck On4C0V3VYwy3/mj/naIzMl++FR6/+MYROUh+WCnOoc1Zf9g2d+dBoVOffSpcd2qIjYaq 2+88uZb6rSaJF/kU4tXOwfL0kJrNes/KvLAJOuQr2LTTyaR5CzX6Ofg95Z2nwgQyCoaF 2qAkjD20EQuvy/jNKvnx8JY9imGI83aGFYf2fZeN1mIwwRwoA9awb5qxh5tUmZEEpZb5 bI5mpqmcVydOJRtDCOFRPPFa9eoES+qZ7euzSw80uZ2/5ytJSf+tLsg1kEt0GOCMaInT x/LA== X-Gm-Message-State: ALoCoQk24PHxfUZSscv9oE/HA/5rweGaMXgf858YCRoyVm5s0xbHxSINHFesGZT1KcOenFSwkVM+ MIME-Version: 1.0 X-Received: by 10.52.175.69 with SMTP id by5mr18519500vdc.16.1402264683418; Sun, 08 Jun 2014 14:58:03 -0700 (PDT) Received: by 10.52.28.230 with HTTP; Sun, 8 Jun 2014 14:58:03 -0700 (PDT) In-Reply-To: <5388CB81.4030409@redhat.com> References: <536BC678.1010309@redhat.com> <5388CB81.4030409@redhat.com> Date: Sun, 08 Jun 2014 21:58:00 -0000 Message-ID: Subject: Re: [RFA 4/9] Explicit locations v2 - Add address locations From: Doug Evans To: Keith Seitz Cc: "gdb-patches@sourceware.org ml" Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2014-06/txt/msg00349.txt.bz2 On Fri, May 30, 2014 at 11:18 AM, Keith Seitz 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 > > > * 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)
: 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?