From patchwork Wed Sep 3 19:31:11 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Keith Seitz X-Patchwork-Id: 2639 Received: (qmail 4180 invoked by alias); 3 Sep 2014 19:32:00 -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 4152 invoked by uid 89); 3 Sep 2014 19:31:58 -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, 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:31:56 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s83JVCF4008752 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Wed, 3 Sep 2014 15:31:12 -0400 Received: from valrhona.uglyboxes.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s83JVBvX013388 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Wed, 3 Sep 2014 15:31:12 -0400 Message-ID: <54076C7F.9050303@redhat.com> Date: Wed, 03 Sep 2014 12:31:11 -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 2/9] Explicit locations v2 - Event locations API References: <536BC5DE.5050707@redhat.com> <21361.21080.298225.332248@ruffy.mtv.corp.google.com> <5388CB00.6040205@redhat.com> In-Reply-To: X-IsSubscribed: yes On 06/05/2014 10:44 PM, Doug Evans wrote: > Alas I've been told we're no longer allowed to add new make_cleanup_foo > functions. > [Can't remember when, but I'm trusting my memory on this one. :-)] > OTOH there's nothing written down in the coding standards section of the wiki. > I think the loss of type safety isn't warranted, > so I'm going to approve this part, and see if anyone complains > (or wants you to do things differently). I guess I'll wait for someone to complain, then! :-P >>> > + >>> > +struct event_location [snip] >>> > +}; >>> >>> Making this struct opaque to its users has benefits. >>> Thoughts? >> >> I briefly considered this but didn't see an real benefits. Can you >> elaborate? If you feel that strongly about it, I can change it. > > Making structs opaque (where there's a choice) helps keep things > maintainable. e.g., only the implementation details one wants to expose > actually do get exposed. If it's a toss up, or close to one, I'd > go the opaque struct route. The patch is already providing accessors, > so why not make those accessors functions instead of macros? > A teensy bit more to write, but I think it's worth it. > [OTOH I haven't seen the rest of the patch set yet, > if it proves excessively onerous I'm happy to revisit.] Ok, in this next revision, I've changed this structure to be completely opaque... So instead of doing: struct location *location, copy_location; location = string_to_event_location (&arg, current_language); copy_location = *location; decode_line_full (©_location, ...); if (event_location_type (location) == LINESPEC_LOCATION) arg = get_linespec_location (©_location); /* to advance ARG over processed input */ the code now does: struct event_location *location, *copy_location; location = string_to_event_location (&arg, current_language); copy_location = copy_event_location_tmp (location); make_cleanup (xfree, copy_location); decode_line_full (copy_location, ...); event_location_advance_input_ptr (&arg, copy_location); do_cleanups (...); > Within location.c I would just access the struct members directly, > instead of with accessor macros. > [I know accessor macros is a GNU-ism, but we don't always use them > in dwarf2read.c (e.g.,: v.quick), and do just fine IMO.] Well, I kept my accessor macros in location.c (and nowhere else)... I like them, but I am not married to them. If you/others really want me to remove them, just say the word, and I'll zap 'em. >>> > +/* Return a string representation of the LOCATION. >>> > + This function may return NULL for unspecified linespecs, >>> > + e.g, EVENT_LOCATION_LINESPEC and addr_string is NULL. */ >>> > + >>> > +extern const char * >>> > + event_location_to_string (const struct event_location *location); >>> >>> I wonder if the string form might be computed >>> for some kind of location. In which case either we always >>> return a malloc'd copy here (remove const from result) >>> or lazily compute it and cache the computed string in the >>> struct (remove the const from the location parameter). >>> [Or switch to c++ and use mutable I guess. :)] As I mentioned the last time around, I've changed the event location API to compute this string when it can (it caches a copy of it inside the struct event_location -- I'm seeing this string computed many, many times). There are two instances in which we save the string instead of computing it: 1) pending breakpoints: we save the entire input string into the location (needed to preserve, e.g., conditions) 2) When converting/canonicalizing a linespec to explicit form, we must save the linespec string representation (so that we can reproduce the linespec the way the user originally specified it). The other option here is to add a flag to event_location to note this. Then we can compute the location. I chose to simply save the linespec string representation. Updated patch attached. Keith Changes since last revision: - Move struct event_location to location.c - Make struct event_location completely opaque - Turn all EVENT_LOCATION_* macros to functions. - Add get_linespec_location, copy_event_location_tmp, event_location_advance_input_ptr, advance_event_location_ptr, set_event_location_string - Remove duplicate function comments/"See description in foo.h." gdb/ChangeLog: * Makefile.in (SFILES): Add location.c. (HFILES_NO_SRCDIR): Add location.h. (COMMON_OBS): Add location.o. * location.c: New file. * location.h: New file. --- gdb/Makefile.in | 6 + gdb/location.c | 257 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ gdb/location.h | 125 +++++++++++++++++++++++++++ 3 files changed, 385 insertions(+), 3 deletions(-) create mode 100644 gdb/location.c create mode 100644 gdb/location.h diff --git a/gdb/Makefile.in b/gdb/Makefile.in index 3efedc8..266a6ec 100644 --- a/gdb/Makefile.in +++ b/gdb/Makefile.in @@ -805,7 +805,7 @@ SFILES = ada-exp.y ada-lang.c ada-typeprint.c ada-valprint.c ada-tasks.c \ inline-frame.c \ interps.c \ jv-exp.y jv-lang.c jv-valprint.c jv-typeprint.c jv-varobj.c \ - language.c linespec.c minidebug.c \ + language.c linespec.c location.c minidebug.c \ m2-exp.y m2-lang.c m2-typeprint.c m2-valprint.c \ macrotab.c macroexp.c macrocmd.c macroscope.c main.c maint.c \ mdebugread.c memattr.c mem-break.c minsyms.c mipsread.c memory-map.c \ @@ -887,7 +887,7 @@ mi/mi-out.h mi/mi-main.h mi/mi-common.h mi/mi-cmds.h linux-nat.h \ complaints.h gdb_proc_service.h gdb_regex.h xtensa-tdep.h inf-loop.h \ common/gdb_wait.h common/gdb_assert.h solib.h ppc-tdep.h cp-support.h glibc-tdep.h \ interps.h auxv.h gdbcmd.h tramp-frame.h mipsnbsd-tdep.h \ -amd64-linux-tdep.h linespec.h i387-tdep.h mn10300-tdep.h \ +amd64-linux-tdep.h linespec.h location.h i387-tdep.h mn10300-tdep.h \ sparc64-tdep.h monitor.h ppcobsd-tdep.h srec.h solib-pa64.h \ coff-pe-read.h parser-defs.h gdb_ptrace.h mips-linux-tdep.h \ m68k-tdep.h spu-tdep.h jv-lang.h environ.h solib-irix.h amd64-tdep.h \ @@ -964,7 +964,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \ source.o value.o eval.o valops.o valarith.o valprint.o printcmd.o \ block.o symtab.o psymtab.o symfile.o symfile-debug.o symmisc.o \ linespec.o dictionary.o \ - infcall.o \ + location.o infcall.o \ infcmd.o infrun.o \ expprint.o environ.o stack.o thread.o \ exceptions.o \ diff --git a/gdb/location.c b/gdb/location.c new file mode 100644 index 0000000..8111d7c --- /dev/null +++ b/gdb/location.c @@ -0,0 +1,257 @@ +/* Data structures and API for event locations in GDB. + Copyright (C) 2013-2014 Free Software Foundation, Inc. + + This file is part of GDB. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see . */ + +#include "defs.h" +#include "gdb_assert.h" +#include "location.h" +#include "symtab.h" +#include "language.h" +#include "linespec.h" +#include "cli/cli-utils.h" +#include "probe.h" + +#include +#include + +/* An event location used to set a stop event in the inferior. + This structure is an amalgam of the various ways + to specify where a stop event should be set. */ + +struct event_location +{ + /* The type of this breakpoint specification. */ + enum event_location_type type; +#define EL_TYPE(PTR) (PTR)->type + + union + { + /* A generic "this is a string specification" for a location. + This representation is used by both "normal" linespecs and + probes. */ + char *addr_string; +#define EL_LINESPEC(PTR) ((PTR)->u.addr_string) + } u; + + /* Cached string representation of this location. This is used, e.g., to + save stop event locations to file. Malloc'd. */ + char *as_string; +#define EL_STRING(PTR) ((PTR)->as_string) +}; + +/* See description in location.h. */ + +enum event_location_type +event_location_type (const struct event_location *location) +{ + return EL_TYPE (location); +} + +/* See description in location.h. */ + +struct event_location * +new_linespec_location (const char *linespec) +{ + struct event_location *location; + + location = XCNEW (struct event_location); + EL_TYPE (location) = LINESPEC_LOCATION; + if (linespec != NULL) + EL_LINESPEC (location) = xstrdup (linespec); + return location; +} + +/* See description in location.h. */ + +char * +get_linespec_location (struct event_location *location) +{ + gdb_assert (EL_TYPE (location) == LINESPEC_LOCATION); + return EL_LINESPEC (location); +} + +/* See description in location.h. */ + +struct event_location * +copy_event_location (const struct event_location *src) +{ + struct event_location *dst; + + dst = XCNEW (struct event_location); + EL_TYPE (dst) = EL_TYPE (src); + if (EL_STRING (src) != NULL) + EL_STRING (dst) = xstrdup (EL_STRING (src)); + + switch (EL_TYPE (src)) + { + case LINESPEC_LOCATION: + if (EL_LINESPEC (src) != NULL) + EL_LINESPEC (dst) = xstrdup (EL_LINESPEC (src)); + break; + + default: + gdb_assert_not_reached ("unknown event location type"); + } + + return dst; +} + +/* A cleanup function for struct event_location. */ + +static void +delete_event_location_cleanup (void *data) +{ + struct event_location *location = (struct event_location *) data; + + delete_event_location (location); +} + +/* See description in location.h. */ + +struct cleanup * +make_cleanup_delete_event_location (struct event_location *location) +{ + return make_cleanup (delete_event_location_cleanup, location); +} + +/* See description in location.h. */ + +void +delete_event_location (struct event_location *location) +{ + if (location != NULL) + { + xfree (EL_STRING (location)); + + switch (EL_TYPE (location)) + { + case LINESPEC_LOCATION: + xfree (EL_LINESPEC (location)); + break; + + default: + gdb_assert_not_reached ("unknown event location type"); + } + + xfree (location); + } +} + +/* See description in location.h. */ + +struct event_location * +copy_event_location_tmp (const struct event_location *src) +{ + struct event_location *tmp = XCNEW (struct event_location); + + memcpy (tmp, src, sizeof (struct event_location)); + return tmp; +} + +/* See description in location.h. */ + +char * +event_location_to_string_const (const struct event_location *location) +{ + char *result = NULL; + + if (EL_STRING (location) != NULL) + return xstrdup (EL_STRING (location)); + + switch (EL_TYPE (location)) + { + case LINESPEC_LOCATION: + if (EL_LINESPEC (location) != NULL) + result = xstrdup (EL_LINESPEC (location)); + break; + + default: + gdb_assert_not_reached ("unknown event location type"); + } + + return result; +} + +/* See description in location.h. */ + +const char * +event_location_to_string (struct event_location *location) +{ + /* Cache a copy of the string representation. */ + if (EL_STRING (location) == NULL) + EL_STRING (location) = event_location_to_string_const (location); + + return EL_STRING (location); +} + +/* See description in location.h. */ + +struct event_location * +string_to_event_location (char **stringp, + const struct language_defn *language) +{ + struct event_location *location; + + location = new_linespec_location (*stringp); + if (*stringp != NULL) + *stringp += strlen (*stringp); + + return location; +} + +/* See description in location.h. */ + +int +event_location_empty_p (const struct event_location *location) +{ + switch (EL_TYPE (location)) + { + case LINESPEC_LOCATION: + /* Linespecs are never "empty." (NULL is a valid linespec) */ + return 0; + + default: + gdb_assert_not_reached ("unknown event location type"); + } +} + +/* See description in location.h. */ + +void event_location_advance_input_ptr (char **inp, + const struct event_location *location) +{ + if (EL_TYPE (location) == LINESPEC_LOCATION) + *inp = EL_LINESPEC (location); +} + +/* See description in location.h. */ + +void +advance_event_location_ptr (struct event_location *location, size_t num) +{ + if (EL_TYPE (location) == LINESPEC_LOCATION) + EL_LINESPEC (location) += num; +} + +/* See description in location.h. */ + +void +set_event_location_string (struct event_location *location, + const char *string) +{ + EL_STRING (location) = xstrdup (string); +} diff --git a/gdb/location.h b/gdb/location.h new file mode 100644 index 0000000..705c83b --- /dev/null +++ b/gdb/location.h @@ -0,0 +1,125 @@ +/* Data structures and API for event locations in GDB. + Copyright (C) 2013, 2014 Free Software Foundation, Inc. + + This file is part of GDB. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see . */ + +#ifndef LOCATIONS_H +#define LOCATIONS_H 1 + +struct language_defn; +struct event_location; + +/* An enumeration of the various ways to specify a stop event + location (used with create_breakpoint). */ + +enum event_location_type +{ + /* A traditional linespec. */ + LINESPEC_LOCATION +}; + +/* Return the type of the given event location. */ + +extern enum event_location_type + event_location_type (const struct event_location *); + +/* Return a string representation of the LOCATION. + This function may return NULL for unspecified linespecs, + e.g, LOCATION_LINESPEC and addr_string is NULL. + + The result is cached in LOCATION. */ + +extern const char * + event_location_to_string (struct event_location *location); + +/* A const version of event_location_to_string that will not cache + the resulting string representation. The result is malloc'd + and must be freed by the caller. */ + +extern char * + event_location_to_string_const (const struct event_location *location); + +/* Create a new linespec location. The return result is malloc'd + and should be freed with delete_event_location. */ + +extern struct event_location * + new_linespec_location (const char *linespec); + +/* Return the linespec location (a string) of the given event_location + (which must be of type LINESPEC_LOCATION). */ + +extern char * + get_linespec_location (struct event_location *location); + +/* Free an event location and any associated data. */ + +extern void delete_event_location (struct event_location *location); + +/* Make a cleanup to free LOCATION. */ + +extern struct cleanup * + make_cleanup_delete_event_location (struct event_location *location); + +/* Return a copy of the given SRC location. */ + +extern struct event_location * + copy_event_location (const struct event_location *src); + +/* Allocate and "copy" the opaque struct event_location. This is used + when decoding locations which must parse their inputs. The return result + should be freed. */ + +extern struct event_location * + copy_event_location_tmp (const struct event_location *src); + +/* Attempt to convert the input string in *ARGP into an event location. + ARGP is advanced past any processed input. Returns a event_location + (malloc'd) if an event location was successfully found in *ARGP, + NULL otherwise. + + This function may call error() if *ARGP looks like properly formed, + but invalid, input, e.g., if it is called with missing argument parameters + or invalid options. + + The return result must be freed with delete_event_location. */ + +extern struct event_location * + string_to_event_location (char **argp, + const struct language_defn *langauge); + +/* A convenience function for testing for unset locations. */ + +extern int event_location_empty_p (const struct event_location *location); + +/* Advance *INP past any processed input in the LOCATION. This function + is used in conjunction with copy_event_location_tmp to allow + linespec and probe locations to advance the input pointer. */ + +extern void + event_location_advance_input_ptr (char **inp, + const struct event_location *location); + +/* Advance the location's input pointer NUM bytes. */ + +extern void + advance_event_location_ptr (struct event_location *tmp_location, size_t num); + +/* Set the location's string representation. */ + +extern void + set_event_location_string (struct event_location *location, + const char *string); +#endif /* LOCATIONS_H */