Re: [RFA_v4 1/8] Add helper functions parse_flags and parse_flags_qcs

[Joel Brobecker <brobecker@adacore.com>]

Hello,

On Tue, Jul 10, 2018 at 11:39:19PM +0200, Philippe Waroquiers wrote:
> Add helper functions parse_flags and parse_flags_qcs.
> parse_flags helper function allows to look for a set of flags at
> the start of a string.
> A flag must be given individually.
> 
> parse_flags_qcs is a specialised helper function to handle
> the flags -q, -c and -s, 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 an alpha (i.e. a flag or an option).
> That is needed for the addition of the [FLAG]... arguments to
> thread apply ID... [FLAG]... COMMAND
> 
> Remove bool number_or_range_parser::m_finished, rather
> implement the 'finished' logic inside number_or_range_parser::finished.
> The new logic properly detects the end of parsing even if not at
> end of the string. This ensures that number_or_range_parser::cur_tok
> really points past the last parsed token when parsing is finished.
> Before, it was always pointing at the end of the string.
> As parsing now is finished directly when not positioned on a number,
> number_is_in_list must do an error check before the loop getting all
> numbers.
> 
> 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-07-09  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> 
> 	* cli-utils.c (number_or_range_parser::get_number): Only handle
> 	numbers or convenience var as numbers.
> 	(parse_flags): New function.
> 	(parse_flags_qcs): New function.
> 	(number_or_range_parser::finished): Ensure parsing end is detected
> 	before end of string.
> 	* cli-utils.h (parse_flags): New function.
> 	(parse_flags_qcs): New function.
> 	(number_or_range_parser): Remove m_finished bool.
> 	(number_or_range_parser::skip_range): Set m_in_range to false.

For the record, this patch is causing some regressions that look
related to memory management (in this case, probably reading memory
that has already been freed). It's not always user-visible, but the
easiest way I have found to demonstrate the issue is to use valgrind
with the following example:

    | $ cat c.c
    | int
    | main (void)
    | {
    | }
    |
    | $ gcc -g c.c -o c
    |
    | $ valgrind /path/to/gdb -q c -ex 'break main'
    | ==107454== Memcheck, a memory error detector
    | ==107454== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
    | ==107454== Using Valgrind-3.10.0 and LibVEX; rerun with -h for copyright info
    | ==107454== Command: /work/brobecke/bld/gdb-public-no-python/gdb/gdb -q c -ex break\ main
    | ==107454==
    | /homes/brobecke/.gdbinit:87: Error in sourced command file:
    | No symbol table is loaded.  Use the "file" command.
    | Reading symbols from c...done.
    | cBreakpoint 1 at 0x401131: file c.c, line 4.
    | (gdb) command 1
    | Type commands for breakpoint(s) 1, one per line.
    | End with a line saying just "end".
    | >end
    | ==119811== Invalid read of size 1
    | [snip]
    | ==119811==  Address 0x65c1349 is 9 bytes inside a block of size 10 free'd

Before this patch, no such valgrind error.

We detected this issue, because, with one of our Ada testcase, we actually
get an error:

    | (gdb) command 1
    | Type commands for breakpoint(s) 1, one per line.
    | End with a line saying just "end".
    | >print msg.all
    | >end
    | No breakpoint number 90.

I will look into tomorrow - this is just a heads up so someone else
doesn't spend time investigating the same issue.

> gdb/testsuite/ChangeLog
> 2018-07-09  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> 
> 	* gdb.base/skip.exp: Update expected error message.
> ---
>  gdb/cli/cli-utils.c             | 96 +++++++++++++++++++++++++++++++--
>  gdb/cli/cli-utils.h             | 45 ++++++++++++++--
>  gdb/testsuite/gdb.base/skip.exp |  6 +--
>  3 files changed, 134 insertions(+), 13 deletions(-)
> 
> diff --git a/gdb/cli/cli-utils.c b/gdb/cli/cli-utils.c
> index c55b5035e4..98b7414991 100644
> --- a/gdb/cli/cli-utils.c
> +++ b/gdb/cli/cli-utils.c
> @@ -137,7 +137,6 @@ number_or_range_parser::number_or_range_parser (const char *string)
>  void
>  number_or_range_parser::init (const char *string)
>  {
> -  m_finished = false;
>    m_cur_tok = string;
>    m_last_retval = 0;
>    m_end_value = 0;
> @@ -169,7 +168,10 @@ number_or_range_parser::get_number ()
>        /* Default case: state->m_cur_tok is pointing either to a solo
>  	 number, or to the first number of a range.  */
>        m_last_retval = get_number_trailer (&m_cur_tok, '-');
> -      if (*m_cur_tok == '-')
> +      /* If get_number_trailer has found a -, it might be the start
> +	 of a command option.  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,8 +198,17 @@ number_or_range_parser::get_number ()
>  	}
>      }
>    else
> -    error (_("negative value"));
> -  m_finished = *m_cur_tok == '\0';
> +    {
> +      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"));
> +	}
> +    }
>    return m_last_retval;
>  }
>  
> @@ -215,6 +226,21 @@ number_or_range_parser::setup_range (int start_value, int end_value,
>    m_end_value = end_value;
>  }
>  
> +/* See documentation in cli-utils.h.  */
> +
> +bool
> +number_or_range_parser::finished () const
> +{
> +  /* Parsing is finished when at end of string or null string,
> +     or we are not in a range and not in front of an integer, negative
> +     integer, convenience var or negative convenience var.  */
> +  return (m_cur_tok == NULL || *m_cur_tok == '\0'
> +	  || (!m_in_range
> +	      && !(isdigit (*m_cur_tok) || *m_cur_tok == '$')
> +	      && !(*m_cur_tok == '-'
> +		   && (isdigit (m_cur_tok[1]) || m_cur_tok[1] == '$'))));
> +}
> +
>  /* Accept a number and a string-form list of numbers such as is 
>     accepted by get_number_or_range.  Return TRUE if the number is
>     in the list.
> @@ -230,12 +256,15 @@ number_is_in_list (const char *list, int number)
>      return 1;
>  
>    number_or_range_parser parser (list);
> +
> +  if (parser.finished ())
> +    error (_("Arguments must be numbers or '$' variables."));
>    while (!parser.finished ())
>      {
>        int gotnum = parser.get_number ();
>  
>        if (gotnum == 0)
> -	error (_("Args must be numbers or '$' variables."));
> +	error (_("Arguments must be numbers or '$' variables."));
>        if (gotnum == number)
>  	return 1;
>      }
> @@ -304,3 +333,60 @@ check_for_argument (const char **str, const char *arg, int arg_len)
>      }
>    return 0;
>  }
> +
> +/* See documentation in cli-utils.h.  */
> +
> +int
> +parse_flags (const char **str, const char *flags)
> +{
> +  const char *p = skip_spaces (*str);
> +
> +  if (p[0] == '-'
> +      && isalpha (p[1])
> +      && (p[2] == '\0' || isspace (p[2])))
> +    {
> +      const char pf = p[1];
> +      const char *f = flags;
> +
> +      while (*f != '\0')
> +	{
> +	  if (*f == pf)
> +	    {
> +	      *str = skip_spaces (p + 2);
> +	      return f - flags + 1;
> +	    }
> +	  f++;
> +	}
> +    }
> +
> +  return 0;
> +}
> +
> +/* See documentation in cli-utils.h.  */
> +
> +bool
> +parse_flags_qcs (const char *which_command, const char **str,
> +		 qcs_flags *flags)
> +{
> +  switch (parse_flags (str, "qcs"))
> +    {
> +    case 0:
> +      return false;
> +    case 1:
> +      flags->quiet = true;
> +      break;
> +    case 2:
> +      flags->cont = true;
> +      break;
> +    case 3:
> +      flags->silent = true;
> +      break;
> +    default:
> +      gdb_assert_not_reached ("int qcs flag out of bound");
> +    }
> +
> +  if (flags->cont && flags->silent)
> +    error (_("%s: -c and -s are mutually exclusive"), which_command);
> +
> +  return true;
> +}
> diff --git a/gdb/cli/cli-utils.h b/gdb/cli/cli-utils.h
> index e34ee0df37..fa7d02d719 100644
> --- a/gdb/cli/cli-utils.h
> +++ b/gdb/cli/cli-utils.h
> @@ -75,8 +75,7 @@ public:
>  		    const char *end_ptr);
>  
>    /* Returns true if parsing has completed.  */
> -  bool finished () const
> -  { return m_finished; }
> +  bool finished () const;
>  
>    /* Return the string being parsed.  When parsing has finished, this
>       points past the last parsed token.  */
> @@ -96,6 +95,7 @@ public:
>    {
>      gdb_assert (m_in_range);
>      m_cur_tok = m_end_ptr;
> +    m_in_range = false;
>    }
>  
>  private:
> @@ -103,9 +103,6 @@ private:
>    number_or_range_parser (const number_or_range_parser &);
>    number_or_range_parser &operator= (const number_or_range_parser &);
>  
> -  /* True if parsing has completed.  */
> -  bool m_finished;
> -
>    /* The string being parsed.  When parsing has finished, this points
>       past the last parsed token.  */
>    const char *m_cur_tok;
> @@ -173,4 +170,42 @@ 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 0 if no valid flag is found at the start of STR.
> +   Otherwise updates *STR, and returns N (which is > 0),
> +   such that FLAGS [N - 1] is the valid found flag.  */
> +extern int parse_flags (const char **str, const char *flags);
> +
> +/* qcs_flags struct regroups the flags parsed by parse_flags_qcs.  */
> +
> +struct qcs_flags
> +{
> +  bool quiet = false;
> +  bool cont = false;
> +  bool silent = false;
> +};
> +
> +/* A helper function that uses parse_flags to handle the flags qcs :
> +     A flag -q sets FLAGS->QUIET to true.
> +     A flag -c sets FLAGS->CONT to true.
> +     A flag -s sets FLAGS->SILENT to true.
> +
> +   The caller is responsible to initialize *FLAGS to false before the (first)
> +   call to parse_flags_qcs.
> +   parse_flags_qcs can then be called iteratively to search for more
> +   valid flags, as part of a 'main parsing loop' searching for -q/-c/-s
> +   flags together with other flags and options.
> +
> +   Returns true and updates *STR and one of FLAGS->QUIET, FLAGS->CONT,
> +   FLAGS->SILENT if it finds a valid flag.
> +   Returns false if no valid flag is found at the beginning of STR.
> +
> +   Throws an error if a flag is found such that both FLAGS->CONT and
> +   FLAGS->SILENT are true.  */
> +
> +extern bool parse_flags_qcs (const char *which_command, const char **str,
> +			     qcs_flags *flags);
>  #endif /* CLI_UTILS_H */
> diff --git a/gdb/testsuite/gdb.base/skip.exp b/gdb/testsuite/gdb.base/skip.exp
> index 4b556b10a5..223c93d0d9 100644
> --- a/gdb/testsuite/gdb.base/skip.exp
> +++ b/gdb/testsuite/gdb.base/skip.exp
> @@ -69,9 +69,9 @@ gdb_test "skip function baz" "Function baz will be skipped when stepping\."
>  gdb_test "skip enable 999" "No skiplist entries found with number 999."
>  gdb_test "skip disable 999" "No skiplist entries found with number 999."
>  gdb_test "skip delete 999" "No skiplist entries found with number 999."
> -gdb_test "skip enable a" "Args must be numbers or '\\$' variables."
> -gdb_test "skip disable a" "Args must be numbers or '\\$' variables."
> -gdb_test "skip delete a" "Args must be numbers or '\\$' variables."
> +gdb_test "skip enable a" "Arguments must be numbers or '\\$' variables."
> +gdb_test "skip disable a" "Arguments must be numbers or '\\$' variables."
> +gdb_test "skip delete a" "Arguments must be numbers or '\\$' variables."
>  
>  # Ask for info on a skiplist entry which doesn't exist.
>  
> -- 
> 2.18.0

-- 
Joel