From patchwork Wed Sep 3 19:32:08 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Keith Seitz X-Patchwork-Id: 2641 Received: (qmail 5019 invoked by alias); 3 Sep 2014 19:32:16 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 4988 invoked by uid 89); 3 Sep 2014 19:32:15 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.8 required=5.0 tests=AWL, BAYES_00, FILL_THIS_FORM_SHORT, RP_MATCHES_RCVD, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Wed, 03 Sep 2014 19:32:13 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s83JW9HU020001 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Wed, 3 Sep 2014 15:32:10 -0400 Received: from valrhona.uglyboxes.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s83JW8Tn008067 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Wed, 3 Sep 2014 15:32:09 -0400 Message-ID: <54076CB8.4060108@redhat.com> Date: Wed, 03 Sep 2014 12:32:08 -0700 From: Keith Seitz User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.7.0 MIME-Version: 1.0 To: Doug Evans CC: "gdb-patches@sourceware.org ml" Subject: Re: [RFA 5/9] Explicit locations v2 - Add probe locations References: <536BC69E.9060008@redhat.com> <5388CB91.4030802@redhat.com> <21469.30354.662367.622712@ruffy.mtv.corp.google.com> In-Reply-To: <21469.30354.662367.622712@ruffy.mtv.corp.google.com> X-IsSubscribed: yes On 08/02/2014 04:38 PM, Doug Evans wrote: > > (event_location_empty_p): Handel probe locations. > > "Handle ...", or just "Likewise." Fixed. > > + case PROBE_LOCATION: > > + /* Probes are handled by their own decoders. */ > > + gdb_assert_not_reached ("attempt to decode probe location"); > > extra space of indentation Fixed. > > + if (cs != NULL && probe_linespec_to_ops (&cs) != NULL) > > + { > > + location = new_probe_location (*stringp); > > + if (*stringp != NULL) > > This test is unnecessary (right?). Yes, that's correct. I've removed it. > btw, when will this function be passed *stringp == NULL? > Can this function have an "early exit" if *stringp == NULL? > [assuming that makes sense] NULL is a valid linespec, e.g, a user does: (gdb) start (gdb) break Breakpoint 1, ... As to an early exit, ... I didn't implement it. If you would like me to, just say the word! > > @@ -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. As mentioned in patch #4, I've fixed in the prescribed manner (free the savestring after calling the ctor). > > > } > > > > - 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] This now uses the more explicit API functions to do this, event_location_advance_ptr and added a comment. [Of course, this is part of the "skip past any processed input" paradigm that we have to deal with for linespecs/probes.] > > > > > return result; > Updated patch attached. Keith Changes since last revision: - Fix "Handel" in ChangeLog - Remove leading whitespace - Remove unnecessary NULL check in string_to_event_location - Pass savestring() to new_probe_location instead of NULL and accessing struct directly - Use functions instead of EVENT_LOCAITON_* macros. - Add comment and use event_location_advance_ptr to "skip" past processed input in parse_probes gdb/ChangeLog: * 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): Likewise. * location.h (enum event_location_type): Add PROBE_LOCATION. * probe.c (parse_probes): Assert that LOCATION is a probe location. Convert linespec into probe location. --- gdb/break-catch-throw.c | 2 + gdb/breakpoint.c | 12 ++++----- gdb/linespec.c | 5 ++++ gdb/location.c | 64 ++++++++++++++++++++++++++++++++++++++++++++--- gdb/location.h | 17 ++++++++++++ gdb/probe.c | 7 +++-- 6 files changed, 92 insertions(+), 15 deletions(-) diff --git a/gdb/break-catch-throw.c b/gdb/break-catch-throw.c index 098ab1a..57b18ad 100644 --- a/gdb/break-catch-throw.c +++ b/gdb/break-catch-throw.c @@ -212,7 +212,7 @@ re_set_exception_catchpoint (struct breakpoint *self) /* We first try to use the probe interface. */ location - = new_linespec_location (ASTRDUP (exception_functions[kind].probe)); + = new_probe_location (ASTRDUP (exception_functions[kind].probe)); cleanup = make_cleanup_delete_event_location (location); copy_location = copy_event_location_tmp (location); make_cleanup (xfree, copy_location); diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 395e0f4..0fceeac 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; } @@ -10082,7 +10082,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; @@ -10090,7 +10089,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; @@ -15453,11 +15453,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 7048d02..bc4484f 100644 --- a/gdb/linespec.c +++ b/gdb/linespec.c @@ -2433,6 +2433,11 @@ event_location_to_sals (linespec_parser *parser, get_address_location (location)); break; + case PROBE_LOCATION: + /* Probes are handled by their own decoders. */ + gdb_assert_not_reached ("attempt to decode probe location"); + break; + default: gdb_assert_not_reached ("unhandled event location type"); } diff --git a/gdb/location.c b/gdb/location.c index 9e86107..8d8fa89 100644 --- a/gdb/location.c +++ b/gdb/location.c @@ -45,6 +45,7 @@ struct event_location probes. */ char *addr_string; #define EL_LINESPEC(PTR) ((PTR)->u.addr_string) +#define EL_PROBE(PTR) ((PTR)->u.addr_string) /* An address in the inferior. */ CORE_ADDR address; @@ -113,6 +114,29 @@ get_address_location (struct event_location *location) /* See description in location.h. */ struct event_location * +new_probe_location (const char *probe) +{ + struct event_location *location; + + location = XCNEW (struct event_location); + EL_TYPE (location) = PROBE_LOCATION; + if (probe != NULL) + EL_PROBE (location) = xstrdup (probe); + return location; +} + +/* See description in location.h. */ + +char * +get_probe_location (struct event_location *location) +{ + gdb_assert (EL_TYPE (location) == PROBE_LOCATION); + return EL_PROBE (location); +} + +/* See description in location.h. */ + +struct event_location * copy_event_location (const struct event_location *src) { struct event_location *dst; @@ -133,6 +157,11 @@ copy_event_location (const struct event_location *src) EL_ADDRESS (dst) = EL_ADDRESS (src); break; + case PROBE_LOCATION: + if (EL_PROBE (src) != NULL) + EL_PROBE (dst) = xstrdup (EL_PROBE (src)); + break; + default: gdb_assert_not_reached ("unknown event location type"); } @@ -177,6 +206,10 @@ delete_event_location (struct event_location *location) /* Nothing to do. */ break; + case PROBE_LOCATION: + xfree (EL_PROBE (location)); + break; + default: gdb_assert_not_reached ("unknown event location type"); } @@ -219,6 +252,10 @@ event_location_to_string_const (const struct event_location *location) core_addr_to_string (EL_ADDRESS (location))); break; + case PROBE_LOCATION: + result = xstrdup (EL_PROBE (location)); + break; + default: gdb_assert_not_reached ("unknown event location type"); } @@ -259,10 +296,22 @@ 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); + *stringp += strlen (*stringp); + } + else + { + /* Everything else is a linespec. */ + location = new_linespec_location (*stringp); + if (*stringp != NULL) + *stringp += strlen (*stringp); + } } return location; @@ -282,6 +331,9 @@ event_location_empty_p (const struct event_location *location) case ADDRESS_LOCATION: return 0; + case PROBE_LOCATION: + return EL_PROBE (location) == NULL; + default: gdb_assert_not_reached ("unknown event location type"); } @@ -294,6 +346,8 @@ void event_location_advance_input_ptr (char **inp, { if (EL_TYPE (location) == LINESPEC_LOCATION) *inp = EL_LINESPEC (location); + else if (EL_TYPE (location) == PROBE_LOCATION) + *inp = EL_PROBE (location); } /* See description in location.h. */ @@ -303,6 +357,8 @@ advance_event_location_ptr (struct event_location *location, size_t num) { if (EL_TYPE (location) == LINESPEC_LOCATION) EL_LINESPEC (location) += num; + else if (EL_TYPE (location) == PROBE_LOCATION) + EL_PROBE (location) += num; } /* See description in location.h. */ diff --git a/gdb/location.h b/gdb/location.h index 14aaffa..fa4d0f5 100644 --- a/gdb/location.h +++ b/gdb/location.h @@ -31,7 +31,10 @@ enum event_location_type LINESPEC_LOCATION, /* An address in the inferior. */ - ADDRESS_LOCATION + ADDRESS_LOCATION, + + /* A probe location. */ + PROBE_LOCATION }; /* Return the type of the given event location. */ @@ -79,6 +82,18 @@ extern struct event_location * extern CORE_ADDR get_address_location (struct event_location *location); +/* 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 the probe location (a string) of the given event_location + (which must be of type PROBE_LOCATION). */ + +extern char * + get_probe_location (struct event_location *location); + /* Free an event location and any associated data. */ extern void delete_event_location (struct event_location *location); diff --git a/gdb/probe.c b/gdb/probe.c index edf16f7..e28082e 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 = get_linespec_location (location); + gdb_assert (event_location_type (location) == PROBE_LOCATION); + arg_start = get_probe_location (location); cs = arg_start; probe_ops = probe_linespec_to_ops (&cs); @@ -175,11 +176,11 @@ parse_probes (struct event_location *location, canonical->special_display = 1; canonical->pre_expanded = 1; - canonical->location = new_linespec_location (canon); + canonical->location = new_probe_location (canon); xfree (canon); } - /* Advance the location past all parsed input. */ + /* Advance the probe location past all parsed input. */ advance_event_location_ptr (location, arg_end - arg_start); do_cleanups (cleanup);