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

[Doug Evans <dje at google dot 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 */
>
>