Re: [PATCHv3] gdb/python/mi: create MI commands using python

[Simon Marchi <simon.marchi@polymtl.ca>]

>>> -static bool
>>> +bool
>>>  insert_mi_cmd_entry (mi_command_up command)
>>>  {
>>>    gdb_assert (command != nullptr);
>>>  
>>> -  const std::string &name = command->name ();
>>> +  const std::string name (command->name ());
>>
>> Is this change needed?
> 
> No, it just felt clearer.  They both compile to the same thing as
> command->name() returns a 'char *', so you end up creating a new
> std::string in both cases.
> 
> Anyway, I reverted this as it's not important for this patch.

Ah, I see.  Well I think it's the same in the end.

I was surprised to see the change from an assignment to constructor
parameters (the parenthesis), I thought we preferred the assignment
syntax.

>>>    if (mi_cmd_table.find (name) != mi_cmd_table.end ())
>>>      return false;
>>> @@ -127,6 +123,20 @@ insert_mi_cmd_entry (mi_command_up command)
>>>    return true;
>>>  }
>>>  
>>> +bool
>>> +remove_mi_cmd_entry (mi_command *command)
>>> +{
>>> +  gdb_assert (command != nullptr);
>>
>> If command can't be nullptr, it could be a reference.
> 
> I wasn't sure about this change.  All the caller ever has is a
> pointer, so this would force every caller to (a) add an assert, then
> (b) call the function with the dereferenced pointer.
> 
> I'd hope the compiler would be smart enough to generate pretty much
> the same code - except for moving the assert out of the function to
> every call site - which doesn't fell like a win to me.
> 
> If we had an actual object at, even just some of, the call sites, I'd
> probably agree with you...
> 
> ... have I convinced you?  Or would you still like this changed to a
> reference?

Well, since it is documented as not accepting a nullptr argument, it
means the callers must already have a check nullptr check or assert
gating the calls to remove_mi_cmd_entry.  Otherwise, there is a bug.
You have two callers, in py-micmd.c:

 - in micmdpy_uninstall_command, there is already a gdb_assert for this
 - in micmdpy_dealloc, the call is in a `if (cmd->mi_command != nullptr)`

It's really not that important.  I have just been wondering lately why
in GDB we tend to use pointers for parameters even if they must not be
nullptr, instead of references.  Perhaps it's just out of habit.  Taking
a reference just takes the "can this parameter be nulltpr?" question out
of the equation.

Although here, actually, we could also just pass a string, since that's
all remove_mi_cmd_entry needs.

Again, not very important, but I am asking to see if we should maybe try
to adjust our habits.

>> I think that error() calls in this function should be replaced with
>> setting the appropriate Python exception type.  For example, the above
>> should raise a ValueError.
> 
> This opened a can of worms :)  The biggest change in this new
> iteration is the error handling.  Far fewer calls to error() now, in
> most cases we set a Python exception instead.

Actually, I think there are some cases that should stay calls to error
:).  See below.

> 
>>
>>> +      return -1;
>>> +    }
>>> +  else if ((name_len < 2) || (name[0] != '-') || !isalnum (name[1]))
>>> +    {
>>> +      error (_("MI command name does not start with '-'"
>>> +               " followed by at least one letter or digit."));
>>> +      return -1;
>>> +    }
>>> +  else
>>> +    {
>>> +      for (int i = 2; i < name_len; i++)
>>> +	{
>>> +	  if (!isalnum (name[i]) && name[i] != '-')
>>> +	    {
>>> +	      error (_("MI command name contains invalid character: %c."),
>>> +		     name[i]);
>>> +	      return -1;
>>> +	    }
>>> +	}
>>> +
>>> +      /* Skip over the leading dash.  For the rest of this function the
>>> +	 dash is not important.  */
>>> +      ++name;
>>> +    }
>>> +
>>> +  /* Check that there's an 'invoke' method.  */
>>> +  if (!PyObject_HasAttr (self, invoke_cst))
>>> +    error (_("-%s: Python command object missing 'invoke' method."), name);
>>> +
>>> +  /* If this object already has a name set, then this object has been
>>> +     initialized before.  We handle this case a little differently.  */
>>> +  if (cmd->mi_command_name != nullptr)
>>
>> Huh, how can this happen?
> 
> Like this
> 
>   class Foo(gdb.MICommand):
>       def __init__(self, name, msg):
>           super(Foo, self).__init__(name)
>           self.__msg = msg
>       def invoke(self, args):
>           return {'msg': self.__msg}
> 
>   cmd = Foo('-foo', 'Hello')
>   cmd.__init__('-foo', 'Goodbye')

Well, that surprised me.  I was going to ask if that was even permitted
(the language allows this, by is the convention in the Python world to
support this?).

But then I read:

  https://docs.python.org/3/c-api/typeobj.html#c.PyTypeObject.tp_init

  This function corresponds to the __init__() method of classes. Like
  __init__(), it is possible to create an instance without calling
  __init__(), and it is possible to reinitialize an instance by calling
  its __init__() method again.

I learned that today, one more thing to watch out for when implementing
Python types in C/C++.

> +/* 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 (!PyString_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)
> +	PyErr_Format (PyExc_TypeError,
> +		      _("Non-string object used as key: %s"),
> +		      key_repr_string.get ());
> +      gdbpy_handle_exception ();

I don't think you need to set a Python exception and call
gdbpy_handle_exception here, you can just call error() directly.  In
fact, I would say that manually setting an exception and calling
gdbpy_handle_exception is an anti-pattern, since it's just an indirect
way of calling error().

> +
> +  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 != '_')
> +	return false;
> +
> +    return true;
> +  };
> +
> +  if (!is_valid_key_name (key_string.get ()))
> +    {
> +      if (*key_string.get () == '\0')
> +	PyErr_Format (PyExc_ValueError, _("Invalid empty key in MI result"));
> +      else
> +	PyErr_Format (PyExc_ValueError, _("Invalid key in MI result: %s"),
> +		      key_string.get ());
> +      gdbpy_handle_exception ();

Another case.

> +    }
> +
> +  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) && !PyString_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
> +    {
> +      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))
> +    {
> +      PyErr_SetString (PyExc_TypeError,
> +		       _("Result from invoke must be a dictionary"));
> +      gdbpy_handle_exception ();
> +    }

Another case.

> +
> +  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.  */
> +
> +void
> +mi_command_py::do_invoke (struct mi_parse *parse) const
> +{
> +  PYMICMD_SCOPED_DEBUG_ENTER_EXIT;
> +
> +  pymicmd_debug_printf ("this = %p, name = %s", this, name ());
> +
> +  mi_parse_argv (parse->args, parse);
> +
> +  if (parse->argv == nullptr)
> +    error (_("Problem parsing arguments: %s %s"), parse->command, parse->args);
> +
> +  PyObject *obj = (PyObject *) this->m_pyobj;
> +  gdb_assert (obj != nullptr);
> +
> +  gdbpy_enter enter_py (get_current_arch (), current_language);
> +
> +  /* Check this object has the invoke attribute.  */
> +  if (!micmdpy_check_invoke_attr (obj, name ()))
> +    gdbpy_handle_exception ();

I would not do this check here.  Just try to do the call, if the method
doesn't exist (for some reason), PyObject_CallMethodObjArgs will fail
with a clear error.

The micmdpy_check_invoke_attr in micmdpy_init is also not necessary, in
my opinion, but I can see how it can be a little convenience for the
user.  We just need to remember that it's not bullet-proof.  For
example, it checks that an invoke method exists, but it doesn't check
that it is callable with a single argument (well, two if you count
self).  Also, it's possible to add / remove methods to a type after the
fact.

Simon