Re: [PATCH 1/4 v18] Add xmethod documentation and NEWS entry

[Doug Evans <xdje42 at gmail dot com>]

Siva Chandra <sivachandra@google.com> writes:

> This part was approved (I think) previously but sending it out with
> version synced to v18. Also, the name changed from "debug method" to
> "xmethod". Hence, I think another review is required.
>
> ChangeLog
> 2014-05-23  Siva Chandra Reddy  <sivachandra@google.com>
>
>         * NEWS (Python Scripting): Add entry about the new xmethods
>         feature.
>
>         doc/
>         *  python.texi (Xmethods In Python, XMethod API)
>         (Writing an Xmethod): New nodes.
>         (Python API): New menu entries "Xmethods In Python",
>         "Xmethod API", "Writing an Xmethod".

Hi.

It was easiest to make comments by editing the code,
adding FIXMEs and whatnot.

// comments are comments for discussion, not intended to be checked in.

I'll apply the relevant comments to pieces 2,3,4 too.

commit 20b935157b64ae20d251bd4e4d5f1c111ffdb868
Author: Doug Evans <xdje42@gmail.com>
Date:   Sun May 25 15:54:45 2014 -0700

    review comments on xmethods v18 patch

diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 72d58ad..026790f 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -2237,7 +2237,7 @@ The @value{GDBN} Python API provides classes, interfaces and functions
 to implement, register and manipulate xmethods.
 @xref{Xmethods In Python}.
 
-A xmethod matcher should be an instance of a class derived from
+An xmethod matcher should be an instance of a class derived from
 @code{XMethodMatcher} defined in the module @code{gdb.xmethod}, or an
 object with similar interface and attributes.  An instance of
 @code{XMethodMatcher} has the following attributes:
@@ -2294,7 +2294,7 @@ corresponding entries in the @code{methods} list are enabled should be
 returned.
 @end defun
 
-A xmethod worker should be an instance of a class derived from
+An xmethod worker should be an instance of a class derived from
 @code{XMethodWorker} defined in the module @code{gdb.xmethod},
 or support the following interface:
 
@@ -2327,8 +2327,8 @@ existing matcher with the same name as @code{matcher} if
 globally.
 @end defun
 
-@node Writing a Xmethod
-@subsubsection Writing a Xmethod
+@node Writing an Xmethod
+@subsubsection Writing an Xmethod
 @cindex writing xmethods in Python
 
 Implementing xmethods in Python will require implementing xmethod
@@ -2372,6 +2372,11 @@ class MyClassMatcher(gdb.xmethod.XMethodMatcher):
     def match(self, class_type, method_name):
         if class_type.tag != 'MyClass':
             return None
+        # FIXME: User code shouldn't have to check the enabled flag.
+        # py-xmethods.c checks it, so can this check just be removed?
+        # If user code wants to add this enabled check, e.g., should a perf
+        # concern arise, then ok; but we shouldn't encourage it as the default
+        # mode of writing xmethods.
         if method_name == 'geta' and self.methods[0].enabled:
             return MyClassWorker_geta()
         elif method_name == 'operator+' and self.methods[1].enabled:
@@ -2386,6 +2391,7 @@ a worker object of type @code{MyClassWorker_geta} for the @code{geta}
 method, and a worker object of type @code{MyClassWorker_plus} for the
 @code{operator+} method.  Also, a worker object is returned only if the
 corresponding entry in the @code{methods} attribute is enabled.
+@c FIXME: See above: User code shouldn't check enabled flag, gdb should.
 
 The implementation of the worker classes returned by the matcher above
 is as follows:
@@ -2403,6 +2409,9 @@ class MyClassWorker_plus(gdb.xmethod.XMethodWorker):
     def get_arg_types(self):
         return gdb.lookup_type('MyClass')
 
+    # FIXME: IWBN if the invoke function could spell out the args instead
+    # of just having "args" as a list of all of them.
+    # If the user wants the args as a list then can write *args here, right?
     def invoke(self, obj, args):
         return obj['a_'] + args[0]['a_']
 @end smallexample
diff --git a/gdb/eval.c b/gdb/eval.c
index d374b6a..2909420 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -1771,6 +1771,15 @@ evaluate_subexp_standard (struct type *expect_type,
 	  return call_internal_function (exp->gdbarch, exp->language_defn,
 					 argvec[0], nargs, argvec + 1);
 	case TYPE_CODE_XMETHOD:
+	  // If call_internal_function gets gdbarch,language here a reasonable
+	  // case can be made that call_xmethod should too.  gdb has too much
+	  // global state, we should make a point of passing things explicitly
+	  // when we can, especially if similar code already is.
+	  // I suspect more state than gdbarch,language could be required,
+	  // and it would be better to create an object (or preferably use an
+	  // existing one) that contains all the info.
+	  // Whether it is important enough to make this change now ... dunno.
+	  // I'd say leave this as is.  Just wanted to write this down.
 	  return call_xmethod (argvec[0], nargs, argvec + 1);
 	default:
 	  return call_function_by_hand (argvec[0], nargs, argvec + 1);
diff --git a/gdb/python/lib/gdb/command/xmethods.py b/gdb/python/lib/gdb/command/xmethods.py
index f61e7fb..31f9cdd 100644
--- a/gdb/python/lib/gdb/command/xmethods.py
+++ b/gdb/python/lib/gdb/command/xmethods.py
@@ -241,7 +241,7 @@ class DisableXMethod(gdb.Command):
     Usage: disable xmethod [locus-regexp [name-regexp]]
 
     LOCUS-REGEXP is a regular expression matching the location of the
-    xmethod matcherss.  If it is omitted, all registered xmethod matchers
+    xmethod matchers.  If it is omitted, all registered xmethod matchers
     from all loci are disabled.  A locus could be 'global', a regular
     expression matching the current program space's filename, or a regular
     expression filenames of objfiles. Locus could be 'progspace' to specify
diff --git a/gdb/python/lib/gdb/xmethod.py b/gdb/python/lib/gdb/xmethod.py
index 9d0deff..124625e 100644
--- a/gdb/python/lib/gdb/xmethod.py
+++ b/gdb/python/lib/gdb/xmethod.py
@@ -109,7 +109,7 @@ class XMethodWorker(object):
         argument, then a gdb.Type object or a sequence with a single gdb.Type
         element is returned.
         """
-        raise NotImplementedError("XMethod get_arg_types")
+        raise NotImplementedError("XMethodWorker get_arg_types")
 
     def invoke(self, obj, args):
         """Invoke the xmethod.
@@ -124,7 +124,7 @@ class XMethodWorker(object):
             A gdb.Value corresponding to the value returned by the xmethod.
             Returns 'None' if the method does not return anything.
         """
-        raise NotImplementedError("XMethod invoke")
+        raise NotImplementedError("XMethodWorker invoke")
 
 
 class SimpleXMethodMatcher(XMethodMatcher):
@@ -192,7 +192,7 @@ class SimpleXMethodMatcher(XMethodMatcher):
 # object if MATCHER is not having the requisite attributes in the proper
 # format.
 
-def validate_xmethod_matcher(matcher):
+def _validate_xmethod_matcher(matcher):
     if not hasattr(matcher, "match"):
         return TypeError("Xmethod matcher is missing method: match")
     if not hasattr(matcher, "name"):
@@ -210,7 +210,7 @@ def validate_xmethod_matcher(matcher):
 # xmethod matcher with NAME in LOCUS.  Returns the index of the xmethod
 # matcher in 'xmethods' sequence attribute of the LOCUS.
 
-def lookup_xmethod_matcher(locus, name):
+def _lookup_xmethod_matcher(locus, name):
     i = 0
     for m in locus.xmethods:
         if m.name == name:
@@ -233,7 +233,7 @@ def register_xmethod_matcher(locus, matcher, replace=False):
             same name in the locus.  Otherwise, if a matcher with the same name
             exists in the locus, raise an exception.
     """
-    err = validate_xmethod_matcher(matcher)
+    err = _validate_xmethod_matcher(matcher)
     if err:
         raise err
     if not locus:
@@ -242,7 +242,7 @@ def register_xmethod_matcher(locus, matcher, replace=False):
         locus_name = "global"
     else:
         locus_name = locus.filename
-    index = lookup_xmethod_matcher(locus, matcher.name)
+    index = _lookup_xmethod_matcher(locus, matcher.name)
     if index:
         if replace:
             del locus.xmethods[index]
diff --git a/gdb/python/py-xmethods.c b/gdb/python/py-xmethods.c
index 0868c9f..7e725b5 100644
--- a/gdb/python/py-xmethods.c
+++ b/gdb/python/py-xmethods.c
@@ -49,34 +49,46 @@ static struct xmethod_worker *new_python_xmethod_worker
 /* Implementation of free_xmethod_worker_data for Python.  */
 
 void
-gdbpy_free_xmethod_worker_data
-  (const struct extension_language_defn *extlang, void *data)
+gdbpy_free_xmethod_worker_data (const struct extension_language_defn *extlang,
+				void *data)
 {
   struct gdbpy_worker_data *worker_data = data;
+  struct cleanup *cleanups;
 
   gdb_assert (worker_data->worker != NULL && worker_data->this_type != NULL);
 
+  /* We don't do much here, but we still need the GIL.  */
+  cleanups = ensure_python_env (get_current_arch (), current_language);
+
   Py_DECREF (worker_data->worker);
   Py_DECREF (worker_data->this_type);
   xfree (worker_data);
+
+  do_cleanups (cleanups);
 }
 
 /* Implementation of clone_xmethod_worker_data for Python.  */
 
 void *
-gdbpy_clone_xmethod_worker_data
-  (const struct extension_language_defn *extlang, void *data)
+gdbpy_clone_xmethod_worker_data (const struct extension_language_defn *extlang,
+				 void *data)
 {
   struct gdbpy_worker_data *worker_data = data, *new_data;
+  struct cleanup *cleanups;
 
   gdb_assert (worker_data->worker != NULL && worker_data->this_type != NULL);
 
+  /* We don't do much here, but we still need the GIL.  */
+  cleanups = ensure_python_env (get_current_arch (), current_language);
+
   new_data = XCNEW (struct gdbpy_worker_data);
   new_data->worker = worker_data->worker;
   new_data->this_type = worker_data->this_type;
   Py_INCREF (new_data->worker);
   Py_INCREF (new_data->this_type);
 
+  do_cleanups (cleanups);
+
   return new_data;
 }
 
@@ -180,7 +192,9 @@ gdbpy_get_matching_xmethod_workers
       return EXT_LANG_RC_ERROR;
     }
 
-  /* Gather debug method matchers registered with the object files.  */
+  /* Gather debug method matchers registered with the object files.
+     This could be done differently by iterating over each objfile's matcher
+     list individually, but there's no data yet to show it's needed.  */
   ALL_OBJFILES (objfile)
     {
       PyObject *py_objfile = objfile_to_objfile_object (objfile);
@@ -196,8 +210,7 @@ gdbpy_get_matching_xmethod_workers
 	}
 
       objfile_matchers = objfpy_get_xmethods (py_objfile, NULL);
-      py_xmethod_matcher_list = PySequence_Concat (temp,
-							objfile_matchers);
+      py_xmethod_matcher_list = PySequence_Concat (temp, objfile_matchers);
       Py_DECREF (temp);
       Py_DECREF (objfile_matchers);
       if (py_xmethod_matcher_list == NULL)
@@ -248,8 +261,7 @@ gdbpy_get_matching_xmethod_workers
 					     matchers_attr_str);
       if (gdb_matchers != NULL)
 	{
-	  py_xmethod_matcher_list = PySequence_Concat (temp,
-							   gdb_matchers);
+	  py_xmethod_matcher_list = PySequence_Concat (temp, gdb_matchers);
 	  Py_DECREF (temp);
 	  Py_DECREF (gdb_matchers);
 	  if (py_xmethod_matcher_list == NULL)
@@ -362,8 +374,8 @@ gdbpy_get_matching_xmethod_workers
 
 enum ext_lang_rc
 gdbpy_get_xmethod_arg_types (const struct extension_language_defn *extlang,
-				  struct xmethod_worker *worker,
-				  int *nargs, struct type ***arg_types)
+			     struct xmethod_worker *worker,
+			     int *nargs, struct type ***arg_types)
 {
   struct gdbpy_worker_data *worker_data = worker->data;
   PyObject *py_worker = worker_data->worker;
@@ -457,13 +469,15 @@ gdbpy_get_xmethod_arg_types (const struct extension_language_defn *extlang,
       struct type *arg_type = type_object_to_type (py_argtype_list);
 
       if (arg_type == NULL)
-	PyErr_SetString (PyExc_TypeError,
-			 _("Arg type returned by the get_arg_types method "
-			   "of a debug method worker object is not a gdb.Type "
-			   "object."));
+	{
+	  PyErr_SetString (PyExc_TypeError,
+			   _("Arg type returned by the get_arg_types method "
+			     "of a debug method worker object is not a "
+			     "gdb.Type object."));
+	}
       else
 	{
-	  type_array[1] = arg_type;
+	  type_array[i] = arg_type;
 	  i++;
 	}
     }
@@ -478,6 +492,7 @@ gdbpy_get_xmethod_arg_types (const struct extension_language_defn *extlang,
 
   /* Add the type of 'this' as the first argument.  */
   obj_type = type_object_to_type (worker_data->this_type);
+  /* FIXME: Add comment explaining why passing 1 for "is const" is ok here.  */
   type_array[0] = make_cv_type (1, 0, lookup_pointer_type (obj_type), NULL);
   *nargs = i;
   *arg_types = type_array;
@@ -490,9 +505,9 @@ gdbpy_get_xmethod_arg_types (const struct extension_language_defn *extlang,
 
 enum ext_lang_rc
 gdbpy_invoke_xmethod (const struct extension_language_defn *extlang,
-			   struct xmethod_worker *worker,
-			   struct value *obj, struct value **args, int nargs,
-			   struct value **result)
+		      struct xmethod_worker *worker,
+		      struct value *obj, struct value **args, int nargs,
+		      struct value **result)
 {
   int i;
   struct cleanup *cleanups;
@@ -516,12 +531,15 @@ gdbpy_invoke_xmethod (const struct extension_language_defn *extlang,
     }
   make_cleanup_py_decref (invoke_method);
 
+  // FIXME: unprotected call to check_typedef.  See py-value.c for example.
+  // Needs TRY_CATCH.  Here and below.
   obj_type = check_typedef (value_type (obj));
   this_type = check_typedef (type_object_to_type (worker_data->this_type));
   if (TYPE_CODE (obj_type) == TYPE_CODE_PTR)
     {
       struct type *this_ptr = lookup_pointer_type (this_type);
 
+      // FIXME: unprotected call to value_cast.  See py-value.c for example.
       if (!types_equal (obj_type, this_ptr))
 	obj = value_cast (this_ptr, obj);
     }
@@ -529,11 +547,13 @@ gdbpy_invoke_xmethod (const struct extension_language_defn *extlang,
     {
       struct type *this_ref = lookup_reference_type (this_type);
 
+      // FIXME: unprotected call to value_cast.  See py-value.c for example.
       if (!types_equal (obj_type, this_ref))
 	obj = value_cast (this_ref, obj);
     }
   else
     {
+      // FIXME: unprotected call to value_cast.  See py-value.c for example.
       if (!types_equal (obj_type, this_type))
 	obj = value_cast (this_type, obj);
     }