From patchwork Fri May 30 18:16:32 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Keith Seitz X-Patchwork-Id: 1210 Received: (qmail 20835 invoked by alias); 30 May 2014 18:16:42 -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 20819 invoked by uid 89); 30 May 2014 18:16:40 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 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 ESMTP; Fri, 30 May 2014 18:16:36 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s4UIGY3k021610 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Fri, 30 May 2014 14:16:34 -0400 Received: from valrhona.uglyboxes.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s4UIGWLK018723 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Fri, 30 May 2014 14:16:33 -0400 Message-ID: <5388CB00.6040205@redhat.com> Date: Fri, 30 May 2014 11:16:32 -0700 From: Keith Seitz User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.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> In-Reply-To: <21361.21080.298225.332248@ruffy.mtv.corp.google.com> X-IsSubscribed: yes Hi, Doug, Thank you for taking a look! On 05/12/2014 03:59 PM, Doug Evans wrote: > Keith Seitz writes: > > +/* Free LOCATION and any associated data. */ > > + > > +void > > +delete_event_location (void *data) > > I'm sure there's a reason why data is void * and not > properly typed. It would be good to document that > reason here. [Or can we make it struct event_location *?] Yes -- it's used as a cleanup. I've changed this to typed and added a utility cleanup, make_cleanup_delete_event_location instead. > > +/* Parse the user input in *STRINGP and turn it into a struct > > + event_location, advancing STRINGP past any parsed input. > > + Return value is malloc'd. */ > > + > > +struct event_location * > > +string_to_event_location (char **stringp, > > + const struct language_defn *language) > > +{ > > + struct event_location *location; > > + > > + location = new_event_location (EVENT_LOCATION_LINESPEC); > > + if (*stringp != NULL) > > + { > > + EVENT_LOCATION_LINESPEC (location) = xstrdup (*stringp); > > + *stringp += strlen (*stringp); > > + } > > + > > + return location; > > +} > > This consumes the entire string, so we can remove the side-effect > of modifying the input argument. > It might make the caller (slightly) more complicated, but I like the > simplification here. > [Or at any rate IWBN to have a version that took a const char *.] It is true that this current version of this bit of code does some non-sensical stuff such as this, but in future versions, they actually do something useful. Because of the requirement to pass create_breakpoint a structure representing the location (instead of simply a string), we must now split the "advancing past the input" problem in two. This function (string_to_event_location) will (eventually) handle explicit locations, address locations, and probe locations. However, for linespec locations, we must defer this until the linespec is parsed since the linespec grammar itself is ambiguous. I could not come up with a scheme that could unify these operations (without violating the struct requirement). If you or anyone else has an idea, I'm all eyes. See, for example, the complete version of this function in patch #7 (where the UI elements for explicit locations is added). > Plus, for robustness sake, should we treat "" as unspecified? > [and thus leave the string as NULL so event_location_empty_p > will return true] ""/NULL is only valid for a linespec (as in the user simply typing "break" with no argument). Not permitted for any other location type. > > +/* An event location used to set a stop event in the inferior. > > + This structure is an amalgam of the various ways > > + to specify a where a stop event should be set. */ > > specify where Doh! Fixed. > > + > > +struct event_location > > +{ > > + /* The type of this breakpoint specification. */ > > + enum event_location_type type; > > +#define EVENT_LOCATION_TYPE(S) ((S)->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 EVENT_LOCATION_LINESPEC(S) ((S)->u.addr_string) > > + } u; > > + > > + /* A string representation of how this location may be > > + saved. This is used to save stop event locations to file. > > + Malloc'd. */ > > + char *save_spec; > > +#define EVENT_LOCATION_SAVE_SPEC(S) ((S)->save_spec) > > +}; > > 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. > > +/* 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. :)] Ah, so here's why it's taken me so long to respond... After reading this, I played around with changing event_location_to_string as you pondered above. At the time, with the version I had, it was easier to "pre-compute" (= "save") the original string the user typed (post-canonicalization). I quickly ran into an edge case that was not satisfactorily handled by the code as submitted. If an MI client sets a breakpoint with an explicit location, how is this breakpoint serialized (for saving to a file or pending)? In the previous version, it would be converted into a linespec. While not horrible, it opens the door for ambiguity. I feel it is better to serialize to an explicit form instead, which is what this next revision does. So, after all that, event_location_to_string will compute the string value if it doesn't have a cached copy available. [Caching is a win, IMO. I see this string representation hit multiple times during breakpoint_re_set.] > > +extern int event_location_empty_p (const struct event_location *location); > > blank line > Fixed. Phew! I appreciate very much that you've taken the time to take a look at this. I know this is going to be painful for us all. Keith PS. I am not reposting #1 -- it hasn't changed. Changes from previous version: - EVENT_LOCATION_SAVE_SPEC removed (sub as_string where necessary) - delete_event_location now takes struct event_location* - add make_cleanup_delete_event_location - add event_location_to_string_const (necessary evil IMO) - fix typo in locations.h (struct event_location decl) - struct event_location.save_spec -> as_string - Update event_location_to_string{,_const} decls - Add missing blank line at end of location.h - new_event_location -> new_linespec_location - EVENT_LOCATION_LINESPEC -> LINESPEC_LOCATION for type - new_linespec_location instead of new_event_location ChangeLog 2014-05-29 Keith Seitz * 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. 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..57900bf --- /dev/null +++ b/gdb/location.c @@ -0,0 +1,194 @@ +/* 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 + +/* Initialize the given LOCATION. */ + +void +initialize_event_location (struct event_location *location, + enum event_location_type type) +{ + memset (location, 0, sizeof (struct event_location)); + EVENT_LOCATION_TYPE (location) = type; +} + +/* Create a new linespec location. The return result is malloc'd + and should be freed with delete_event_location. */ + +struct event_location * +new_linespec_location (const char *linespec) +{ + struct event_location *location; + + location = XNEW (struct event_location); + initialize_event_location (location, LINESPEC_LOCATION); + if (linespec != NULL) + EVENT_LOCATION_LINESPEC (location) = xstrdup (linespec); + return location; +} + +/* Return a copy of the given SRC location. */ + +struct event_location * +copy_event_location (const struct event_location *src) +{ + struct event_location *dst; + + dst = XCNEW (struct event_location); + EVENT_LOCATION_TYPE (dst) = EVENT_LOCATION_TYPE (src); + if (src->as_string != NULL) + dst->as_string = xstrdup (src->as_string); + + switch (EVENT_LOCATION_TYPE (src)) + { + case LINESPEC_LOCATION: + EVENT_LOCATION_LINESPEC (dst) = xstrdup (EVENT_LOCATION_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); +} + +/* Make a cleanup to free LOCATION. */ + +struct cleanup * +make_cleanup_delete_event_location (struct event_location *location) +{ + return make_cleanup (delete_event_location_cleanup, location); +} + +/* Free LOCATION and any associated data. */ + +void +delete_event_location (struct event_location *location) +{ + if (location != NULL) + { + xfree (location->as_string); + + switch (EVENT_LOCATION_TYPE (location)) + { + case LINESPEC_LOCATION: + xfree (EVENT_LOCATION_LINESPEC (location)); + break; + + default: + gdb_assert_not_reached ("unknown event location type"); + } + + xfree (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. */ + +char * +event_location_to_string_const (const struct event_location *location) +{ + char *result = NULL; + + if (location->as_string != NULL) + return xstrdup (location->as_string); + + switch (EVENT_LOCATION_TYPE (location)) + { + case LINESPEC_LOCATION: + if (EVENT_LOCATION_LINESPEC (location) != NULL) + result = xstrdup (EVENT_LOCATION_LINESPEC (location)); + break; + + default: + gdb_assert_not_reached ("unknown event location type"); + } + + return result; +} + +/* Return a string representation of the LOCATION. + This function may return NULL for unspecified linespecs, + e.g, LINESPEC_LOCATION and addr_string is NULL. + + The result is cached in LOCATION. */ + +const char * +event_location_to_string (struct event_location *location) +{ + /* Cache a copy of the string representation. */ + if (location->as_string == NULL) + location->as_string = event_location_to_string_const (location); + + return location->as_string; +} + +/* Parse the user input in *STRINGP and turn it into a struct + event_location, advancing STRINGP past any parsed input. + Return value is malloc'd. */ + +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; +} + +/* A convenience function for testing for unset locations. */ + +int +event_location_empty_p (const struct event_location *location) +{ + switch (EVENT_LOCATION_TYPE (location)) + { + case LINESPEC_LOCATION: + return EVENT_LOCATION_LINESPEC (location) == NULL; + + default: + gdb_assert_not_reached ("unknown event location type"); + } +} diff --git a/gdb/location.h b/gdb/location.h new file mode 100644 index 0000000..418e271 --- /dev/null +++ b/gdb/location.h @@ -0,0 +1,115 @@ +/* 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; + +/* 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 +}; + +/* 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 EVENT_LOCATION_TYPE(S) ((S)->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 EVENT_LOCATION_LINESPEC(S) ((S)->u.addr_string) + } u; + + /* Cached string representation of this location. This is used to + save stop event locations to file. Malloc'd. */ + char *as_string; +}; + +/* 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. + + 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); + +/* 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); + +/* 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 a copy of the given SRC location. */ + +extern struct event_location * + copy_event_location (const struct event_location *src); + +/* Initialize the given LOCATION. */ + +extern void initialize_event_location (struct event_location *location, + enum event_location_type type); + +/* 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. */ + +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); + +#endif /* LOCATIONS_H */