[RFA,1/2] Check results of get_darwin_inferior
Commit Message
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(-)
Comments
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
>
@@ -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;
+
/* 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