From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4038 invoked by alias); 19 May 2015 21:30:19 -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 2030 invoked by uid 89); 19 May 2015 21:30:19 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.3 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=no 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; Tue, 19 May 2015 21:30:16 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t4JLUDIY015186 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Tue, 19 May 2015 17:30:13 -0400 Received: from valrhona.uglyboxes.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t4JLUBma010330 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 19 May 2015 17:30:13 -0400 Message-ID: <555BAB63.6000009@redhat.com> Date: Tue, 19 May 2015 21:30:00 -0000 From: Keith Seitz User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Doug Evans CC: gdb-patches@sourceware.org Subject: Re: [PATCH v4 3/9] Explicit locations: use new location API References: <20150507180523.19629.77846.stgit@valrhona.uglyboxes.com> <20150507180551.19629.1965.stgit@valrhona.uglyboxes.com> In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2015-05/txt/msg00501.txt.bz2 On 05/17/2015 10:19 PM, Doug Evans wrote: > Keith Seitz writes: >> diff --git a/gdb/break-catch-throw.c b/gdb/break-catch-throw.c >> index 927176f..9449aa5 100644 >> --- a/gdb/break-catch-throw.c >> +++ b/gdb/break-catch-throw.c >> @@ -35,6 +35,7 @@ >> #include "cp-abi.h" >> #include "gdb_regex.h" >> #include "cp-support.h" >> +#include "location.h" >> >> /* Enums for exception-handling support. */ >> enum exception_event_kind >> @@ -210,25 +211,31 @@ re_set_exception_catchpoint (struct breakpoint *self) >> struct symtabs_and_lines sals_end = {0}; >> struct cleanup *cleanup; >> enum exception_event_kind kind = classify_exception_breakpoint (self); >> + struct event_location *location; >> >> /* We first try to use the probe interface. */ >> TRY >> { >> char *spec = ASTRDUP (exception_functions[kind].probe); >> > > Not something to be done with this patch (let's reach > closure and get this sucker checked in :-)), > but IWBN to have an API where we didn't have to do the > ASTRDUP. E.g., have new_linespec_location_const or some such. Understood. > >> - sals = parse_probes (&spec, NULL); >> + location = new_linespec_location (&spec); >> + cleanup = make_cleanup_delete_event_location (location); >> + sals = parse_probes (location, NULL); >> + do_cleanups (cleanup); >> } >> >> CATCH (e, RETURN_MASK_ERROR) >> { >> - >> /* Using the probe interface failed. Let's fallback to the normal >> catchpoint mode. */ >> TRY >> { >> char *spec = ASTRDUP (exception_functions[kind].function); >> >> - self->ops->decode_location (self, &spec, &sals); >> + location = new_linespec_location (&spec); >> + cleanup = make_cleanup_delete_event_location (location); >> + self->ops->decode_location (self, location, &sals); >> + do_cleanups (cleanup); >> } >> CATCH (ex, RETURN_MASK_ERROR) >> { >> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c >> index 31b1f82..549bfd0 100644 >> --- a/gdb/breakpoint.c >> +++ b/gdb/breakpoint.c [snip] >> @@ -13451,19 +13525,28 @@ dprintf_after_condition_true (struct bpstats *bs) >> markers (`-m'). */ >> >> static void >> -strace_marker_create_sals_from_location (char **arg, >> +strace_marker_create_sals_from_location (const struct event_location *location, >> struct linespec_result *canonical, >> - enum bptype type_wanted, >> - char *addr_start, char **copy_arg) >> + enum bptype type_wanted) >> { >> struct linespec_sals lsal; >> + const char *arg_start, *arg; >> >> - lsal.sals = decode_static_tracepoint_spec (arg); >> + arg = arg_start = get_linespec_location (location); >> + lsal.sals = decode_static_tracepoint_spec (&arg); >> >> - *copy_arg = savestring (addr_start, *arg - addr_start); >> + if (canonical != NULL) > > Why the canonical != NULL test here? > Outside this "if" below it's assumed canonical != NULL. > Right -- fixed that. Thank you. >> + { >> + char *str; >> + struct cleanup *cleanup; >> >> - canonical->addr_string = xstrdup (*copy_arg); >> - lsal.canonical = xstrdup (*copy_arg); >> + str = savestring (arg_start, arg - arg_start); >> + cleanup = make_cleanup (xfree, str); >> + canonical->location = new_linespec_location (&str); >> + do_cleanups (cleanup); >> + } >> + >> + lsal.canonical = xstrdup (event_location_to_string (canonical->location)); >> VEC_safe_push (linespec_sals, canonical->sals, &lsal); >> } >> [snip] >> @@ -14068,22 +14158,21 @@ update_breakpoint_locations (struct breakpoint *b, >> update_global_location_list (UGLL_MAY_INSERT); >> } >> >> -/* Find the SaL locations corresponding to the given ADDR_STRING. >> +/* Find the SaL locations corresponding to the given LOCATION. >> On return, FOUND will be 1 if any SaL was found, zero otherwise. */ >> >> static struct symtabs_and_lines >> -location_to_sals (struct breakpoint *b, char *addr_string, int *found) >> +location_to_sals (struct breakpoint *b, struct event_location *location, >> + int *found) >> { >> - char *s; >> struct symtabs_and_lines sals = {0}; >> struct gdb_exception exception = exception_none; >> >> gdb_assert (b->ops != NULL); >> - s = addr_string; >> >> TRY >> { >> - b->ops->decode_location (b, &s, &sals); >> + b->ops->decode_location (b, location, &sals); >> } >> CATCH (e, RETURN_MASK_ERROR) >> { >> @@ -14125,12 +14214,13 @@ location_to_sals (struct breakpoint *b, char *addr_string, int *found) >> >> for (i = 0; i < sals.nelts; ++i) >> resolve_sal_pc (&sals.sals[i]); >> - if (b->condition_not_parsed && s && s[0]) >> + if (b->condition_not_parsed && b->extra_string != NULL) >> { >> char *cond_string, *extra_string; >> int thread, task; >> + const char *orig = b->extra_string; >> >> - find_condition_and_thread (s, sals.sals[0].pc, >> + find_condition_and_thread (b->extra_string, sals.sals[0].pc, >> &cond_string, &thread, &task, >> &extra_string); >> if (cond_string) > > The code is a bit odd here, at first glance. > We xfree b->extra_string before assigning a new value, > but not b->cond_string. > I'm guessing this is because the latter will always be NULL here. > Can you add an assert to that effect? I.e., > > gdb_assert (b->cond_string == NULL); Yeah, you have that right. At least that's how I read and interpreted that. I've added the assert. New revision coming your way shortly. Keith