[PATCH] Fix assert failure in invocation of "[kill|detach] inferiors 1"

[PATCH] Fix assert failure in invocation of "[kill|detach] inferiors 1"

[Patrick Palka]

When only the dummy PID 0 inferior exists, invoking either of the above
commands triggers the following assert failure:

  .../binutils-gdb/gdb/thread.c:514: internal-error: any_thread_of_process: Assertion `pid != 0' failed.

The fix is straightforward.  Tested on x86_64 Debian Stretch.

gdb/ChangeLog:

	* inferior.c (detach_inferior_command): Don't call
	any_thread_of_process when pid is 0.
	(kill_inferior_command): Likewise.

gdb/testsuite/ChangeLog:

	* gdb.base/no-inferiors.exp: New test file.
---
 gdb/inferior.c                          | 12 ++++++++++--
 gdb/testsuite/gdb.base/no-inferiors.exp | 24 ++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 2 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/no-inferiors.exp

diff --git a/gdb/inferior.c b/gdb/inferior.c
index 5e98df5..ff85a1f 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -626,7 +626,11 @@ detach_inferior_command (char *args, int from_tty)
 
       pid = gdb_inferior_id_to_pid (num);
 
-      tp = any_thread_of_process (pid);
+      if (pid != 0)
+	tp = any_thread_of_process (pid);
+      else
+	tp = 0;
+
       if (!tp)
 	{
 	  warning (_("Inferior ID %d has no threads."), num);
@@ -662,7 +666,11 @@ kill_inferior_command (char *args, int from_tty)
 
       pid = gdb_inferior_id_to_pid (num);
 
-      tp = any_thread_of_process (pid);
+      if (pid != 0)
+	tp = any_thread_of_process (pid);
+      else
+	tp = 0;
+
       if (!tp)
 	{
 	  warning (_("Inferior ID %d has no threads."), num);
diff --git a/gdb/testsuite/gdb.base/no-inferiors.exp b/gdb/testsuite/gdb.base/no-inferiors.exp
new file mode 100644
index 0000000..2ccf4ce
--- /dev/null
+++ b/gdb/testsuite/gdb.base/no-inferiors.exp
@@ -0,0 +1,24 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2015 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Check that invoking various commands that take an inferior ID does not cause
+# an internal error when only the dummy inferior with PID 0 exists.
+
+gdb_start
+
+gdb_test "detach inferiors 1" "warning: Inferior ID 1 has no threads."
+gdb_test "kill inferiors 1" "warning: Inferior ID 1 has no threads."
-- 
2.5.0.rc2.22.g69b1679.dirty

Re: [PATCH] Fix assert failure in invocation of "[kill|detach] inferiors 1"

[Pedro Alves]

On 07/28/2015 09:29 PM, Patrick Palka wrote:
> When only the dummy PID 0 inferior exists, invoking either of the above
> commands triggers the following assert failure:
> 
>   .../binutils-gdb/gdb/thread.c:514: internal-error: any_thread_of_process: Assertion `pid != 0' failed.
> 
> The fix is straightforward.  Tested on x86_64 Debian Stretch.

Thanks.

> 
> gdb/ChangeLog:
> 
> 	* inferior.c (detach_inferior_command): Don't call
> 	any_thread_of_process when pid is 0.
> 	(kill_inferior_command): Likewise.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.base/no-inferiors.exp: New test file.
> ---
>  gdb/inferior.c                          | 12 ++++++++++--
>  gdb/testsuite/gdb.base/no-inferiors.exp | 24 ++++++++++++++++++++++++
>  2 files changed, 34 insertions(+), 2 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.base/no-inferiors.exp
> 
> diff --git a/gdb/inferior.c b/gdb/inferior.c
> index 5e98df5..ff85a1f 100644
> --- a/gdb/inferior.c
> +++ b/gdb/inferior.c
> @@ -626,7 +626,11 @@ detach_inferior_command (char *args, int from_tty)
>  
>        pid = gdb_inferior_id_to_pid (num);
>  
> -      tp = any_thread_of_process (pid);
> +      if (pid != 0)
> +	tp = any_thread_of_process (pid);
> +      else
> +	tp = 0;

Should be tp = NULL.

This is old code that predates the multi-executable support
(6c95b8df7fef), and was never really updated since.  Guess
not many people use it...

Doesn't make that much sense to say "has no threads"
nowadays.  I think this should give out a warning similar
to the error "detach_command" throws, like:

+      if (pid == 0)
+        {
+          warning (_("Inferior ID %s is not being run."), num);
+          continue;
+        }

      tp = any_thread_of_process (pid);
      if (!tp)
	{
	  warning (_("Inferior ID %d has no threads."), num);
	  continue;
	}

The "has no threads" case is then more of an internal error,
but there's no need to be so drastic, so leave it as is.

(Likewise for kill, of course.)

OK with that change.

Thanks,
Pedro Alves

Re: [PATCH] Fix assert failure in invocation of "[kill|detach] inferiors 1"

[Patrick Palka]

On Tue, Jul 28, 2015 at 7:42 PM, Pedro Alves <palves@redhat.com> wrote:
> On 07/28/2015 09:29 PM, Patrick Palka wrote:
>> When only the dummy PID 0 inferior exists, invoking either of the above
>> commands triggers the following assert failure:
>>
>>   .../binutils-gdb/gdb/thread.c:514: internal-error: any_thread_of_process: Assertion `pid != 0' failed.
>>
>> The fix is straightforward.  Tested on x86_64 Debian Stretch.
>
> Thanks.
>
>>
>> gdb/ChangeLog:
>>
>>       * inferior.c (detach_inferior_command): Don't call
>>       any_thread_of_process when pid is 0.
>>       (kill_inferior_command): Likewise.
>>
>> gdb/testsuite/ChangeLog:
>>
>>       * gdb.base/no-inferiors.exp: New test file.
>> ---
>>  gdb/inferior.c                          | 12 ++++++++++--
>>  gdb/testsuite/gdb.base/no-inferiors.exp | 24 ++++++++++++++++++++++++
>>  2 files changed, 34 insertions(+), 2 deletions(-)
>>  create mode 100644 gdb/testsuite/gdb.base/no-inferiors.exp
>>
>> diff --git a/gdb/inferior.c b/gdb/inferior.c
>> index 5e98df5..ff85a1f 100644
>> --- a/gdb/inferior.c
>> +++ b/gdb/inferior.c
>> @@ -626,7 +626,11 @@ detach_inferior_command (char *args, int from_tty)
>>
>>        pid = gdb_inferior_id_to_pid (num);
>>
>> -      tp = any_thread_of_process (pid);
>> +      if (pid != 0)
>> +     tp = any_thread_of_process (pid);
>> +      else
>> +     tp = 0;
>
> Should be tp = NULL.

Oops, ok.

>
> This is old code that predates the multi-executable support
> (6c95b8df7fef), and was never really updated since.  Guess
> not many people use it...

I see.  Though I think the regression may have been introduced more
recently, by commit 32990ad, when the gdb_assert in question was
added.

>
> Doesn't make that much sense to say "has no threads"
> nowadays.  I think this should give out a warning similar
> to the error "detach_command" throws, like:
>
> +      if (pid == 0)
> +        {
> +          warning (_("Inferior ID %s is not being run."), num);
> +          continue;
> +        }
>
>       tp = any_thread_of_process (pid);
>       if (!tp)
>         {
>           warning (_("Inferior ID %d has no threads."), num);
>           continue;
>         }
>
> The "has no threads" case is then more of an internal error,
> but there's no need to be so drastic, so leave it as is.
>
> (Likewise for kill, of course.)
>
> OK with that change.

Ah, so the internal error may also be triggered by passing to these
commands an inferior that is not being run, not just by passing the
initial dummy inferior.  I will change the test file to cover these
bases.  I notice that "[kill|attach] inferiors" has no test coverage
at all, so this will be a good time to add some.

Re: [PATCH] Fix assert failure in invocation of "[kill|detach] inferiors 1"

[Pedro Alves]

On 07/29/2015 12:22 PM, Patrick Palka wrote:

> Ah, so the internal error may also be triggered by passing to these
> commands an inferior that is not being run, not just by passing the
> initial dummy inferior.  I will change the test file to cover these
> bases. 

Yeah.  The initial inferior is no different from any other inferior
that is created afterwards.

> I notice that "[kill|attach] inferiors" has no test coverage
> at all, so this will be a good time to add some.

Thanks, that's super.

-- 
Pedro Alves