[RFA,1/2] Check results of get_darwin_inferior

  I noticed that the existing kill-detach-inferiors-cmd.exp test was
causing gdb to crash on macOS 10.13.  The bug was that an inferior
that hadn't yet been started would cause get_darwin_inferior to return
NULL, and this was not checked.

I went through the places calling get_darwin_inferior and added checks
where appropriate.  This makes the test get a bit further.  Not all of
these spots are exercised by the test, but they seem safe enough in
any case.

gdb/ChangeLog
2018-07-13  Tom Tromey  <tom@tromey.com>

	* darwin-nat.c (find_inferior_task_it, darwin_find_thread)
	(darwin_suspend_inferior, darwin_resume_inferior)
	(darwin_decode_notify_message, darwin_resume_inferior_threads)
	(darwin_check_new_threads): Check result of get_darwin_inferior.
---
 gdb/ChangeLog    |  7 +++++++
 gdb/darwin-nat.c | 35 +++++++++++++++++++++--------------
 2 files changed, 28 insertions(+), 14 deletions(-)
  

Le 7/13/18 à 10:44 PM, Tom Tromey a écrit :
> I noticed that the existing kill-detach-inferiors-cmd.exp test was
> causing gdb to crash on macOS 10.13.  The bug was that an inferior
> that hadn't yet been started would cause get_darwin_inferior to return
> NULL, and this was not checked.
> 
> I went through the places calling get_darwin_inferior and added checks
> where appropriate.  This makes the test get a bit further.  Not all of
> these spots are exercised by the test, but they seem safe enough in
> any case.
> 
> gdb/ChangeLog
> 2018-07-13  Tom Tromey  <tom@tromey.com>
> 
> 	* darwin-nat.c (find_inferior_task_it, darwin_find_thread)
> 	(darwin_suspend_inferior, darwin_resume_inferior)
> 	(darwin_decode_notify_message, darwin_resume_inferior_threads)
> 	(darwin_check_new_threads): Check result of get_darwin_inferior.
> ---
>   gdb/ChangeLog    |  7 +++++++
>   gdb/darwin-nat.c | 35 +++++++++++++++++++++--------------
>   2 files changed, 28 insertions(+), 14 deletions(-)
> 
> diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
> index bb46cfbfcfa..6d7b80ddcb1 100644
> --- a/gdb/darwin-nat.c
> +++ b/gdb/darwin-nat.c
> @@ -259,6 +259,9 @@ darwin_check_new_threads (struct inferior *inf)
>     darwin_inferior *darwin_inf = get_darwin_inferior (inf);
>     std::vector<darwin_thread_t *> new_thread_vec;
>   
> +  if (darwin_inf == nullptr)
> +    return;

Wouldn't return NULL be better here ?

> +
>     /* Get list of threads.  */
>     kret = task_threads (darwin_inf->task, &thread_list, &new_nbr);
>     MACH_CHECK_ERROR (kret);
> @@ -374,7 +377,7 @@ find_inferior_task_it (struct inferior *inf, void *port_ptr)
>   {
>     darwin_inferior *priv = get_darwin_inferior (inf);
>   
> -  return priv->task == *(task_t *)port_ptr;
> +  return priv != nullptr && priv->task == *(task_t *)port_ptr;
>   }
>   
>   static int
> @@ -403,11 +406,12 @@ darwin_find_thread (struct inferior *inf, thread_t thread)
>   {
>     darwin_inferior *priv = get_darwin_inferior (inf);
>   
> -  for (darwin_thread_t *t : priv->threads)
> -    {
> -      if (t->gdb_port == thread)
> -	return t;
> -    }
> +  if (priv != nullptr)
> +    for (darwin_thread_t *t : priv->threads)
> +      {
> +	if (t->gdb_port == thread)
> +	  return t;
> +      }
>   
>     return NULL;
>   }
> @@ -419,7 +423,7 @@ darwin_suspend_inferior (struct inferior *inf)
>   {
>     darwin_inferior *priv = get_darwin_inferior (inf);
>   
> -  if (!priv->suspended)
> +  if (priv != nullptr && !priv->suspended)
>       {
>         kern_return_t kret;
>   
> @@ -437,7 +441,7 @@ darwin_resume_inferior (struct inferior *inf)
>   {
>     darwin_inferior *priv = get_darwin_inferior (inf);
>   
> -  if (priv->suspended)
> +  if (priv != nullptr && priv->suspended)
>       {
>         kern_return_t kret;
>   
> @@ -766,11 +770,13 @@ darwin_decode_notify_message (mach_msg_header_t *hdr, struct inferior **pinf)
>     inf = darwin_find_inferior_by_task (task_port);
>     *pinf = inf;
>   
> -  darwin_inferior *priv = get_darwin_inferior (inf);
> -
>     /* Check message destination.  */
> -  if (inf != NULL && hdr->msgh_local_port != priv->notify_port)
> -    return -4;
> +  if (inf != NULL)
> +    {
> +      darwin_inferior *priv = get_darwin_inferior (inf);
> +      if (hdr->msgh_local_port != priv->notify_port)
> +	return -4;
> +    }
>   
>     return 0;
>   }
> @@ -882,8 +888,9 @@ darwin_resume_inferior_threads (struct inferior *inf, int step, int nsignal)
>   {
>     darwin_inferior *priv = get_darwin_inferior (inf);
>   
> -  for (darwin_thread_t *thread : priv->threads)
> -    darwin_resume_thread (inf, thread, step, nsignal);
> +  if (priv != nullptr)
> +    for (darwin_thread_t *thread : priv->threads)
> +      darwin_resume_thread (inf, thread, step, nsignal);
>   }
>   
>   struct resume_inferior_threads_param
> 

Except minor return question in darwin_check_new_threads function, LGTM.

Xavier
  

>>>>> "Xavier" == Xavier Roirand <roirand@adacore.com> writes:

>> diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
>> index bb46cfbfcfa..6d7b80ddcb1 100644
>> --- a/gdb/darwin-nat.c
>> +++ b/gdb/darwin-nat.c
>> @@ -259,6 +259,9 @@ darwin_check_new_threads (struct inferior *inf)
>> darwin_inferior *darwin_inf = get_darwin_inferior (inf);
>> std::vector<darwin_thread_t *> new_thread_vec;
>> +  if (darwin_inf == nullptr)
>> +    return;

Xavier> Wouldn't return NULL be better here ?

In git, darwin_check_new_threads returns void.
Maybe it's different in your tree?

Tom
  

Le 8/2/18 à 9:30 PM, Tom Tromey a écrit :
>>>>>> "Xavier" == Xavier Roirand <roirand@adacore.com> writes:
> 
>>> diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
>>> index bb46cfbfcfa..6d7b80ddcb1 100644
>>> --- a/gdb/darwin-nat.c
>>> +++ b/gdb/darwin-nat.c
>>> @@ -259,6 +259,9 @@ darwin_check_new_threads (struct inferior *inf)
>>> darwin_inferior *darwin_inf = get_darwin_inferior (inf);
>>> std::vector<darwin_thread_t *> new_thread_vec;
>>> +  if (darwin_inf == nullptr)
>>> +    return;
> 
> Xavier> Wouldn't return NULL be better here ?
> 
> In git, darwin_check_new_threads returns void.
> Maybe it's different in your tree?

Agree, return only is fine :)

> 
> Tom
>