[RFA_v2 0/8] Implement 'frame apply COMMAND', enhance 'thread apply COMMAND'

[RFA_v2 0/8] Implement 'frame apply COMMAND', enhance 'thread apply COMMAND'

[Philippe Waroquiers]

This is the third iteration of the patch series that:
 * implements a new command
     'frame apply [all | COUNT | -COUNT] [FLAGS...] COMMAND'.
 * enhance 'thread apply COMMAND' by adding FLAGS arguments.
 * adds some shortcuts commands
 * documents the above in gdb.texinfo and NEWS.
 * adds a unit test for cli-utils.c
 * adds test for 'frame apply'
 * modify gdb.threads/pthreads.exp to test 'thread apply' -FLAGS argument

The first version was an RFC
  * Code and doc was complete, but was lacking ChangeLog and tests.
  * Comments received from Eli and Simon.

The second version was RFA v1:
  * Replied to all comments of Eli and Simon.
  * Tests and ChangeLog entries added.
  * Comments received from Pedro

This version handles the comments of Pedro about giving the flags
individually.

The changes compared to RFA v1 is:
  * The flags must be given individually, such as -v -v -c.
  * The documentation and on-line help was updated accordingly.
  * ChangeLog information is distributed in each commit log message.

[RFA_v2 1/8] Add helper functions check_for_flags and check_for_flags_vqcs

[Philippe Waroquiers]

Add helper functions check_for_flags and check_for_flags_vqcs
check_for_flags helper function allows to look for a set of flags at
the start of a string.
Each flag must be given individually.

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... arguments to
thread apply ID... [FLAGS...] COMMAND

The error message for 'thread apply -$unknownconvvar p 1'
is now the more clear:
  Convenience variable must have integer value.
  Invalid thread ID: -$unknownconvvar p 1
instead of previously:
  negative value

gdb/ChangeLog
2018-06-05  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* cli-utils.c (number_or_range_parser::get_number): Only handle
	numbers or convenience var as numbers.
	(check_for_flags): New function.
	(check_for_flags_vqcs): New function.
	* cli-utils.h (check_for_flags): New function.
	(check_for_flags_vqcs): New function.
---
 gdb/cli/cli-utils.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 gdb/cli/cli-utils.h |  30 +++++++++++++
 2 files changed, 150 insertions(+), 2 deletions(-)

diff --git a/gdb/cli/cli-utils.c b/gdb/cli/cli-utils.c
index c55b5035e4..7ce923a060 100644
--- a/gdb/cli/cli-utils.c
+++ b/gdb/cli/cli-utils.c
@@ -169,7 +169,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 +199,17 @@ number_or_range_parser::get_number ()
 	}
     }
   else
-    error (_("negative value"));
+    {
+      if (isdigit (*(m_cur_tok + 1)))
+	error (_("negative value"));
+      if (*(m_cur_tok + 1) == '$')
+	{
+	  // Convenience variable.
+	  m_last_retval = ::get_number (&m_cur_tok);
+	  if (m_last_retval < 0)
+	    error (_("negative value"));
+	}
+    }
   m_finished = *m_cur_tok == '\0';
   return m_last_retval;
 }
@@ -304,3 +317,108 @@ check_for_argument (const char **str, const char *arg, int arg_len)
     }
   return 0;
 }
+
+/* See documentation in cli-utils.h.  */
+
+int
+check_for_flags (const char **str, const char *flags,
+		 int *flags_counts)
+{
+  const char *p = skip_spaces (*str);
+  bool flag_found = false;
+  bool all_valid = true;
+
+  /* First set the flags_counts to 0.  */
+  memset (flags_counts, 0, sizeof (flags_counts[0]) * strlen (flags));
+
+  while (*p == '-' && all_valid)
+    {
+      const char pf = *(p + 1);
+
+      /* If - is followed by anything else than an alpha (including \0),
+	 then we do not have a flag.  This also cover the case of a command
+	 that accepts optional flags followed by a negative integer.
+	 In such a case, we will not handle a negative integer as invalid
+	 flags : rather let the caller handle the negative integer.  */
+      if (!isalpha (pf))
+	break;
+
+      /* Check that what follows pf is the end of string, or a space.  */
+      if (*(p + 2) != '\0' && !isspace (*(p + 2)))
+	{
+	  all_valid = false;
+	  break;
+	}
+
+      /* We have an alpha pf flag : validate the flag, and update
+	 flags_counts for a valid flag.  */
+      const char *f = flags;
+      bool valid = false;
+
+      while (*f != '\0')
+	{
+	  valid = *f == pf;
+	  if (valid)
+	    {
+	      flags_counts[f - flags]++;
+	      flag_found = true;
+	      p += 2;
+	      p = skip_spaces (p);
+	      break;
+	    }
+	  f++;
+	}
+
+      all_valid = all_valid && valid;
+    }
+
+  if (all_valid)
+    {
+      if (flag_found)
+	{
+	  *str = p;
+	  return 1;
+	}
+      else
+	return 0;
+    }
+  else
+    return -1;
+}
+
+/* See documentation in 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[strlen (flags)];
+  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 given individually"),
+	   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: flags c and s are mutually exclusive"),
+	   which_command);
+
+  return res;
+}
diff --git a/gdb/cli/cli-utils.h b/gdb/cli/cli-utils.h
index e34ee0df37..9e74725393 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.
+   A flag in STR must 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 flag.  */
+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 */
-- 
2.11.0

Re: [RFA_v2 1/8] Add helper functions check_for_flags and check_for_flags_vqcs

[Pedro Alves]

Hi Philippe,

Been taking a better look at this, finally.

On 06/05/2018 09:48 PM, Philippe Waroquiers wrote:
> Add helper functions check_for_flags and check_for_flags_vqcs
> check_for_flags helper function allows to look for a set of flags at
> the start of a string.
> Each flag must be given individually.
> 
> 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... arguments to
> thread apply ID... [FLAGS...] COMMAND
> 
> The error message for 'thread apply -$unknownconvvar p 1'
> is now the more clear:
>   Convenience variable must have integer value.
>   Invalid thread ID: -$unknownconvvar p 1
> instead of previously:
>   negative value
> 
> gdb/ChangeLog
> 2018-06-05  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> 
> 	* cli-utils.c (number_or_range_parser::get_number): Only handle
> 	numbers or convenience var as numbers.
> 	(check_for_flags): New function.
> 	(check_for_flags_vqcs): New function.
> 	* cli-utils.h (check_for_flags): New function.
> 	(check_for_flags_vqcs): New function.

I'm not super happy with this design, because first it is
still at places biased toward "flags" instead of "command
options", and most importantly because it doesn't seem to
make it easy to add further options to commands that
use check_for_flags_vqcs, without the artificial limitation
of requiring all vqcs flags specified together.  E.g., what if we want to
add an option like "-foo NUM" to "thread apply" for example.

I'd seem to me that at least an iterative function which
could be interleaved with other options would be better.
Something like:

struct vqcs_flags
{
  /* Number of times -c was seen.  */
  int c = 0;

  /* Number of times -v was seen.  */
  int v = 0;
};

int parse_flags_vqcs (const char **args, vqcs_flags *flags);

and then

  vqcs_flags flags;

  while (*args != '\0')
    {
       if (parse_flags_vqcs (&args, &flags))
         continue;

       /* check other options here.  error if unknown.  */
    }

maybe even interleave the number-or-range parsing
in that loop.

> ---
>  gdb/cli/cli-utils.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  gdb/cli/cli-utils.h |  30 +++++++++++++
>  2 files changed, 150 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/cli/cli-utils.c b/gdb/cli/cli-utils.c
> index c55b5035e4..7ce923a060 100644
> --- a/gdb/cli/cli-utils.c
> +++ b/gdb/cli/cli-utils.c
> @@ -169,7 +169,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.  */

I'd prefer to say "start of a command option" instead of "start of
flags".  (comment applies throughout)

> +      if (*m_cur_tok == '-' && !isalpha (*(m_cur_tok + 1)))
>  	{
>  	  const char **temp;
>  
> @@ -196,7 +199,17 @@ number_or_range_parser::get_number ()
>  	}
>      }
>    else
> -    error (_("negative value"));
> +    {
> +      if (isdigit (*(m_cur_tok + 1)))
> +	error (_("negative value"));
> +      if (*(m_cur_tok + 1) == '$')
> +	{
> +	  // Convenience variable.

We use /**/ style throughout.

> +	  m_last_retval = ::get_number (&m_cur_tok);
> +	  if (m_last_retval < 0)
> +	    error (_("negative value"));
> +	}
> +    }
>    m_finished = *m_cur_tok == '\0';
>    return m_last_retval;
>  }
> @@ -304,3 +317,108 @@ check_for_argument (const char **str, const char *arg, int arg_len)
>      }
>    return 0;
>  }
> +
> +/* See documentation in cli-utils.h.  */
> +
> +int
> +check_for_flags (const char **str, const char *flags,
> +		 int *flags_counts)
> +{
> +  const char *p = skip_spaces (*str);
> +  bool flag_found = false;
> +  bool all_valid = true;
> +
> +  /* First set the flags_counts to 0.  */
> +  memset (flags_counts, 0, sizeof (flags_counts[0]) * strlen (flags));
> +
> +  while (*p == '-' && all_valid)
> +    {
> +      const char pf = *(p + 1);
> +
> +      /* If - is followed by anything else than an alpha (including \0),

I think "anything other than" is more idiomatic.

> +	 then we do not have a flag.  This also cover the case of a command
> +	 that accepts optional flags followed by a negative integer.
> +	 In such a case, we will not handle a negative integer as invalid
> +	 flags : rather let the caller handle the negative integer.  */
> +      if (!isalpha (pf))
> +	break;
> +
> +      /* Check that what follows pf is the end of string, or a space.  */

s/pf/PF/

> +      if (*(p + 2) != '\0' && !isspace (*(p + 2)))
> +	{
> +	  all_valid = false;
> +	  break;
> +	}
> +
> +      /* We have an alpha pf flag : validate the flag, and update

s/pf/PF/

> +	 flags_counts for a valid flag.  */
> +      const char *f = flags;
> +      bool valid = false;
> +
> +      while (*f != '\0')
> +	{
> +	  valid = *f == pf;
> +	  if (valid)
> +	    {
> +	      flags_counts[f - flags]++;
> +	      flag_found = true;
> +	      p += 2;
> +	      p = skip_spaces (p);
> +	      break;
> +	    }
> +	  f++;
> +	}
> +
> +      all_valid = all_valid && valid;
> +    }
> +
> +  if (all_valid)
> +    {
> +      if (flag_found)
> +	{
> +	  *str = p;
> +	  return 1;
> +	}
> +      else
> +	return 0;
> +    }
> +  else
> +    return -1;
> +}
> +
> +/* See documentation in 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[strlen (flags)];

Variable-length arrays are not standard C++, but I'll ignore.  ;-)

> +  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 given individually"),
> +	   which_command, flags);

I think this error message might look a bit odd.  What does
it really mean?

> +  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: flags c and s are mutually exclusive"),
> +	   which_command);

Here I think say "options -c and -s" or just "-c and -s"
instead of "flags c and s".

> +
> +  return res;
> +}
> diff --git a/gdb/cli/cli-utils.h b/gdb/cli/cli-utils.h
> index e34ee0df37..9e74725393 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.
> +   A flag in STR must 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 flag.  */
> +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.

We use US English, initialize.

> +   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 */
> 

Thanks,
Pedro Alves

Re: [RFA_v2 1/8] Add helper functions check_for_flags and check_for_flags_vqcs

[Philippe Waroquiers]

On Wed, 2018-06-13 at 20:52 +0100, Pedro Alves wrote:
> Hi Philippe,
> 
> Been taking a better look at this, finally.
Thanks for the review, I will handle all the comments,
I have some feedback/questions on a few of them below.


> > 	* cli-utils.c (number_or_range_parser::get_number): Only handle
> > 	numbers or convenience var as numbers.
> > 	(check_for_flags): New function.
> > 	(check_for_flags_vqcs): New function.
> > 	* cli-utils.h (check_for_flags): New function.
> > 	(check_for_flags_vqcs): New function.
> 
> I'm not super happy with this design, because first it is
> still at places biased toward "flags" instead of "command
> options", and most importantly because it doesn't seem to
> make it easy to add further options to commands that
> use check_for_flags_vqcs, without the artificial limitation
> of requiring all vqcs flags specified together.  E.g., what if we want to
> add an option like "-foo NUM" to "thread apply" for example.

Yes I agree, the current way to give the vqcs option is too unflexible,
so I will rework based on the iterative function you suggest below.

Just one clarification: I assume that by 'at places biased toward "flags"',
you mean that the names should be 'check_for_options_vqcs' ?
Otherwise, can you explain what you mean with bias ?

> 
> I'd seem to me that at least an iterative function which
> could be interleaved with other options would be better.
> Something like:
> 
> struct vqcs_flags
> {
>   /* Number of times -c was seen.  */
>   int c = 0;
> 
>   /* Number of times -v was seen.  */
>   int v = 0;
> };
> 
> int parse_flags_vqcs (const char **args, vqcs_flags *flags);
> 
> and then
> 
>   vqcs_flags flags;
> 
>   while (*args != '\0')
>     {
>        if (parse_flags_vqcs (&args, &flags))
>          continue;
> 
>        /* check other options here.  error if unknown.  */
>     }
> 
> maybe even interleave the number-or-range parsing
> in that loop.
Probably that can be done, but isn't this a little bit cumbersome ?
E.g. it means the help 
  thread apply ID... [OPTIONS] COMMAND
will become something like
  thread apply OPTIONS_OR_ID... COMMAND
and then we have to explain what OPTIONS_OR_ID can be.

(and I guess such syntax might make the 'generalised option parser'
more difficult to implement/use : we better keep ID... as a 
'positional argument' for an easy conversion to an generalised
option/arg parser).

So, I am more keen to keep
   thread apply ID... [OPTIONS] COMMAND
(note: we can always change it in a backward compatible
way in the future if we really believe mixing OPTIONS and ID...
has a strong value).

+  res = check_for_flags (str, flags, flags_counts);
> > +  if (res == 0)
> > +    return 0;
> > +  if (res == -1)
> > +    error (_("%s only accepts flags %s given individually"),
> > +	   which_command, flags);
> 
> I think this error message might look a bit odd.  What does
> it really mean?
It catches the below erroneous case, but probably this will become
'unknown option -vc' when I do the iterative design:
  (gdb) thread apply all -vc p 1
  thread apply all only accepts flags vqcs given individually
  (gdb) 

(I think that the generalised option/arg parser you have prototyped
will be a nice help to have a consistent and easier to code
gdb command parsing :).

Thanks

Philippe

Re: [RFA_v2 1/8] Add helper functions check_for_flags and check_for_flags_vqcs

[Pedro Alves]

On 06/14/2018 10:40 PM, Philippe Waroquiers wrote:
> On Wed, 2018-06-13 at 20:52 +0100, Pedro Alves wrote:
>> Hi Philippe,
>>
>> Been taking a better look at this, finally.
> Thanks for the review, I will handle all the comments,
> I have some feedback/questions on a few of them below.
> 
> 
>>> 	* cli-utils.c (number_or_range_parser::get_number): Only handle
>>> 	numbers or convenience var as numbers.
>>> 	(check_for_flags): New function.
>>> 	(check_for_flags_vqcs): New function.
>>> 	* cli-utils.h (check_for_flags): New function.
>>> 	(check_for_flags_vqcs): New function.
>>
>> I'm not super happy with this design, because first it is
>> still at places biased toward "flags" instead of "command
>> options", and most importantly because it doesn't seem to
>> make it easy to add further options to commands that
>> use check_for_flags_vqcs, without the artificial limitation
>> of requiring all vqcs flags specified together.  E.g., what if we want to
>> add an option like "-foo NUM" to "thread apply" for example.
> 
> Yes I agree, the current way to give the vqcs option is too unflexible,
> so I will rework based on the iterative function you suggest below.
> 
> Just one clarification: I assume that by 'at places biased toward "flags"',
> you mean that the names should be 'check_for_options_vqcs' ?
> Otherwise, can you explain what you mean with bias ?

Yeah, on second thought I think using "flags" for the function is
fine since it only handles flag-like options.  On my option-revamping
prototype I was calling those kind of options "switches" but I never
like that name.  "flags" sounds more usual.  So my concern is more
with the user-visible aspects.

>> maybe even interleave the number-or-range parsing
>> in that loop.
> Probably that can be done, but isn't this a little bit cumbersome ?

Maybe.

> E.g. it means the help 
>   thread apply ID... [OPTIONS] COMMAND
> will become something like
>   thread apply OPTIONS_OR_ID... COMMAND
> and then we have to explain what OPTIONS_OR_ID can be.

I guess we could also say that there's only one "thread apply"
command, and that is looks like this:

 thread apply [OPTION]... [ID... | all] COMMAND

and then specifying options after the ID list works
just because we don't care about order of optional vs
non-optional arguments.

But yeah, there's no need to go there.  Options after
ID is fine with me.

> 
> (and I guess such syntax might make the 'generalised option parser'
> more difficult to implement/use : we better keep ID... as a 
> 'positional argument' for an easy conversion to an generalised
> option/arg parser).
> 
> So, I am more keen to keep
>    thread apply ID... [OPTIONS] COMMAND
> (note: we can always change it in a backward compatible
> way in the future if we really believe mixing OPTIONS and ID...
> has a strong value).

Agreed.

> 
> +  res = check_for_flags (str, flags, flags_counts);
>>> +  if (res == 0)
>>> +    return 0;
>>> +  if (res == -1)
>>> +    error (_("%s only accepts flags %s given individually"),
>>> +	   which_command, flags);
>>
>> I think this error message might look a bit odd.  What does
>> it really mean?
> It catches the below erroneous case, but probably this will become
> 'unknown option -vc' when I do the iterative design:
>   (gdb) thread apply all -vc p 1
>   thread apply all only accepts flags vqcs given individually
>   (gdb) 
> 
> (I think that the generalised option/arg parser you have prototyped
> will be a nice help to have a consistent and easier to code
> gdb command parsing :).

I hope so.  :-)

Thanks,
Pedro Alves

Re: [RFA_v2 1/8] Add helper functions check_for_flags and check_for_flags_vqcs

[Philippe Waroquiers]

On Fri, 2018-06-15 at 17:24 +0100, Pedro Alves wrote:
> Yeah, on second thought I think using "flags" for the function is
> fine since it only handles flag-like options.  On my option-revamping
> prototype I was calling those kind of options "switches" but I never
> like that name.  "flags" sounds more usual.  So my concern is more
> with the user-visible aspects.
I have just send RFA_v3.
Note that following the above discussion, I have kept the word
'flag' for the cases where the code and/or documentation are
only related to flags, and have no other type of arguments.
The wording can be changed whenever other types of options
are added and/or when arg parsing is converted to the
generalized option parser

Thanks for the review comments and suggestions ...

Philippe

[RFA_v2 4/8] Documentation changes for 'frame apply' and 'thread apply'

[Philippe Waroquiers]

Documents the new commands
   'frame apply'
   faas
   taas
   tfaas

Documents the new arguments FLAGS... added to 'thread apply'

gdb/doc/ChangeLog
2018-05-21  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* gdb.texinfo (Debugging Programs with Multiple Threads):
	Document changes to 'thread apply'. Document 'taas'.
	Document 'tfaas'.
	(Examining the Stack): Document 'frame apply'. Document 'faas'.
---
 gdb/doc/gdb.texinfo | 227 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 224 insertions(+), 3 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 4968b374af..4a2bba6139 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -2936,7 +2936,7 @@ programs:
 @item automatic notification of new threads
 @item @samp{thread @var{thread-id}}, a command to switch among threads
 @item @samp{info threads}, a command to inquire about existing threads
-@item @samp{thread apply [@var{thread-id-list}] [@var{all}] @var{args}},
+@item @samp{thread apply [@var{thread-id-list} | all] @var{args}},
 a command to apply a command to a list of threads
 @item thread-specific breakpoints
 @item @samp{set print thread-events}, which controls printing of 
@@ -3173,7 +3173,7 @@ threads.
 
 @kindex thread apply
 @cindex apply command to several threads
-@item thread apply [@var{thread-id-list} | all [-ascending]] @var{command}
+@item thread apply [@var{thread-id-list} | all [-ascending]] [@var{flags@dots{}}] @var{command}
 The @code{thread apply} command allows you to apply the named
 @var{command} to one or more threads.  Specify the threads that you
 want affected using the thread ID list syntax (@pxref{thread ID
@@ -3182,6 +3182,64 @@ command to all threads in descending order, type @kbd{thread apply all
 @var{command}}.  To apply a command to all threads in ascending order,
 type @kbd{thread apply all -ascending @var{command}}.
 
+The @var{flags} control what output to produce and how to handle
+errors raised when applying @var{command} to a thread.  @var{flags}
+must start with a @code{-} directly followed by one letter in
+@code{vqcs}.  If several flags are provided, they must be given
+individually, such as @code{-c -v -v}.
+
+By default, @value{GDBN} displays some thread information before the
+output produced by @var{command}, and an error raised during the
+execution of a @var{command} will abort @code{thread apply}.  The
+following flags can be used to fine-tune this behavior:
+
+@table @code
+@item -c
+The flag @code{-c}, which stands for @samp{continue}, causes any
+errors in @var{command} to be displayed, and the execution of
+@code{thread apply} then continues.
+@item -s
+The flag @code{-s}, which stands for @samp{silent}, causes any errors
+or empty output produced by a @var{command} to be silently ignored.
+That is, the execution continues, but the thread information and errors
+are not printed.
+@item -v
+The flag @code{-v} (@samp{verbose}) increases the verbosity.
+@item -q
+The flag @code{-q} (@samp{quiet}) decreases the verbosity.
+@end table
+
+Flags @code{-c} and @code{-s} cannot be used together.
+
+The default value of verbosity, 1, prints the per-inferior thread
+number and the target system's thread ID.  Under verbosity 0, no
+thread info is printed.
+
+@kindex taas
+@cindex apply command to all threads (ignoring errors and empty output)
+@item taas @var{command}
+Shortcut for @code{thread apply all -s @var{command}}.
+Applies @var{command} on all threads, ignoring errors and empty output.
+
+@kindex tfaas
+@cindex apply a command to all frames of all threads (ignoring errors and empty output)
+@item tfaas @var{command}
+Shortcut for @code{thread apply all -s frame apply all -s @var{command}}.
+Applies @var{command} on all frames of all threads, ignoring errors
+and empty output.  Note that the flag @code{-s} is specified twice:
+The first @code{-s} ensures that @code{thread apply} only shows the thread
+information of the threads for which @code{frame apply} produces
+some output.  The second @code{-s} is needed to ensure that @code{frame
+apply} shows the frame information of a frame only if the
+@var{command} successfully produced some output.
+
+It can for example be used to print a local variable or a function
+argument without knowing the thread or frame where this variable or argument
+is, using:
+@smallexample
+(@value{GDBP}) tfaas p some_local_var_i_do_not_remember_where_it_is
+@end smallexample
+
 
 @kindex thread name
 @cindex name a thread
@@ -7303,6 +7361,7 @@ currently executing frame and describes it briefly, similar to the
 * Backtrace::                   Backtraces
 * Selection::                   Selecting a frame
 * Frame Info::                  Information on a frame
+* Frame Apply::                 Applying a command to several frames
 * Frame Filter Management::     Managing frame filters
 
 @end menu
@@ -7715,6 +7774,168 @@ accessible at the point of execution of the selected frame.
 
 @end table
 
+@node Frame Apply
+@section Applying a Command to Several Frames.
+@kindex frame apply
+@cindex apply command to several frames
+@table @code
+@item frame apply [all | @var{count} | @var{-count} ] [@var{flags@dots{}}] @var{command}
+The @code{frame apply} command allows you to apply the named
+@var{command} to one or more frames.
+
+@table @code
+@item @code{all}
+Specify @code{all} to apply @var{command} to all frames.
+
+@item @var{count}
+Use @var{count} to apply @var{command} to the innermost @var{count}
+frames, where @var{count} is a positive number.
+
+@item @var{-count}
+Use @var{-count} to apply @var{command} to the outermost @var{count}
+frames, where @var{count} is a positive number.
+@end table
+
+@end table
+
+Note that the frames on which @code{frame apply} applies a command are
+also influenced by the @code{set backtrace} settings such as @code{set
+backtrace past-main} and @code{set backtrace limit N}.  See
+@xref{Backtrace,,Backtraces}.
+
+The @var{flags} control what output to produce and how to handle
+errors raised when applying @var{command} to a frame.  @var{flags}
+must start with a @code{-} directly followed by one letter in
+@code{vqcs}.  If several flags are provided, they must be given
+individually, such as @code{-c -v -v}.
+
+By default, @value{GDBN} displays some frame information before the
+output produced by @var{command}, and an error raised during the
+execution of a @var{command} will abort @code{frame apply}.  The
+following flags can be used to fine-tune this behavior:
+
+@table @code
+@item -c
+The flag @code{-c}, which stands for @samp{continue}, causes any
+errors in @var{command} to be displayed, and the execution of
+@code{frame apply} then continues.
+@item -s
+The flag @code{-s}, which stands for @samp{silent}, causes any errors
+or empty output produced by a @var{command} to be silently ignored.
+That is, the execution continues, but the frame information and errors
+are not printed.
+@item -v
+The flag @code{-v} (@samp{verbose}) increases the verbosity.
+@item -q
+The flag @code{-q} (@samp{quiet}) decreases the verbosity.
+@end table
+
+The following example shows how the flags @code{-c} and @code{-s} are
+working when applying the command @code{p j} to all frames, where
+variable @code{j} can only be successfully printed in the outermost
+@code{#1 main} frame.
+
+@smallexample
+@group
+(gdb) frame apply all p j
+#0  some_function (i=5) at fun.c:4
+No symbol "j" in current context.
+(gdb) frame apply all -c p j
+#0  some_function (i=5) at fun.c:4
+No symbol "j" in current context.
+#1  0x565555fb in main (argc=1, argv=0xffffd2c4) at fun.c:11
+$1 = 5
+(gdb) frame apply all -s p j
+#1  0x565555fb in main (argc=1, argv=0xffffd2c4) at fun.c:11
+$2 = 5
+(gdb)
+@end group
+@end smallexample
+
+The default value of verbosity, 2, prints the location before
+the command output:
+
+@smallexample
+@group
+(gdb) frame apply all p $sp
+#0  some_function (i=5) at fun.c:4
+$4 = (void *) 0xffffd1e0
+#1  0x565555fb in main (argc=1, argv=0xffffd2c4) at fun.c:11
+$5 = (void *) 0xffffd1f0
+(gdb)
+@end group
+@end smallexample
+
+The verbosity 3 prints the location and address:
+
+@smallexample
+@group
+(gdb) frame apply all -v p $sp
+#0  0x565555b1 in some_function (i=5) at fun.c:4
+$6 = (void *) 0xffffd1e0
+#1  0x565555fb in main (argc=1, argv=0xffffd2c4) at fun.c:11
+$7 = (void *) 0xffffd1f0
+(gdb)
+@end group
+@end smallexample
+
+The verbosity 4 prints the source line and address:
+@smallexample
+@group
+(gdb) frame apply all -v -v p $sp
+#0  some_function (i=5) at fun.c:4
+4	   printf ("value of i is %d\n", i);
+$8 = (void *) 0xffffd1e0
+#1  0x565555fb in main (argc=1, argv=0xffffd2c4) at fun.c:11
+11	   some_function (j);
+$9 = (void *) 0xffffd1f0
+(gdb)
+@end group
+@end smallexample
+
+At verbosity 1, only the source line is printed:
+
+@smallexample
+@group
+(gdb) frame apply all -q p $sp
+4	   printf ("value of i is %d\n", i);
+$10 = (void *) 0xffffd1e0
+0x565555fb	11	   some_function (j);
+$11 = (void *) 0xffffd1f0
+(gdb)
+@end group
+@end smallexample
+
+At verbosity 0, no frame information is printed:
+@smallexample
+@group
+(gdb) frame apply all -q -q p $sp
+$12 = (void *) 0xffffd1e0
+$13 = (void *) 0xffffd1f0
+(gdb)
+@end group
+@end smallexample
+
+@table @code
+
+@kindex faas
+@cindex apply a command to all frames (ignoring errors and empty output)
+@item faas @var{command}
+Shortcut for @code{frame apply all -s @var{command}}.
+Applies @var{command} on all frames, ignoring errors and empty output.
+
+It can for example be used to print a local variable or a function
+argument without knowing the frame where this variable or argument
+is, using:
+@smallexample
+(@value{GDBP}) faas p some_local_var_i_do_not_remember_where_it_is
+@end smallexample
+
+Note that the command @code{tfaas @var{command}} applies @var{command}
+on all frames of all threads.  See @xref{Threads,,Threads}.
+@end table
+
+
 @node Frame Filter Management
 @section Management of Frame Filters.
 @cindex managing frame filters
@@ -11093,7 +11314,7 @@ Visiting node of type NODE_INTEGER
 convenience functions.
 
 @table @code
-@item help function
+@item help functionu
 @kindex help function
 @cindex show all convenience functions
 Print a list of all convenience functions.
-- 
2.11.0

Re: [RFA_v2 4/8] Documentation changes for 'frame apply' and 'thread apply'

[Eli Zaretskii]

> From: Philippe Waroquiers <philippe.waroquiers@skynet.be>
> Cc: Philippe Waroquiers <philippe.waroquiers@skynet.be>
> Date: Tue,  5 Jun 2018 22:49:01 +0200
> 
> Documents the new commands
>    'frame apply'
>    faas
>    taas
>    tfaas
> 
> Documents the new arguments FLAGS... added to 'thread apply'
> 
> gdb/doc/ChangeLog
> 2018-05-21  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> 
> 	* gdb.texinfo (Debugging Programs with Multiple Threads):
> 	Document changes to 'thread apply'. Document 'taas'.
> 	Document 'tfaas'.
> 	(Examining the Stack): Document 'frame apply'. Document 'faas'.

OK.

[RFA_v2 5/8] Announce in NEWS 'frame apply', faas, taas, tfaas commands and FLAGS... arg for thread apply

[Philippe Waroquiers]

Announce the user visible changes in NEWS:
'frame apply', faas, taas, tfaas commands and FLAGS... arg for thread apply.

gdb/ChangeLog
2018-06-05  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* NEWS: Mention new commands. Mention change to 'thread apply'.
---
 gdb/NEWS | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/gdb/NEWS b/gdb/NEWS
index 8fb6a2ad48..78e392d8b9 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -23,6 +23,24 @@
 
 * New commands
 
+frame apply [all | COUNT | -COUNT] [FLAGS...] COMMAND
+  Apply a command to a number of frames.
+  The FLAGS allows to control what output to produce and how to handle
+  errors raised when applying COMMAND to a frame.
+
+taas COMMAND
+  Apply a command to all threads (ignoring errors and empty output).
+  Shortcut for 'thread apply all -s COMMAND'.
+
+faas COMMAND
+  Apply a command to all frames (ignoring errors and empty output).
+  Shortcut for 'frame apply all -s COMMAND'.
+
+tfaas COMMAND
+  Apply a command to all frames of all threads (ignoring errors and empty
+  output).
+  Shortcut for 'thread apply all -s frame apply all -s COMMAND'.
+
 set debug fbsd-nat
 show debug fbsd-nat
   Control display of debugging info regarding the FreeBSD native target.
@@ -36,6 +54,13 @@ set|show record btrace cpu
   Controls the processor to be used for enabling errata workarounds for
   branch trace decode.
 
+* Changed commands
+
+thread apply [all | COUNT | -COUNT] [FLAGS...] COMMAND
+  The 'thread apply' command accepts new FLAGS arguments.
+  The FLAGS allows to control what output to produce and how to handle errors
+  raised when applying COMMAND to a frame.
+
 * Python API
 
   ** Type alignment is now exposed via the "align" attribute of a gdb.Type.
-- 
2.11.0

Re: [RFA_v2 5/8] Announce in NEWS 'frame apply', faas, taas, tfaas commands and FLAGS... arg for thread apply

[Eli Zaretskii]

> From: Philippe Waroquiers <philippe.waroquiers@skynet.be>
> Cc: Philippe Waroquiers <philippe.waroquiers@skynet.be>
> Date: Tue,  5 Jun 2018 22:49:02 +0200
> 
> Announce the user visible changes in NEWS:
> 'frame apply', faas, taas, tfaas commands and FLAGS... arg for thread apply.
> 
> gdb/ChangeLog
> 2018-06-05  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> 
> 	* NEWS: Mention new commands. Mention change to 'thread apply'.

OK.

[RFA_v2 6/8] Add a test for 'frame apply'

[Philippe Waroquiers]

gdb/testsuite/ChangeLog
2018-06-05  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* gdb.base/frameapply.c: New file.
	* gdb.base/frameapply.exp: New file.
---
 gdb/testsuite/gdb.base/frameapply.c   |  71 +++++++++++++++
 gdb/testsuite/gdb.base/frameapply.exp | 167 ++++++++++++++++++++++++++++++++++
 2 files changed, 238 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/frameapply.c
 create mode 100644 gdb/testsuite/gdb.base/frameapply.exp

diff --git a/gdb/testsuite/gdb.base/frameapply.c b/gdb/testsuite/gdb.base/frameapply.c
new file mode 100644
index 0000000000..dccf4031ed
--- /dev/null
+++ b/gdb/testsuite/gdb.base/frameapply.c
@@ -0,0 +1,71 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2018 Free Software Foundation, Inc.
+
+   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 <http://www.gnu.org/licenses/>.  */
+
+static void
+setup_done (void)
+{
+}
+
+static int
+f1 (int f1arg)
+{
+  int f1loc;
+
+  f1loc = f1arg - 1;
+
+  setup_done ();
+  return f1loc;
+}
+
+static int
+f2 (int f2arg)
+{
+  int f2loc;
+
+  f2loc = f1 (f2arg - 1);
+
+  return f2loc;
+}
+
+static int
+f3 (int f3arg)
+{
+  int f3loc;
+
+  f3loc = f2 (f3arg - 1);
+
+  return f3loc;
+}
+
+static int
+f4 (int f4arg)
+{
+  int f4loc;
+
+  f4loc = f3 (f4arg - 1);
+
+  return f4loc;
+}
+
+int
+main (void)
+{
+  int result;
+
+  result = f4 (4);
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/frameapply.exp b/gdb/testsuite/gdb.base/frameapply.exp
new file mode 100644
index 0000000000..4b0cce7ee3
--- /dev/null
+++ b/gdb/testsuite/gdb.base/frameapply.exp
@@ -0,0 +1,167 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2018 Free Software Foundation, Inc.
+
+# 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 <http://www.gnu.org/licenses/>.
+
+
+# This test checks 'frame apply [all | COUNT | -COUNT] [FLAGS...] COMMAND'
+
+standard_testfile
+
+if { [prepare_for_testing "failed to prepare" ${testfile}] } {
+    return -1
+}
+
+clean_restart ${binfile}
+
+
+if ![runto setup_done] then {
+    gdb_suppress_tests
+}
+
+set any "\[^\r\n\]*"
+set ws "\[ \t\]\+"
+set number "\[0-9]\+"
+
+# check all | COUNT | -COUNT with a simple command
+gdb_test "frame apply all p /x 20" \
+    [multi_line \
+	 "#0${ws}setup_done ${any}" \
+	 "\\\$\[0-9]+ = 0x14${any}" \
+	 "#1${ws}${any} f1 ${any}" \
+	 "\\\$\[0-9]+ = 0x14${any}" \
+	 "#2${ws}${any} f2 ${any}" \
+	 "\\\$\[0-9]+ = 0x14${any}" \
+	 "#3${ws}${any} f3 ${any}" \
+	 "\\\$\[0-9]+ = 0x14${any}" \
+	 "#4${ws}${any} f4 ${any}" \
+	 "\\\$\[0-9]+ = 0x14${any}" \
+	 "#5${ws}${any} main ${any}" \
+	 "\\\$\[0-9]+ = 0x14${any}" \
+	] \
+    "Run a simple command on all frames"
+
+gdb_test "frame apply 3 p /x 20" \
+    [multi_line \
+	 "#0${ws}setup_done ${any}" \
+	 "\\\$\[0-9]+ = 0x14${any}" \
+	 "#1${ws}${any} f1 ${any}" \
+	 "\\\$\[0-9]+ = 0x14${any}" \
+	 "#2${ws}${any} f2 ${any}" \
+	 "\\\$\[0-9]+ = 0x14${any}" \
+	] \
+    "Run a simple command on the 3 innermost frames"
+
+gdb_test "frame apply -3 p /x 20" \
+    [multi_line \
+	 "#3${ws}${any} f3 ${any}" \
+	 "\\\$\[0-9]+ = 0x14${any}" \
+	 "#4${ws}${any} f4 ${any}" \
+	 "\\\$\[0-9]+ = 0x14${any}" \
+	 "#5${ws}${any} main ${any}" \
+	 "\\\$\[0-9]+ = 0x14${any}" \
+	] \
+    "Run a simple command on the 3 outermost frames"
+
+# check -c (continue- and -s (silently continue) flags
+gdb_test "frame apply all p f3arg" \
+    [multi_line \
+	 "#0${ws}setup_done ${any}" \
+	 "No symbol \\\"f3arg\\\" in current context." \
+	] \
+    "Run a failing command that aborts frame apply"
+
+gdb_test "frame apply all -c p f3arg" \
+    [multi_line \
+	 "#0${ws}setup_done ${any}" \
+	 "No symbol \\\"f3arg\\\" in current context." \
+	 "#1${ws}${any} f1 ${any}" \
+	 "No symbol \\\"f3arg\\\" in current context." \
+	 "#2${ws}${any} f2 ${any}" \
+	 "No symbol \\\"f3arg\\\" in current context." \
+	 "#3${ws}${any} f3 ${any}" \
+	 "\\\$\[0-9]+ = 3${any}" \
+	 "#4${ws}${any} f4 ${any}" \
+	 "No symbol \\\"f3arg\\\" in current context." \
+	 "#5${ws}${any} main ${any}" \
+	 "No symbol \\\"f3arg\\\" in current context." \
+	] \
+    "Run a command failing in all frames except #3, -c to continue"
+
+foreach cmd {"frame apply all -s p f3arg" "faas p f3arg"} {
+    gdb_test $cmd \
+	[multi_line \
+	     "#3${ws}${any} f3 ${any}" \
+	     "\\\$\[0-9]+ = 3${any}" \
+	    ] \
+	"Run a command failing in all frames except #3, -s to silently continue"
+}
+
+# check verbosity
+gdb_test "frame apply 2 p /x 20" \
+    [multi_line \
+	 "#0${ws}setup_done ${any}" \
+	 "\\\$\[0-9]+ = 0x14${any}" \
+	 "#1${ws}${any} f1 ${any}" \
+	 "\\\$\[0-9]+ = 0x14${any}" \
+	] \
+    "Run a command at default verbosity 2, printing location"
+
+gdb_test "frame apply 2 -q p /x 20" \
+    [multi_line \
+	 "${number}${ws}\\\}${any}" \
+	 "\\\$\[0-9]+ = 0x14${any}" \
+	 "${any}${ws}${number}${ws} setup_done ()${any}" \
+	 "\\\$\[0-9]+ = 0x14${any}" \
+	] \
+    "Run a command at verbosity 1, printing source line"
+
+gdb_test "frame apply 2 -q -q p /x 20" \
+    [multi_line \
+	 "\\\$\[0-9]+ = 0x14${any}" \
+	 "\\\$\[0-9]+ = 0x14${any}" \
+	] \
+    "Run a command at verbosity 0, printing only command results"
+
+gdb_test "frame apply 2 -v p /x 20" \
+    [multi_line \
+	 "#0${ws}0x${any}${ws}in setup_done ${any}" \
+	 "\\\$\[0-9]+ = 0x14${any}" \
+	 "#1${ws}0x${any} in f1 ${any}" \
+	 "\\\$\[0-9]+ = 0x14${any}" \
+	] \
+    "Run a command at verbosity 3, printing location and address"
+
+gdb_test "frame apply 2 -v -v p /x 20" \
+    [multi_line \
+	 "#0${ws}setup_done ${any}" \
+	 "${number}${ws}\\\}${any}" \
+	 "\\\$\[0-9]+ = 0x14${any}" \
+	 "#1${ws}${any} f1 ${any}" \
+	 "${number}${ws} setup_done ()${any}" \
+	 "\\\$\[0-9]+ = 0x14${any}" \
+	] \
+    "Run a command at verbosity 4, printing location and source line"
+
+# check multiple flags together
+gdb_test "frame apply all -q -s -q p f3arg" \
+    "\\\$\[0-9]+ = 3${any}" \
+    "Run a command failing in all frames except #3, -s to silently continue, verbosity 0"
+
+# check invalid flag combinations
+gdb_test "frame apply all -c -s p f3arg" \
+    "frame apply all: flags c and s are mutually exclusive" \
+    "Check c and s cannot be used simultaneously"
+
-- 
2.11.0

[RFA_v2 8/8] Add a self-test for cli-utils.c

[Philippe Waroquiers]

tests added for:
* number_or_range_parser
  number_or_range_parser is somewhat cumbersome to use and/or does
  not work as described in cli-utils.h
  In particular, cur_tok () is currently not maintained as described
  in cli-utils.h, but changing this is better done in another patch,
  as it implies to change callers.
  The SELF_CHECK for cumbersome (or unexpected) behaviour are commented
  now. Note that the same commented tests are similarly wrong in unpatched gdb.

* check_for_flags_vqcs

* check_for_flags

gdb/ChangeLog
2018-06-05  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
	unittests/cli-utils-selftests.c
	* unittests/cli-utils-selftests.c: New file.
---
 gdb/Makefile.in                     |   1 +
 gdb/unittests/cli-utils-selftests.c | 238 ++++++++++++++++++++++++++++++++++++
 2 files changed, 239 insertions(+)
 create mode 100644 gdb/unittests/cli-utils-selftests.c

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 354a6361b7..f8cdf9a560 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -416,6 +416,7 @@ SUBDIR_PYTHON_CFLAGS =
 
 SUBDIR_UNITTESTS_SRCS = \
 	unittests/array-view-selftests.c \
+	unittests/cli-utils-selftests.c \
 	unittests/common-utils-selftests.c \
 	unittests/environ-selftests.c \
 	unittests/format_pieces-selftests.c \
diff --git a/gdb/unittests/cli-utils-selftests.c b/gdb/unittests/cli-utils-selftests.c
new file mode 100644
index 0000000000..68ae89008b
--- /dev/null
+++ b/gdb/unittests/cli-utils-selftests.c
@@ -0,0 +1,238 @@
+/* Unit tests for the cli-utils.c file.
+
+   Copyright (C) 2018 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 <http://www.gnu.org/licenses/>.  */
+
+#include "defs.h"
+#include "cli/cli-utils.h"
+#include "selftest.h"
+
+namespace selftests {
+namespace cli_utils {
+
+static void
+test_number_or_range_parser ()
+{
+  number_or_range_parser one ("1");
+
+  SELF_CHECK (one.finished () == false);
+  SELF_CHECK (one.get_number () == 1);
+  SELF_CHECK (one.finished () == true);
+  SELF_CHECK (strcmp (one.cur_tok (), "") == 0);
+
+  number_or_range_parser one_after ("1 after");
+
+  SELF_CHECK (one_after.finished () == false);
+  SELF_CHECK (one_after.get_number () == 1);
+  // SELF_CHECK (one_after.finished () == true);
+  // number_or_range_parser is cumbersome : to have finished == true,
+  // you have to call get_number once more. But then, the cur_tok is wrong.
+  // It should be restructured to have the setup (e.g. discovering if
+  // we have a number or a range or something else) in the init phase.
+  // This allows to indicate finished == true just after init,
+  // in case the init string is not a number or range.
+  SELF_CHECK (strcmp (one_after.cur_tok (), "after") == 0);
+
+  number_or_range_parser one_three ("1-3");
+
+  for (int i = 1; i < 4; i++) {
+    SELF_CHECK (one_three.finished () == false);
+    SELF_CHECK (one_three.get_number () == i);
+  }
+  SELF_CHECK (one_three.finished () == true);
+  SELF_CHECK (strcmp (one_three.cur_tok (), "") == 0);
+
+  number_or_range_parser one_three_after ("1-3 after");
+
+  for (int i = 1; i < 4; i++) {
+    SELF_CHECK (one_three_after.finished () == false);
+    SELF_CHECK (one_three_after.get_number () == i);
+  }
+  // SELF_CHECK (one_three_after.finished () == true);
+  // See above.
+  SELF_CHECK (strcmp (one_three_after.cur_tok (), "after") == 0);
+
+  number_or_range_parser minus_one ("-1");
+
+  SELF_CHECK (minus_one.finished () == false);
+  TRY
+    {
+      minus_one.get_number ();
+      SELF_CHECK (false);
+    }
+  CATCH (ex, RETURN_MASK_ERROR)
+    {
+      SELF_CHECK (ex.reason == RETURN_ERROR);
+      SELF_CHECK (ex.error == GENERIC_ERROR);
+      SELF_CHECK (strcmp (ex.message, "negative value") == 0);
+      SELF_CHECK (strcmp (minus_one.cur_tok (), "-1") == 0);
+    }
+  END_CATCH;
+
+  number_or_range_parser nan ("-whatever");
+
+  // SELF_CHECK (nan.finished () == true);
+  // See above
+  SELF_CHECK (one_three_after.get_number () == 0);
+  SELF_CHECK (strcmp (nan.cur_tok (), "-whatever") == 0);
+}
+
+static void
+test_check_for_flags ()
+{
+  const char *flags = "abc";
+  const char *non_flags_args = "non flags args";
+  int flags_counts[strlen (flags)];
+  int res;
+
+  const char *t1 = "-a -a non flags args";
+
+  SELF_CHECK (check_for_flags (&t1, flags, flags_counts) == 1);
+  SELF_CHECK (strcmp (t1, non_flags_args) == 0);
+  SELF_CHECK (flags_counts[0] == 2
+	      && flags_counts[1] == 0
+	      && flags_counts[2] == 0);
+
+  const char *t2 = "-c     -b -c  -b -c    non flags args";
+
+  SELF_CHECK (check_for_flags (&t2, flags, flags_counts) == 1);
+  SELF_CHECK (strcmp (t2, non_flags_args) == 0);
+  SELF_CHECK (flags_counts[0] == 0
+	      && flags_counts[1] == 2
+	      && flags_counts[2] == 3);
+
+  const char *t3 = non_flags_args;
+
+  SELF_CHECK (check_for_flags (&t3, flags, flags_counts) == 0);
+  SELF_CHECK (strcmp (t3, non_flags_args) == 0);
+  SELF_CHECK (flags_counts[0] == 0
+	      && flags_counts[1] == 0
+	      && flags_counts[2] == 0);
+
+  const char *t4 = "-c -b -x -y -z -c";
+  const char *orig_t4 = t4;
+
+  SELF_CHECK (check_for_flags (&t4, flags, flags_counts) == -1);
+  SELF_CHECK (strcmp (t4, orig_t4) == 0);
+  SELF_CHECK (flags_counts[0] == 0
+	      && flags_counts[1] == 1
+	      && flags_counts[2] == 1);
+
+  const char *t5 = "-c -cb -c";
+  const char *orig_t5 = t5;
+
+  SELF_CHECK (check_for_flags (&t5, flags, flags_counts) == -1);
+  SELF_CHECK (strcmp (t5, orig_t5) == 0);
+  SELF_CHECK (flags_counts[0] == 0
+	      && flags_counts[1] == 0
+	      && flags_counts[2] == 1);
+
+}
+
+static void
+test_check_for_flags_vqcs ()
+{
+  int verbosity;
+  bool cont;
+  bool silent;
+  const char *non_flags_args = "non flags args";
+
+  const char *t1 = "-v -v -v -q -q -q -s    non flags args";
+
+  verbosity = 1;
+  cont = true;
+  silent = false;
+  SELF_CHECK (check_for_flags_vqcs ("test_check_for_flags_vqcs.t1",
+				    &t1,
+				    &verbosity,
+				    4, &cont, &silent) == 1);
+  SELF_CHECK (verbosity == 1);
+  SELF_CHECK (cont == false && silent == true);
+  SELF_CHECK (strcmp (t1, non_flags_args) == 0);
+
+  const char *t2 = "non flags args";
+
+  verbosity = 2;
+  cont = true;
+  silent = true;
+  SELF_CHECK (check_for_flags_vqcs ("test_check_for_flags_vqcs.t2",
+				    &t2,
+				    &verbosity,
+				    4, &cont, &silent) == 0);
+  SELF_CHECK (verbosity == 2);
+  SELF_CHECK (cont == false && silent == false);
+  SELF_CHECK (strcmp (t2, non_flags_args) == 0);
+
+  const char *t3 = "-123 non flags args";
+  const char *orig_t3 = t3;
+
+  verbosity = 3;
+  cont = true;
+  silent = true;
+  SELF_CHECK (check_for_flags_vqcs ("test_check_for_flags_vqcs.t3",
+				    &t3,
+				    &verbosity,
+				    4, &cont, &silent) == 0);
+  SELF_CHECK (verbosity == 3);
+  SELF_CHECK (cont == false && silent == false);
+  SELF_CHECK (strcmp (t3, orig_t3) == 0);
+
+  const char *t4 = "-q -q -x -y -z non flags args";
+  const char *orig_t4 = t4;
+  TRY
+    {
+      verbosity = 4;
+      cont = true;
+      silent = true;
+      (void) check_for_flags_vqcs ("test_check_for_flags_vqcs.t4",
+				   &t4,
+				   &verbosity,
+				   4, &cont, &silent);
+      SELF_CHECK (false);
+    }
+  CATCH (ex, RETURN_MASK_ERROR)
+    {
+      SELF_CHECK (ex.reason == RETURN_ERROR);
+      SELF_CHECK (ex.error == GENERIC_ERROR);
+      SELF_CHECK
+	(strcmp (ex.message,
+		 "test_check_for_flags_vqcs.t4 "
+		 "only accepts flags vqcs given individually") == 0);
+      SELF_CHECK (strcmp (t4, orig_t4) == 0);
+      SELF_CHECK (verbosity == 4);
+    }
+  END_CATCH;
+
+}
+
+static void
+test_cli_utils ()
+{
+  selftests::cli_utils::test_number_or_range_parser ();
+  selftests::cli_utils::test_check_for_flags ();
+  selftests::cli_utils::test_check_for_flags_vqcs ();
+}
+
+}
+}
+
+void
+_initialize_cli_utils_selftests ()
+{
+  selftests::register_test ("cli_utils",
+			    selftests::cli_utils::test_cli_utils);
+}
-- 
2.11.0

[RFA_v2 7/8] Modify gdb.threads/threads.exp to test FLAGS vqcs for thread apply

[Philippe Waroquiers]

gdb/testsuite/ChangeLog
2018-06-05  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* gdb.threads/pthreads.exp: Test vqcs FLAGS.
---
 gdb/testsuite/gdb.threads/pthreads.exp | 65 ++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/gdb/testsuite/gdb.threads/pthreads.exp b/gdb/testsuite/gdb.threads/pthreads.exp
index 830432b833..7ac9476dfc 100644
--- a/gdb/testsuite/gdb.threads/pthreads.exp
+++ b/gdb/testsuite/gdb.threads/pthreads.exp
@@ -267,6 +267,70 @@ proc check_backtraces {} {
     }
 }
 
+proc check_vqcs {} {
+    set any "\[^\r\n\]*"
+    set ws "\[ \t\]\+"
+    set number "\[0-9]\+"
+
+    # check -c (continue) and -s (silently continue) flags
+    gdb_test "thread apply 2-3 p notfound" \
+	[multi_line \
+	     "" \
+	     "Thread 2 ${any}" \
+	     "No symbol \\\"notfound\\\" in current context." \
+	    ] \
+	"Run a failing command that aborts thread apply"
+
+    gdb_test "thread apply 2-3 -c p notfound" \
+	[multi_line \
+	     "" \
+	     "Thread 2 ${any}" \
+	     "No symbol \\\"notfound\\\" in current context." \
+	     "" \
+	     "Thread 3 ${any}" \
+	     "No symbol \\\"notfound\\\" in current context." \
+	    ] \
+	"Run a failing command, -c to continue"
+
+    foreach cmd {"thread apply all -s frame apply all -s p i" "tfaas p i" "taas faas p i"} {
+	gdb_test $cmd \
+	    [multi_line \
+		 "" \
+		 "Thread 3 ${any}" \
+		 "#${number}${ws}${any} in thread2 ${any}" \
+		 "\\\$\[0-9]+ = ${number}${any}" \
+		 "" \
+		 "Thread 2 ${any}" \
+		 "#${number}${ws}${any} in thread1 ${any}" \
+		 "\\\$\[0-9]+ = ${number}${any}" \
+		] \
+	    "Run a failing command except in one frame of thread 2,3, -s to silently continue"
+    }
+
+    # Verbosity tests
+    gdb_test "thread apply all -s -q frame apply all -s p i" \
+	[multi_line \
+	     "#${number}${ws}${any} in thread2 ${any}" \
+	     "\\\$\[0-9]+ = ${number}${any}" \
+	     "#${number}${ws}${any} in thread1 ${any}" \
+	     "\\\$\[0-9]+ = ${number}${any}" \
+	    ] \
+	"Run a failing command except in one frame of thread 2,3, -s to silently continue. Verbosity 0 threads"
+
+    gdb_test "thread apply all -s -q frame apply all -s -q -q p i" \
+	[multi_line \
+	     "\\\$\[0-9]+ = ${number}${any}" \
+	     "\\\$\[0-9]+ = ${number}${any}" \
+	    ] \
+	"Run a failing command except in one frame of thread 2,3, -s to silently continue. Verbosity 0 threads and frames"
+
+    # check invalid flag combinations
+    gdb_test "thread apply all -c -s p 1" \
+	"thread apply all: flags c and s are mutually exclusive" \
+	"Check c and s cannot be used simultaneously"
+
+}
+
 if [runto_main] then {
     if [test_startup] then {
 	if [check_control_c] then {
@@ -274,5 +338,6 @@ if [runto_main] then {
 	    return
 	}
 	check_backtraces
+	check_vqcs
     }
 }
-- 
2.11.0

[RFA_v2 2/8] Implement frame apply [all | COUNT | -COUNT] [FLAGS...] COMMAND.

[Philippe Waroquiers]

Implement frame apply [all | COUNT | -COUNT] [FLAGS...] COMMAND.
Also implement the command 'faas COMMAND', a shortcut for
'frame apply all -s COMMAND'.

Note: the syntax of 'frame apply' to specify some innermost or outermost
frames is similar to 'backtrace' command.
An alternative could be to make 'frame apply' similar to
'thread apply'. This was not chosen as:
  * the typical use cases for frame apply are all/some innermost/some outermost
    frames.
  * a range based syntax would have obliged the user to use absolute numbers
    to specify the outermost N frames, which is cumbersone.
    Or an syntax different from the thread range would have been needed
    (e.g. -0-4 to specify the 5 outermost frames).
So, making frame apply similar to backtrace is found better.

Th new command 'frame apply' allows to apply a COMMAND to a number of frames,
or to all frames.
The optional FLAGS... arguments allow to control what output to produce
and how to handle errors raised when applying COMMAND to a frame.

Some examples usages for this new command:
   frame apply all info frame
      Produce info frame for all frames
   frame apply all p $sp
      For each frame, print the location, followed by the frame sp
   frame apply all -qq p $sp
      Same as before, but -qq flags (q = quiet) indicate to only print
      the frames sp.
   frame apply all -vv p $sp
      Same as before, but -vv flags (v = verbose) indicate to print
      location and source line for each frame.
   frame apply all p some_local_var_somewhere
      Print some_local_var_somewhere in all frames. 'frame apply'
      will abort as soon as the print command fails.
   frame apply all -c p some_local_var_somewhere
      Same as before, but -c flag (c = continue) means to
      print the error and continue applying command in case the
      print command fails.
   frame apply all -s p some_local_var_somewhere
      Same as before, but -s flag (s = silent) means to
      be silent for frames where the print command fails.
      In other words, this allows to 'search' the frame in which
      some_local_var_somewhere can be printed.

gdb/ChangeLog
2018-06-04  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* stack.c: (trailing_outermost_frame): New function, mostly
	extracted from backtrace_command_1.
	(backtrace_command_1): Update to call trailing_outermost_frame.
	(frame_apply_command_count): New function.
	(frame_apply_all_command): New function.
	(frame_apply_command): New function.
	(faas_command): New function.
	(frame_cmd_list): New variable.
	(_initialize_stack): Update to setup the new commands 'frame apply'
	and 'faas'.
---
 gdb/stack.c | 246 ++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 223 insertions(+), 23 deletions(-)

diff --git a/gdb/stack.c b/gdb/stack.c
index 97ebc8bc23..090483cd12 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -1687,6 +1687,38 @@ info_frame_command (const char *addr_exp, int from_tty)
   }
 }
 
+/* trailing_outermost_frame returns the starting frame
+   needed to handle COUNT outermost frames.  */
+
+static struct frame_info *
+trailing_outermost_frame (int count)
+{
+  struct frame_info *current;
+  struct frame_info *trailing;
+
+  trailing = get_current_frame ();
+
+  gdb_assert (count > 0);
+
+  current = trailing;
+  while (current != nullptr && count--)
+    {
+      QUIT;
+      current = get_prev_frame (current);
+    }
+
+  /* Will stop when CURRENT reaches the top of the stack.
+     TRAILING will be COUNT below it.  */
+  while (current != nullptr)
+    {
+      QUIT;
+      trailing = get_prev_frame (trailing);
+      current = get_prev_frame (current);
+    }
+
+  return trailing;
+}
+
 /* Print briefly all stack frames or just the innermost COUNT_EXP
    frames.  */
 
@@ -1751,32 +1783,14 @@ backtrace_command_1 (const char *count_exp, frame_filter_flags flags,
 	 variable TRAILING to the frame from which we should start
 	 printing.  Second, it must set the variable count to the number
 	 of frames which we should print, or -1 if all of them.  */
-      trailing = get_current_frame ();
 
       if (count_exp != NULL && count < 0)
 	{
-	  struct frame_info *current;
-
-	  count = -count;
-
-	  current = trailing;
-	  while (current && count--)
-	    {
-	      QUIT;
-	      current = get_prev_frame (current);
-	    }
-
-	  /* Will stop when CURRENT reaches the top of the stack.
-	     TRAILING will be COUNT below it.  */
-	  while (current)
-	    {
-	      QUIT;
-	      trailing = get_prev_frame (trailing);
-	      current = get_prev_frame (current);
-	    }
-
+	  trailing = trailing_outermost_frame (-count);
 	  count = -1;
 	}
+      else
+	trailing = get_current_frame ();
 
       for (fi = trailing; fi && count--; fi = get_prev_frame (fi))
 	{
@@ -2494,9 +2508,160 @@ func_command (const char *arg, int from_tty)
     }
 }
 
+/* Apply a GDB command to a all stack frames, or innermost COUNT frames.
+   With a negative COUNT, apply command on outermost -COUNT frames.
+
+   frame apply 3 info frame     Apply 'info frame' to frames 0, 1, 2
+   frame apply -3 info frame    Apply 'info frame' to outermost 3 frames.
+   frame apply all x/i $pc      Apply 'x/i $pc' cmd to all frames.
+   frame apply all -s p local_var_no_idea_in_which_frame
+                If a frame has a local variable called
+                local_var_no_idea_in_which_frame, print frame
+                and value of local_var_no_idea_in_which_frame.
+   frame apply all -sqq p local_var_no_idea_in_which_frame
+                Same as before, but only print the variable value.  */
+
+/* Apply a GDB command to COUNT stack frames, starting at TRAILING.
+   COUNT -1 means all frames starting at TRAILING.  WHICH_COMMAND is used
+   for error messages.  */
+static void
+frame_apply_command_count (const char* which_command,
+			   const char *cmd, int from_tty,
+			   struct frame_info *trailing, int count)
+{
+  int print_what_v = 2; /* Corresponding to LOCATION.  */
+  enum print_what print_what[5] =
+    {
+      LOC_AND_ADDRESS, /* Should never be used, this is verbosity 0.  */
+      SRC_LINE,
+      LOCATION,
+      LOC_AND_ADDRESS,
+      SRC_AND_LOC
+    };
+  bool cont;
+  bool silent;
+  struct frame_info *fi;
+
+  if (cmd != NULL)
+    check_for_flags_vqcs (which_command, &cmd,
+			  &print_what_v, 4,
+			  &cont, &silent);
+
+  if (cmd == NULL || *cmd == '\0')
+    error (_("Please specify a command to apply on the selected frames"));
+
+  /* The below will restore the current inferior/thread/frame.
+     Usually, only the frame is effectively to be restored.
+     But in case CMD switches of inferior/thread, better restore
+     these also.  */
+  scoped_restore_current_thread restore_thread;
+
+  for (fi = trailing; fi && count--; fi = get_prev_frame (fi))
+    {
+      struct frame_id frame_id = get_frame_id (fi);
+
+      QUIT;
+
+      select_frame (fi);
+      TRY
+	{
+	  std::string cmd_result;
+	  {
+	    /* In case CMD switches of inferior/thread/frame, the below
+	       restores the inferior/thread.  We can then re-initialise
+	       FI based on FRAME_ID.  */
+	    scoped_restore_current_thread restore_fi_current_frame;
+
+	    cmd_result = execute_command_to_string (cmd, from_tty);
+	  }
+	  fi = frame_find_by_id (frame_id);
+	  if (fi == NULL)
+	    {
+	      warning (_("Unable to restore previously selected frame."));
+	      break;
+	    }
+	  if (!silent || cmd_result.length () > 0)
+	    {
+	      if (print_what_v > 0)
+		print_stack_frame (fi, 1, print_what[print_what_v], 0);
+	      printf_filtered ("%s", cmd_result.c_str ());
+	    }
+	}
+      CATCH (ex, RETURN_MASK_ERROR)
+	{
+	  fi = frame_find_by_id (frame_id);
+	  if (fi == NULL)
+	    {
+	      warning (_("Unable to restore previously selected frame."));
+	      break;
+	    }
+	  if (!silent)
+	    {
+	      if (print_what_v > 0)
+		print_stack_frame (fi, 1, print_what [print_what_v], 0);
+	      if (cont)
+		printf_filtered ("%s\n", ex.message);
+	      else
+		throw_exception (ex);
+	    }
+	}
+      END_CATCH;
+    }
+}
+
+static void
+frame_apply_all_command (const char *cmd, int from_tty)
+{
+  if (!target_has_stack)
+    error (_("No stack."));
+
+  frame_apply_command_count ("frame apply all", cmd, from_tty,
+			     get_current_frame (), INT_MAX);
+}
+
+/* Implementation of the "frame apply" command.  */
+
+static void
+frame_apply_command (const char* cmd, int from_tty)
+{
+  int count;
+  struct frame_info *trailing;
+
+  if (!target_has_stack)
+    error (_("No stack."));
+
+  count = get_number_trailer (&cmd, 0);
+
+  if (count < 0)
+    {
+      trailing = trailing_outermost_frame (-count);
+      count = -1;
+    }
+  else
+    trailing = get_current_frame ();
+
+  frame_apply_command_count ("frame apply", cmd, from_tty,
+			     trailing, count);
+}
+
+/* Implementation of the "faas" command.  */
+
+static void
+faas_command (const char *cmd, int from_tty)
+{
+  std::string expanded = std::string ("frame apply all -s ") + std::string (cmd);
+  execute_command (expanded.c_str (), from_tty);
+}
+
+
+/* Commands with a prefix of `frame'.  */
+struct cmd_list_element *frame_cmd_list = NULL;
+
 void
 _initialize_stack (void)
 {
+  static struct cmd_list_element *frame_apply_list = NULL;
+
   add_com ("return", class_stack, return_command, _("\
 Make selected stack frame return to its caller.\n\
 Control remains in the debugger, but when you continue\n\
@@ -2519,14 +2684,49 @@ An argument says how many frames down to go."));
 Same as the `down' command, but does not print anything.\n\
 This is useful in command scripts."));
 
-  add_com ("frame", class_stack, frame_command, _("\
+  add_prefix_cmd ("frame", class_stack, frame_command, _("\
 Select and print a stack frame.\nWith no argument, \
 print the selected stack frame.  (See also \"info frame\").\n\
 An argument specifies the frame to select.\n\
-It can be a stack frame number or the address of the frame."));
+It can be a stack frame number or the address of the frame."),
+		  &frame_cmd_list, "frame ", 1, &cmdlist);
 
   add_com_alias ("f", "frame", class_stack, 1);
 
+#define FRAME_APPLY_FLAGS_HELP "\
+FLAGS are -v (increase verbosity), -q (decrease verbosity)\n\
+  -c (continue), -s (silent).\n\
+Verbosity (default 2) controls what to print for a frame:\n\
+  0 : no frame info is printed\n\
+  1 : source line\n\
+  2 : location\n\
+  3 : location and address\n\
+  4 : source line and location\n\
+By default, if a COMMAND raises an error, frame apply is aborted.\n\
+Flag -c indicates to print the error and continue.\n\
+Flag -s indicates to silently ignore a COMMAND that raises an error\n\
+or produces no output."
+
+  add_prefix_cmd ("apply", class_stack, frame_apply_command,
+		  _("Apply a command to a number of frames.\n\
+Usage: frame apply COUNT [FLAGS...] COMMAND\n\
+With a negative COUNT argument, applies the command on outermost -COUNT frames.\n"
+FRAME_APPLY_FLAGS_HELP),
+		  &frame_apply_list, "frame apply ", 1, &frame_cmd_list);
+
+  add_cmd ("all", class_stack, frame_apply_all_command,
+	   _("\
+Apply a command to all frames.\n\
+\n\
+Usage: frame apply all [FLAGS...] COMMAND\n"
+FRAME_APPLY_FLAGS_HELP),
+	   &frame_apply_list);
+
+  add_com ("faas", class_stack, faas_command, _("\
+Apply a command to all frames (ignoring errors and empty output).\n\
+Usage: faas COMMAND\n\
+shortcut for 'frame apply all -s COMMAND'"));
+
   add_com_suppress_notification ("select-frame", class_stack, select_frame_command, _("\
 Select a stack frame without printing anything.\n\
 An argument specifies the frame to select.\n\
-- 
2.11.0

Re: [RFA_v2 2/8] Implement frame apply [all | COUNT | -COUNT] [FLAGS...] COMMAND.

[Pedro Alves]

On 06/05/2018 09:48 PM, Philippe Waroquiers wrote:
> Implement frame apply [all | COUNT | -COUNT] [FLAGS...] COMMAND.

coreutils puts the "..." after the [], and uses singular.
E.g., ls --help says "Usage: ls [OPTION]... [FILE]...".

I still think it's better to say OPTION, because future
options may not be flags, they may take arguments.  So:

  Implement frame apply [all | COUNT | -COUNT] [OPTION]... COMMAND.

Same comments apply to the "thread apply" patch too, of course.

> Also implement the command 'faas COMMAND', a shortcut for
> 'frame apply all -s COMMAND'.
> 
> Note: the syntax of 'frame apply' to specify some innermost or outermost
> frames is similar to 'backtrace' command.
> An alternative could be to make 'frame apply' similar to
> 'thread apply'. This was not chosen as:
>   * the typical use cases for frame apply are all/some innermost/some outermost
>     frames.
>   * a range based syntax would have obliged the user to use absolute numbers
>     to specify the outermost N frames, which is cumbersone.
>     Or an syntax different from the thread range would have been needed
>     (e.g. -0-4 to specify the 5 outermost frames).
> So, making frame apply similar to backtrace is found better.

Hmm, I played with this a bit now, AFAICT, this syntax forces the applicable
range to start at either the innermost or outermost?  How does one e.g., apply
the command to frames 10 to 20 (because e.g., they are the frames that
are running some library code), when there exist frames 0-50?  

> 
> Th new command 'frame apply' allows to apply a COMMAND to a number of frames,
> or to all frames.

This stands out as a limitation to me.

"Th" -> "The"

> The optional FLAGS... arguments allow to control what output to produce
> and how to handle errors raised when applying COMMAND to a frame.
> 
> Some examples usages for this new command:
>    frame apply all info frame
>       Produce info frame for all frames
>    frame apply all p $sp
>       For each frame, print the location, followed by the frame sp
>    frame apply all -qq p $sp
>       Same as before, but -qq flags (q = quiet) indicate to only print
>       the frames sp.
>    frame apply all -vv p $sp

This needs updating for non-combining options.

>       Same as before, but -vv flags (v = verbose) indicate to print
>       location and source line for each frame.
>    frame apply all p some_local_var_somewhere
>       Print some_local_var_somewhere in all frames. 'frame apply'
>       will abort as soon as the print command fails.
>    frame apply all -c p some_local_var_somewhere
>       Same as before, but -c flag (c = continue) means to
>       print the error and continue applying command in case the
>       print command fails.
>    frame apply all -s p some_local_var_somewhere
>       Same as before, but -s flag (s = silent) means to
>       be silent for frames where the print command fails.
>       In other words, this allows to 'search' the frame in which
>       some_local_var_somewhere can be printed.

I played with this a bit, and I have to admit that I found the
-v / -q combinations, and relativeness of the "quietness/verbosity"
level, a bit unintuitive and confusing.

E.g.,:

 (top-gdb) frame apply 2 echo
 #0  0x00007ffff54e0c6b in __GI___poll (fds=0x1a343d0, nfds=4, timeout=-1) at ../sysdeps/unix/sysv/linux/poll.c:29
 #1  0x00000000007083e4 in gdb_wait_for_event (block=1) at src/gdb/event-loop.c:771

 (top-gdb) frame apply 2 -q echo
 0x00007ffff54e0c6b      29        return SYSCALL_CANCEL (poll, fds, nfds, timeout);
 0x00000000007083e4      771           num_found = poll (gdb_notifier.poll_fds,
 (top-gdb) 

Here, "-q" didn't make the output really be quieter, only different.

Consider also if we add a knob to tweak the default verbosity.
Like "set frame-apply-default-format N" or whatever.
With that, "frame apply -q" does a different thing depending on
the value of the default setting.

So I'm wondering whether "frame apply -quiet/-source/-location/-etc."
options wouldn't be better.  

Or "frame apply -format [0-4]" and/or 
"frame apply -format quiet/source/location/location".

Maybe alternatively consider ditching the format ideas, and have
users combine "frame apply -q" with "list" if they want to see
sources.  OK, I expect resistance to that idea.  :-)

> 
> gdb/ChangeLog
> 2018-06-04  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> 
> 	* stack.c: (trailing_outermost_frame): New function, mostly
> 	extracted from backtrace_command_1.
> 	(backtrace_command_1): Update to call trailing_outermost_frame.
> 	(frame_apply_command_count): New function.
> 	(frame_apply_all_command): New function.
> 	(frame_apply_command): New function.
> 	(faas_command): New function.
> 	(frame_cmd_list): New variable.
> 	(_initialize_stack): Update to setup the new commands 'frame apply'
> 	and 'faas'.
> ---
>  gdb/stack.c | 246 ++++++++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 223 insertions(+), 23 deletions(-)
> 
> diff --git a/gdb/stack.c b/gdb/stack.c
> index 97ebc8bc23..090483cd12 100644
> --- a/gdb/stack.c
> +++ b/gdb/stack.c
> @@ -1687,6 +1687,38 @@ info_frame_command (const char *addr_exp, int from_tty)
>    }
>  }
>  
> +/* trailing_outermost_frame returns the starting frame

Don't repeat function names:

 s/trailing_outermost_frame returns/Return/

> +   needed to handle COUNT outermost frames.  */
> +
> +static struct frame_info *
> +trailing_outermost_frame (int count)
> +{
> +  struct frame_info *current;
> +  struct frame_info *trailing;
> +
> +  trailing = get_current_frame ();
> +
> +  gdb_assert (count > 0);
> +
> +  current = trailing;
> +  while (current != nullptr && count--)
> +    {
> +      QUIT;
> +      current = get_prev_frame (current);
> +    }
> +
> +  /* Will stop when CURRENT reaches the top of the stack.
> +     TRAILING will be COUNT below it.  */
> +  while (current != nullptr)
> +    {
> +      QUIT;
> +      trailing = get_prev_frame (trailing);
> +      current = get_prev_frame (current);
> +    }
> +
> +  return trailing;
> +}
> +
>  /* Print briefly all stack frames or just the innermost COUNT_EXP
>     frames.  */
>  
> @@ -1751,32 +1783,14 @@ backtrace_command_1 (const char *count_exp, frame_filter_flags flags,
>  	 variable TRAILING to the frame from which we should start
>  	 printing.  Second, it must set the variable count to the number
>  	 of frames which we should print, or -1 if all of them.  */
> -      trailing = get_current_frame ();
>  
>        if (count_exp != NULL && count < 0)
>  	{
> -	  struct frame_info *current;
> -
> -	  count = -count;
> -
> -	  current = trailing;
> -	  while (current && count--)
> -	    {
> -	      QUIT;
> -	      current = get_prev_frame (current);
> -	    }
> -
> -	  /* Will stop when CURRENT reaches the top of the stack.
> -	     TRAILING will be COUNT below it.  */
> -	  while (current)
> -	    {
> -	      QUIT;
> -	      trailing = get_prev_frame (trailing);
> -	      current = get_prev_frame (current);
> -	    }
> -
> +	  trailing = trailing_outermost_frame (-count);
>  	  count = -1;
>  	}
> +      else
> +	trailing = get_current_frame ();
>  
>        for (fi = trailing; fi && count--; fi = get_prev_frame (fi))
>  	{
> @@ -2494,9 +2508,160 @@ func_command (const char *arg, int from_tty)
>      }
>  }
>  
> +/* Apply a GDB command to a all stack frames, or innermost COUNT frames.
> +   With a negative COUNT, apply command on outermost -COUNT frames.
> +
> +   frame apply 3 info frame     Apply 'info frame' to frames 0, 1, 2
> +   frame apply -3 info frame    Apply 'info frame' to outermost 3 frames.
> +   frame apply all x/i $pc      Apply 'x/i $pc' cmd to all frames.
> +   frame apply all -s p local_var_no_idea_in_which_frame
> +                If a frame has a local variable called
> +                local_var_no_idea_in_which_frame, print frame
> +                and value of local_var_no_idea_in_which_frame.
> +   frame apply all -sqq p local_var_no_idea_in_which_frame
> +                Same as before, but only print the variable value.  */

Needs updating for non-combined options.

> +
> +/* Apply a GDB command to COUNT stack frames, starting at TRAILING.
> +   COUNT -1 means all frames starting at TRAILING.  WHICH_COMMAND is used
> +   for error messages.  */
> +static void
> +frame_apply_command_count (const char* which_command,

"const char* " => "const char *"

> +			   const char *cmd, int from_tty,
> +			   struct frame_info *trailing, int count)
> +{
> +  int print_what_v = 2; /* Corresponding to LOCATION.  */
> +  enum print_what print_what[5] =

No need for this explicit "5", I think.

> +    {
> +      LOC_AND_ADDRESS, /* Should never be used, this is verbosity 0.  */
> +      SRC_LINE,
> +      LOCATION,
> +      LOC_AND_ADDRESS,
> +      SRC_AND_LOC
> +    };
> +  bool cont;
> +  bool silent;
> +  struct frame_info *fi;
> +
> +  if (cmd != NULL)
> +    check_for_flags_vqcs (which_command, &cmd,
> +			  &print_what_v, 4,
> +			  &cont, &silent);
> +
> +  if (cmd == NULL || *cmd == '\0')
> +    error (_("Please specify a command to apply on the selected frames"));
> +
> +  /* The below will restore the current inferior/thread/frame.
> +     Usually, only the frame is effectively to be restored.
> +     But in case CMD switches of inferior/thread, better restore
> +     these also.  */
> +  scoped_restore_current_thread restore_thread;
> +
> +  for (fi = trailing; fi && count--; fi = get_prev_frame (fi))
> +    {
> +      struct frame_id frame_id = get_frame_id (fi);
> +
> +      QUIT;
> +
> +      select_frame (fi);
> +      TRY
> +	{
> +	  std::string cmd_result;
> +	  {
> +	    /* In case CMD switches of inferior/thread/frame, the below
> +	       restores the inferior/thread.  We can then re-initialise
> +	       FI based on FRAME_ID.  */
> +	    scoped_restore_current_thread restore_fi_current_frame;
> +
> +	    cmd_result = execute_command_to_string (cmd, from_tty);
> +	  }
> +	  fi = frame_find_by_id (frame_id);
> +	  if (fi == NULL)
> +	    {
> +	      warning (_("Unable to restore previously selected frame."));

scoped_restore_current_thread also restores the frame, and, also
warns.

> +	      break;

This "break" isn't really breaking out of the for loop, because
of the way TRY is implemented.

> +	    }
> +	  if (!silent || cmd_result.length () > 0)
> +	    {
> +	      if (print_what_v > 0)
> +		print_stack_frame (fi, 1, print_what[print_what_v], 0);
> +	      printf_filtered ("%s", cmd_result.c_str ());
> +	    }
> +	}
> +      CATCH (ex, RETURN_MASK_ERROR)
> +	{
> +	  fi = frame_find_by_id (frame_id);
> +	  if (fi == NULL)
> +	    {
> +	      warning (_("Unable to restore previously selected frame."));
> +	      break;
> +	    }

Ditto on the warning.

> +	  if (!silent)
> +	    {
> +	      if (print_what_v > 0)
> +		print_stack_frame (fi, 1, print_what [print_what_v], 0);
> +	      if (cont)
> +		printf_filtered ("%s\n", ex.message);
> +	      else
> +		throw_exception (ex);
> +	    }
> +	}
> +      END_CATCH;
> +    }
> +}
> +

Missing intro comment:

/* Implementation of the "frame apply all" command.  */

> +static void
> +frame_apply_all_command (const char *cmd, int from_tty)
> +{
> +  if (!target_has_stack)
> +    error (_("No stack."));
> +
> +  frame_apply_command_count ("frame apply all", cmd, from_tty,
> +			     get_current_frame (), INT_MAX);
> +}
> +
> +/* Implementation of the "frame apply" command.  */
> +
> +static void
> +frame_apply_command (const char* cmd, int from_tty)
> +{
> +  int count;
> +  struct frame_info *trailing;
> +
> +  if (!target_has_stack)
> +    error (_("No stack."));
> +
> +  count = get_number_trailer (&cmd, 0);

You can declare and initialize at the same time:

   int count = get_number_trailer (&cmd, 0);

> +
> +  if (count < 0)
> +    {
> +      trailing = trailing_outermost_frame (-count);
> +      count = -1;
> +    }
> +  else
> +    trailing = get_current_frame ();
> +
> +  frame_apply_command_count ("frame apply", cmd, from_tty,
> +			     trailing, count);
> +}
> +
> +/* Implementation of the "faas" command.  */
> +
> +static void
> +faas_command (const char *cmd, int from_tty)
> +{
> +  std::string expanded = std::string ("frame apply all -s ") + std::string (cmd);

 std::string expanded = std::string ("frame apply all -s ") + cmd;

> +  execute_command (expanded.c_str (), from_tty);
> +}
> +
> +
> +/* Commands with a prefix of `frame'.  */
> +struct cmd_list_element *frame_cmd_list = NULL;
> +
>  void
>  _initialize_stack (void)
>  {
> +  static struct cmd_list_element *frame_apply_list = NULL;
> +
>    add_com ("return", class_stack, return_command, _("\
>  Make selected stack frame return to its caller.\n\
>  Control remains in the debugger, but when you continue\n\
> @@ -2519,14 +2684,49 @@ An argument says how many frames down to go."));
>  Same as the `down' command, but does not print anything.\n\
>  This is useful in command scripts."));
>  
> -  add_com ("frame", class_stack, frame_command, _("\
> +  add_prefix_cmd ("frame", class_stack, frame_command, _("\
>  Select and print a stack frame.\nWith no argument, \
>  print the selected stack frame.  (See also \"info frame\").\n\
>  An argument specifies the frame to select.\n\
> -It can be a stack frame number or the address of the frame."));
> +It can be a stack frame number or the address of the frame."),
> +		  &frame_cmd_list, "frame ", 1, &cmdlist);
>  
>    add_com_alias ("f", "frame", class_stack, 1);
>  
> +#define FRAME_APPLY_FLAGS_HELP "\
> +FLAGS are -v (increase verbosity), -q (decrease verbosity)\n\
> +  -c (continue), -s (silent).\n\
> +Verbosity (default 2) controls what to print for a frame:\n\
> +  0 : no frame info is printed\n\
> +  1 : source line\n\
> +  2 : location\n\
> +  3 : location and address\n\
> +  4 : source line and location\n\
> +By default, if a COMMAND raises an error, frame apply is aborted.\n\
> +Flag -c indicates to print the error and continue.\n\
> +Flag -s indicates to silently ignore a COMMAND that raises an error\n\
> +or produces no output."
> +
> +  add_prefix_cmd ("apply", class_stack, frame_apply_command,
> +		  _("Apply a command to a number of frames.\n\
> +Usage: frame apply COUNT [FLAGS...] COMMAND\n\
> +With a negative COUNT argument, applies the command on outermost -COUNT frames.\n"
> +FRAME_APPLY_FLAGS_HELP),
> +		  &frame_apply_list, "frame apply ", 1, &frame_cmd_list);
> +
> +  add_cmd ("all", class_stack, frame_apply_all_command,
> +	   _("\
> +Apply a command to all frames.\n\
> +\n\
> +Usage: frame apply all [FLAGS...] COMMAND\n"
> +FRAME_APPLY_FLAGS_HELP),
> +	   &frame_apply_list);
> +
> +  add_com ("faas", class_stack, faas_command, _("\
> +Apply a command to all frames (ignoring errors and empty output).\n\
> +Usage: faas COMMAND\n\
> +shortcut for 'frame apply all -s COMMAND'"));
> +
>    add_com_suppress_notification ("select-frame", class_stack, select_frame_command, _("\
>  Select a stack frame without printing anything.\n\
>  An argument specifies the frame to select.\n\
> 
Thanks,
Pedro Alves

Re: [RFA_v2 2/8] Implement frame apply [all | COUNT | -COUNT] [FLAGS...] COMMAND.

[Philippe Waroquiers]

On Wed, 2018-06-13 at 20:52 +0100, Pedro Alves wrote:
>  Implement frame apply [all | COUNT | -COUNT] [OPTION]... COMMAND.
> 
> Same comments apply to the "thread apply" patch too, of course.
> 
> > Also implement the command 'faas COMMAND', a shortcut for
> > 'frame apply all -s COMMAND'.
> > 
> > Note: the syntax of 'frame apply' to specify some innermost or outermost
> > frames is similar to 'backtrace' command.
> > An alternative could be to make 'frame apply' similar to
> > 'thread apply'. This was not chosen as:
> >   * the typical use cases for frame apply are all/some innermost/some outermost
> >     frames.
> >   * a range based syntax would have obliged the user to use absolute numbers
> >     to specify the outermost N frames, which is cumbersone.
> >     Or an syntax different from the thread range would have been needed
> >     (e.g. -0-4 to specify the 5 outermost frames).
> > So, making frame apply similar to backtrace is found better.
> 
> Hmm, I played with this a bit now, AFAICT, this syntax forces the applicable
> range to start at either the innermost or outermost?  How does one e.g., apply
> the command to frames 10 to 20 (because e.g., they are the frames that
> are running some library code), when there exist frames 0-50?  
> 
> > 
> > Th new command 'frame apply' allows to apply a COMMAND to a number of frames,
> > or to all frames.
> 
> This stands out as a limitation to me.
Yes, it allows to (only) do what backtrace allows, and so, similarly
to backtrace, does not allow to 'cherry pick' frames (or range of frames).

I initially tried to find a reasonable syntax to allow a range based
selection of frames, but could not obtain anything intuitive to specify
the outermost N frames.

Assuming no better syntax is found, maybe we need to explain better
in the user manual that 'frame apply' selection of frame is similar/compatible
with backtrace, maybe something like:
    'frame apply' uses the same convention as 'backtrace' to specify the
    frames to apply COMMAND on : N innermost frames, or N outermost frames,
    or all frames.
    Note that 'thread apply' uses  a list of thread IDs or thread IDs range.

I understand that the differences is not ideal, but IMO, there
are inherent differences between thread and frames, making it difficult
to use a common syntax for 'frame lists' and 'thread lists'.
E.g. there is no concept of innermost/outermost thread list, while
I think we need a syntax for this concept in 'frame apply'.

So, in summary, would be fine for me to change 'frame apply'
for a better syntax, but which better syntax allows to specify
as intuitively as backtrace N outermost frames ?

And if we have another syntax than backtrace, then it should be
similar to thread apply THREADID ranges, otherwise that would be very
confusing.
=> I do not see a syntax consistent with thread apply ID ranges
or individual IDs, that would allow to specify N innermost or
outermost frames.

> 
> I played with this a bit, and I have to admit that I found the
> -v / -q combinations, and relativeness of the "quietness/verbosity"
> level, a bit unintuitive and confusing.
> 
> E.g.,:
> 
>  (top-gdb) frame apply 2 echo
>  #0  0x00007ffff54e0c6b in __GI___poll (fds=0x1a343d0, nfds=4, timeout=-1) at ../sysdeps/unix/sysv/linux/poll.c:29
>  #1  0x00000000007083e4 in gdb_wait_for_event (block=1) at src/gdb/event-loop.c:771
> 
>  (top-gdb) frame apply 2 -q echo
>  0x00007ffff54e0c6b      29        return SYSCALL_CANCEL (poll, fds, nfds, timeout);
>  0x00000000007083e4      771           num_found = poll (gdb_notifier.poll_fds,
>  (top-gdb) 
> 
> Here, "-q" didn't make the output really be quieter, only different.
Yes, the verbosity just controls the choice of the value of
frame.h enum print_what:
 enum print_what
  { 
    /* Print only the source line, like in stepi.  */
    SRC_LINE = -1, 
    /* Print only the location, i.e. level, address (sometimes)
       function, args, file, line, line num.  */
    LOCATION,
    /* Print both of the above.  */
    SRC_AND_LOC, 
    /* Print location only, but always include the address.  */
    LOC_AND_ADDRESS 
  };

and that is not really a 'monotonically increasing function'.



> 
> Consider also if we add a knob to tweak the default verbosity.
> Like "set frame-apply-default-format N" or whatever.
> With that, "frame apply -q" does a different thing depending on
> the value of the default setting.
> 
> So I'm wondering whether "frame apply -quiet/-source/-location/-etc."
> options wouldn't be better.  
> 
> Or "frame apply -format [0-4]" and/or 
> "frame apply -format quiet/source/location/location".
> 
> Maybe alternatively consider ditching the format ideas, and have
> users combine "frame apply -q" with "list" if they want to see
> sources.  OK, I expect resistance to that idea.  :-)

Not too much resistance on my side to drop the format idea :).

On a second thought, this verbosity looks all too 'over-engineered' :
  thread apply verbosity currently only allows to print nothing
    or a fixed set of information per thread
  backtrace prints a fixed information per frame
  frame apply allows to choose whatever in enum print_what with
    non intuitive verbosity control.

=> I suggest to then just have the -q option : by default,
frame information is printed like backtrace, and if -q is given,
no information is printed.
And same for 'thread apply' : by default, it prints a fixed
set of info. And -q means do not print it.
(we for sure need at least the -q to allow only the thread/frame info
to be
printed for successful COMMAND).

(again, in the future, we can if we want add other options
to control what to print, if we really need this flexibility).

So, what do you think about the (simpler) idea of just having
  -q for both thread apply and frame apply ?

Philippe

Re: [RFA_v2 2/8] Implement frame apply [all | COUNT | -COUNT] [FLAGS...] COMMAND.

[Pedro Alves]

On 06/15/2018 12:01 AM, Philippe Waroquiers wrote:
> On Wed, 2018-06-13 at 20:52 +0100, Pedro Alves wrote:
>>  Implement frame apply [all | COUNT | -COUNT] [OPTION]... COMMAND.
>>
>> Same comments apply to the "thread apply" patch too, of course.
>>
>>> Also implement the command 'faas COMMAND', a shortcut for
>>> 'frame apply all -s COMMAND'.
>>>
>>> Note: the syntax of 'frame apply' to specify some innermost or outermost
>>> frames is similar to 'backtrace' command.
>>> An alternative could be to make 'frame apply' similar to
>>> 'thread apply'. This was not chosen as:
>>>   * the typical use cases for frame apply are all/some innermost/some outermost
>>>     frames.
>>>   * a range based syntax would have obliged the user to use absolute numbers
>>>     to specify the outermost N frames, which is cumbersone.
>>>     Or an syntax different from the thread range would have been needed
>>>     (e.g. -0-4 to specify the 5 outermost frames).
>>> So, making frame apply similar to backtrace is found better.
>>
>> Hmm, I played with this a bit now, AFAICT, this syntax forces the applicable
>> range to start at either the innermost or outermost?  How does one e.g., apply
>> the command to frames 10 to 20 (because e.g., they are the frames that
>> are running some library code), when there exist frames 0-50?  
>>
>>>
>>> Th new command 'frame apply' allows to apply a COMMAND to a number of frames,
>>> or to all frames.
>>
>> This stands out as a limitation to me.
> Yes, it allows to (only) do what backtrace allows, and so, similarly
> to backtrace, does not allow to 'cherry pick' frames (or range of frames).
> 
> I initially tried to find a reasonable syntax to allow a range based
> selection of frames, but could not obtain anything intuitive to specify
> the outermost N frames.
> 
> Assuming no better syntax is found, maybe we need to explain better
> in the user manual that 'frame apply' selection of frame is similar/compatible
> with backtrace, maybe something like:
>     'frame apply' uses the same convention as 'backtrace' to specify the
>     frames to apply COMMAND on : N innermost frames, or N outermost frames,
>     or all frames.
>     Note that 'thread apply' uses  a list of thread IDs or thread IDs range.
> 
> I understand that the differences is not ideal, but IMO, there
> are inherent differences between thread and frames, making it difficult
> to use a common syntax for 'frame lists' and 'thread lists'.
> E.g. there is no concept of innermost/outermost thread list, while
> I think we need a syntax for this concept in 'frame apply'.
> 
> So, in summary, would be fine for me to change 'frame apply'
> for a better syntax, but which better syntax allows to specify
> as intuitively as backtrace N outermost frames ?
> 
> And if we have another syntax than backtrace, then it should be
> similar to thread apply THREADID ranges, otherwise that would be very
> confusing.
> => I do not see a syntax consistent with thread apply ID ranges
> or individual IDs, that would allow to specify N innermost or
> outermost frames.

Hmm, maybe the problem is with the trying to fit two concepts
into the same syntax.

"backtrace" suggests in its name a trace of calls going
back.  When you think of filtering the output, you're usually
interested in some number of innermost frames leading up to the
current frame, so an optional count argument there seems kind
of natural.

Note also that you can do "bt" without a COUNT argument,
again given that order is inherent to the command.

But you can't do that with "frame apply", like:
  (gdb) frame apply COMMAND
at least not in your implementation.

(Funnily you get no error if you try it.)

For "frame apply", a count doesn't seem to fit as
naturally to me.  But that might be because I was
drawing the parallel to "thread apply".  I do see
the argument for modeling on "backtrace".

Maybe we could have this (1):

 frame apply all COMMAND
 frame apply 1 3 5 7-9 100 COMMAND
 frame apply count [COUNT | -COUNT] COMMAND

Or if we go with number == count by default, this (2):

 frame apply all COMMAND
 frame apply [COUNT | -COUNT] COMMAND
 frame apply id 1 3 5 7-9 100 COMMAND

I think I'd mildly prefer option (1) above.  It'd avoid
having to decide whether to spell it "id" or "level"
too.  :-)  But both ways are fine with me.

Note, instead of "count", we could have for example:

 frame apply inner N COMMAND
 frame apply outer N COMMAND

dunno.

I could also see adding different kinds of filters
in the future, like, e.g., applying a command to
frames running function FUNCTION:

 frame apply function FUNCTION COMMAND

Or apply a command to frames running on a library or
executable FILE:

 frame apply objfile FILE COMMAND

Though it could also make sense to be able to combine
the filters, like:

 frame apply all -function foo -objfile FOO -whatnot BAR COMMAND

which might be a little to far into over-engineeringland.  Not sure.
Maybe not.

> 
>>
>> I played with this a bit, and I have to admit that I found the
>> -v / -q combinations, and relativeness of the "quietness/verbosity"
>> level, a bit unintuitive and confusing.
>>
>> E.g.,:
>>
>>  (top-gdb) frame apply 2 echo
>>  #0  0x00007ffff54e0c6b in __GI___poll (fds=0x1a343d0, nfds=4, timeout=-1) at ../sysdeps/unix/sysv/linux/poll.c:29
>>  #1  0x00000000007083e4 in gdb_wait_for_event (block=1) at src/gdb/event-loop.c:771
>>
>>  (top-gdb) frame apply 2 -q echo
>>  0x00007ffff54e0c6b      29        return SYSCALL_CANCEL (poll, fds, nfds, timeout);
>>  0x00000000007083e4      771           num_found = poll (gdb_notifier.poll_fds,
>>  (top-gdb) 
>>
>> Here, "-q" didn't make the output really be quieter, only different.
> Yes, the verbosity just controls the choice of the value of
> frame.h enum print_what:
>  enum print_what
>   { 
>     /* Print only the source line, like in stepi.  */
>     SRC_LINE = -1, 
>     /* Print only the location, i.e. level, address (sometimes)
>        function, args, file, line, line num.  */
>     LOCATION,
>     /* Print both of the above.  */
>     SRC_AND_LOC, 
>     /* Print location only, but always include the address.  */
>     LOC_AND_ADDRESS 
>   };
> 
> and that is not really a 'monotonically increasing function'.

Yeah.


>> Consider also if we add a knob to tweak the default verbosity.
>> Like "set frame-apply-default-format N" or whatever.
>> With that, "frame apply -q" does a different thing depending on
>> the value of the default setting.
>>
>> So I'm wondering whether "frame apply -quiet/-source/-location/-etc."
>> options wouldn't be better.  
>>
>> Or "frame apply -format [0-4]" and/or 
>> "frame apply -format quiet/source/location/location".
>>
>> Maybe alternatively consider ditching the format ideas, and have
>> users combine "frame apply -q" with "list" if they want to see
>> sources.  OK, I expect resistance to that idea.  :-)
> 
> Not too much resistance on my side to drop the format idea :).

Eh. :-)

> 
> On a second thought, this verbosity looks all too 'over-engineered' :
>   thread apply verbosity currently only allows to print nothing
>     or a fixed set of information per thread
>   backtrace prints a fixed information per frame
>   frame apply allows to choose whatever in enum print_what with
>     non intuitive verbosity control.
> 
> => I suggest to then just have the -q option : by default,
> frame information is printed like backtrace, and if -q is given,
> no information is printed.
> And same for 'thread apply' : by default, it prints a fixed
> set of info. And -q means do not print it.
> (we for sure need at least the -q to allow only the thread/frame info
> to be
> printed for successful COMMAND).
> 
> (again, in the future, we can if we want add other options
> to control what to print, if we really need this flexibility).
> 
> So, what do you think about the (simpler) idea of just having
>   -q for both thread apply and frame apply ?

I like it.  :-)  As you say, we can always think more about
this later and add ability to fine tune in the future.

Thanks,
Pedro Alves

[RFA_v2 3/8] Add FLAGS... arguments to 'thread apply'.

[Philippe Waroquiers]

Enhance 'thread apply' command to also accepts  FLAGS... arguments.

Some examples usages for this new argument:
   thread apply all -s frame apply all -s p some_local_var_somewhere
      Prints the thread id, frame location and some_local_var_somewhere
      value in frames of threads that have such local var.

To make the life of the user easier, the most typical use cases
have shortcuts :
   taas  : shortcut for 'thread apply all -s'
   tfaas : shortcut for 'thread apply all -s frame apply all -s"

An example usage :
   tfaas p some_local_var_somewhere
     same as the longer:
        'thread apply all -s frame apply all -s p some_local_var_somewhere'

gdb/ChangeLog
2018-05-21  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* thread.c (thr_try_catch_cmd): New function.
	(thread_apply_all_command): Handle vqcs flags.
	(thread_apply_command): Handle vqcs flags.
	(taas_command): New function.
	(tfaas_command): New function.
	(_initialize_thread): Update to setup the new commands 'taas
	and 'tfaas'. Change doc string for 'thread apply'.
---
 gdb/thread.c | 132 ++++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 112 insertions(+), 20 deletions(-)

diff --git a/gdb/thread.c b/gdb/thread.c
index f5a29f5cc1..a3776c95b1 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -1581,6 +1581,45 @@ tp_array_compar (const thread_info *a, const thread_info *b)
     return (a->per_inf_num > b->per_inf_num);
 }
 
+/* Switch to thread THR and executes CMD.
+   PRINT_WHAT_V verbosity controls the printing of the thread information.
+   CONT and SILENT control how to handle errors.  */
+
+static void
+thr_try_catch_cmd (thread_info *thr, const char *cmd, int from_tty,
+		   int print_what_v,
+		   bool cont, bool silent)
+{
+  switch_to_thread (thr->ptid);
+  TRY
+    {
+      std::string cmd_result = execute_command_to_string (cmd, from_tty);
+      if (!silent || cmd_result.length () > 0)
+	{
+	  if (print_what_v > 0)
+	    printf_filtered (_("\nThread %s (%s):\n"),
+			     print_thread_id (thr),
+			     target_pid_to_str (inferior_ptid));
+	  printf_filtered ("%s", cmd_result.c_str ());
+	}
+    }
+  CATCH (ex, RETURN_MASK_ERROR)
+    {
+      if (!silent)
+	{
+	  if (print_what_v > 0)
+	    printf_filtered (_("\nThread %s (%s):\n"),
+			     print_thread_id (thr),
+			     target_pid_to_str (inferior_ptid));
+	  if (cont)
+	    printf_filtered ("%s\n", ex.message);
+	  else
+	    throw_exception (ex);
+	}
+    }
+  END_CATCH;
+}
+
 /* Apply a GDB command to a list of threads.  List syntax is a whitespace
    separated list of numbers, or ranges, or the keyword `all'.  Ranges consist
    of two numbers separated by a hyphen.  Examples:
@@ -1592,6 +1631,10 @@ tp_array_compar (const thread_info *a, const thread_info *b)
 static void
 thread_apply_all_command (const char *cmd, int from_tty)
 {
+  int print_what_v = 1; /* Print thread id/thread/lwp.  */
+  bool cont;
+  bool silent;
+
   tp_array_compar_ascending = false;
   if (cmd != NULL
       && check_for_argument (&cmd, "-ascending", strlen ("-ascending")))
@@ -1600,8 +1643,13 @@ thread_apply_all_command (const char *cmd, int from_tty)
       tp_array_compar_ascending = true;
     }
 
+  if (cmd != NULL)
+    check_for_flags_vqcs ("thread apply all", &cmd,
+			  &print_what_v, 1,
+			  &cont, &silent);
+
   if (cmd == NULL || *cmd == '\000')
-    error (_("Please specify a command following the thread ID list"));
+    error (_("Please specify a command at the end of 'thread apply all'"));
 
   update_thread_list ();
 
@@ -1637,14 +1685,9 @@ thread_apply_all_command (const char *cmd, int from_tty)
 
       for (thread_info *thr : thr_list_cpy)
 	if (thread_alive (thr))
-	  {
-	    switch_to_thread (thr->ptid);
-	    printf_filtered (_("\nThread %s (%s):\n"),
-			     print_thread_id (thr),
-			     target_pid_to_str (inferior_ptid));
-
-	    execute_command (cmd, from_tty);
-	  }
+	  thr_try_catch_cmd (thr, cmd, from_tty,
+			     print_what_v,
+			     cont, silent);
     }
 }
 
@@ -1653,7 +1696,11 @@ thread_apply_all_command (const char *cmd, int from_tty)
 static void
 thread_apply_command (const char *tidlist, int from_tty)
 {
+  int print_what_v = 1; /* Print thread id/thread/lwp.  */
+  bool cont;
+  bool silent;
   const char *cmd = NULL;
+  const char *cmd_or_flags;
   tid_range_parser parser;
 
   if (tidlist == NULL || *tidlist == '\000')
@@ -1671,6 +1718,12 @@ thread_apply_command (const char *tidlist, int from_tty)
 	}
     }
 
+  cmd_or_flags = cmd;
+  if (cmd != NULL)
+    check_for_flags_vqcs ("thread apply", &cmd,
+			  &print_what_v, 1,
+			  &cont, &silent);
+
   if (cmd == NULL)
     error (_("Please specify a command following the thread ID list"));
 
@@ -1680,7 +1733,7 @@ thread_apply_command (const char *tidlist, int from_tty)
   scoped_restore_current_thread restore_thread;
 
   parser.init (tidlist, current_inferior ()->num);
-  while (!parser.finished () && parser.cur_tok () < cmd)
+  while (!parser.finished () && parser.cur_tok () < cmd_or_flags)
     {
       struct thread_info *tp = NULL;
       struct inferior *inf;
@@ -1725,14 +1778,31 @@ thread_apply_command (const char *tidlist, int from_tty)
 	  continue;
 	}
 
-      switch_to_thread (tp->ptid);
-
-      printf_filtered (_("\nThread %s (%s):\n"), print_thread_id (tp),
-		       target_pid_to_str (inferior_ptid));
-      execute_command (cmd, from_tty);
+      thr_try_catch_cmd (tp, cmd, from_tty,
+			 print_what_v,
+			 cont, silent);
     }
 }
 
+
+/* Implementation of the "taas" command.  */
+
+static void
+taas_command (const char *cmd, int from_tty)
+{
+  std::string expanded = std::string ("thread apply all -s ") + std::string (cmd);
+  execute_command (expanded.c_str (), from_tty);
+}
+
+/* Implementation of the "tfaas" command.  */
+
+static void
+tfaas_command (const char *cmd, int from_tty)
+{
+  std::string expanded = std::string ("thread apply all -s frame apply all -s ") + std::string (cmd);
+  execute_command (expanded.c_str (), from_tty);
+}
+
 /* Switch to the specified thread, or print the current thread.  */
 
 void
@@ -2027,22 +2097,44 @@ Use this command to switch between threads.\n\
 The new thread ID must be currently known."),
 		  &thread_cmd_list, "thread ", 1, &cmdlist);
 
+#define THREAD_APPLY_FLAGS_HELP "\
+FLAGS are -v (increase verbosity), -q (decrease verbosity)\n\
+  -c (continue), -s (silent).\n\
+Verbosity (default 1) controls what to print for a thread:\n\
+  0 : no thread info is printed\n\
+  1 : print per-inferior thread number and target system's thread id\n\
+By default, if a COMMAND raises an error, thread apply is aborted.\n\
+Flag -c indicates to print the error and continue.\n\
+Flag -s indicates to silently ignore a COMMAND that raises an error\n\
+or produces no output."
+
   add_prefix_cmd ("apply", class_run, thread_apply_command,
 		  _("Apply a command to a list of threads.\n\
-Usage: thread apply ID... COMMAND\n\
-ID is a space-separated list of IDs of threads to apply COMMAND on."),
+Usage: thread apply ID... [FLAGS...] COMMAND\n\
+ID is a space-separated list of IDs of threads to apply COMMAND on.\n"
+THREAD_APPLY_FLAGS_HELP),
 		  &thread_apply_list, "thread apply ", 1, &thread_cmd_list);
 
   add_cmd ("all", class_run, thread_apply_all_command,
 	   _("\
 Apply a command to all threads.\n\
 \n\
-Usage: thread apply all [-ascending] COMMAND\n\
+Usage: thread apply all [-ascending] [FLAGS...] COMMAND\n\
 -ascending: Call COMMAND for all threads in ascending order.\n\
-            The default is descending order.\
-"),
+            The default is descending order.\n"
+THREAD_APPLY_FLAGS_HELP),
 	   &thread_apply_list);
 
+  add_com ("taas", class_run, taas_command, _("\
+Apply a command to all threads (ignoring errors and empty output).\n\
+Usage: taas COMMAND\n\
+shortcut for 'thread apply all -s COMMAND'"));
+
+  add_com ("tfaas", class_run, tfaas_command, _("\
+Apply a command to all frames of all threads (ignoring errors and empty output).\n\
+Usage: tfaas COMMAND\n\
+shortcut for 'thread apply all -s frame apply all -s COMMAND'"));
+
   add_cmd ("name", class_run, thread_name_command,
 	   _("Set the current thread's name.\n\
 Usage: thread name [NAME]\n\
-- 
2.11.0

Re: [RFA_v2 3/8] Add FLAGS... arguments to 'thread apply'.

[Pedro Alves]

On 06/05/2018 09:49 PM, Philippe Waroquiers wrote:
> Enhance 'thread apply' command to also accepts  FLAGS... arguments.
> 
> Some examples usages for this new argument:
>    thread apply all -s frame apply all -s p some_local_var_somewhere
>       Prints the thread id, frame location and some_local_var_somewhere
>       value in frames of threads that have such local var.
> 
> To make the life of the user easier, the most typical use cases
> have shortcuts :
>    taas  : shortcut for 'thread apply all -s'
>    tfaas : shortcut for 'thread apply all -s frame apply all -s"
> 
> An example usage :
>    tfaas p some_local_var_somewhere
>      same as the longer:
>         'thread apply all -s frame apply all -s p some_local_var_somewhere'

Same comments on -q/-s and [FLAGS...]/[OPTION...] etc. also apply here.

>     separated list of numbers, or ranges, or the keyword `all'.  Ranges consist
>     of two numbers separated by a hyphen.  Examples:
> @@ -1592,6 +1631,10 @@ tp_array_compar (const thread_info *a, const thread_info *b)
>  static void
>  thread_apply_all_command (const char *cmd, int from_tty)
>  {
> +  int print_what_v = 1; /* Print thread id/thread/lwp.  */
> +  bool cont;
> +  bool silent;
> +
>    tp_array_compar_ascending = false;
>    if (cmd != NULL
>        && check_for_argument (&cmd, "-ascending", strlen ("-ascending")))
> @@ -1600,8 +1643,13 @@ thread_apply_all_command (const char *cmd, int from_tty)
>        tp_array_compar_ascending = true;
>      }
>  
> +  if (cmd != NULL)
> +    check_for_flags_vqcs ("thread apply all", &cmd,
> +			  &print_what_v, 1,
> +			  &cont, &silent);

So here we see a case where there's really no good
reason this shouldn't work:

 (gdb) thread apply all -q -ascending -c p 1
 thread apply all only accepts flags vqcs given individually

>  
> +
> +/* Implementation of the "taas" command.  */
> +
> +static void
> +taas_command (const char *cmd, int from_tty)
> +{
> +  std::string expanded = std::string ("thread apply all -s ") + std::string (cmd);
> +  execute_command (expanded.c_str (), from_tty);
> +}
> +
> +/* Implementation of the "tfaas" command.  */
> +
> +static void
> +tfaas_command (const char *cmd, int from_tty)
> +{
> +  std::string expanded = std::string ("thread apply all -s frame apply all -s ") + std::string (cmd);
> +  execute_command (expanded.c_str (), from_tty);

Overly long lines above.

Thanks,
Pedro Alves