[patch][python] PR python/16324 (frame_id_eq check invalid)

[patch][python] PR python/16324 (frame_id_eq check invalid)

[Phil Muldoon]

This patch removes an invalid call to frame_id_eq.  As the bug
reporter noted, a frame_id_eq on two null_frame_ids will always return
False (not True as expected in code).  Add an assert over a Python
error because this scenario is fatal (a null_frame_id should never be the
result from a valid frame).  A null_frame_id can be returned if the
frame fetching function was passed a NULL (already checked earlier in
the code), or if the frame is the SENTINEL (which it can't be, we are
traversing the opposite way to encounter that).

In the case of this check, GDB itself will have somehow corrupted the
frame stack, in which case, the only thing to do is exit.  I normally
don't do this level of paranoia checking, but I wanted to respect the
original author's intention.

OK?

Cheers,

Phil

--

2014-02-26  Phil Muldoon  <pmuldoon@redhat.com>

    PR python/16324
    * python/py-finishbreakpoint.c (bpfinishpy_init): Assert on
    frame_id_p failing from a valid frame.  Remove old frame_id_eq
    check.

diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
index 712a9ee..15952e3 100644
--- a/gdb/python/py-finishbreakpoint.c
+++ b/gdb/python/py-finishbreakpoint.c
@@ -207,9 +207,12 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
       else
         {
           frame_id = get_frame_id (prev_frame);
-          if (frame_id_eq (frame_id, null_frame_id))
-        PyErr_SetString (PyExc_ValueError,
-                 _("Invalid ID for the `frame' object."));
+
+          /* If prev_frame != NULL, and get_frame_id cannot return
+         the frame_id, there is nothing more to be done.  At
+         this point, assert that a valid frame id is
+         returned.  */
+          gdb_assert (frame_id_p (frame_id) != 0);
         }
     }
     }

Re: [patch][python] PR python/16324 (frame_id_eq check invalid)

[Pedro Alves]

On 02/26/2014 08:49 AM, Phil Muldoon wrote:

> +
> +          /* If prev_frame != NULL, and get_frame_id cannot return
> +         the frame_id, there is nothing more to be done.  At
> +         this point, assert that a valid frame id is
> +         returned.  */

A nit (sorry!) - I find this "there is nothing more to be
done" in the comment a little bit confusing/distracting.  That
reads to me like a comment we'd write when doing a regular
early return, because whatever the function was supposed to
had already been done.

You had a good rationale in the intro.  I'd suggest basing on it,
saying this instead:

          /* All frames other than the sentinel (which PREV_FRAME can't
	     be, as we are traversing the opposite direction)
	     must have a valid id.  If we get back a null_frame_id with
	     prev_frame != NULL, something is messed up.  */

> +          gdb_assert (frame_id_p (frame_id) != 0);

Please write:

          gdb_assert (frame_id_p (frame_id));

The '_p' suffix stands for predicate, indicating the function
returns a boolean, and for those, we use 'if (foo())' / 'if (!foo())'.

OK with that change.

Thanks,
-- 
Pedro Alves