Re: [patch][python] 1 of 5 - Frame filter Python C code changes.

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:

Phil> This email and patch covers the python/ changes for Python Frame Filters.

Thanks.

Phil> diff --git a/gdb/python/lib/gdb/FrameIterator.py b/gdb/python/lib/gdb/FrameIterator.py
...
Phil> +class FrameIterator(object):
...
Phil> +    def __getitem__(self, index):
Phil> +        """__getitem__ implementation.
Phil> +
Phil> +        Arguments:
Phil> +            index: A specific index to fetch."""
Phil> +
Phil> +        return next(itertools.islice(self.frame, index, index+1))

I don't think this will actually work.
self.frame isn't an iterator, so I don't think islice will do the right
thing here.

I don't think this method is needed.  Iterators only really need 'next'
(though supplying __iter__ is also nice).

Phil> +    if hasattr(filter_item, "priority"):
Phil> +        return filter_item.priority
Phil> +    else:
Phil> +        return 0

You can use getattr(filter_item, "priority", 0) here

Phil> +    if hasattr(filter_item, "enabled"):
Phil> +        return filter_item.enabled
Phil> +    else:
Phil> +        return False

getattr here too.

Phil> +static enum py_bt_status
Phil> +extract_sym (PyObject *obj, char **name, struct symbol **sym,
Phil> +	     const struct language_defn **language)

It occurs to me now that this return value convention is going to make
this code less amenable to analysis with the CPython checker.

Hmm, maybe it would work ok if we set PY_BT_ERROR = -1.  Then we could
use the "negative means failure" attribute.

Phil> +static enum py_bt_status
Phil> +extract_value (PyObject *obj, struct value **value)
...
Phil> +	  return 1;

Should be one of the enum values, I guess PY_BT_OK.

Phil> +static enum py_bt_status
Phil> +py_print_value (struct ui_out *out, struct value *val,

Many of the callers of this make assumptions about the return value,
e.g.:

+      if (! py_print_type (out, val))
+	goto error;

These have to be checks against enum constants.
The same goes for py_print_value and py_print_single_arg and more.
I feel like this has come up in every review.

Phil> +  /* Never set an indent level for common_val_print if MI.  */
Phil> +  if (ui_out_is_mi_like_p (out))
Phil> +      local_indent = 0;

The second line here is intended too much.

Phil> +  /* MI does not print certain values, differentiated by type,
Phil> +     depending on what ARGS_TYPE indicates.  Test type against option.
Phil> +     For CLI print all values.  */
Phil> +  if (args_type == MI_PRINT_SIMPLE_VALUES
Phil> +      || args_type == MI_PRINT_ALL_VALUES)
Phil> +
Phil> +    {

Extra blank line here.

Phil> +static enum py_bt_status
Phil> +py_print_single_arg (struct ui_out *out,
...
Phil> +  /*  MI has varying rules for tuples, but generally if there is only
Phil> +      one element in each item in the list, do not start a tuple.  The
Phil> +      exception is -stack-list-variables which emits an ARGS="1" field
Phil> +      if the value is a frame argument.  This is denoted in this
Phil> +      function with PRINT_ARGS_FIELD which is flag from the caller to
Phil> +      emit the ARGS field.  */
Phil> +  if (ui_out_is_mi_like_p (out))
Phil> +    {
Phil> +      if (print_args_field || args_type != NO_VALUES)
Phil> +	make_cleanup_ui_out_tuple_begin_end (out, NULL);
Phil> +    }
Phil> +
Phil> +    TRY_CATCH (except, RETURN_MASK_ALL)
Phil> +      {

I think the make_cleanup_ui_out_tuple_begin_end call should probably go
inside this TRY_CATCH.

Phil> +static enum py_bt_status
Phil> +enumerate_locals (PyObject *iter,
...
Phil> +      make_cleanup (null_cleanup, NULL);

I don't think this is needed.

Phil> +      do_cleanups (cleanups);
Phil> +    }

I think this call is wrong.  In particular if the loop runs more than
once, it will try to re-do a cleanup that was already run.

Maybe you meant to introduce a new inner cleanup in the loop, and that
was what the null_cleanup was for.

Phil> +  if (item == NULL && PyErr_Occurred ())
Phil> +    goto error;
Phil> +
Phil> +  return PY_BT_OK;
Phil> +
Phil> + error:
Phil> +  do_cleanups (cleanups);
Phil> +  return PY_BT_ERROR;

Similarly, this should do the cleanups along both branches, or neither
branch.

Phil> +static enum py_bt_status
Phil> +py_print_frame (PyObject *filter, int flags, enum py_frame_args args_type,
...
Phil> +  /* stack-list-variables.  */
Phil> +  if (print_locals && print_args && ! print_frame_info)
Phil> +    {
Phil> +      if (! py_mi_print_variables (filter, out, &opts,
Phil> +				   args_type, frame))
Phil> +	goto error;
Phil> +      else
Phil> +	return PY_BT_COMPLETED;
Phil> +    }

This early return has to run the cleanups.

Phil> +	  int success = py_print_frame (item, flags, args_type, out, indent,
Phil> +					 levels_printed);
Phil> +
Phil> +	  if (success == PY_BT_ERROR && PyErr_Occurred ())

It seems like it should be impossible to see PY_BT_ERROR and not have
PyErr_Occurred be true.  If it can happen then it seems like that is an
API contract violation.

Phil> +int
Phil> +apply_frame_filter (struct frame_info *frame, int flags,

Seems like this should return enum py_bt_status.
It needs to be updated in a few spots.

Tom