Patchwork [RFC,1/5] Add helper functions check_for_flags and check_for_flags_vqcs

login
register
mail settings
Submitter Philippe Waroquiers
Date May 5, 2018, 7:28 p.m.
Message ID <20180505192804.12731-2-philippe.waroquiers@skynet.be>
Download mbox | patch
Permalink /patch/27124/
State New
Headers show

Comments

Philippe Waroquiers - May 5, 2018, 7:28 p.m.
check_for_flags helper function allows to look for a set of flags at
the start of a string.

check_for_flags_vqcs is a specialised helper function to handle
the flags vqcs, that are used in the new command 'frame apply'
and in the command 'thread apply.

Modify number_or_range_parser::get_number to differentiate a
- followed by digits from a - followed by a flag (i.e. an alpha).
That is needed for the addition of the optional -FLAGS... argument to
thread apply ID... [-FLAGS...] COMMAND

Modify gdb/testsuite/gdb.multi/tids.exp, as the gdb error message
for 'thread apply -$unknownconvvar p 1'
is now the more clear:
  Invalid thread ID: -$unknownconvvar p 1
instead of previously:
  negative value
---
 gdb/cli/cli-utils.c              | 116 ++++++++++++++++++++++++++++++++++++++-
 gdb/cli/cli-utils.h              |  30 ++++++++++
 gdb/testsuite/gdb.multi/tids.exp |   4 +-
 3 files changed, 145 insertions(+), 5 deletions(-)
Simon Marchi - May 18, 2018, 1:22 a.m.
Hi Philippe,

I don't see the full picture yet just with this patch, so here is a
first pass of comments, more nits than anything else.

On 2018-05-05 03:28 PM, Philippe Waroquiers wrote:
> check_for_flags helper function allows to look for a set of flags at
> the start of a string.
> 
> check_for_flags_vqcs is a specialised helper function to handle
> the flags vqcs, that are used in the new command 'frame apply'
> and in the command 'thread apply.
> 
> Modify number_or_range_parser::get_number to differentiate a
> - followed by digits from a - followed by a flag (i.e. an alpha).
> That is needed for the addition of the optional -FLAGS... argument to
> thread apply ID... [-FLAGS...] COMMAND
> 
> Modify gdb/testsuite/gdb.multi/tids.exp, as the gdb error message
> for 'thread apply -$unknownconvvar p 1'
> is now the more clear:
>   Invalid thread ID: -$unknownconvvar p 1
> instead of previously:
>   negative value
> ---
>  gdb/cli/cli-utils.c              | 116 ++++++++++++++++++++++++++++++++++++++-
>  gdb/cli/cli-utils.h              |  30 ++++++++++
>  gdb/testsuite/gdb.multi/tids.exp |   4 +-
>  3 files changed, 145 insertions(+), 5 deletions(-)
> 
> diff --git a/gdb/cli/cli-utils.c b/gdb/cli/cli-utils.c
> index c55b5035e4..4abd501d52 100644
> --- a/gdb/cli/cli-utils.c
> +++ b/gdb/cli/cli-utils.c
> @@ -22,7 +22,6 @@
>  #include "value.h"
>  
>  #include <ctype.h>
> -

Unintended change?

>  /* See documentation in cli-utils.h.  */
>  
>  int
> @@ -169,7 +168,10 @@ number_or_range_parser::get_number ()
>        /* Default case: state->m_cur_tok is pointing either to a solo
>  	 number, or to the first number of a range.  */
>        m_last_retval = get_number_trailer (&m_cur_tok, '-');
> -      if (*m_cur_tok == '-')
> +      /* if get_number_trailer has found a -, it might be the start

Capital "i"

> +	 of flags. So, do not parse a range if the - is followed by
> +	 an alpha.  */
> +      if (*m_cur_tok == '-' && !isalpha(*(m_cur_tok+1)))

Missing space after isalpha, spaces around +.  It could also be written
m_cur_token[1].

Could you add some simple unit tests in the unittests/ directory for
number_or_range_parser (I don't think there are any so far)?  It will
help illustrate your use case.

>  	{
>  	  const char **temp;
>  
> @@ -196,7 +198,10 @@ number_or_range_parser::get_number ()
>  	}
>      }
>    else
> -    error (_("negative value"));
> +    {
> +      if (*m_cur_tok >= '0' && *m_cur_tok <= '9')

Use isdigit?

> +	error (_("negative value"));
> +    }
>    m_finished = *m_cur_tok == '\0';
>    return m_last_retval;
>  }
> @@ -304,3 +309,108 @@ check_for_argument (const char **str, const char *arg, int arg_len)
>      }
>    return 0;
>  }
> +

Add a

  /* See cli-utils.h.  */

> +int
> +check_for_flags (const char **str, const char *flags,
> +		 int *flags_counts)
> +{
> +  const char *p = *str;
> +  bool all_valid = true;
> +
> +  /* First set the flags_counts to 0.  */
> +  {
> +    const char *f = flags;
> +    while (*f)
> +      {
> +	flags_counts[f - flags] = 0;
> +	f++;
> +      }
> +  }

What about something like

  memset (flags_count, 0, sizeof (flags_count[0]) * strlen (flags));

> +
> +  if (*p != '-')
> +    return 0;
> +
> +  p++;
> +  /* If - is the last char, or is followed by a space or a $, then
> +     we do not have flags.  */
> +  if (*p == '\0' || (isspace (*p) || *p == '$'))

You can remove some parenthesis:

  if (*p == '\0' || isspace (*p) || *p == '$')

But could we be more restrictive here and only allow isalpha characters?

  if (!isalpha (*p))
    return 0;

> +    return 0;
> +
> +  /* We might have a command that accepts optional flags followed by
> +     a negative integer. So, do not handle a negative integer as invalid
> +     flags : rather let the caller handle the negative integer.  */
> +  {
> +    const char *p1 = p;
> +    while (*p1 >= '0' && *p1 <= '9')

isdigit?

> +      ++p1;
> +    if (p != p1 && (*p1 == '\0' || isspace (*p1)))
> +      return 0;
> +  }
> +
> +  /* We have a set of flags :
> +     Scan and validate the flags, and update flags_counts for valid flags.  */
> +  while (*p != '\0' && !isspace (*p))
> +    {
> +      const char *f = flags;
> +      bool valid = false;
> +
> +      while (*f && !valid)

*f != nullptr

If I understand the algorithm right, I think it is not necessary to check for
!valid here.

> +	{
> +	  valid = *f == *p;
> +	  if (valid)
> +	    {
> +	      flags_counts[f - flags]++;
> +	      break;
> +	    }
> +	  f++;
> +	}
> +      all_valid = all_valid && valid;
> +      p++;
> +    }
> +
> +  /* Skip the space(s) */
> +  while (*p && isspace((int) *p))

Space after isspace, but it could probably use the skip_spaces function.

> +    ++p;
> +
> +  if (all_valid)
> +    {
> +      *str = p;
> +      return 1;
> +    }
> +  else
> +    return -1;
> +}
> +

/* See cli-utils.h.  */

> +int
> +check_for_flags_vqcs (const char *which_command, const char **str,
> +		      int *print_what_v, int max_v,
> +		      bool *cont, bool *silent)
> +{
> +  const char *flags = "vqcs";
> +  int flags_counts[4];

Maybe use strlen (flags) instead of the literal 4?

> +  int res;
> +
> +  *cont = false;
> +  *silent = false;
> +
> +  res = check_for_flags (str, flags, flags_counts);
> +  if (res == 0)
> +    return 0;
> +  if (res == -1)
> +    error (_("%s only accepts flags %s"), which_command, flags);
> +  gdb_assert (res == 1);
> +
> +  *print_what_v = *print_what_v + flags_counts[0] - flags_counts[1];
> +  if (*print_what_v < 0)
> +    *print_what_v = 0;
> +  if (*print_what_v > max_v)
> +    *print_what_v = max_v;
> +
> +  *cont = flags_counts[2] > 0;
> +  *silent = flags_counts[3] > 0;
> +  if (*cont && *silent)
> +    error (_("%s does not accept at the same time flags c and s"),
> +	   which_command);

Another common way to say this would be

  ""%s: flags c and s are mutually exclusive."

Thanks,

Simon
Philippe Waroquiers - May 18, 2018, 9:42 p.m.
On Thu, 2018-05-17 at 21:22 -0400, Simon Marchi wrote:
> > +  /* First set the flags_counts to 0.  */
> > +  {
> > +    const char *f = flags;
> > +    while (*f)
> > +      {
> > +	flags_counts[f - flags] = 0;
> > +	f++;
> > +      }
> > +  }
> 
> What about something like
> 
>   memset (flags_count, 0, sizeof (flags_count[0]) * strlen (flags));

The code initialising the flags_counts is somewhat similar in structure
to the code that increments the flags_counts.

So, it looks more clear to me to have the zero-ing code and
the incrementing code looking like each other.

But if for gdb, using memset is the typical pattern to zero
an array of int, fine for me.

What do you think ?

Philippe
Simon Marchi - May 18, 2018, 11:38 p.m.
On 2018-05-18 05:42 PM, Philippe Waroquiers wrote:
> On Thu, 2018-05-17 at 21:22 -0400, Simon Marchi wrote:
>>> +  /* First set the flags_counts to 0.  */
>>> +  {
>>> +    const char *f = flags;
>>> +    while (*f)
>>> +      {
>>> +	flags_counts[f - flags] = 0;
>>> +	f++;
>>> +      }
>>> +  }
>>
>> What about something like
>>
>>   memset (flags_count, 0, sizeof (flags_count[0]) * strlen (flags));
> 
> The code initialising the flags_counts is somewhat similar in structure
> to the code that increments the flags_counts.
> 
> So, it looks more clear to me to have the zero-ing code and
> the incrementing code looking like each other.
> 
> But if for gdb, using memset is the typical pattern to zero
> an array of int, fine for me.
> 
> What do you think ?

I don't really mind, I just thought the memset was short and clear,
but both work.

Simon
Philippe Waroquiers - May 19, 2018, 5:15 a.m.
On Fri, 2018-05-18 at 19:38 -0400, Simon Marchi wrote:
> I don't really mind, I just thought the memset was short and clear,
> but both work.
Ok, I will use memset (clearly, the usual pattern).

Thanks for the feedback.
I will start working on the comments.

Philippe

Patch

diff --git a/gdb/cli/cli-utils.c b/gdb/cli/cli-utils.c
index c55b5035e4..4abd501d52 100644
--- a/gdb/cli/cli-utils.c
+++ b/gdb/cli/cli-utils.c
@@ -22,7 +22,6 @@ 
 #include "value.h"
 
 #include <ctype.h>
-
 /* See documentation in cli-utils.h.  */
 
 int
@@ -169,7 +168,10 @@  number_or_range_parser::get_number ()
       /* Default case: state->m_cur_tok is pointing either to a solo
 	 number, or to the first number of a range.  */
       m_last_retval = get_number_trailer (&m_cur_tok, '-');
-      if (*m_cur_tok == '-')
+      /* if get_number_trailer has found a -, it might be the start
+	 of flags. So, do not parse a range if the - is followed by
+	 an alpha.  */
+      if (*m_cur_tok == '-' && !isalpha(*(m_cur_tok+1)))
 	{
 	  const char **temp;
 
@@ -196,7 +198,10 @@  number_or_range_parser::get_number ()
 	}
     }
   else
-    error (_("negative value"));
+    {
+      if (*m_cur_tok >= '0' && *m_cur_tok <= '9')
+	error (_("negative value"));
+    }
   m_finished = *m_cur_tok == '\0';
   return m_last_retval;
 }
@@ -304,3 +309,108 @@  check_for_argument (const char **str, const char *arg, int arg_len)
     }
   return 0;
 }
+
+int
+check_for_flags (const char **str, const char *flags,
+		 int *flags_counts)
+{
+  const char *p = *str;
+  bool all_valid = true;
+
+  /* First set the flags_counts to 0.  */
+  {
+    const char *f = flags;
+    while (*f)
+      {
+	flags_counts[f - flags] = 0;
+	f++;
+      }
+  }
+
+  if (*p != '-')
+    return 0;
+
+  p++;
+  /* If - is the last char, or is followed by a space or a $, then
+     we do not have flags.  */
+  if (*p == '\0' || (isspace (*p) || *p == '$'))
+    return 0;
+
+  /* We might have a command that accepts optional flags followed by
+     a negative integer. So, do not handle a negative integer as invalid
+     flags : rather let the caller handle the negative integer.  */
+  {
+    const char *p1 = p;
+    while (*p1 >= '0' && *p1 <= '9')
+      ++p1;
+    if (p != p1 && (*p1 == '\0' || isspace (*p1)))
+      return 0;
+  }
+
+  /* We have a set of flags :
+     Scan and validate the flags, and update flags_counts for valid flags.  */
+  while (*p != '\0' && !isspace (*p))
+    {
+      const char *f = flags;
+      bool valid = false;
+
+      while (*f && !valid)
+	{
+	  valid = *f == *p;
+	  if (valid)
+	    {
+	      flags_counts[f - flags]++;
+	      break;
+	    }
+	  f++;
+	}
+      all_valid = all_valid && valid;
+      p++;
+    }
+
+  /* Skip the space(s) */
+  while (*p && isspace((int) *p))
+    ++p;
+
+  if (all_valid)
+    {
+      *str = p;
+      return 1;
+    }
+  else
+    return -1;
+}
+
+int
+check_for_flags_vqcs (const char *which_command, const char **str,
+		      int *print_what_v, int max_v,
+		      bool *cont, bool *silent)
+{
+  const char *flags = "vqcs";
+  int flags_counts[4];
+  int res;
+
+  *cont = false;
+  *silent = false;
+
+  res = check_for_flags (str, flags, flags_counts);
+  if (res == 0)
+    return 0;
+  if (res == -1)
+    error (_("%s only accepts flags %s"), which_command, flags);
+  gdb_assert (res == 1);
+
+  *print_what_v = *print_what_v + flags_counts[0] - flags_counts[1];
+  if (*print_what_v < 0)
+    *print_what_v = 0;
+  if (*print_what_v > max_v)
+    *print_what_v = max_v;
+
+  *cont = flags_counts[2] > 0;
+  *silent = flags_counts[3] > 0;
+  if (*cont && *silent)
+    error (_("%s does not accept at the same time flags c and s"),
+	   which_command);
+
+  return res;
+}
diff --git a/gdb/cli/cli-utils.h b/gdb/cli/cli-utils.h
index e34ee0df37..defbedf980 100644
--- a/gdb/cli/cli-utils.h
+++ b/gdb/cli/cli-utils.h
@@ -173,4 +173,34 @@  check_for_argument (char **str, const char *arg, int arg_len)
 			     arg, arg_len);
 }
 
+/* A helper function that looks for a set of flags at the start of a
+   string.  The possible flags are given as a null terminated string.
+   The flags must also either be at the end of the string,
+   or be followed by whitespace.  Returns 1 if it finds only valid flags,
+   0 if no flags are found, -1 if invalid flags are found.
+   FLAGS_COUNTS must be an int array of length equal to strlen(flags).
+   Sets *FLAGS_COUNTS to the number of times the corresponding flag
+   was found in *STR.
+   If only valid flags are found, it updates *STR.  Note that a negative
+   integer (e.g. -123) will not be considered as an invalid set of flags.  */
+extern int check_for_flags (const char **str, const char *flags,
+			    int *flags_counts);
+
+/* A helper function that uses check_for_flags to handle the flags vqcs :
+     A flag v increases *print_what_v.
+     A flag q decreases *print_what_v.
+     A flag c sets *cont to true, otherwise to false.
+     A flag s sets *silent to true, otherwise to false.
+   The caller must initialise *PRINT_WHAT_V to the default verbosity.
+   MAX_V gives the maximum verbosity : the value returned in *PRINT_WHAT_V
+   will always be >= 0 and <= MAX_V.
+   WHICH_COMMAND is used in the error message in case invalid flags are found.
+
+   Returns 1 and updates *STR if it finds only valid flags.
+   Returns 0 if no flags are found.
+   Raises an error if invalid flags are found.
+*/
+extern int check_for_flags_vqcs (const char *which_command, const char **str,
+				 int *print_what_v, int max_v,
+				 bool *cont, bool *silent);
 #endif /* CLI_UTILS_H */
diff --git a/gdb/testsuite/gdb.multi/tids.exp b/gdb/testsuite/gdb.multi/tids.exp
index 67349b5759..ae8328d997 100644
--- a/gdb/testsuite/gdb.multi/tids.exp
+++ b/gdb/testsuite/gdb.multi/tids.exp
@@ -350,8 +350,8 @@  with_test_prefix "two inferiors" {
 	thr_apply_info_thr_error "${prefix}1-" "inverted range"
 	thr_apply_info_thr_error "${prefix}2-1" "inverted range"
 	thr_apply_info_thr_error "${prefix}2-\$one" "inverted range"
-	thr_apply_info_thr_error "${prefix}-1" "negative value"
-	thr_apply_info_thr_error "${prefix}-\$one" "negative value"
+	thr_apply_info_thr_error "${prefix}-1" "Invalid thread ID: ${prefix_re}-1"
+	thr_apply_info_thr_error "${prefix}-\$one" "Invalid thread ID: ${prefix_re}-\\\$one"
 	thr_apply_info_thr_error "${prefix}\$minus_one" \
 	    "negative value: ${prefix_re}\\\$minus_one"