Re: [RFA] Python: raise exception on field-related gdb.Type methods if it's not struct or union

[Doug Evans <dje@google.com>]

On Tue, Nov 8, 2011 at 1:40 PM, Paul Koning <paulkoning@comcast.net> wrote:
>
> On Nov 5, 2011, at 5:04 PM, Doug Evans wrote:
>
>> On Sat, Nov 5, 2011 at 7:36 AM, Paul Koning <paulkoning@comcast.net> wrote:
>>>> Maybe I'd be happy if gdb.Type (and maybe gdb.Value) were simply more
>>>> rigorous in throwing exceptions for invalid cases.
>>>
>>> I agree.  I'll put that together.
>>
>> Sounds great if it's possible.
>>
>> I don't know enough about the C Python API, but when I tried a simple
>> hack to have len(scalar_type) throw an exception "not scalar_type"
>> started throwing exceptions too.  :-(  See PyObject_IsTrue.
>
> I found the answer to that question with a bit of searching.   Attached is a patch that raises an exception (TypeError) for len() and all the field reference methods if the type isn't struct or union.

I wasn't sure using as_number here was kosher.  "works for me" if it
does the job.

> Does this need new testcases?

Yeah, I think so.
Verify "not type" DTRT, and affected operations on scalar types flag
the right error.

One question I have is: for struct types that have an empty field list
(say an empty struct), what's the result of "not type".  True or
False?  I can argue either way, and I'm not sure what's the best
answer, long term.
[But then again, my python fu isn't enough to be comfortable with any
conclusion I reach.]

Thanks very much for doing this!

btw, a couple of nits:
1) Please put a blank line between a function's comment and its definition.
There are several occurrences of this.
2) Can you rename typy_deref?  typy_get_struct works for me, I'm sure
there's a better name.
    "deref" feels confusing, one can deref a pointer to anything, not
just structs/unions, and
    the function doesn't necessarily do a dereference.


>        paul
>
> ChangeLog:
>
> 2011-11-08  Paul Koning  <paul_koning@dell.com>
>
>        * python/py-type.c (typy_deref): New function.
>        (typy_nonzero): New function.
>        (typy_length): Raise exception if not struct or union type.
>        (typy_getitem): Ditto.
>        (typy_has_key): Ditto.
>        (typy_make_iter): Ditto.
>
> Index: python/py-type.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/python/py-type.c,v
> retrieving revision 1.28
> diff -u -p -r1.28 py-type.c
> --- python/py-type.c    4 Nov 2011 11:57:04 -0000       1.28
> +++ python/py-type.c    8 Nov 2011 21:36:40 -0000
> @@ -350,6 +350,45 @@ typy_strip_typedefs (PyObject *self, PyO
>   return type_to_type_object (check_typedef (type));
>  }
>
> +/* Strip typedefs and pointers/reference from a type.  Then check that
> +   it is a struct or union type.  If not, raise TypeError.  */
> +static struct type *
> +typy_deref (struct type *type)
> +{
> +  volatile struct gdb_exception except;
> +
> +  for (;;)
> +    {
> +      TRY_CATCH (except, RETURN_MASK_ALL)
> +               {
> +         CHECK_TYPEDEF (type);
> +       }
> +      /* Don't use GDB_PY_HANDLE_EXCEPTION here because that returns
> +        a (NULL) pointer of the wrong type.  */
> +      if (except.reason < 0)
> +       {
> +         gdbpy_convert_exception (except);
> +         return NULL;
> +       }
> +
> +      if (TYPE_CODE (type) != TYPE_CODE_PTR
> +         && TYPE_CODE (type) != TYPE_CODE_REF)
> +       break;
> +      type = TYPE_TARGET_TYPE (type);
> +    }
> +
> +  /* If this is not a struct or union type, raise TypeError exception.  */
> +  if (TYPE_CODE (type) != TYPE_CODE_STRUCT
> +      && TYPE_CODE (type) != TYPE_CODE_UNION)
> +    {
> +      PyErr_SetString (PyExc_TypeError,
> +                      "Type is not a structure or union type.");
> +      return NULL;
> +    }
> +
> +  return type;
> +}
> +
>  /* Return an array type.  */
>
>  static PyObject *
> @@ -1124,9 +1163,22 @@ typy_length (PyObject *self)
>  {
>   struct type *type = ((type_object *) self)->type;
>
> +  type = typy_deref (type);
> +  if (type == NULL)
> +    return -1;
> +
>   return TYPE_NFIELDS (type);
>  }
>
> +/* Implements boolean evaluation of gdb.Type.  Handle this like other
> +   Python objects that don't have a meaningful truth value -- all
> +   values are true.  */
> +static int
> +typy_nonzero (PyObject *self)
> +{
> +  return 1;
> +}
> +
>  /* Return a gdb.Field object for the field named by the argument.  */
>
>  static PyObject *
> @@ -1145,20 +1197,10 @@ typy_getitem (PyObject *self, PyObject *
>      using lookup_struct_elt_type, portions of that function are
>      copied here.  */
>
> -  for (;;)
> -    {
> -      TRY_CATCH (except, RETURN_MASK_ALL)
> -       {
> -         CHECK_TYPEDEF (type);
> -       }
> -      GDB_PY_HANDLE_EXCEPTION (except);
> -
> -      if (TYPE_CODE (type) != TYPE_CODE_PTR
> -         && TYPE_CODE (type) != TYPE_CODE_REF)
> -       break;
> -      type = TYPE_TARGET_TYPE (type);
> -    }
> -
> +  type = typy_deref (type);
> +  if (type == NULL)
> +    return NULL;
> +
>   for (i = 0; i < TYPE_NFIELDS (type); i++)
>     {
>       char *t_field_name = TYPE_FIELD_NAME (type, i);
> @@ -1216,18 +1258,9 @@ typy_has_key (PyObject *self, PyObject *
>      using lookup_struct_elt_type, portions of that function are
>      copied here.  */
>
> -  for (;;)
> -    {
> -      TRY_CATCH (except, RETURN_MASK_ALL)
> -       {
> -         CHECK_TYPEDEF (type);
> -       }
> -      GDB_PY_HANDLE_EXCEPTION (except);
> -      if (TYPE_CODE (type) != TYPE_CODE_PTR
> -         && TYPE_CODE (type) != TYPE_CODE_REF)
> -       break;
> -      type = TYPE_TARGET_TYPE (type);
> -    }
> +  type = typy_deref (type);
> +  if (type == NULL)
> +    return NULL;
>
>   for (i = 0; i < TYPE_NFIELDS (type); i++)
>     {
> @@ -1246,6 +1279,10 @@ typy_make_iter (PyObject *self, enum gdb
>  {
>   typy_iterator_object *typy_iter_obj;
>
> +  /* Check that "self" is a structure or union type.  */
> +  if (typy_deref (((type_object *) self)->type) == NULL)
> +    return NULL;
> +
>   typy_iter_obj = PyObject_New (typy_iterator_object,
>                                &type_iterator_object_type);
>   if (typy_iter_obj == NULL)
> @@ -1499,6 +1536,32 @@ Return a volatile variant of this type"
>   { NULL }
>  };
>
> +static PyNumberMethods type_object_as_number = {
> +  NULL,                              /* nb_add */
> +  NULL,                              /* nb_subtract */
> +  NULL,                              /* nb_multiply */
> +  NULL,                              /* nb_divide */
> +  NULL,                              /* nb_remainder */
> +  NULL,                              /* nb_divmod */
> +  NULL,                              /* nb_power */
> +  NULL,                              /* nb_negative */
> +  NULL,                              /* nb_positive */
> +  NULL,                              /* nb_absolute */
> +  typy_nonzero,                      /* nb_nonzero */
> +  NULL,                              /* nb_invert */
> +  NULL,                              /* nb_lshift */
> +  NULL,                              /* nb_rshift */
> +  NULL,                              /* nb_and */
> +  NULL,                              /* nb_xor */
> +  NULL,                              /* nb_or */
> +  NULL,                              /* nb_coerce */
> +  NULL,                              /* nb_int */
> +  NULL,                              /* nb_long */
> +  NULL,                              /* nb_float */
> +  NULL,                              /* nb_oct */
> +  NULL                       /* nb_hex */
> +};
> +
>  static PyMappingMethods typy_mapping = {
>   typy_length,
>   typy_getitem,
> @@ -1518,7 +1581,7 @@ static PyTypeObject type_object_type =
>   0,                             /*tp_setattr*/
>   0,                             /*tp_compare*/
>   0,                             /*tp_repr*/
> -  0,                             /*tp_as_number*/
> +  &type_object_as_number,        /*tp_as_number*/
>   0,                             /*tp_as_sequence*/
>   &typy_mapping,                 /*tp_as_mapping*/
>   0,                             /*tp_hash */
>
>