Re: [PATCH] Fix crash in gdbpy_parse_register_id

[Simon Marchi <simark@simark.ca>]

> @@ -417,6 +423,8 @@ gdbpy_parse_register_id (struct gdbarch *gdbarch, PyObject *pyo_reg_id,
>  	PyErr_SetString (PyExc_ValueError,
>  			 _("Invalid Architecture in RegisterDescriptor"));
>      }
> +  else
> +    PyErr_SetString (PyExc_ValueError, _("Invalid type for register"));

I think this one should be a TypeError.  We could argue that it is an
API break to change the exception type thrown when passing an argument
with the wrong type, but given that it previously crashed GDB, I don't
think anybody is relying on it.

> diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
> index b2fd1402e93..e2cd67a0785 100644
> --- a/gdb/python/py-unwind.c
> +++ b/gdb/python/py-unwind.c
> @@ -262,10 +262,7 @@ unwind_infopy_add_saved_register (PyObject *self, PyObject *args)
>  			  &pyo_reg_id, &pyo_reg_value))
>      return NULL;
>    if (!gdbpy_parse_register_id (pending_frame->gdbarch, pyo_reg_id, &regnum))
> -    {
> -      PyErr_SetString (PyExc_ValueError, "Bad register");
> -      return NULL;
> -    }
> +    return NULL;
>  
>    /* If REGNUM identifies a user register then *maybe* we can convert this
>       to a real (i.e. non-user) register.  The maybe qualifier is because we
> @@ -383,10 +380,7 @@ pending_framepy_read_register (PyObject *self, PyObject *args)
>    if (!PyArg_UnpackTuple (args, "read_register", 1, 1, &pyo_reg_id))
>      return NULL;
>    if (!gdbpy_parse_register_id (pending_frame->gdbarch, pyo_reg_id, &regnum))
> -    {
> -      PyErr_SetString (PyExc_ValueError, "Bad register");
> -      return NULL;
> -    }
> +    PyErr_SetString (PyExc_ValueError, "Bad register");

It seems like you kept the wrong line here?  If this is really an error
and no test has caught this, it means we're lacking some coverage.

>  
>    try
>      {
> diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
> index d947b96033b..dffcd3f1b7f 100644
> --- a/gdb/python/python-internal.h
> +++ b/gdb/python/python-internal.h
> @@ -799,7 +799,8 @@ typedef std::unique_ptr<Py_buffer, Py_buffer_deleter> Py_buffer_up;
>  
>     If a register is parsed successfully then *REG_NUM will have been
>     updated, and true is returned.  Otherwise the contents of *REG_NUM are
> -   undefined, and false is returned.
> +   undefined, and false is returned.  When false is returned, the
> +   Python error is set.
>  
>     The PYO_REG_ID object can be a string, the name of the register.  This
>     is the slowest approach as GDB has to map the name to a number for each
> diff --git a/gdb/testsuite/gdb.python/py-frame.exp b/gdb/testsuite/gdb.python/py-frame.exp
> index b91ffe62a83..881219342e3 100644
> --- a/gdb/testsuite/gdb.python/py-frame.exp
> +++ b/gdb/testsuite/gdb.python/py-frame.exp
> @@ -128,3 +128,8 @@ if { $pc != "" } {
>  	" = True" \
>  	"test Frame.read_register($pc)"
>  }
> +
> +# This previously caused a crash.

This comment is a bit too broad, can you make it a bit more precise?
What is it we are testing exactly?  I suppose "passing an object with an
unexpected type to read_register".

Simon