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

[Phil Muldoon <pmuldoon@redhat.com>]

On 06/10/2010 07:40 PM, Tom Tromey wrote:
>>>>>> "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.

Me either ...

> 
> 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.

I've no complaint to using obstacks.  This function basically
wraps/tidies the existing code that was just coded directly in a loop
in parse_find_args.  That code just realloc'd by a factor of two whenever
the buffer was too small.  This code is exactly the same, except it
has been squirrelled away in a function.  So we are not introducing or
adding any more growable types in this patch, just moving the code
bits that already existed into function.  I'm not adverse to changing
that code to use obstacks, that being said!

> 
> 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.

Ok, missed this!


> 
> 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.


Do you mean using the Python object's cmp inbuilt method here?


> 
> 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.

Ok.

> 
> 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.

I'll investigate this further. I had concerns about this (I think we
chatted a little about this on irc way back when).  I'll write an
inferior test and see.  Maybe Pedro or someone else in that area knows
a little more.


Cheers

Phil