Re: [PATCH 3/4] Change thread_to_thread_object to return a new reference

Simon Marchi
2018-09-17T00:52:34Z

On 2018-09-16 10:05, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
>
>>> + if (result == NULL)
>>> + result = gdbpy_ref<>::new_reference (Py_None);
>
> Simon> I would suggest using Py_RETURN_NONE, which we already use at
> many places.
>
> Simon> Instead of setting that result variable, is there something
> preventing you
> Simon> from returning directly above, like this?
>
> Simon> if (thread_info != NULL)
> Simon> return thread_to_thread_object (thread_info).release ();
>
> Simon> The bottom line could just be unconditionally Py_RETURN_NONE.
>
> I made this change.
>
> In the past you couldn't return from inside a try/catch but I think it
> is ok now.
>
>
> I couldn't use Py_RETURN_NONE in py_get_event_thread because it returns
> a gdbpy_ref<> -- and I think the latter is more important.

Oh right. While working on this code I though adding a
GDB_Py_REF_RETURN_NONE macro, which would be like Py_RETURN_NONE but
returning a gdbpy_ref. Not sure it's really that useful, since what you
wrote is explicit and clear.

> I re-read all of this and I think thread_to_thread_object has a latent
> bug. It can return NULL early due to an error:
>
> gdbpy_ref<inferior_object> inf_obj (inferior_to_inferior_object
> (thr->inf));
> if (inf_obj == NULL)
> return NULL;
>
> or it can return NULL at the end meaning "thread not found".

I am not sure how these two mode of failure differ. Don't we expect the
passed thread_info object to always be valid, and therefore both cases
returning NULL would be an internal GDB logic error?

> I think it is best to have a single style - returning NULL should also
> set the Python exception.

That makes sense, so the caller can just check for NULL and return NULL
right away (as the snippet above does). We should expect
inferior_to_inferior_object to set the exception on failure.

> It seems to me that the second NULL return should just set an
> exception. It's not a normal case.

Right.

Responding directly to your next message:

> Maybe the gdbpy_ref (1-argument) constructor and release methods could
> assert that the Python exception is set if the underlying pointer is
> NULL. That would not get full checking but maybe it would catch some
> problems. And maybe we should simply use gdbpy_ref in many more places
> in the Python layer -- ideally, reserve raw pointers solely for
> parameters which are borrowed references.

Makes sense.

> Let me know what you think of this.
>
> My apologies again for missing this review and pushing too eagerly.

No problem, LGTM.

Simon