From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15642 invoked by alias); 2 Aug 2014 23:39:04 -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 15612 invoked by uid 89); 2 Aug 2014 23:39:04 -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 autolearn=ham version=3.3.2 X-HELO: mail-yh0-f74.google.com Received: from mail-yh0-f74.google.com (HELO mail-yh0-f74.google.com) (209.85.213.74) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Sat, 02 Aug 2014 23:39:01 +0000 Received: by mail-yh0-f74.google.com with SMTP id 29so722899yhl.1 for ; Sat, 02 Aug 2014 16:38:59 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:mime-version:content-type :content-transfer-encoding:message-id:date:to:cc:subject:in-reply-to :references; bh=h41zRRUGfMfJO2jurC5nr5X+hKzO47O7urp1fosl9wA=; b=bPEWGnLf4CE+X8ZR00Z86oHkiMbXNnX8jCjx044ELndfpcGHbfMqKtnx7XR71Augw+ lcDUbv7hUAPNuUvznQ55q6QsMt8jvq+yIvR6R/RKIobQOFWhAR3xX/D/zM2h2OoSfX+5 pGRRb3zbqo+ekOsDO2hCV3mF6JO16zYFWE3AXQmFTBtgEzscFShQUAWNzm3J2qA4jmJX 3ewcN9FLvQ5JTMpbBZiiti4y7QIO1C7xdpa95jWnBDvbHyqI4L50Ptyo00n37PJJqQo/ SdXZz7yIIAIALIHvLYcci0aM4nUlY6prBgp7urmamy1YMAR2WZDpYtf2H1tJp13wuKlF JDLQ== X-Gm-Message-State: ALoCoQkKD0Mag3xq4DK2uOPewS3vpB9mM/2bw+k+7xEtcy2WblaM7M31Cd59DHQ3x1UdE37T5/Hx X-Received: by 10.236.102.239 with SMTP id d75mr543872yhg.21.1407022739737; Sat, 02 Aug 2014 16:38:59 -0700 (PDT) Received: from corp2gmr1-1.hot.corp.google.com (corp2gmr1-1.hot.corp.google.com [172.24.189.92]) by gmr-mx.google.com with ESMTPS id z50si795729yhb.3.2014.08.02.16.38.59 for (version=TLSv1.1 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Sat, 02 Aug 2014 16:38:59 -0700 (PDT) Received: from ruffy.mtv.corp.google.com (ruffy.mtv.corp.google.com [172.17.128.44]) by corp2gmr1-1.hot.corp.google.com (Postfix) with ESMTP id 3A78731C173; Sat, 2 Aug 2014 16:38:59 -0700 (PDT) From: Doug Evans MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-ID: <21469.30354.662367.622712@ruffy.mtv.corp.google.com> Date: Sat, 02 Aug 2014 23:39:00 -0000 To: Keith Seitz Cc: "gdb-patches@sourceware.org ml" Subject: Re: [RFA 5/9] Explicit locations v2 - Add probe locations In-Reply-To: <5388CB91.4030802@redhat.com> References: <536BC69E.9060008@redhat.com> <5388CB91.4030802@redhat.com> X-IsSubscribed: yes X-SW-Source: 2014-08/txt/msg00034.txt.bz2 Hi. Comments inline, mostly nits. Keith Seitz writes: > This is also an update of this patch based on feedback from previous > patch reviews. > > Keith > > Changes since last revision: > - use make_cleanup_delete_event_location > - remove SAVE_SPEC > - new_probe_location > > ChangeLog > 2014-05-29 Keith Seitz > > * break-catch-throw.c (re_set_exception_catchpoint): Convert > linespec for stap probe to probe location. > * breakpoint.c (create_longjmp_master_breakpoint): Likewise. > (create_exception_master_breakpoint): Likewise. > (break_command_1): Remove local variable `arg_cp'. > Check location type to set appropriate breakpoint ops methods. > (trace_command): Likewise. > * linespec.c (event_location_to_sals): Handle probe locations. > * location.c (copy_event_location): Likewise. > (delete_event_location): Likewise. > (event_location_to_string): Likewise. > (string_to_event_location): Likewise. > (event_location_empty_p): Handel probe locations. "Handle ...", or just "Likewise." > * location.h (enum event_location_type): Add PROBE_LOCATION. > (EVENT_LOCATION_PROBE): Define accessor macro. > * probe.c (parse_probes): Assert that LOCATION is a probe location. > Convert linespec into probe location. > > > > diff --git a/gdb/break-catch-throw.c b/gdb/break-catch-throw.c > index e2c8365..207e7af 100644 > --- a/gdb/break-catch-throw.c > +++ b/gdb/break-catch-throw.c > @@ -214,8 +214,8 @@ re_set_exception_catchpoint (struct breakpoint *self) > { > struct event_location location; > > - initialize_event_location (&location, LINESPEC_LOCATION); > - EVENT_LOCATION_LINESPEC (&location) > + initialize_event_location (&location, PROBE_LOCATION); > + EVENT_LOCATION_PROBE (&location) > = ASTRDUP (exception_functions[kind].probe); > sals = parse_probes (&location, NULL); > } > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index ef60f44..345b104 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -3387,7 +3387,7 @@ create_longjmp_master_breakpoint (void) > bp_longjmp_master, > &internal_breakpoint_ops); > b->location > - = new_linespec_location ("-probe-stap libc:longjmp"); > + = new_probe_location ("-probe-stap libc:longjmp"); > b->enable_state = bp_disabled; > } > > @@ -3551,7 +3551,7 @@ create_exception_master_breakpoint (void) > bp_exception_master, > &internal_breakpoint_ops); > b->location > - = new_linespec_location ("-probe-stap libgcc:unwind"); > + = new_probe_location ("-probe-stap libgcc:unwind"); > b->enable_state = bp_disabled; > } > > @@ -10068,7 +10068,6 @@ break_command_1 (char *arg, int flag, int from_tty) > ? bp_hardware_breakpoint > : bp_breakpoint); > struct breakpoint_ops *ops; > - const char *arg_cp = arg; > struct event_location *location; > struct cleanup *cleanup; > > @@ -10076,7 +10075,8 @@ break_command_1 (char *arg, int flag, int from_tty) > cleanup = make_cleanup_delete_event_location (location); > > /* Matching breakpoints on probes. */ > - if (arg_cp != NULL && probe_linespec_to_ops (&arg_cp) != NULL) > + if (location != NULL > + && EVENT_LOCATION_TYPE (location) == PROBE_LOCATION) > ops = &bkpt_probe_breakpoint_ops; > else > ops = &bkpt_breakpoint_ops; > @@ -15432,11 +15432,11 @@ trace_command (char *arg, int from_tty) > struct breakpoint_ops *ops; > struct event_location *location; > struct cleanup *back_to; > - const char *arg_cp = arg; > > location = string_to_event_location (&arg, current_language); > back_to = make_cleanup_delete_event_location (location); > - if (arg_cp != NULL && probe_linespec_to_ops (&arg_cp) != NULL) > + if (location != NULL > + && EVENT_LOCATION_TYPE (location) == PROBE_LOCATION) > ops = &tracepoint_probe_breakpoint_ops; > else > ops = &tracepoint_breakpoint_ops; > diff --git a/gdb/linespec.c b/gdb/linespec.c > index 8ea0c7b..deda86e 100644 > --- a/gdb/linespec.c > +++ b/gdb/linespec.c > @@ -2433,6 +2433,11 @@ event_location_to_sals (linespec_parser *parser, > EVENT_LOCATION_ADDRESS (location)); > break; > > + case PROBE_LOCATION: > + /* Probes are handled by their own decoders. */ > + gdb_assert_not_reached ("attempt to decode probe location"); extra space of indentation > + break; > + > default: > gdb_assert_not_reached ("unhandled event location type"); > } > diff --git a/gdb/location.c b/gdb/location.c > index 3c430db..eafa27d 100644 > --- a/gdb/location.c > +++ b/gdb/location.c > @@ -67,6 +67,21 @@ new_address_location (CORE_ADDR addr) > return location; > } > > +/* Create a new probe location. The return result is malloc'd > + and should be freed with delete_event_location. */ > + > +struct event_location * > +new_probe_location (const char *probe) > +{ > + struct event_location *location; > + > + location = XNEW (struct event_location); > + initialize_event_location (location, PROBE_LOCATION); > + if (probe != NULL) > + EVENT_LOCATION_PROBE (location) = xstrdup (probe); > + return location; > +} > + > /* Return a copy of the given SRC location. */ > > struct event_location * > @@ -89,6 +104,10 @@ copy_event_location (const struct event_location *src) > EVENT_LOCATION_ADDRESS (dst) = EVENT_LOCATION_ADDRESS (src); > break; > > + case PROBE_LOCATION: > + EVENT_LOCATION_PROBE (dst) = xstrdup (EVENT_LOCATION_PROBE (src)); > + break; > + > default: > gdb_assert_not_reached ("unknown event location type"); > } > @@ -133,6 +152,10 @@ delete_event_location (struct event_location *location) > /* Nothing to do. */ > break; > > + case PROBE_LOCATION: > + xfree (EVENT_LOCATION_PROBE (location)); > + break; > + > default: > gdb_assert_not_reached ("unknown event location type"); > } > @@ -166,6 +189,10 @@ event_location_to_string_const (const struct event_location *location) > core_addr_to_string (EVENT_LOCATION_ADDRESS (location))); > break; > > + case PROBE_LOCATION: > + result = xstrdup (EVENT_LOCATION_PROBE (location)); > + break; > + > default: > gdb_assert_not_reached ("unknown event location type"); > } > @@ -212,10 +239,23 @@ string_to_event_location (char **stringp, > } > else > { > - /* Everything else is a linespec. */ > - location = new_linespec_location (*stringp); > - if (*stringp != NULL) > - *stringp += strlen (*stringp); > + const char *cs; > + > + /* Next, try the input as a probe spec. */ > + cs = *stringp; > + if (cs != NULL && probe_linespec_to_ops (&cs) != NULL) > + { > + location = new_probe_location (*stringp); > + if (*stringp != NULL) This test is unnecessary (right?). btw, when will this function be passed *stringp == NULL? Can this function have an "early exit" if *stringp == NULL? [assuming that makes sense] > + *stringp += strlen (*stringp); > + } > + else > + { > + /* Everything else is a linespec. */ > + location = new_linespec_location (*stringp); > + if (*stringp != NULL) > + *stringp += strlen (*stringp); > + } > } > > return location; > @@ -234,6 +274,9 @@ event_location_empty_p (const struct event_location *location) > case ADDRESS_LOCATION: > return 0; > > + case PROBE_LOCATION: > + return EVENT_LOCATION_PROBE (location) == NULL; > + > default: > gdb_assert_not_reached ("unknown event location type"); > } > diff --git a/gdb/location.h b/gdb/location.h > index 1daefb6..e6f14d9 100644 > --- a/gdb/location.h > +++ b/gdb/location.h > @@ -30,7 +30,10 @@ enum event_location_type > LINESPEC_LOCATION, > > /* An address in the inferior. */ > - ADDRESS_LOCATION > + ADDRESS_LOCATION, > + > + /* A probe location. */ > + PROBE_LOCATION > }; > > /* An event location used to set a stop event in the inferior. > @@ -50,6 +53,7 @@ struct event_location > probes. */ > char *addr_string; > #define EVENT_LOCATION_LINESPEC(S) ((S)->u.addr_string) > +#define EVENT_LOCATION_PROBE(S) ((S)->u.addr_string) > > /* An address in the inferior. */ > CORE_ADDR address; > @@ -98,6 +102,12 @@ extern struct event_location * > extern struct event_location * > new_address_location (CORE_ADDR addr); > > +/* Create a new probe location. The return result is malloc'd > + and should be freed with delete_event_location. */ > + > +extern struct event_location * > + new_probe_location (const char *probe); > + > /* Return a copy of the given SRC location. */ > > extern struct event_location * > diff --git a/gdb/probe.c b/gdb/probe.c > index c7597d9..2ff5d86 100644 > --- a/gdb/probe.c > +++ b/gdb/probe.c > @@ -58,7 +58,8 @@ parse_probes (struct event_location *location, > result.sals = NULL; > result.nelts = 0; > > - arg_start = EVENT_LOCATION_LINESPEC (location); > + gdb_assert (EVENT_LOCATION_TYPE (location) == PROBE_LOCATION); > + arg_start = EVENT_LOCATION_PROBE (location); > > cs = arg_start; > probe_ops = probe_linespec_to_ops (&cs); > @@ -173,12 +174,12 @@ parse_probes (struct event_location *location, > { > canonical->special_display = 1; > canonical->pre_expanded = 1; > - canonical->location = new_linespec_location (NULL); > - EVENT_LOCATION_LINESPEC (canonical->location) > + canonical->location = new_probe_location (NULL); > + EVENT_LOCATION_PROBE (canonical->location) > = savestring (arg_start, arg_end - arg_start); I see why you pass NULL to new_probe_location and then set EVENT_LOCATION_PROBE afterwards (otherwise two copies of the string would be malloc'd, and you'd need to deal with freeing one of them). One could add a version of new_probe_location that took a char* and a length, but that seems excessive. Another place where c++ would allow cleaner code. > } > > - EVENT_LOCATION_LINESPEC (location) = arg_end; > + EVENT_LOCATION_PROBE (location) = arg_end; > do_cleanups (cleanup); The function starts out with: arg_start = EVENT_LOCATION_PROBE (location); and at the end we do this: EVENT_LOCATION_PROBE (location) = arg_end; IWBN to document why we do this, it's not obvious to me why that is. [doesn't have to be part of this patch set though] > > return result;