Re: [PATCH v2] Raise exception if ambiguous name is used in gdb.parameter

[Andrew Burgess <aburgess@redhat.com>]

Hannes Domani <ssbssa@yahoo.de> writes:

> Currently gdb.parameter doesn't raise an exception if an
> ambiguous name is used, it instead returns the value of the
> last partly matching parameter:
> ```
> (gdb) show print sym
> Ambiguous show print command "sym": symbol, symbol-filename, symbol-loading.
> (gdb) show print symbol-loading
> Printing of symbol loading messages is "full".
> (gdb) py print(gdb.parameter("print sym"))
> full
> ```
>
> It's because lookup_cmd_composition_1 tries to detect
> ambigous names by checking the return value of find_cmd
> for CMD_LIST_AMBIGUOUS, which never happens, since only
> lookup_cmd_1 returns CMD_LIST_AMBIGUOUS.
> Instead the nfound argument contains the number of found
> matches.
>
> By using it instead, and by setting *CMD to the special value
> CMD_LIST_AMBIGUOUS in this case, gdbpy_parameter can now show
> the appropriate error message:
> ```
> (gdb) py print(gdb.parameter("print sym"))
> Traceback (most recent call last):
>   File "<string>", line 1, in <module>
> RuntimeError: Parameter `print sym' is ambiguous.
> Error while executing Python code.
> (gdb) py print(gdb.parameter("print symbol"))
> True
> (gdb) py print(gdb.parameter("print symbol-"))
> Traceback (most recent call last):
>   File "<string>", line 1, in <module>
> RuntimeError: Parameter `print symbol-' is ambiguous.
> Error while executing Python code.
> (gdb) py print(gdb.parameter("print symbol-load"))
> full
> ```
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=14639
> ---
> v2:
> - Small changes in commit message.
> - Use CMD_LIST_AMBIGUOUS in lookup_cmd_composition_1 to mark
>   ambiguous commands, so gdbpy_parameter can show a better
>   error message.
> ---
>  gdb/cli/cli-decode.c                      | 13 ++++++---
>  gdb/python/python.c                       |  5 +++-
>  gdb/testsuite/gdb.python/py-parameter.exp | 32 +++++++++++++++++++++++
>  3 files changed, 46 insertions(+), 4 deletions(-)
>
> diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
> index 940cd6a2c8e..db57764578d 100644
> --- a/gdb/cli/cli-decode.c
> +++ b/gdb/cli/cli-decode.c
> @@ -2549,7 +2549,8 @@ deprecated_cmd_warning (const char *text, struct cmd_list_element *list)
>     If TEXT is a subcommand (i.e. one that is preceded by a prefix
>     command) set *PREFIX_CMD.
>  
> -   Set *CMD to point to the command TEXT indicates.
> +   Set *CMD to point to the command TEXT indicates, or to
> +   CMD_LIST_AMBIGUOUS if there are multiple possible matches.
>  
>     If any of *ALIAS, *PREFIX_CMD, or *CMD cannot be determined or do not
>     exist, they are NULL when we return.
> @@ -2589,7 +2590,12 @@ lookup_cmd_composition_1 (const char *text,
>        *cmd = find_cmd (command.c_str (), len, cur_list, 1, &nfound);
>  
>        /* We only handle the case where a single command was found.  */
> -      if (*cmd == CMD_LIST_AMBIGUOUS || *cmd == nullptr)
> +      if (nfound > 1)
> +	{
> +	  *cmd = CMD_LIST_AMBIGUOUS;
> +	  return 0;

This changes the API of lookup_cmd_composition.  You need to audit all
the calls to this function to check that non are depending on CMD being
set to nullptr.  I only looked at one in cli/cli-script.c in the
function do_document_command that is a problem, try this:

  (gdb) define foo_1
  Type commands for definition of "foo_1".
  End with a line saying just "end".
  >echo aa\n
  >end
  (gdb) define foo_2
  Type commands for definition of "foo_2".
  End with a line saying just "end".
  >echo bb\n
  >end
  (gdb) document foo
  
  
  Fatal signal: Segmentation fault
  ... etc ...

Not every use of lookup_cmd_composition is a problem, e.g. in
alias_command in cli/cli-cmds.c it's not an issue, so you'll need to
check 'em all.

If you do find cases that would have triggered bugs with this change, it
would be awesome if you could add tests to cover these cases, though I
guess that's not a hard requirement.

Thanks,
Andrew


> +	}
> +      else if (*cmd == nullptr)
>  	return 0;
>        else
>  	{
> @@ -2623,7 +2629,8 @@ lookup_cmd_composition_1 (const char *text,
>     If TEXT is a subcommand (i.e. one that is preceded by a prefix
>     command) set *PREFIX_CMD.
>  
> -   Set *CMD to point to the command TEXT indicates.
> +   Set *CMD to point to the command TEXT indicates, or to
> +   CMD_LIST_AMBIGUOUS if there are multiple possible matches.
>  
>     If any of *ALIAS, *PREFIX_CMD, or *CMD cannot be determined or do not
>     exist, they are NULL when we return.
> diff --git a/gdb/python/python.c b/gdb/python/python.c
> index 951de82ef06..fa0b1ca6604 100644
> --- a/gdb/python/python.c
> +++ b/gdb/python/python.c
> @@ -559,7 +559,10 @@ gdbpy_parameter (PyObject *self, PyObject *args)
>        GDB_PY_HANDLE_EXCEPTION (ex);
>      }
>  
> -  if (!found)
> +  if (cmd == CMD_LIST_AMBIGUOUS)
> +    return PyErr_Format (PyExc_RuntimeError,
> +			 _("Parameter `%s' is ambiguous."), arg);
> +  else if (!found)
>      return PyErr_Format (PyExc_RuntimeError,
>  			 _("Could not find parameter `%s'."), arg);
>  
> diff --git a/gdb/testsuite/gdb.python/py-parameter.exp b/gdb/testsuite/gdb.python/py-parameter.exp
> index c1f8a80c6b7..828fe84694d 100644
> --- a/gdb/testsuite/gdb.python/py-parameter.exp
> +++ b/gdb/testsuite/gdb.python/py-parameter.exp
> @@ -588,6 +588,37 @@ proc_with_prefix test_language {} {
>      gdb_test_no_output "set lang auto"
>  }
>  
> +proc_with_prefix test_ambiguous_parameter {} {
> +    gdb_test_multiline "create parameter" \
> +	"python" "" \
> +	"class TestAmbiguousParam (gdb.Parameter):" "" \
> +	"   def __init__ (self, name, value):" "" \
> +	"      super (TestAmbiguousParam, self).__init__ (name, gdb.COMMAND_DATA, gdb.PARAM_INTEGER)" "" \
> +	"      self.value = value" "" \
> +	"end"
> +
> +    # Create parameters.
> +    gdb_test "python TestAmbiguousParam('test-ambiguous-value-1', 1)" ""
> +    gdb_test "python TestAmbiguousParam('test-ambiguous-value-2-extra', 2)" ""
> +    gdb_test "python TestAmbiguousParam('test-ambiguous', 3)" ""
> +
> +    # Test unambiguous matches.
> +    gdb_test "python print(gdb.parameter('test-ambiguous-value-1'))" "1"
> +    gdb_test "python print(gdb.parameter('test-ambiguous-value-2-extra'))" "2"
> +    gdb_test "python print(gdb.parameter('test-ambiguous-value-2'))" "2"
> +    gdb_test "python print(gdb.parameter('test-ambiguous'))" "3"
> +
> +    # Test ambiguous names.
> +    gdb_test "python print(gdb.parameter('test-ambiguou'))" \
> +	"Parameter .* is ambiguous.*Error while executing Python code."
> +    gdb_test "python print(gdb.parameter('test-ambiguous-'))" \
> +	"Parameter .* is ambiguous.*Error while executing Python code."
> +    gdb_test "python print(gdb.parameter('test-ambiguous-v'))" \
> +	"Parameter .* is ambiguous.*Error while executing Python code."
> +    gdb_test "python print(gdb.parameter('test-ambiguous-value-1a'))" \
> +	"Could not find parameter.*Error while executing Python code."
> +}
> +
>  test_directories
>  test_data_directory
>  test_boolean_parameter
> @@ -600,5 +631,6 @@ test_gdb_parameter
>  test_integer_parameter
>  test_throwing_parameter
>  test_language
> +test_ambiguous_parameter
>  
>  rename py_param_test_maybe_no_output ""
> -- 
> 2.35.1