From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12410 invoked by alias); 17 Dec 2013 11:03:48 -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 12395 invoked by uid 89); 17 Dec 2013 11:03:48 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.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 ESMTP; Tue, 17 Dec 2013 11:03:47 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rBHB3jSI013648 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 17 Dec 2013 06:03:46 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id rBHB3YHK029659; Tue, 17 Dec 2013 06:03:44 -0500 Message-ID: <52B02F82.3020907@redhat.com> Date: Tue, 17 Dec 2013 11:03:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: Sergio Durigan Junior CC: GDB Patches Subject: Re: [PATCH] Extend SystemTap SDT probe argument parser References: <1386734160-29837-1-git-send-email-sergiodj@redhat.com> In-Reply-To: <1386734160-29837-1-git-send-email-sergiodj@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-12/txt/msg00621.txt.bz2 On 12/11/2013 03:55 AM, Sergio Durigan Junior wrote: > I have chosen to implement this as a list of const strings, which needs > to be declared as "static const char *const *", and is provided to > gdbarch on initialization. I think it is cleaner to implement it this > way, but for a moment I wondered whether demanding the variables to be > declared as "static" is a good idea... After some thought and > discussion, I decided to leave it as is. AFAICS, nothing in the interface "demands" static. What is required is that the array outlives the gdbarch method call. So usually, you'll make the array either static global, or static to a function. That is, this would work just as well with the same gdbarch interface: --- a/gdb/amd64-tdep.c +++ b/gdb/amd64-tdep.c +const char *const stap_integer_prefix[] = { "$", NULL }; +const char *const stap_register_prefix[] = { "%", NULL }; +const char *const stap_register_indirection_prefix[] = { "(", NULL }; +const char *const stap_register_indirection_suffix[] = { ")", NULL }; + void amd64_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch) { struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); const struct target_desc *tdesc = info.target_desc; Or even, allocating "stap_integer_prefix" etc. on the heap (xmalloc, etc.). "static" or not is a detail of the arch's gdbarch hook implementation. > + * gdbarch.sh (stap_integer_prefix, stap_integer_suffix) > + (stap_register_prefix, stap_register_suffix) > + (stap_register_indirection_prefix) > + (stap_register_indirection_suffix): Declare as "const char *const > + *" instead of "const char *". Adjust printing function. As we're touching all the hooks anyway, can we rename them to the plural "prefixes" and "suffixes" ? I'd find that clearer. > > +static char * > +pstring_list (const char *const *list) Please add an intro comment. > +{ > + static char ret[100]; > + const char *const *p; > + int offset = 0; size_t. > + > + if (list == NULL) > + return "(null)"; > + > + ret[0] = '\0'; > + for (p = list; *p != NULL && offset < sizeof (ret); ++p) > + { > + xsnprintf (ret + offset, sizeof (ret) - offset, "%s, ", *p); > + offset += 2 + strlen (*p); You can use the return of xsnprintf instead of strlen. > + } > + Also, currently unlikely, but if this function ends up used in the future with a bigger list, we'll get silent truncation. I think we should assert instead that doesn't happen. > + if (offset > 0) > + ret[offset - 2] = '\0'; > + > + return ret; > +} > + > # in this case, this prefix would be the character \`\$\'. > -v:const char *:stap_integer_prefix:::0:0::0:pstring (gdbarch->stap_integer_prefix) > +# > +# This variable must be declared as \`static const char *const var\[\]\', > +# and must also be NUL-terminated, like the following example: > +# It's a NULL pointer, not NUL, the null character. So, "NULL-terminated". I'd remove the "must be" part, and just say: - Prefix used to mark an integer constant on the architecture's assembly + A NULL-terminated array of prefixes used to mark an integer constant on the architecture's assembly. For example, on x86 integer constants are written as: ( reindented, of course ) > +/* Helper function to check for a generic list of prefixes. Return 1 > + if any prefix has been found, zero otherwise. */ Please describe the parameters and the contract. Some of these arguments can be NULL. We do a case insensitive match. Etc. Likewise stap_generic_check_suffix and possibly others. If there's some central function that describes all this, then you can point at it: "arguments are like foo", or some such. > + > +static int > +stap_is_generic_prefix (struct gdbarch *gdbarch, const char *s, > + const char **r, const char *const *prefixes) > +{ > + /* A NULL value here means that integers do not have prefix. We > + just check for a digit then. */ "have a prefix" ? Otherwise looks good to me. Thanks, -- Pedro Alves