Re: [PATCH 1/2] gdb/python: generalize serialize_mi_result()

[Andrew Burgess <aburgess@redhat.com>]

Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> writes:

> This commit renames serialize_mi_result() to serialize_mi_data() and makes
> it usable in different contexts than printing result of custom MI command
> (hence the rename).
>
> To do so, the check whether passed Python object is a dictionary has been
> moved to the caller - at the very least, different usages require different
> error messages. Also it seemed to me that py-utils.c is a better place
> for its code as it is now a utility function not specific to Python MI
> commands.

It's no longer just for MI commands, but it probably is only for MI
"stuff".  I wonder if moving it to py-mi.c would be better?

>
> This is a preparation for implementing Python support for sending custom
> MI async events.
> ---
>  gdb/python/py-micmd.c        | 179 ++---------------------------------
>  gdb/python/py-utils.c        | 145 ++++++++++++++++++++++++++++
>  gdb/python/python-internal.h |  15 +++
>  3 files changed, 166 insertions(+), 173 deletions(-)
>
> diff --git a/gdb/python/py-micmd.c b/gdb/python/py-micmd.c
> index 01fc6060ece..26231fc37e8 100644
> --- a/gdb/python/py-micmd.c
> +++ b/gdb/python/py-micmd.c
> @@ -173,178 +173,6 @@ extern PyTypeObject micmdpy_object_type
>  
>  static PyObject *invoke_cst;
>  
> -/* Convert KEY_OBJ into a string that can be used as a field name in MI
> -   output.  KEY_OBJ must be a Python string object, and must only contain
> -   characters suitable for use as an MI field name.
> -
> -   If KEY_OBJ is not a string, or if KEY_OBJ contains invalid characters,
> -   then an error is thrown.  Otherwise, KEY_OBJ is converted to a string
> -   and returned.  */
> -
> -static gdb::unique_xmalloc_ptr<char>
> -py_object_to_mi_key (PyObject *key_obj)
> -{
> -  /* The key must be a string.  */
> -  if (!PyUnicode_Check (key_obj))
> -    {
> -      gdbpy_ref<> key_repr (PyObject_Repr (key_obj));
> -      gdb::unique_xmalloc_ptr<char> key_repr_string;
> -      if (key_repr != nullptr)
> -	key_repr_string = python_string_to_target_string (key_repr.get ());
> -      if (key_repr_string == nullptr)
> -	gdbpy_handle_exception ();
> -
> -      gdbpy_error (_("non-string object used as key: %s"),
> -		   key_repr_string.get ());
> -    }
> -
> -  gdb::unique_xmalloc_ptr<char> key_string
> -    = python_string_to_target_string (key_obj);
> -  if (key_string == nullptr)
> -    gdbpy_handle_exception ();
> -
> -  /* Predicate function, returns true if NAME is a valid field name for use
> -     in MI result output, otherwise, returns false.  */
> -  auto is_valid_key_name = [] (const char *name) -> bool
> -  {
> -    gdb_assert (name != nullptr);
> -
> -    if (*name == '\0' || !isalpha (*name))
> -      return false;
> -
> -    for (; *name != '\0'; ++name)
> -      if (!isalnum (*name) && *name != '_' && *name != '-')
> -	return false;
> -
> -    return true;
> -  };
> -
> -  if (!is_valid_key_name (key_string.get ()))
> -    {
> -      if (*key_string.get () == '\0')
> -	gdbpy_error (_("Invalid empty key in MI result"));
> -      else
> -	gdbpy_error (_("Invalid key in MI result: %s"), key_string.get ());
> -    }
> -
> -  return key_string;
> -}
> -
> -/* Serialize RESULT and print it in MI format to the current_uiout.
> -   FIELD_NAME is used as the name of this result field.
> -
> -   RESULT can be a dictionary, a sequence, an iterator, or an object that
> -   can be converted to a string, these are converted to the matching MI
> -   output format (dictionaries as tuples, sequences and iterators as lists,
> -   and strings as named fields).
> -
> -   If anything goes wrong while formatting the output then an error is
> -   thrown.
> -
> -   This function is the recursive inner core of serialize_mi_result, and
> -   should only be called from that function.  */
> -
> -static void
> -serialize_mi_result_1 (PyObject *result, const char *field_name)
> -{
> -  struct ui_out *uiout = current_uiout;
> -
> -  if (PyDict_Check (result))
> -    {
> -      PyObject *key, *value;
> -      Py_ssize_t pos = 0;
> -      ui_out_emit_tuple tuple_emitter (uiout, field_name);
> -      while (PyDict_Next (result, &pos, &key, &value))
> -	{
> -	  gdb::unique_xmalloc_ptr<char> key_string
> -	    (py_object_to_mi_key (key));
> -	  serialize_mi_result_1 (value, key_string.get ());
> -	}
> -    }
> -  else if (PySequence_Check (result) && !PyUnicode_Check (result))
> -    {
> -      ui_out_emit_list list_emitter (uiout, field_name);
> -      Py_ssize_t len = PySequence_Size (result);
> -      if (len == -1)
> -	gdbpy_handle_exception ();
> -      for (Py_ssize_t i = 0; i < len; ++i)
> -	{
> -	  gdbpy_ref<> item (PySequence_ITEM (result, i));
> -	  if (item == nullptr)
> -	    gdbpy_handle_exception ();
> -	  serialize_mi_result_1 (item.get (), nullptr);
> -	}
> -    }
> -  else if (PyIter_Check (result))
> -    {
> -      gdbpy_ref<> item;
> -      ui_out_emit_list list_emitter (uiout, field_name);
> -      while (true)
> -	{
> -	  item.reset (PyIter_Next (result));
> -	  if (item == nullptr)
> -	    {
> -	      if (PyErr_Occurred () != nullptr)
> -		gdbpy_handle_exception ();
> -	      break;
> -	    }
> -	  serialize_mi_result_1 (item.get (), nullptr);
> -	}
> -    }
> -  else
> -    {
> -      if (PyLong_Check (result))
> -	{
> -	  int overflow = 0;
> -	  gdb_py_longest val = gdb_py_long_as_long_and_overflow (result,
> -								 &overflow);
> -	  if (PyErr_Occurred () != nullptr)
> -	    gdbpy_handle_exception ();
> -	  if (overflow == 0)
> -	    {
> -	      uiout->field_signed (field_name, val);
> -	      return;
> -	    }
> -	  /* Fall through to the string case on overflow.  */
> -	}
> -
> -      gdb::unique_xmalloc_ptr<char> string (gdbpy_obj_to_string (result));
> -      if (string == nullptr)
> -	gdbpy_handle_exception ();
> -      uiout->field_string (field_name, string.get ());
> -    }
> -}
> -
> -/* Serialize RESULT and print it in MI format to the current_uiout.
> -
> -   This function handles the top-level result initially returned from the
> -   invoke method of the Python command implementation.  At the top-level
> -   the result must be a dictionary.  The values within this dictionary can
> -   be a wider range of types.  Handling the values of the top-level
> -   dictionary is done by serialize_mi_result_1, see that function for more
> -   details.
> -
> -   If anything goes wrong while parsing and printing the MI output then an
> -   error is thrown.  */
> -
> -static void
> -serialize_mi_result (PyObject *result)
> -{
> -  /* At the top-level, the result must be a dictionary.  */
> -
> -  if (!PyDict_Check (result))
> -    gdbpy_error (_("Result from invoke must be a dictionary"));
> -
> -  PyObject *key, *value;
> -  Py_ssize_t pos = 0;
> -  while (PyDict_Next (result, &pos, &key, &value))
> -    {
> -      gdb::unique_xmalloc_ptr<char> key_string
> -	(py_object_to_mi_key (key));
> -      serialize_mi_result_1 (value, key_string.get ());
> -    }
> -}
> -
>  /* Called when the MI command is invoked.  PARSE contains the parsed
>     command line arguments from the user.  */
>  
> @@ -388,7 +216,12 @@ mi_command_py::invoke (struct mi_parse *parse) const
>      gdbpy_handle_exception ();
>  
>    if (result != Py_None)
> -    serialize_mi_result (result.get ());
> +    {
> +      /* At the top-level, the result must be a dictionary.  */
> +      if (!PyDict_Check (result.get ()))
> +        gdbpy_error (_("Result from invoke must be a dictionary"));
> +      serialize_mi_data (result.get ());
> +    }
>  }
>  
>  /* See declaration above.  */
> diff --git a/gdb/python/py-utils.c b/gdb/python/py-utils.c
> index d5b07a80d82..72a232f251d 100644
> --- a/gdb/python/py-utils.c
> +++ b/gdb/python/py-utils.c
> @@ -597,3 +597,148 @@ gdbpy_fix_doc_string_indentation (gdb::unique_xmalloc_ptr<char> doc)
>  
>    return doc;
>  }
> +
> +/* Convert KEY_OBJ into a string that can be used as a field name in MI
> +   output.  KEY_OBJ must be a Python string object, and must only contain
> +   characters suitable for use as an MI field name.
> +
> +   If KEY_OBJ is not a string, or if KEY_OBJ contains invalid characters,
> +   then an error is thrown.  Otherwise, KEY_OBJ is converted to a string
> +   and returned.  */
> +
> +static gdb::unique_xmalloc_ptr<char>
> +py_object_to_mi_key (PyObject *key_obj)
> +{
> +  /* The key must be a string.  */
> +  if (!PyUnicode_Check (key_obj))
> +    {
> +      gdbpy_ref<> key_repr (PyObject_Repr (key_obj));
> +      gdb::unique_xmalloc_ptr<char> key_repr_string;
> +      if (key_repr != nullptr)
> +	key_repr_string = python_string_to_target_string (key_repr.get ());
> +      if (key_repr_string == nullptr)
> +	gdbpy_handle_exception ();
> +
> +      gdbpy_error (_("non-string object used as key: %s"),
> +		   key_repr_string.get ());
> +    }
> +
> +  gdb::unique_xmalloc_ptr<char> key_string
> +    = python_string_to_target_string (key_obj);
> +  if (key_string == nullptr)
> +    gdbpy_handle_exception ();
> +
> +  /* Predicate function, returns true if NAME is a valid field name for use
> +     in MI result output, otherwise, returns false.  */
> +  auto is_valid_key_name = [] (const char *name) -> bool
> +  {
> +    gdb_assert (name != nullptr);
> +
> +    if (*name == '\0' || !isalpha (*name))
> +      return false;
> +
> +    for (; *name != '\0'; ++name)
> +      if (!isalnum (*name) && *name != '_' && *name != '-')
> +	return false;
> +
> +    return true;
> +  };
> +
> +  if (!is_valid_key_name (key_string.get ()))
> +    {
> +      if (*key_string.get () == '\0')
> +	gdbpy_error (_("Invalid empty key in MI result"));
> +      else
> +	gdbpy_error (_("Invalid key in MI result: %s"), key_string.get ());
> +    }
> +
> +  return key_string;
> +}
> +
> +static void
> +serialize_mi_data_1 (PyObject *data, const char *field_name)

The old version of this function `serialize_mi_result_1` had a comment,
which is now missing.

> +{
> +  struct ui_out *uiout = current_uiout;
> +
> +  if (PyDict_Check (data))
> +    {
> +      PyObject *key, *value;
> +      Py_ssize_t pos = 0;
> +      ui_out_emit_tuple tuple_emitter (uiout, field_name);
> +      while (PyDict_Next (data, &pos, &key, &value))
> +	{
> +	  gdb::unique_xmalloc_ptr<char> key_string
> +	    (py_object_to_mi_key (key));
> +	  serialize_mi_data_1 (value, key_string.get ());
> +	}
> +    }
> +  else if (PySequence_Check (data) && !PyUnicode_Check (data))
> +    {
> +      ui_out_emit_list list_emitter (uiout, field_name);
> +      Py_ssize_t len = PySequence_Size (data);
> +      if (len == -1)
> +	gdbpy_handle_exception ();
> +      for (Py_ssize_t i = 0; i < len; ++i)
> +	{
> +	  gdbpy_ref<> item (PySequence_ITEM (data, i));
> +	  if (item == nullptr)
> +	    gdbpy_handle_exception ();
> +	  serialize_mi_data_1 (item.get (), nullptr);
> +	}
> +    }
> +  else if (PyIter_Check (data))
> +    {
> +      gdbpy_ref<> item;
> +      ui_out_emit_list list_emitter (uiout, field_name);
> +      while (true)
> +	{
> +	  item.reset (PyIter_Next (data));
> +	  if (item == nullptr)
> +	    {
> +	      if (PyErr_Occurred () != nullptr)
> +		gdbpy_handle_exception ();
> +	      break;
> +	    }
> +	  serialize_mi_data_1 (item.get (), nullptr);
> +	}
> +    }
> +  else
> +    {
> +      if (PyLong_Check (data))
> +	{
> +	  int overflow = 0;
> +	  gdb_py_longest val = gdb_py_long_as_long_and_overflow (data,
> +								 &overflow);
> +	  if (PyErr_Occurred () != nullptr)
> +	    gdbpy_handle_exception ();
> +	  if (overflow == 0)
> +	    {
> +	      uiout->field_signed (field_name, val);
> +	      return;
> +	    }
> +	  /* Fall through to the string case on overflow.  */
> +	}
> +
> +      gdb::unique_xmalloc_ptr<char> string (gdbpy_obj_to_string (data));
> +      if (string == nullptr)
> +	gdbpy_handle_exception ();
> +      uiout->field_string (field_name, string.get ());
> +    }
> +}
> +
> +/* See python-internal.h.  */
> +
> +void
> +serialize_mi_data (PyObject *data)
> +{
> +  gdb_assert (PyDict_Check (data));
> +
> +  PyObject *key, *value;
> +  Py_ssize_t pos = 0;
> +  while (PyDict_Next (data, &pos, &key, &value))
> +    {
> +      gdb::unique_xmalloc_ptr<char> key_string
> +	(py_object_to_mi_key (key));
> +      serialize_mi_data_1 (value, key_string.get ());
> +    }
> +}
> diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
> index 93217375cc5..3f53b0ab6f0 100644
> --- a/gdb/python/python-internal.h
> +++ b/gdb/python/python-internal.h
> @@ -486,6 +486,21 @@ struct gdbarch *arch_object_to_gdbarch (PyObject *obj);
>  extern PyObject *gdbpy_execute_mi_command (PyObject *self, PyObject *args,
>  					   PyObject *kw);
>  
> +/* Serialize DATA and print it in MI format to the current_uiout.
> +
> +   This function handles the top-level result initially returned from the
> +   invoke method of the Python command implementation.  At the top-level
> +   the data must be a dictionary.  Caller of this function is responsible
> +   for ensuring that.  The values within this dictionary can
> +   be a wider range of types.  Handling the values of the top-level
> +   dictionary is done by serialize_mi_data_1, see that function for more
> +   details.

Given the change coming in the next commit, I wonder if this comment
should be reworded to avoid talking about the 'invoke method'.  Maybe
just say how it formats DATA as an MI record.

> +
> +   If anything goes wrong while parsing and printing the MI output then an
> +   error is thrown.  */
> +
> +extern void serialize_mi_data (PyObject *result);

s/result/data/ here.

Thanks,
Andrew

> +
>  /* Convert Python object OBJ to a program_space pointer.  OBJ must be a
>     gdb.Progspace reference.  Return nullptr if the gdb.Progspace is not
>     valid (see gdb.Progspace.is_valid), otherwise return the program_space
> -- 
> 2.40.1