Re: [python][patch] Inferior and Thread information support.

[Tom Tromey <tromey@redhat.com>]

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

Phil> This patch adds Python support to inferiors and threads.

Sorry for the delay in this review.

Phil>  /* Copied from bfd_put_bits.  */
Phil> -static void
Phil> +void
Phil>  put_bits (bfd_uint64_t data, char *buf, int bits, bfd_boolean big_p)

I don't think you need this change.  Use store_unsigned_integer instead.

I don't understand why this function exists at all.

Phil> +void
Phil> +allocate_pattern_buffer (char **pattern_buf, char **pattern_buf_end,
Phil> +			 ULONGEST *pattern_buf_size)

Phil> +void
Phil> +increase_pattern_buffer (char **pattern_buf, char **pattern_buf_end,
Phil> +			 ULONGEST *pattern_buf_size, int val_bytes)

I think this stuff can be easily done with obstacks.  I prefer we not
add more growable types in situations where we can reuse the ones we
already have.

Phil> +  /* While creating new inferior no inferior thread is available.
Phil> +     Therefore get_current_arch has no valid current frame (and it
Phil> +     would crash).  */
Phil> +  cleanup = ensure_python_env (target_gdbarch, current_language);

You should use python_gdbarch and python_language here.
This occurs a couple of times.

Phil> +/* An observer callback function that is called when an inferior has
Phil> +   been deleted.  Removes the corresponding Python object from the
Phil> +   inferior list, and removes the list's reference to the object.  */
Phil> +static void
Phil> +delete_inferior_object (struct inferior *inf)
Phil> +{
Phil> +  struct cleanup *cleanup;
Phil> +  struct inflist_entry **inf_entry, *inf_tmp;
Phil> +  struct threadlist_entry *th_entry, *th_tmp;
Phil> +
Phil> +  /* Find inferior_object for the given PID.  */
Phil> +  for (inf_entry = &gdbpy_inferior_list; *inf_entry != NULL;
Phil> +       inf_entry = &(*inf_entry)->next)
Phil> +    if ((*inf_entry)->inf_obj->inferior->pid == inf->pid)
Phil> +      break;

It seems strange to compare the pid fields when we could just compare
the inferior objects themselves.

Phil> +/* Implementation of Inferior.frames () -> (gdb.Frame, ...).
Phil> +   Returns a tuple of all frame objects.  */
Phil> +PyObject *
Phil> +thpy_frames (PyObject *self, PyObject *args)
[...]

Phil> +      for (frame = get_current_frame (); frame;
Phil> +	   frame = get_prev_frame (frame))
Phil> +	{
Phil> +	  frame_obj = frame_info_to_frame_object (frame);
Phil> +	  if (frame_obj == NULL)
Phil> +	    {

I don't think this is wise.  It is not uncommon for a crash to cause a
thread to have thousands of frames.

Hm, maybe there is no way to return a frame-in-a-thread and then be able
to iterate.  IOW, a gdb internals limitation.  That is unfortunate (if
true) but I don't think it is a reason for us to go with this API.

One short-term solution would be to get rid of this method.

Phil> +/* Implementation of InferiorThread.newest_frame () -> gdb.Frame.
Phil> +   Returns the newest frame object.  */
Phil> +PyObject *
Phil> +thpy_newest_frame (PyObject *self, PyObject *args)
Phil> +{
Phil> +  struct frame_info *frame;
Phil> +  PyObject *frame_obj = NULL;   /* Initialize to appease gcc warning.  */
Phil> +  thread_object *thread_obj = (thread_object *) self;
Phil> +  struct cleanup *cleanup;
Phil> +  volatile struct gdb_exception except;
Phil> +
Phil> +  THPY_REQUIRE_VALID (thread_obj);
Phil> +
Phil> +  cleanup = make_cleanup_restore_current_thread ();
Phil> +
Phil> +  TRY_CATCH (except, RETURN_MASK_ALL)
Phil> +    {
Phil> +      switch_to_thread (thread_obj->thread->ptid);
Phil> +
Phil> +      frame = get_current_frame ();
Phil> +      frame_obj = frame_info_to_frame_object (frame);
Phil> +    }
Phil> +  GDB_PY_HANDLE_EXCEPTION (except);

I am really not sure about this.

Doesn't switch_to_thread reset the frame cache?
Meaning that the returned frame_obj will immediately be invalid?

You would have to try this with a multi-threaded program, where you are
stopped in thread A but then request a frame in thread B.

Phil> +void
Phil> +gdbpy_initialize_thread (void)
Phil> +{
Phil> +
Phil> +  if (PyType_Ready (&thread_object_type) < 0)

Spurious blank line.

Tom