[PATCH 0/4] Python API: Accept named arguments in a few more places

[PATCH 0/4] Python API: Accept named arguments in a few more places

[Andrew Burgess]

While working on the Python Unwinder API for a previous patch set I
spotted that UnwindInfo.add_saved_register doesn't accept named
arguments.

As this function takes two arguments I though that accepting named
arguments might be a nice improvement.  So I started out by fixing
that.

Then for completeness I updated the other two methods in py-unwind.c
that also didn't accept named arguments, even though these functions
only take a single argument I think taking named arguments is in
general better, and consistency is good too.

But having updated PendingFrame.read_register I really _had_ to update
Frame.read_register otherwise the API would be inconsistent -- two
frame like objects both with a read_register method, one taking named
arguments and one not, that seems bad to me.  So then I updated
Frame.read_register.

Having updated one function in py-frame.c, that left just one function
that was not taking named arguments, Frame.read_var, so I updated that
one too.  This one does take multiple arguments, so having named
argument support is more useful here, plus I improved the error
message when an argument of the wrong type is passed.

Thanks,
Andrew


---

Andrew Burgess (4):
  gdb/python: have UnwindInfo.add_saved_register accept named args
  gdb/python: have PendingFrame methods accept keyword arguments
  gdb/python: convert Frame.read_register to take named arguments
  gdb/python: allow Frame.read_var to accept named arguments

 gdb/doc/python.texi                    |  18 ++---
 gdb/python/py-frame.c                  |  39 +++++----
 gdb/python/py-unwind.c                 | 107 +++++++++++++------------
 gdb/testsuite/gdb.python/py-frame.exp  |  40 +++++++++
 gdb/testsuite/gdb.python/py-unwind.exp |  15 +++-
 gdb/testsuite/gdb.python/py-unwind.py  |  32 +++++---
 6 files changed, 160 insertions(+), 91 deletions(-)


base-commit: 3712e78cab09017bf59105d44e2f745c5e608c5a
-- 
2.25.4

[PATCH 1/4] gdb/python: have UnwindInfo.add_saved_register accept named args

[Andrew Burgess]

Update gdb.UnwindInfo.add_saved_register to accept named keyword
arguments.

As part of this update we now use gdb_PyArg_ParseTupleAndKeywords
instead of PyArg_UnpackTuple to parse the function arguments.

By switching to gdb_PyArg_ParseTupleAndKeywords, we can now use 'O!'
as the argument format for the function's value argument.  This means
that we can check the argument type (is gdb.Value) as part of the
argument processing rather than manually performing the check later in
the function.  One result of this is that we now get a better error
message (at least, I think so).  Previously we would get something
like:

  ValueError: Bad register value

Now we get:

  TypeError: argument 2 must be gdb.Value, not XXXX

It's unfortunate that the exception type changed, but I think the new
exception type actually makes more sense.

For existing unwinder code that doesn't throw any exceptions nothing
should change with this commit.  It is possible that a user has some
code that throws and catches the ValueError, and this code will break
after this commit, but I think this is going to be sufficiently rare
that we can take the risk here.
---
 gdb/doc/python.texi                    |  4 +-
 gdb/python/py-unwind.c                 | 84 +++++++++++++-------------
 gdb/testsuite/gdb.python/py-unwind.exp | 15 +++--
 gdb/testsuite/gdb.python/py-unwind.py  | 28 ++++++---
 4 files changed, 75 insertions(+), 56 deletions(-)

diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index c74d586ef39..ef50e6dbe7b 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -2882,8 +2882,8 @@
 create a @code{gdb.UnwindInfo} instance.  Use the following method to
 specify caller registers that have been saved in this frame:
 
-@defun gdb.UnwindInfo.add_saved_register (reg, value)
-@var{reg} identifies the register, for a description of the acceptable
+@defun gdb.UnwindInfo.add_saved_register (@var{register}, @var{value})
+@var{register} identifies the register, for a description of the acceptable
 values see @ref{gdbpy_frame_read_register,,Frame.read_register}.
 @var{value} is a register value (a @code{gdb.Value} object).
 @end defun
diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
index 409dbd3a470..2e13b84eb12 100644
--- a/gdb/python/py-unwind.c
+++ b/gdb/python/py-unwind.c
@@ -292,7 +292,7 @@ pyuw_create_unwind_info (PyObject *pyo_pending_frame,
    gdb.UnwindInfo.add_saved_register (REG, VALUE) -> None.  */
 
 static PyObject *
-unwind_infopy_add_saved_register (PyObject *self, PyObject *args)
+unwind_infopy_add_saved_register (PyObject *self, PyObject *args, PyObject *kw)
 {
   unwind_info_object *unwind_info = (unwind_info_object *) self;
   pending_frame_object *pending_frame
@@ -305,11 +305,15 @@ unwind_infopy_add_saved_register (PyObject *self, PyObject *args)
     {
       PyErr_SetString (PyExc_ValueError,
 		       "UnwindInfo instance refers to a stale PendingFrame");
-      return NULL;
+      return nullptr;
     }
-  if (!PyArg_UnpackTuple (args, "previous_frame_register", 2, 2,
-			  &pyo_reg_id, &pyo_reg_value))
-    return NULL;
+
+  static const char *keywords[] = { "register", "value", nullptr };
+  if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "OO!", keywords,
+					&pyo_reg_id, &value_object_type,
+					&pyo_reg_value))
+    return nullptr;
+
   if (!gdbpy_parse_register_id (pending_frame->gdbarch, pyo_reg_id, &regnum))
     return nullptr;
 
@@ -332,43 +336,38 @@ unwind_infopy_add_saved_register (PyObject *self, PyObject *args)
 	}
     }
 
-  {
-    struct value *value;
-    size_t data_size;
+  /* The argument parsing above guarantees that PYO_REG_VALUE will be a
+     gdb.Value object, as a result the value_object_to_value call should
+     succeed.  */
+  gdb_assert (pyo_reg_value != nullptr);
+  struct value *value = value_object_to_value (pyo_reg_value);
+  gdb_assert (value != nullptr);
+
+  ULONGEST reg_size = register_size (pending_frame->gdbarch, regnum);
+  if (reg_size != value->type ()->length ())
+    {
+      PyErr_Format (PyExc_ValueError,
+		    "The value of the register returned by the Python "
+		    "sniffer has unexpected size: %s instead of %s.",
+		    pulongest (value->type ()->length ()),
+		    pulongest (reg_size));
+      return nullptr;
+    }
+
+  gdbpy_ref<> new_value = gdbpy_ref<>::new_reference (pyo_reg_value);
+  bool found = false;
+  for (saved_reg &reg : *unwind_info->saved_regs)
+    {
+      if (regnum == reg.number)
+	{
+	  found = true;
+	  reg.value = std::move (new_value);
+	  break;
+	}
+    }
+  if (!found)
+    unwind_info->saved_regs->emplace_back (regnum, std::move (new_value));
 
-    if (pyo_reg_value == NULL
-      || (value = value_object_to_value (pyo_reg_value)) == NULL)
-      {
-	PyErr_SetString (PyExc_ValueError, "Bad register value");
-	return NULL;
-      }
-    data_size = register_size (pending_frame->gdbarch, regnum);
-    if (data_size != value->type ()->length ())
-      {
-	PyErr_Format (
-	    PyExc_ValueError,
-	    "The value of the register returned by the Python "
-	    "sniffer has unexpected size: %u instead of %u.",
-	    (unsigned) value->type ()->length (),
-	    (unsigned) data_size);
-	return NULL;
-      }
-  }
-  {
-    gdbpy_ref<> new_value = gdbpy_ref<>::new_reference (pyo_reg_value);
-    bool found = false;
-    for (saved_reg &reg : *unwind_info->saved_regs)
-      {
-	if (regnum == reg.number)
-	  {
-	    found = true;
-	    reg.value = std::move (new_value);
-	    break;
-	  }
-      }
-    if (!found)
-      unwind_info->saved_regs->emplace_back (regnum, std::move (new_value));
-  }
   Py_RETURN_NONE;
 }
 
@@ -1087,7 +1086,8 @@ PyTypeObject pending_frame_object_type =
 static PyMethodDef unwind_info_object_methods[] =
 {
   { "add_saved_register",
-    unwind_infopy_add_saved_register, METH_VARARGS,
+    (PyCFunction) unwind_infopy_add_saved_register,
+    METH_VARARGS | METH_KEYWORDS,
     "add_saved_register (REG, VALUE) -> None\n"
     "Set the value of the REG in the previous frame to VALUE." },
   { NULL }  /* Sentinel */
diff --git a/gdb/testsuite/gdb.python/py-unwind.exp b/gdb/testsuite/gdb.python/py-unwind.exp
index fddf4f15393..37c8fd8fbb1 100644
--- a/gdb/testsuite/gdb.python/py-unwind.exp
+++ b/gdb/testsuite/gdb.python/py-unwind.exp
@@ -131,10 +131,17 @@ gdb_test "disable unwinder global \"test unwinder\"" \
 check_for_broken_backtrace "stack is broken after command disabling"
 check_info_unwinder "info unwinder after command disabling" off
 
-# Check that invalid register names cause errors.
-gdb_test "python print(add_saved_register_error)" "True" \
-    "add_saved_register error"
-gdb_test "python print(read_register_error)" "True" \
+# Check that invalid register names and values cause errors.
+gdb_test "python print(add_saved_register_errors\[\"unknown_name\"\])" \
+    "Bad register" \
+    "add_saved_register error when an unknown register name is used"
+gdb_test "python print(add_saved_register_errors\[\"unknown_number\"\])" \
+    "Bad register" \
+    "add_saved_register error when an unknown register number is used"
+gdb_test "python print(add_saved_register_errors\[\"bad_value\"\])" \
+    "argument 2 must be gdb.Value, not int" \
+    "add_saved_register error when invalid register value is used"
+gdb_test "python print(read_register_error)" "Bad register" \
     "read_register error"
 
 # Try to create an unwinder object with a non-string name.
diff --git a/gdb/testsuite/gdb.python/py-unwind.py b/gdb/testsuite/gdb.python/py-unwind.py
index 4e110c51e3b..5853abc7486 100644
--- a/gdb/testsuite/gdb.python/py-unwind.py
+++ b/gdb/testsuite/gdb.python/py-unwind.py
@@ -18,7 +18,7 @@ from gdb.unwinder import Unwinder, FrameId
 
 
 # These are set to test whether invalid register names cause an error.
-add_saved_register_error = False
+add_saved_register_errors = {}
 read_register_error = False
 
 
@@ -94,9 +94,9 @@ class TestUnwinder(Unwinder):
 
             try:
                 pending_frame.read_register("nosuchregister")
-            except ValueError:
+            except ValueError as ve:
                 global read_register_error
-                read_register_error = True
+                read_register_error = str(ve)
 
             frame_id = FrameId(
                 pending_frame.read_register(TestUnwinder.AMD64_RSP),
@@ -104,13 +104,25 @@ class TestUnwinder(Unwinder):
             )
             unwind_info = pending_frame.create_unwind_info(frame_id)
             unwind_info.add_saved_register(TestUnwinder.AMD64_RBP, previous_bp)
-            unwind_info.add_saved_register("rip", previous_ip)
-            unwind_info.add_saved_register("rsp", previous_sp)
+            unwind_info.add_saved_register(value=previous_ip, register="rip")
+            unwind_info.add_saved_register(register="rsp", value=previous_sp)
+
+            global add_saved_register_errors
             try:
                 unwind_info.add_saved_register("nosuchregister", previous_sp)
-            except ValueError:
-                global add_saved_register_error
-                add_saved_register_error = True
+            except ValueError as ve:
+                add_saved_register_errors["unknown_name"] = str(ve)
+
+            try:
+                unwind_info.add_saved_register(999, previous_sp)
+            except ValueError as ve:
+                add_saved_register_errors["unknown_number"] = str(ve)
+
+            try:
+                unwind_info.add_saved_register("rsp", 1234)
+            except TypeError as ve:
+                add_saved_register_errors["bad_value"] = str(ve)
+
             return unwind_info
         except (gdb.error, RuntimeError):
             return None
-- 
2.25.4

[PATCH 2/4] gdb/python: have PendingFrame methods accept keyword arguments

[Andrew Burgess]

Update the two gdb.PendingFrame methods gdb.PendingFrame.read_register
and gdb.PendingFrame.create_unwind_info accept keyword arguments.

There's no huge benefit for making this change, both of these methods
only take a single argument, so it is (maybe) less likely that a user
will take advantage of the keyword arguments in these cases, but I
think it's nice to be consistent, and I don't see any particular draw
backs to making this change.

There should be no user visible changes (for existing code) after this
commit.
---
 gdb/doc/python.texi                   | 10 +++++-----
 gdb/python/py-unwind.c                | 23 ++++++++++++++---------
 gdb/testsuite/gdb.python/py-unwind.py |  4 ++--
 3 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index ef50e6dbe7b..4f62ca7d95e 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -2756,11 +2756,11 @@
 An object passed to an unwinder (a @code{gdb.PendingFrame} instance)
 provides a method to read frame's registers:
 
-@defun PendingFrame.read_register (reg)
-This method returns the contents of the register @var{reg} in the
+@defun PendingFrame.read_register (@var{register})
+This method returns the contents of the register @var{register} in the
 frame as a @code{gdb.Value} object.  For a description of the
-acceptable values of @var{reg} see
-@ref{gdbpy_frame_read_register,,Frame.read_register}.  If @var{reg}
+acceptable values of @var{register} see
+@ref{gdbpy_frame_read_register,,Frame.read_register}.  If @var{register}
 does not name a register for the current architecture, this method
 will throw an exception.
 
@@ -2783,7 +2783,7 @@
 instance to be returned to @value{GDBN}:
 
 @anchor{gdb.PendingFrame.create_unwind_info}
-@defun PendingFrame.create_unwind_info (frame_id)
+@defun PendingFrame.create_unwind_info (@var{frame_id})
 Returns a new @code{gdb.UnwindInfo} instance identified by given
 @var{frame_id}.  The @var{frame_id} is used internally by @value{GDBN}
 to identify the frames within the current thread's stack.  The
diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
index 2e13b84eb12..d83979bed2b 100644
--- a/gdb/python/py-unwind.c
+++ b/gdb/python/py-unwind.c
@@ -443,16 +443,17 @@ pending_framepy_repr (PyObject *self)
    Returns the value of register REG as gdb.Value instance.  */
 
 static PyObject *
-pending_framepy_read_register (PyObject *self, PyObject *args)
+pending_framepy_read_register (PyObject *self, PyObject *args, PyObject *kw)
 {
   pending_frame_object *pending_frame = (pending_frame_object *) self;
   PENDING_FRAMEPY_REQUIRE_VALID (pending_frame);
 
-  int regnum;
   PyObject *pyo_reg_id;
+  static const char *keywords[] = { "register", nullptr };
+  if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "O", keywords, &pyo_reg_id))
+    return nullptr;
 
-  if (!PyArg_UnpackTuple (args, "read_register", 1, 1, &pyo_reg_id))
-    return NULL;
+  int regnum;
   if (!gdbpy_parse_register_id (pending_frame->gdbarch, pyo_reg_id, &regnum))
     return nullptr;
 
@@ -681,7 +682,8 @@ pending_framepy_function (PyObject *self, PyObject *args)
    PendingFrame.create_unwind_info (self, frameId) -> UnwindInfo.  */
 
 static PyObject *
-pending_framepy_create_unwind_info (PyObject *self, PyObject *args)
+pending_framepy_create_unwind_info (PyObject *self, PyObject *args,
+				    PyObject *kw)
 {
   PyObject *pyo_frame_id;
   CORE_ADDR sp;
@@ -690,7 +692,9 @@ pending_framepy_create_unwind_info (PyObject *self, PyObject *args)
 
   PENDING_FRAMEPY_REQUIRE_VALID ((pending_frame_object *) self);
 
-  if (!PyArg_ParseTuple (args, "O:create_unwind_info", &pyo_frame_id))
+  static const char *keywords[] = { "frame_id", nullptr };
+  if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "O", keywords,
+					&pyo_frame_id))
     return nullptr;
 
   pyuw_get_attr_code code
@@ -1002,11 +1006,12 @@ gdbpy_initialize_unwind (void)
 
 static PyMethodDef pending_frame_object_methods[] =
 {
-  { "read_register", pending_framepy_read_register, METH_VARARGS,
+  { "read_register", (PyCFunction) pending_framepy_read_register,
+    METH_VARARGS | METH_KEYWORDS,
     "read_register (REG) -> gdb.Value\n"
     "Return the value of the REG in the frame." },
-  { "create_unwind_info",
-    pending_framepy_create_unwind_info, METH_VARARGS,
+  { "create_unwind_info", (PyCFunction) pending_framepy_create_unwind_info,
+    METH_VARARGS | METH_KEYWORDS,
     "create_unwind_info (FRAME_ID) -> gdb.UnwindInfo\n"
     "Construct UnwindInfo for this PendingFrame, using FRAME_ID\n"
     "to identify it." },
diff --git a/gdb/testsuite/gdb.python/py-unwind.py b/gdb/testsuite/gdb.python/py-unwind.py
index 5853abc7486..201b629f9fe 100644
--- a/gdb/testsuite/gdb.python/py-unwind.py
+++ b/gdb/testsuite/gdb.python/py-unwind.py
@@ -99,7 +99,7 @@ class TestUnwinder(Unwinder):
                 read_register_error = str(ve)
 
             frame_id = FrameId(
-                pending_frame.read_register(TestUnwinder.AMD64_RSP),
+                pending_frame.read_register(register=TestUnwinder.AMD64_RSP),
                 pending_frame.read_register(TestUnwinder.AMD64_RIP),
             )
             unwind_info = pending_frame.create_unwind_info(frame_id)
@@ -156,7 +156,7 @@ class simple_unwinder(Unwinder):
             captured_pending_frame = pending_frame
             captured_pending_frame_repr = repr(pending_frame)
             fid = FrameId(self._sp, self._pc)
-            uw = pending_frame.create_unwind_info(fid)
+            uw = pending_frame.create_unwind_info(frame_id=fid)
             uw.add_saved_register("rip", gdb.Value(0x123))
             uw.add_saved_register("rbp", gdb.Value(0x456))
             uw.add_saved_register("rsp", gdb.Value(0x789))
-- 
2.25.4

[PATCH 3/4] gdb/python: convert Frame.read_register to take named arguments

[Andrew Burgess]

Following on from the previous commit, this updates
Frame.read_register to accept named arguments.  As with the previous
commit there's no huge benefit for the users in accepting named
arguments here -- this function only takes a single argument after
all.

But I do think it is worth keeping Frame.read_register method in sync
with the PendingFrame.read_register method, this allows for the
possibility that the user has some code that can operate on either a
Frame or a Pending frame.

Minor update to allow for named arguments, and an extra test to check
the new functionality.
---
 gdb/doc/python.texi                   |  2 +-
 gdb/python/py-frame.c                 | 11 +++++++----
 gdb/testsuite/gdb.python/py-frame.exp |  6 ++++++
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 4f62ca7d95e..f54909cc05d 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -5380,7 +5380,7 @@
 @end defun
 
 @anchor{gdbpy_frame_read_register}
-@defun Frame.read_register (register)
+@defun Frame.read_register (@var{register})
 Return the value of @var{register} in this frame.  Returns a
 @code{Gdb.Value} object.  Throws an exception if @var{register} does
 not exist.  The @var{register} argument must be one of the following:
diff --git a/gdb/python/py-frame.c b/gdb/python/py-frame.c
index 00cd4bee492..3a033ac7570 100644
--- a/gdb/python/py-frame.c
+++ b/gdb/python/py-frame.c
@@ -238,13 +238,15 @@ frapy_pc (PyObject *self, PyObject *args)
    Returns the value of a register in this frame.  */
 
 static PyObject *
-frapy_read_register (PyObject *self, PyObject *args)
+frapy_read_register (PyObject *self, PyObject *args, PyObject *kw)
 {
   PyObject *pyo_reg_id;
   PyObject *result = nullptr;
 
-  if (!PyArg_UnpackTuple (args, "read_register", 1, 1, &pyo_reg_id))
-    return NULL;
+  static const char *keywords[] = { "register", nullptr };
+  if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "O", keywords, &pyo_reg_id))
+    return nullptr;
+
   try
     {
       scoped_value_mark free_values;
@@ -766,7 +768,8 @@ Return the reason why it's not possible to find frames older than this." },
   { "pc", frapy_pc, METH_NOARGS,
     "pc () -> Long.\n\
 Return the frame's resume address." },
-  { "read_register", frapy_read_register, METH_VARARGS,
+  { "read_register", (PyCFunction) frapy_read_register,
+    METH_VARARGS | METH_KEYWORDS,
     "read_register (register_name) -> gdb.Value\n\
 Return the value of the register in the frame." },
   { "block", frapy_block, METH_NOARGS,
diff --git a/gdb/testsuite/gdb.python/py-frame.exp b/gdb/testsuite/gdb.python/py-frame.exp
index 5aebb6beb5f..07997326d09 100644
--- a/gdb/testsuite/gdb.python/py-frame.exp
+++ b/gdb/testsuite/gdb.python/py-frame.exp
@@ -115,6 +115,12 @@ gdb_test "python print ('result = %s' % (f0.read_register('pc') == f0.pc()))" \
   " = True" \
   "test Frame.read_register(pc)"
 
+# Repeat the previous test, but this time use named arguments for the
+# read_register method call.
+gdb_test "python print ('result = %s' % (f0.read_register(register = 'pc') == f0.pc()))" \
+  " = True" \
+  "test Frame.read_register() using named arguments"
+
 # Test arch-specific register name.
 set pc ""
 if {[is_amd64_regs_target]} {
-- 
2.25.4

[PATCH 4/4] gdb/python: allow Frame.read_var to accept named arguments

[Andrew Burgess]

This commit allows Frame.read_var to accept named arguments, and also
improves (I think) some of the error messages emitted when values of
the wrong type are passed to this function.

The read_var method takes two arguments, one a variable, which is
either a gdb.Symbol or a string, while the second, optional, argument
is always a gdb.Block.

I'm now using 'O!' as the format specifier for the second argument,
which allows the argument type to be checked early on.  Currently, if
the second argument is of the wrong type then we get this error:

  (gdb) python print(gdb.selected_frame().read_var("a1", "xxx"))
  Traceback (most recent call last):
    File "<string>", line 1, in <module>
  RuntimeError: Second argument must be block.
  Error while executing Python code.
  (gdb)

After this commit, we now get an error like this:

  (gdb) python print(gdb.selected_frame().read_var("a1", "xxx"))
  Traceback (most recent call last):
    File "<string>", line 1, in <module>
  TypeError: argument 2 must be gdb.Block, not str
  Error while executing Python code.
  (gdb)

Changes are:

  1. Exception type is TypeError not RuntimeError, this is unfortunate
  as user code _could_ be relying on this, but I think the improvement
  is worth the risk, user code relying on the exact exception type is
  likely to be pretty rare,

  2. New error message gives argument position and expected argument
  type, as well as the type that was passed.

If the first argument, the variable, has the wrong type then the
previous exception was already a TypeError, however, I've updated the
text of the exception to more closely match the "standard" error
message we see above.  If the first argument has the wrong type then
before this commit we saw this:

  (gdb) python print(gdb.selected_frame().read_var(123))
  Traceback (most recent call last):
    File "<string>", line 1, in <module>
  TypeError: Argument must be a symbol or string.
  Error while executing Python code.
  (gdb)

And after we see this:

  (gdb) python print(gdb.selected_frame().read_var(123))
  Traceback (most recent call last):
    File "<string>", line 1, in <module>
  TypeError: argument 1 must be gdb.Symbol or str, not int
  Error while executing Python code.
  (gdb)

For existing code that doesn't use named arguments and doesn't rely on
exceptions, there will be no changes after this commit.
---
 gdb/doc/python.texi                   |  2 +-
 gdb/python/py-frame.c                 | 28 ++++++++++++----------
 gdb/testsuite/gdb.python/py-frame.exp | 34 +++++++++++++++++++++++++++
 3 files changed, 50 insertions(+), 14 deletions(-)

diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index f54909cc05d..881c743034e 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -5406,7 +5406,7 @@
 object.
 @end defun
 
-@defun Frame.read_var (variable @r{[}, block@r{]})
+@defun Frame.read_var (@var{variable} @r{[}, @var{block}@r{]})
 Return the value of @var{variable} in this frame.  If the optional
 argument @var{block} is provided, search for the variable from that
 block; otherwise start at the frame's current block (which is
diff --git a/gdb/python/py-frame.c b/gdb/python/py-frame.c
index 3a033ac7570..082358effab 100644
--- a/gdb/python/py-frame.c
+++ b/gdb/python/py-frame.c
@@ -475,15 +475,18 @@ frapy_find_sal (PyObject *self, PyObject *args)
    gdb.Symbol.  The block argument must be an instance of gdb.Block.  Returns
    NULL on error, with a python exception set.  */
 static PyObject *
-frapy_read_var (PyObject *self, PyObject *args)
+frapy_read_var (PyObject *self, PyObject *args, PyObject *kw)
 {
   frame_info_ptr frame;
   PyObject *sym_obj, *block_obj = NULL;
   struct symbol *var = NULL;	/* gcc-4.3.2 false warning.  */
   const struct block *block = NULL;
 
-  if (!PyArg_ParseTuple (args, "O|O", &sym_obj, &block_obj))
-    return NULL;
+  static const char *keywords[] = { "variable", "block", nullptr };
+  if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "O|O!", keywords,
+					&sym_obj, &block_object_type,
+					&block_obj))
+    return nullptr;
 
   if (PyObject_TypeCheck (sym_obj, &symbol_object_type))
     var = symbol_object_to_symbol (sym_obj);
@@ -495,15 +498,13 @@ frapy_read_var (PyObject *self, PyObject *args)
       if (!var_name)
 	return NULL;
 
-      if (block_obj)
+      if (block_obj != nullptr)
 	{
+	  /* This call should only fail if the type of BLOCK_OBJ is wrong,
+	     and we ensure the type is correct when we parse the arguments,
+	     so we can just assert the return value is not nullptr.  */
 	  block = block_object_to_block (block_obj);
-	  if (!block)
-	    {
-	      PyErr_SetString (PyExc_RuntimeError,
-			       _("Second argument must be block."));
-	      return NULL;
-	    }
+	  gdb_assert (block != nullptr);
 	}
 
       try
@@ -533,8 +534,9 @@ frapy_read_var (PyObject *self, PyObject *args)
     }
   else
     {
-      PyErr_SetString (PyExc_TypeError,
-		       _("Argument must be a symbol or string."));
+      PyErr_Format (PyExc_TypeError,
+		    _("argument 1 must be gdb.Symbol or str, not %s"),
+		    Py_TYPE (sym_obj)->tp_name);
       return NULL;
     }
 
@@ -787,7 +789,7 @@ Return the frame called by this frame." },
   { "find_sal", frapy_find_sal, METH_NOARGS,
     "find_sal () -> gdb.Symtab_and_line.\n\
 Return the frame's symtab and line." },
-  { "read_var", frapy_read_var, METH_VARARGS,
+  { "read_var", (PyCFunction) frapy_read_var, METH_VARARGS | METH_KEYWORDS,
     "read_var (variable) -> gdb.Value.\n\
 Return the value of the variable in this frame." },
   { "select", frapy_select, METH_NOARGS,
diff --git a/gdb/testsuite/gdb.python/py-frame.exp b/gdb/testsuite/gdb.python/py-frame.exp
index 07997326d09..16177c8a5f8 100644
--- a/gdb/testsuite/gdb.python/py-frame.exp
+++ b/gdb/testsuite/gdb.python/py-frame.exp
@@ -45,6 +45,10 @@ gdb_test "python print (bf1.read_var(\"i\"))" "\"stuff\"" "test i"
 gdb_test "python print (bf1.read_var(\"f\"))" "\"foo\"" "test f"
 gdb_test "python print (bf1.read_var(\"b\"))" "\"bar\"" "test b"
 
+# Check we can use a single named argument with read_var.
+gdb_test "python print (bf1.read_var(variable = \"b\"))" "\"bar\"" \
+    "test b using named arguments"
+
 # Test the read_var function in another block other than the current
 # block (in this case, the super block). Test thar read_var is reading
 # the correct variables of i and f but they are the correct value and type.
@@ -54,12 +58,42 @@ gdb_test "python print (bf1.read_var(\"i\", sb).type)" "double" "test double i"
 gdb_test "python print (bf1.read_var(\"f\", sb))" "2.2.*" "test f = 2.2"
 gdb_test "python print (bf1.read_var(\"f\", sb).type)" "double" "test double f"
 
+# Now test read_var with a variable and block using named arguments.
+gdb_test "python print (bf1.read_var(block = sb, variable = \"i\"))" "1.1.*" \
+    "test i = 1.1 usign named arguments"
+gdb_test "python print (bf1.read_var(block = sb, variable = \"f\"))" "2.2.*" \
+    "test f = 2.2 using named arguments"
+
 # And again test another outerblock, this time testing "i" is the
 # correct value and type.
 gdb_py_test_silent_cmd "python sb = sb.superblock" "get superblock" 0
 gdb_test "python print (bf1.read_var(\"i\", sb))" "99" "test i = 99"
 gdb_test "python print (bf1.read_var(\"i\", sb).type)" "int" "test int i"
 
+# Test what happens when we provide a block of the wrong type.
+gdb_test "python print (bf1.read_var(\"i\", \"some_block\"))" \
+    [multi_line \
+	 "TypeError: argument 2 must be gdb\\.Block, not str" \
+	 "Error while executing Python code\\."] \
+    "check invalid block type error"
+gdb_test "python print (bf1.read_var(block = \"some_block\", variable = \"i\"))" \
+    [multi_line \
+	 "TypeError: argument 2 must be gdb\\.Block, not str" \
+	 "Error while executing Python code\\."] \
+    "check invalid block type error when named args are used"
+
+# Test what happens when we provide a variable of the wrong type.
+gdb_test "python print (bf1.read_var(None))" \
+    [multi_line \
+	 "TypeError: argument 1 must be gdb\\.Symbol or str, not NoneType" \
+	 "Error while executing Python code\\."] \
+    "check read_var error when variable is None"
+gdb_test "python print (bf1.read_var(sb))" \
+    [multi_line \
+	 "TypeError: argument 1 must be gdb\\.Symbol or str, not gdb\\.Block" \
+	 "Error while executing Python code\\."] \
+    "check read_var error when variable is a gdb.Block"
+
 gdb_breakpoint "f2"
 gdb_continue_to_breakpoint "breakpoint at f2"
 gdb_py_test_silent_cmd "python bframe = gdb.selected_frame()" \
-- 
2.25.4

Re: [PATCH 1/4] gdb/python: have UnwindInfo.add_saved_register accept named args

[Eli Zaretskii]

> Cc: Andrew Burgess <aburgess@redhat.com>
> Date: Thu, 30 Mar 2023 13:10:20 +0100
> From: Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>
> 
> --- a/gdb/doc/python.texi
> +++ b/gdb/doc/python.texi
> @@ -2882,8 +2882,8 @@
>  create a @code{gdb.UnwindInfo} instance.  Use the following method to
>  specify caller registers that have been saved in this frame:
>  
> -@defun gdb.UnwindInfo.add_saved_register (reg, value)
> -@var{reg} identifies the register, for a description of the acceptable
> +@defun gdb.UnwindInfo.add_saved_register (@var{register}, @var{value})
> +@var{register} identifies the register, for a description of the acceptable
>  values see @ref{gdbpy_frame_read_register,,Frame.read_register}.
>  @var{value} is a register value (a @code{gdb.Value} object).
>  @end defun

Why did you use @var inside the argument list on a @defun line?  That
shouldn't be necessary, and the original text didn't use that.

Otherwise, this is okay (although the change just modifies the name of
a single argument, so I'm not sure why we are doing this).  Thanks.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Re: [PATCH 3/4] gdb/python: convert Frame.read_register to take named arguments

[Eli Zaretskii]

> Cc: Andrew Burgess <aburgess@redhat.com>
> Date: Thu, 30 Mar 2023 13:10:22 +0100
> From: Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>
> 
> diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
> index 4f62ca7d95e..f54909cc05d 100644
> --- a/gdb/doc/python.texi
> +++ b/gdb/doc/python.texi
> @@ -5380,7 +5380,7 @@
>  @end defun
>  
>  @anchor{gdbpy_frame_read_register}
> -@defun Frame.read_register (register)
> +@defun Frame.read_register (@var{register})

Same comment here: I don't understand why you need @var in the
argument list of a @defun.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Re: [PATCH 4/4] gdb/python: allow Frame.read_var to accept named arguments

[Eli Zaretskii]

> Cc: Andrew Burgess <aburgess@redhat.com>
> Date: Thu, 30 Mar 2023 13:10:23 +0100
> From: Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>
> 
> -@defun Frame.read_var (variable @r{[}, block@r{]})
> +@defun Frame.read_var (@var{variable} @r{[}, @var{block}@r{]})

And here.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Re: [PATCH 2/4] gdb/python: have PendingFrame methods accept keyword arguments

[Eli Zaretskii]

> Cc: Andrew Burgess <aburgess@redhat.com>
> Date: Thu, 30 Mar 2023 13:10:21 +0100
> From: Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>
> 
> diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
> index ef50e6dbe7b..4f62ca7d95e 100644
> --- a/gdb/doc/python.texi
> +++ b/gdb/doc/python.texi
> @@ -2756,11 +2756,11 @@
>  An object passed to an unwinder (a @code{gdb.PendingFrame} instance)
>  provides a method to read frame's registers:
>  
> -@defun PendingFrame.read_register (reg)
> -This method returns the contents of the register @var{reg} in the
> +@defun PendingFrame.read_register (@var{register})

Same issue with @var here.

> +This method returns the contents of the register @var{register} in the

It is usually much better to say "...contents of @var{register}".  It
avoids unnecessary tautology.

Thanks.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Re: [PATCH 0/4] Python API: Accept named arguments in a few more places

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

Andrew> As this function takes two arguments I though that accepting named
Andrew> arguments might be a nice improvement.

We actually document this.

       Functions and methods which have two or more optional arguments allow
    them to be specified using keyword syntax.  This allows passing some
    optional arguments while skipping others.  Example:
    'gdb.some_function ('foo', bar = 1, baz = 2)'.

Andrew> Then for completeness I updated the other two methods in py-unwind.c
Andrew> that also didn't accept named arguments, even though these functions
Andrew> only take a single argument I think taking named arguments is in
Andrew> general better, and consistency is good too.

It seems reasonable to me; certainly doesn't hurt anything.

This all looks fine to me.  Thanks for doing it.

Reviewed-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH 1/4] gdb/python: have UnwindInfo.add_saved_register accept named args

[Andrew Burgess]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: Andrew Burgess <aburgess@redhat.com>
>> Date: Thu, 30 Mar 2023 13:10:20 +0100
>> From: Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>
>> 
>> --- a/gdb/doc/python.texi
>> +++ b/gdb/doc/python.texi
>> @@ -2882,8 +2882,8 @@
>>  create a @code{gdb.UnwindInfo} instance.  Use the following method to
>>  specify caller registers that have been saved in this frame:
>>  
>> -@defun gdb.UnwindInfo.add_saved_register (reg, value)
>> -@var{reg} identifies the register, for a description of the acceptable
>> +@defun gdb.UnwindInfo.add_saved_register (@var{register}, @var{value})
>> +@var{register} identifies the register, for a description of the acceptable
>>  values see @ref{gdbpy_frame_read_register,,Frame.read_register}.
>>  @var{value} is a register value (a @code{gdb.Value} object).
>>  @end defun
>
> Why did you use @var inside the argument list on a @defun line?  That
> shouldn't be necessary, and the original text didn't use that.

I've removed the use of @var from this and all the patches in this
series.  This completely removes all doc changes from patches #3 and #4.

> Otherwise, this is okay (although the change just modifies the name of
> a single argument, so I'm not sure why we are doing this).  Thanks.

I've added a paragraph to the commit message that explains this change,
here's the summary:

This commit is adding support for Python named arguments, so a user can
do:

  obj.add_saved_register(register="...", value="...")

if they want.  I prefer 'register' rather than 'reg' in situations like
this.  The argument names GDB accepts has to match what's in the docs,
hence the change in patches #1 and #2.

Thanks,
Andrew

>
> Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Re: [PATCH 0/4] Python API: Accept named arguments in a few more places

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Andrew> As this function takes two arguments I though that accepting named
> Andrew> arguments might be a nice improvement.
>
> We actually document this.
>
>        Functions and methods which have two or more optional arguments allow
>     them to be specified using keyword syntax.  This allows passing some
>     optional arguments while skipping others.  Example:
>     'gdb.some_function ('foo', bar = 1, baz = 2)'.
>
> Andrew> Then for completeness I updated the other two methods in py-unwind.c
> Andrew> that also didn't accept named arguments, even though these functions
> Andrew> only take a single argument I think taking named arguments is in
> Andrew> general better, and consistency is good too.
>
> It seems reasonable to me; certainly doesn't hurt anything.
>
> This all looks fine to me.  Thanks for doing it.
>
> Reviewed-By: Tom Tromey <tom@tromey.com>

Pushed.

Thanks,
Andrew