From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11517 invoked by alias); 12 May 2014 22:59:41 -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 11506 invoked by uid 89); 12 May 2014 22:59:40 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.0 required=5.0 tests=none 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; Mon, 12 May 2014 22:59:39 +0000 Received: by mail-yh0-f74.google.com with SMTP id 29so1083250yhl.1 for ; Mon, 12 May 2014 15:59:37 -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=MNhUfir8wQ1clDXZTi8SegCXlZ7672HWal0OMkAaT4g=; b=H89rb6MJDwByf9AajTsO21m6pa6/+btdItubYQvyUbwRYlp2uI8lZZUrrTKhaeYwM7 DyR4XfQhnIdY0p4xBc9JKuj7VbIvJ7tQBrz9KvY624HOwk9uuWKcqa6LrMMXax6jxPOj WXgcCLe8Ow3qYjXUttupw2SgURa7KhYdbOqgXxad0q1lWcslYOWK0sQM/niDxFK4JprE TnGwyvtHWrHxJ4Dw+RabgtIvkz7I6tSAoVgl+N+/v4enp5XZTXk8hzxV2ep3Qm4xXRlq X8fx+sTJcUeDOQ9wvbCvFuEZaquXKkmInkLfCWuiPRCosa5uz3ObEyAA8FycQPMykKjl VWuQ== X-Gm-Message-State: ALoCoQlEhv3CLZOOpotoP9J8aZhx47a/TJQHZlRCPl4LfIl/IXnh+LXih2na5hOZxeX1ZZAuWwGL X-Received: by 10.58.30.78 with SMTP id q14mr15022809veh.10.1399935577435; Mon, 12 May 2014 15:59:37 -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 x22si688389yhd.5.2014.05.12.15.59.37 for (version=TLSv1.1 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Mon, 12 May 2014 15:59:37 -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 DFAA831C1CC; Mon, 12 May 2014 15:59:36 -0700 (PDT) From: Doug Evans MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-ID: <21361.21080.298225.332248@ruffy.mtv.corp.google.com> Date: Mon, 12 May 2014 22:59:00 -0000 To: Keith Seitz Cc: "gdb-patches@sourceware.org ml" Subject: Re: [RFA 2/9] Explicit locations v2 - Event locations API In-Reply-To: <536BC5DE.5050707@redhat.com> References: <536BC5DE.5050707@redhat.com> X-IsSubscribed: yes X-SW-Source: 2014-05/txt/msg00140.txt.bz2 Keith Seitz writes: > Hi, > > This patch is also a nop. In fact, the code added by this patch isn't > even used. I present here the new locations API which will be > used/elaborated upon by subsequent patches. > > The basic premise here is that the breakpoint methods will no longer > accept char*/char** argument "address strings." They will take a > "location", which is a structure: > > struct event_location > { > /* The type of this breakpoint specification. */ > enum event_location_type type; > #define EVENT_LOCATION_TYPE(S) ((S)->type) > > union > { > /* data needed for the various location types */ > } u; > }; > > The UIs will use the new API function string_to_event_location to turn > arbitrary text input into one of these structures. > > This patch and the next only implement linespec locations. In other > words, it only massages the current API without introducing any new > features. > > Keith > > ChangeLog > 2014-05-08 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..3f50bed > --- /dev/null > +++ b/gdb/location.c > @@ -0,0 +1,159 @@ > +/* 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 location with the given TYPE. */ > + > +struct event_location * > +new_event_location (enum event_location_type type) > +{ > + struct event_location *location; > + > + location = XNEW (struct event_location); > + initialize_event_location (location, type); > + 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 (EVENT_LOCATION_SAVE_SPEC (src) != NULL) > + EVENT_LOCATION_SAVE_SPEC (dst) = xstrdup (EVENT_LOCATION_SAVE_SPEC (src)); > + > + switch (EVENT_LOCATION_TYPE (src)) > + { > + case EVENT_LOCATION_LINESPEC: > + EVENT_LOCATION_LINESPEC (dst) = xstrdup (EVENT_LOCATION_LINESPEC (src)); > + break; > + > + default: > + gdb_assert_not_reached ("unknown event location type"); > + } > + > + return dst; > +} > + > +/* 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 *?] > +{ > + struct event_location *location > + = (struct event_location *) data; > + > + if (location != NULL) > + { > + xfree (EVENT_LOCATION_SAVE_SPEC (location)); > + > + switch (EVENT_LOCATION_TYPE (location)) > + { > + case EVENT_LOCATION_LINESPEC: > + xfree (EVENT_LOCATION_LINESPEC (location)); > + break; > + > + default: > + gdb_assert_not_reached ("unknown event location type"); > + } > + > + xfree (location); > + } > +} > + > +/* 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. */ > + > +const char * > +event_location_to_string (const struct event_location *location) > +{ > + const char *result = NULL; > + > + switch (EVENT_LOCATION_TYPE (location)) > + { > + case EVENT_LOCATION_LINESPEC: > + result = EVENT_LOCATION_LINESPEC (location); > + break; > + > + default: > + gdb_assert_not_reached ("unknown event location type"); > + } > + > + return result; > +} > + > +/* 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 *.] Plus, for robustness sake, should we treat "" as unspecified? [and thus leave the string as NULL so event_location_empty_p will return true] > + > +/* A convenience function for testing for unset locations. */ > + > +int > +event_location_empty_p (const struct event_location *location) > +{ > + switch (EVENT_LOCATION_TYPE (location)) > + { > + case EVENT_LOCATION_LINESPEC: > + 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..83fc3a4 > --- /dev/null > +++ b/gdb/location.h > @@ -0,0 +1,100 @@ > +/* 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. */ > + EVENT_LOCATION_LINESPEC > +}; > + > +/* 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 > + > +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? > + > +/* 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. :)] > + > +/* Free an event location and any associated data. */ > + > +extern void delete_event_location (void *data); > + > +/* Create a new event location with the given TYPE. */ > + > +extern struct event_location * > + new_event_location (enum event_location_type type); > + > +/* 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); blank line > +#endif /* LOCATIONS_H */ -- /dje