Re: [PATCH] Extend SystemTap SDT probe argument parser

[Pedro Alves <palves@redhat.com>]

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