Unbreaking gdb on Solaris post-multitarget [PR 25939]

Unbreaking gdb on Solaris post-multitarget [PR 25939]

[Rainer Orth]

[-- Attachment #1: Type: text/plain, Size: 2718 bytes --]

Some time ago, when testing gdb master on Solaris again after several
months, I discovered that gdb couldn't execute even a trivial program
anymore.  This had gone unnoticed by the Solaris buildbots since the
code continued to compile just fine.  Those bots are build-only since
many tests (especially thread tests) are either flaky or time out.

A reghunt identified the multi-target merge as the culprit.

Here's what I see:

$ ./gdb -D ./data-directory ./hello
GNU gdb (GDB) 10.0.50.20200106-git
[...]
Reading symbols from ./hello...
(gdb) run
Starting program: /vol/obj/gnu/gdb/gdb/reghunt/no-resync/122448/gdb/hello 
/vol/src/gnu/gdb/hg/master/reghunt/gdb/thread.c:336: internal-error: thread_info::thread_info(inferior*, ptid_t): Assertion `inf_ != NULL' failed.

Here's the start of the corresponding stack trace:

#0  internal_error (
    file=file@entry=0x966150 "/vol/src/gnu/gdb/hg/master/reghunt/gdb/thread.c", line=line@entry=336, fmt=0x9ddb94 "%s: Assertion `%s' failed.")
    at /vol/src/gnu/gdb/hg/master/reghunt/gdb/gdbsupport/errors.c:51
#1  0x0000000000ef81f4 in thread_info::thread_info (this=0x1212020, 
    inf_=<optimized out>, ptid_=...)
    at /vol/src/gnu/gdb/hg/master/reghunt/gdb/thread.c:344
#2  0x0000000000ef82cd in new_thread (inf=inf@entry=0x0, ptid=...)
    at /vol/src/gnu/gdb/hg/master/reghunt/gdb/thread.c:239
#3  0x0000000000efac3c in add_thread_silent (
    targ=targ@entry=0x11b0940 <the_procfs_target>, ptid=...)
    at /vol/src/gnu/gdb/hg/master/reghunt/gdb/thread.c:304
#4  0x0000000000d90692 in procfs_target::create_inferior (
    this=0x11b0940 <the_procfs_target>, 
    exec_file=0x13dbef0 "/vol/obj/gnu/gdb/gdb/reghunt/no-resync/122448/gdb/hello", allargs="", env=0x13c48f0, from_tty=<optimized out>)
    at /vol/src/gnu/gdb/hg/master/reghunt/gdb/gdbsupport/ptid.h:47
#5  0x0000000000c84e64 in run_command_1 (args=<optimized out>, from_tty=1, 
    run_how=run_how@entry=RUN_NORMAL)
    at /vol/gcc-9/include/c++/9.1.0/bits/basic_string.h:263
#6  0x0000000000c85007 in run_command (args=<optimized out>, 
    from_tty=<optimized out>)
    at /vol/src/gnu/gdb/hg/master/reghunt/gdb/infcmd.c:687

Looking closer, I found that in add_thread_silent as called from
procfs.c (procfs_target::create_inferior) find_inferior_ptid returns
NULL.  The all_inferiors (targ) iterator comes up empty.

Going from there, I see that in add_thread_silent

m_target_stack = {m_top = file_stratum, m_stack = {0x20190e0 <the_dummy_target>, 0x200b8c0 <exec_ops>, 0x0, 0x0, 0x0, 0x0, 0x0}}}

i.e. the_procfs_target is missing compared to the_amd64_linux_nat_target
on Linux/x86_64.

I've managed to get a bit further with the following patch which is
intended to push the procfs target first:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: reghunt.patch --]
[-- Type: text/x-patch, Size: 354 bytes --]

diff --git a/gdb/procfs.c b/gdb/procfs.c
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -3086,6 +3090,9 @@ procfs_target::create_inferior (const ch
       shell_file = tryname;
     }
 
+  if (!target_is_pushed (this))
+    push_target (this);
+
   pid = fork_inferior (exec_file, allargs, env, procfs_set_exec_trap,
 		       NULL, NULL, shell_file, NULL);
 

[-- Attachment #3: Type: text/plain, Size: 1316 bytes --]


However, while I now get over the initial assertion failure, I run
instead into

procfs: couldn't find pid 0 in procinfo list.
procfs: init_inferior, open_proc_files line 2878, /proc/6031: No such file or directory.

When I break in procfs.c (procfs_init_inferior), I can see that
create_procinfo succeeds.  However, looking at the process tree at this
point, I see that the debuggee is still marked as defunct

                  18377 /vol/gcc/bin/gdb -i=mi /vol/gnu/obj/gdb/gdb/reghunt/no-r
                    18379 /vol/gnu/obj/gdb/gdb/reghunt/no-resync/122457/gdb/gdb 
                      18382 <defunct>

so open_procinfo_files fails because /proc/<pid> only contains psinfo
and usage, but no ctl file yet.

I tried to do the same with a version of gdb from immediately before the
multi-target merge: while that can run a test program interactively just
fine, running that gdb under gdb itself most often leads to the same
error.  This very much seems like a race condition to me, but at the
moment I'm pretty much at a loss how to investigate this further.

So I'd very much appreciate any suggestions and help to make gdb work
again on Solaris.

Thanks.
	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: Unbreaking gdb on Solaris post-multitarget [PR 25939]

[Pedro Alves]

On 6/16/20 3:21 PM, Rainer Orth wrote:
> Some time ago, when testing gdb master on Solaris again after several
> months, I discovered that gdb couldn't execute even a trivial program
> anymore.  This had gone unnoticed by the Solaris buildbots since the
> code continued to compile just fine.  Those bots are build-only since
> many tests (especially thread tests) are either flaky or time out.
> 
> A reghunt identified the multi-target merge as the culprit.

I'm sorry about that.

> I've managed to get a bit further with the following patch which is
> intended to push the procfs target first:

That patch looks good to me.

> 
> 
> However, while I now get over the initial assertion failure, I run
> instead into
> 
> procfs: couldn't find pid 0 in procinfo list.
> procfs: init_inferior, open_proc_files line 2878, /proc/6031: No such file or directory.
> 
> When I break in procfs.c (procfs_init_inferior), I can see that
> create_procinfo succeeds.  However, looking at the process tree at this
> point, I see that the debuggee is still marked as defunct
> 
>                   18377 /vol/gcc/bin/gdb -i=mi /vol/gnu/obj/gdb/gdb/reghunt/no-r
>                     18379 /vol/gnu/obj/gdb/gdb/reghunt/no-resync/122457/gdb/gdb 
>                       18382 <defunct>
> 
> so open_procinfo_files fails because /proc/<pid> only contains psinfo
> and usage, but no ctl file yet.
> 
> I tried to do the same with a version of gdb from immediately before the
> multi-target merge: while that can run a test program interactively just
> fine, 

It's not clear to me whether you're saying that a version from before
the multi-target changes can run a test program fine due to not needing
the push_target fix, or whether the multi-target patchset itself caused
this second issue you're observing even when debugging a simple hello
program.

running that gdb under gdb itself most often leads to the same
> error.  This very much seems like a race condition to me, but at the
> moment I'm pretty much at a loss how to investigate this further.

Could this be a race somehow more exposed now due to GDB now spawning worker
threads?  What happens if you debug a GDB that doesn't spawn worker
threads?  Like:

./gdb -D ./data-directory --args ./gdb -ex "maint set worker-threads 0"

Does that problem trigger as often that way?

Or, what happens if you use master GDB with your push_target fix
to debug an older GDB?

Thanks,
Pedro Alves

Re: Unbreaking gdb on Solaris post-multitarget [PR 25939]

[Rainer Orth]

Hi Pedro,

> On 6/16/20 3:21 PM, Rainer Orth wrote:
>> Some time ago, when testing gdb master on Solaris again after several
>> months, I discovered that gdb couldn't execute even a trivial program
>> anymore.  This had gone unnoticed by the Solaris buildbots since the
>> code continued to compile just fine.  Those bots are build-only since
>> many tests (especially thread tests) are either flaky or time out.
>> 
>> A reghunt identified the multi-target merge as the culprit.
>
> I'm sorry about that.

no worries: the Solaris port had been in a relatively bad shape even
before, so maybe this will allow to get to the bottom of things and fix
them.

>> I've managed to get a bit further with the following patch which is
>> intended to push the procfs target first:
>
> That patch looks good to me.

Thanks.

>> However, while I now get over the initial assertion failure, I run
>> instead into
>> 
>> procfs: couldn't find pid 0 in procinfo list.
>> procfs: init_inferior, open_proc_files line 2878, /proc/6031: No such file or directory.
>> 
>> When I break in procfs.c (procfs_init_inferior), I can see that
>> create_procinfo succeeds.  However, looking at the process tree at this
>> point, I see that the debuggee is still marked as defunct
>> 
>>                   18377 /vol/gcc/bin/gdb -i=mi /vol/gnu/obj/gdb/gdb/reghunt/no-r
>>                     18379 /vol/gnu/obj/gdb/gdb/reghunt/no-resync/122457/gdb/gdb 
>>                       18382 <defunct>
>> 
>> so open_procinfo_files fails because /proc/<pid> only contains psinfo
>> and usage, but no ctl file yet.
>> 
>> I tried to do the same with a version of gdb from immediately before the
>> multi-target merge: while that can run a test program interactively just
>> fine, 
>
> It's not clear to me whether you're saying that a version from before
> the multi-target changes can run a test program fine due to not needing
> the push_target fix, or whether the multi-target patchset itself caused
> this second issue you're observing even when debugging a simple hello
> program.

I've experimented a bit more yesterday.  Immediately before the
multi-target patch, I have:

$ cat top-gdb.gdb
file ./gdb
run -q -D data-directory -x bottom-gdb.gdb
$ cat bottom-gdb.gdb
file ./hello
b main
run
$ gdb-9 -q -x top-gdb.gdb
Setting up the environment for debugging gdb.
Breakpoint 1 at 0x196c898: file /vol/gnu/src/gdb/hg/master/reghunt-122456/gdb/gdbsupport/errors.c, line 54.
Breakpoint 2 at 0x179e138: file /vol/gnu/src/gdb/hg/master/reghunt-122456/gdb/cli/cli-cmds.c, line 201.
[Thread debugging using libthread_db enabled]
[New Thread 1 (LWP 1)]
[New LWP    2        ]
[New LWP    3        ]
[New LWP    4        ]
[New LWP    5        ]
[New LWP    6        ]
[New LWP    7        ]
[New LWP    8        ]
[New LWP    9        ]
Breakpoint 1 at 0x401036: file hello.c, line 6.
[Thread debugging using libthread_db enabled]
[New Thread 1 (LWP 1)]
[Switching to Thread 1 (LWP 1)]

Thread 2 hit Breakpoint 1, main () at hello.c:6
6	  printf ("Hello world\n");

At that point the process hierarchy is as expected:

                22745 gdb-9 -q -x top-gdb.gdb
                  22761 /vol/gnu/obj/gdb/gdb/reghunt/no-resync/122456/gdb/gdb -q
                    22768 /vol/gnu/obj/gdb/gdb/reghunt/no-resync/122456/gdb/hell

With the multi-target merge, my push_target and the worker-threads
disabled (more below), I get instead

$ gdb -q -x ~/top-gdb.gdb 
Setting up the environment for debugging gdb.
Breakpoint 1 at 0x197ca44: file /vol/gnu/src/gdb/hg/master/reghunt/gdb/gdbsupport/errors.c, line 54.
Breakpoint 2 at 0x17adf8a: file /vol/gnu/src/gdb/hg/master/reghunt/gdb/cli/cli-cmds.c, line 201.
[Thread debugging using libthread_db enabled]
[New Thread 1 (LWP 1)]
Breakpoint 1 at 0x401036: file hello.c, line 6.
bottom-gdb.gdb:3: Error in sourced command file:
procfs: couldn't find pid 0 in procinfo list.

and this process tree:

                23011 gdb-9 -q -x top-gdb.gdb
                  23012 /vol/gnu/obj/gdb/gdb/reghunt/no-resync/122457/gdb/gdb -q
                    23013 /vol/gnu/obj/gdb/gdb/reghunt/no-resync/122457/gdb/hell

However, if I add

b find_procinfo_or_die

to investigate the above error ("couldn't find pid 0), with the mt patch
there's

Setting up the environment for debugging gdb.
Breakpoint 1 at 0x197ca44: file /vol/gnu/src/gdb/hg/master/reghunt/gdb/gdbsupport/errors.c, line 54.
Breakpoint 2 at 0x17adf8a: file /vol/gnu/src/gdb/hg/master/reghunt/gdb/cli/cli-cmds.c, line 201.
Breakpoint 3 at 0x1afc288: file /vol/gnu/src/gdb/hg/master/reghunt/gdb/procfs.c, line 327.
[Thread debugging using libthread_db enabled]
[New Thread 1 (LWP 1)]
Breakpoint 1 at 0x401036: file hello.c, line 6.
bottom-gdb.gdb:3: Error in sourced command file:
procfs: init_inferior, open_proc_files line 2879, /proc/23022: No such file or directory.
[Switching to Thread 1 (LWP 1)]

Thread 2 hit Breakpoint 3, find_procinfo_or_die (pid=23022, tid=0)
    at /vol/gnu/src/gdb/hg/master/reghunt/gdb/procfs.c:327
327	  procinfo *pi = find_procinfo (pid, tid);

which is no wonder given the child process is marked as defunct, so its
/proc files cannot be opened:

                23020 gdb-9 -q -x top-gdb.gdb
                  23021 /vol/gnu/obj/gdb/gdb/reghunt/no-resync/122457/gdb/gdb -q
                    23022 <defunct>

However, when I try the same in the pre-mt-patch gdb:

Setting up the environment for debugging gdb.
Breakpoint 1 at 0x196c898: file /vol/gnu/src/gdb/hg/master/reghunt-122456/gdb/gdbsupport/errors.c, line 54.
Breakpoint 2 at 0x179e138: file /vol/gnu/src/gdb/hg/master/reghunt-122456/gdb/cli/cli-cmds.c, line 201.
Breakpoint 3 at 0x1ae7e26: file /vol/gnu/src/gdb/hg/master/reghunt-122456/gdb/procfs.c, line 325.
[Thread debugging using libthread_db enabled]
[New Thread 1 (LWP 1)]
[New LWP    2        ]
[New LWP    3        ]
[New LWP    4        ]
[New LWP    5        ]
[New LWP    6        ]
[New LWP    7        ]
[New LWP    8        ]
[New LWP    9        ]
Breakpoint 1 at 0x401036: file hello.c, line 6.
bottom-gdb.gdb:3: Error in sourced command file:
procfs: init_inferior, open_proc_files line 2870, /proc/23028: No such file or directory.
[New Thread 2        ]
[New Thread 3        ]
[New Thread 4        ]
[New Thread 5        ]
[New Thread 6        ]
[New Thread 7        ]
[New Thread 8        ]
[New Thread 9        ]
[Switching to Thread 1 (LWP 1)]

Thread 2 hit Breakpoint 3, find_procinfo_or_die (pid=23028, tid=0) at /vol/gnu/src/gdb/hg/master/reghunt-122456/gdb/procfs.c:325
325	  procinfo *pi = find_procinfo (pid, tid);

I get the same error and the same defunct process:

                23026 gdb-9 -q -x top-gdb.gdb
                  23027 /vol/gnu/obj/gdb/gdb/reghunt/no-resync/122456/gdb/gdb -q
                    23028 <defunct>

This obviously makes debugging extra hard ;-(  However, this error isn't
entirely new: when running the gdb testsuite before the mt merge, I get
several variations of this error

$ grep -a "couldn't find pid" gdb.log |sort|uniq -c
      2 Error in re-setting breakpoint 2: procfs: couldn't find pid 0 in procinfo list.
      2 Error in re-setting breakpoint 5: procfs: couldn't find pid 0 in procinfo list.
     99 procfs: couldn't find pid -1 in procinfo list.
     22 procfs: couldn't find pid 0 in procinfo list.
      5 procfs: couldn't find pid 21415 in procinfo list.
      5 procfs: couldn't find pid 21618 in procinfo list.
     10 procfs: couldn't find pid 22032 in procinfo list.
      5 procfs: couldn't find pid 22457 in procinfo list.
      5 procfs: couldn't find pid 22678 in procinfo list.
     10 procfs: couldn't find pid 22985 in procinfo list.

> running that gdb under gdb itself most often leads to the same
>> error.  This very much seems like a race condition to me, but at the
>> moment I'm pretty much at a loss how to investigate this further.
>
> Could this be a race somehow more exposed now due to GDB now spawning worker
> threads?  What happens if you debug a GDB that doesn't spawn worker
> threads?  Like:
>
> ./gdb -D ./data-directory --args ./gdb -ex "maint set worker-threads 0"

This doesn't work because master gdb cannot debug anything, without or
with the push_target fix.

When instead I use a gdb 9.1 as top gdb, I get

$ gdb-9 -q --args ./gdb -D data-directory -ex "maint set worker-threads 0"
Reading symbols from ./gdb...
Setting up the environment for debugging gdb.
Breakpoint 1 at 0x197ca44: file /vol/gnu/src/gdb/hg/master/reghunt/gdb/gdbsupport/errors.c, line 54.
Breakpoint 2 at 0x17adf8a: file /vol/gnu/src/gdb/hg/master/reghunt/gdb/cli/cli-cmds.c, line 201.
(top-gdb) run
Starting program: /vol/gnu/obj/gdb/gdb/reghunt/no-resync/122457/gdb/gdb can't handle command-line argument containing whitespace

When instead I use

$ cat top-gdb-mt.gdb
file ./gdb-mt
run -q -D data-directory -x bottom-gdb-mt.gdb
$ cat bottom-gdb-mt.gdb
maint set worker-threads 0
file ./hello
b main
run
$ gdb-9 -q -x top-gdb-mt.gdb
[Thread debugging using libthread_db enabled]
[New Thread 1 (LWP 1)]
[New LWP    2        ]
[New LWP    3        ]
[New LWP    4        ]
[New LWP    5        ]
[New LWP    6        ]
[New LWP    7        ]
[New LWP    8        ]
[New LWP    9        ]
[LWP    8         exited]
[New LWP    8        ]
[LWP    6         exited]
[New LWP    6        ]
[LWP    9         exited]
[New LWP    9        ]
[LWP    5         exited]
[New LWP    5        ]
[LWP    7         exited]
[New LWP    7        ]
[LWP    2         exited]
[New LWP    2        ]
[LWP    3         exited]
[New LWP    3        ]
[LWP    4         exited]
[New LWP    4        ]
Breakpoint 1 at 0x401036: file hello.c, line 6.
bottom-gdb-mt.gdb:4: Error in sourced command file:
procfs: couldn't find pid 0 in procinfo list.

> Does that problem trigger as often that way?

The failure is still reproducible that way, but even more verbose
(imagine that on that 160-core system I spoke of ;-)

To avoid that for the moment, I've changed n_worker_threads to 0 for now.

> Or, what happens if you use master GDB with your push_target fix
> to debug an older GDB?

Master GDB cannot debug anything, unfortunately.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: Unbreaking gdb on Solaris post-multitarget [PR 25939]

[Tom Tromey]

Pedro> Could this be a race somehow more exposed now due to GDB now spawning worker
Pedro> threads?  What happens if you debug a GDB that doesn't spawn worker
Pedro> threads?  Like:

Pedro> ./gdb -D ./data-directory --args ./gdb -ex "maint set worker-threads 0"

You have to be sure to build without Guile if you want to try this; the
garbage collector automatically starts threads as well.

Tom

Re: Unbreaking gdb on Solaris post-multitarget [PR 25939]

[Rainer Orth]

Hi Tom,

> Pedro> Could this be a race somehow more exposed now due to GDB now
> Pedro> spawning worker
> Pedro> threads?  What happens if you debug a GDB that doesn't spawn worker
> Pedro> threads?  Like:
>
> Pedro> ./gdb -D ./data-directory --args ./gdb -ex "maint set worker-threads 0"
>
> You have to be sure to build without Guile if you want to try this; the
> garbage collector automatically starts threads as well.

thanks for the hint.  Fortunately, Guile is no longer bundled with
Solaris 11.4 and the libguile I had built some time ago for autogen is
32-bit only.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: Unbreaking gdb on Solaris post-multitarget [PR 25939]

[Pedro Alves]

On 6/17/20 3:45 PM, Rainer Orth wrote:
> [Thread debugging using libthread_db enabled]
> [New Thread 1 (LWP 1)]
> Breakpoint 1 at 0x401036: file hello.c, line 6.
> bottom-gdb.gdb:3: Error in sourced command file:
> procfs: couldn't find pid 0 in procinfo list.

I see what this is.  This is procfs_target::wait relying on
inferior_ptid.  Since the multi-target series, inferior_ptid
is null_ptid before we call target_wait:

static ptid_t
do_target_wait_1 (inferior *inf, ptid_t ptid,
		  target_waitstatus *status, int options)
{
  ptid_t event_ptid;
  struct thread_info *tp;

  /* We know that we are looking for an event in the target of inferior
     INF, but we don't know which thread the event might come from.  As
     such we want to make sure that INFERIOR_PTID is reset so that none of
     the wait code relies on it - doing so is always a mistake.  */
  switch_to_inferior_no_thread (inf);


I'm working on a patch.

Thanks,
Pedro Alves

Re: Unbreaking gdb on Solaris post-multitarget [PR 25939]

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 1323 bytes --]

On 6/18/20 3:55 PM, Pedro Alves via Gdb-patches wrote:
> On 6/17/20 3:45 PM, Rainer Orth wrote:
>> [Thread debugging using libthread_db enabled]
>> [New Thread 1 (LWP 1)]
>> Breakpoint 1 at 0x401036: file hello.c, line 6.
>> bottom-gdb.gdb:3: Error in sourced command file:
>> procfs: couldn't find pid 0 in procinfo list.
> 
> I see what this is.  This is procfs_target::wait relying on
> inferior_ptid.  Since the multi-target series, inferior_ptid
> is null_ptid before we call target_wait:
> 
> static ptid_t
> do_target_wait_1 (inferior *inf, ptid_t ptid,
> 		  target_waitstatus *status, int options)
> {
>   ptid_t event_ptid;
>   struct thread_info *tp;
> 
>   /* We know that we are looking for an event in the target of inferior
>      INF, but we don't know which thread the event might come from.  As
>      such we want to make sure that INFERIOR_PTID is reset so that none of
>      the wait code relies on it - doing so is always a mistake.  */
>   switch_to_inferior_no_thread (inf);
> 
> 
> I'm working on a patch.

Here it is.  This works for me on a Solaris 11.3 (virtual and slow...) machine.
Debugging GDB itself works for me, and I've checked that the gdb.base/break.exp
testcase passes cleanly, at least.

Your push_target fix is still necessary, FAOD.

Could you give it a try?

Thanks,
Pedro Alves

[-- Attachment #2: 0001-Solaris-target_wait-don-t-rely-on-inferior_ptid.patch --]
[-- Type: text/x-patch, Size: 9821 bytes --]

From d31cd9842ab489d1adfd8d904572db28ef3fcd23 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 18 Jun 2020 14:54:08 +0100
Subject: [PATCH] Solaris, target_wait(), don't rely on inferior_ptid

Debugging on Solaris is broken, with the procfs target backend failing
with:

 procfs: couldn't find pid 0 in procinfo list.

as soon as you start a program.

The problem is procfs_target::wait assuming that inferior_ptid is
meaningful on entry, but, since the multi-target series, inferior_ptid
is null_ptid before we call target_wait, in infrun.c:

  static ptid_t
  do_target_wait_1 (inferior *inf, ptid_t ptid,
		    target_waitstatus *status, int options)
  {
...
    /* We know that we are looking for an event in the target of inferior
       INF, but we don't know which thread the event might come from.  As
       such we want to make sure that INFERIOR_PTID is reset so that none of
       the wait code relies on it - doing so is always a mistake.  */
    switch_to_inferior_no_thread (inf);

This patch tweaks the backend to remove the assumption that
inferior_ptid points at something.  sol-thread.c (the thread_stratum
that sits on top of procfs.c) also has the same issue.

Some spots in procfs_target::wait were returning
TARGET_WAITKIND_SPURIOUS+inferior_ptid.  This commit replaces those
with waiting again without returning to the core.  This fixes the
relying on inferior_ptid, and also should fix the issue discussed
here:
  https://sourceware.org/pipermail/gdb/2020-May/048616.html
  https://sourceware.org/pipermail/gdb/2020-June/048660.html

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* procfs.c (procfs_target::wait): Don't reference inferior_ptid.
	Use the current inferior instead.  Don't return
	TARGET_WAITKIND_SPURIOUS/inferior_ptid -- instead continue and
	wait again.
	* sol-thread.c (sol_thread_target::wait): Don't reference
	inferior_ptid.
	(ps_lgetregs, ps_lsetregs, ps_lgetfpregs, ps_lsetfpregs)
	(sol_update_thread_list_callback): Use the current inferior's pid
	instead of inferior_ptid.
---
 gdb/procfs.c     | 45 +++++++++++++++++----------------------------
 gdb/sol-thread.c | 30 ++++++++++--------------------
 2 files changed, 27 insertions(+), 48 deletions(-)

diff --git a/gdb/procfs.c b/gdb/procfs.c
index f5cb5a9320b..c2e3af71c24 100644
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -2126,7 +2126,11 @@ procfs_target::wait (ptid_t ptid, struct target_waitstatus *status,
   retval   = ptid_t (-1);
 
   /* Find procinfo for main process.  */
-  pi = find_procinfo_or_die (inferior_ptid.pid (), 0);
+
+  /* procfs_target currently only supports one inferior.  */
+  inferior *inf = current_inferior ();
+
+  pi = find_procinfo_or_die (inf->pid, 0);
   if (pi)
     {
       /* We must assume that the status is stale now...  */
@@ -2153,10 +2157,10 @@ procfs_target::wait (ptid_t ptid, struct target_waitstatus *status,
 	      wait_retval = ::wait (&wstat); /* "wait" for the child's exit.  */
 
 	      /* Wrong child?  */
-	      if (wait_retval != inferior_ptid.pid ())
+	      if (wait_retval != inf->pid)
 		error (_("procfs: couldn't stop "
 			 "process %d: wait returned %d."),
-		       inferior_ptid.pid (), wait_retval);
+		       inf->pid, wait_retval);
 	      /* FIXME: might I not just use waitpid?
 		 Or try find_procinfo to see if I know about this child?  */
 	      retval = ptid_t (wait_retval);
@@ -2211,13 +2215,11 @@ procfs_target::wait (ptid_t ptid, struct target_waitstatus *status,
 		      printf_unfiltered (_("[%s exited]\n"),
 					 target_pid_to_str (retval).c_str ());
 		    delete_thread (find_thread_ptid (this, retval));
-		    status->kind = TARGET_WAITKIND_SPURIOUS;
-		    return retval;
+		    target_continue_no_signal (ptid);
+		    goto wait_again;
 		  }
 		else if (syscall_is_exit (pi, what))
 		  {
-		    struct inferior *inf;
-
 		    /* Handle SYS_exit call only.  */
 		    /* Stopped at entry to SYS_exit.
 		       Make it runnable, resume it, then use
@@ -2232,14 +2234,13 @@ procfs_target::wait (ptid_t ptid, struct target_waitstatus *status,
 		    if (!proc_run_process (pi, 0, 0))
 		      proc_error (pi, "target_wait, run_process", __LINE__);
 
-		    inf = find_inferior_pid (this, pi->pid);
 		    if (inf->attach_flag)
 		      {
 			/* Don't call wait: simulate waiting for exit,
 			   return a "success" exit code.  Bogus: what if
 			   it returns something else?  */
 			wstat = 0;
-			retval = inferior_ptid;  /* ? ? ? */
+			retval = ptid_t (inf->pid);  /* ? ? ? */
 		      }
 		    else
 		      {
@@ -2278,19 +2279,9 @@ procfs_target::wait (ptid_t ptid, struct target_waitstatus *status,
 					   i, sysargs[i]);
 		      }
 
-		    if (status)
-		      {
-			/* How to exit gracefully, returning "unknown
-			   event".  */
-			status->kind = TARGET_WAITKIND_SPURIOUS;
-			return inferior_ptid;
-		      }
-		    else
-		      {
-			/* How to keep going without returning to wfi: */
-			target_continue_no_signal (ptid);
-			goto wait_again;
-		      }
+		    /* How to keep going without returning to wfi: */
+		    target_continue_no_signal (ptid);
+		    goto wait_again;
 		  }
 		break;
 	      case PR_SYSEXIT:
@@ -2322,9 +2313,8 @@ procfs_target::wait (ptid_t ptid, struct target_waitstatus *status,
 		    if (!in_thread_list (this, temp_ptid))
 		      add_thread (this, temp_ptid);
 
-		    /* Return to WFI, but tell it to immediately resume.  */
-		    status->kind = TARGET_WAITKIND_SPURIOUS;
-		    return inferior_ptid;
+		    target_continue_no_signal (ptid);
+		    goto wait_again;
 		  }
 		else if (syscall_is_lwp_exit (pi, what))
 		  {
@@ -2364,8 +2354,8 @@ procfs_target::wait (ptid_t ptid, struct target_waitstatus *status,
 					   i, sysargs[i]);
 		      }
 
-		    status->kind = TARGET_WAITKIND_SPURIOUS;
-		    return inferior_ptid;
+		    target_continue_no_signal (ptid);
+		    goto wait_again;
 		  }
 		break;
 	      case PR_REQUESTED:
@@ -2416,7 +2406,6 @@ procfs_target::wait (ptid_t ptid, struct target_waitstatus *status,
 	      /* Got this far without error: If retval isn't in the
 		 threads database, add it.  */
 	      if (retval.pid () > 0
-		  && retval != inferior_ptid
 		  && !in_thread_list (this, retval))
 		{
 		  /* We have a new thread.  We need to add it both to
diff --git a/gdb/sol-thread.c b/gdb/sol-thread.c
index 9addf8de3ab..a24d51d1db2 100644
--- a/gdb/sol-thread.c
+++ b/gdb/sol-thread.c
@@ -427,16 +427,6 @@ ptid_t
 sol_thread_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
 			 int options)
 {
-  ptid_t rtnval;
-  ptid_t save_ptid;
-
-  save_ptid = inferior_ptid;
-  scoped_restore save_inferior_ptid = make_scoped_restore (&inferior_ptid);
-
-  inferior_ptid = thread_to_lwp (inferior_ptid, main_ph.ptid.pid ());
-  if (inferior_ptid.pid () == -1)
-    inferior_ptid = procfs_first_available ();
-
   if (ptid.pid () != -1)
     {
       ptid_t ptid_for_warning = ptid;
@@ -449,17 +439,17 @@ sol_thread_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
 		 ptid_for_warning.tid ());
     }
 
-  rtnval = beneath ()->wait (ptid, ourstatus, options);
+  ptid_t rtnval = beneath ()->wait (ptid, ourstatus, options);
 
   if (ourstatus->kind != TARGET_WAITKIND_EXITED)
     {
       /* Map the LWP of interest back to the appropriate thread ID.  */
-      rtnval = lwp_to_thread (rtnval);
-      if (rtnval.pid () == -1)
-	rtnval = save_ptid;
+      ptid_t thr_ptid = lwp_to_thread (rtnval);
+      if (thr_ptid.pid () != -1)
+	rtnval = thr_ptid;
 
       /* See if we have a new thread.  */
-      if (rtnval.tid_p () && rtnval != save_ptid)
+      if (rtnval.tid_p ())
 	{
 	  thread_info *thr = find_thread_ptid (current_inferior (), rtnval);
 	  if (thr == NULL || thr->state == THREAD_EXITED)
@@ -855,7 +845,7 @@ ps_ptwrite (struct ps_prochandle *ph, psaddr_t addr,
 ps_err_e
 ps_lgetregs (struct ps_prochandle *ph, lwpid_t lwpid, prgregset_t gregset)
 {
-  ptid_t ptid = ptid_t (inferior_ptid.pid (), lwpid, 0);
+  ptid_t ptid = ptid_t (current_inferior ()->pid, lwpid, 0);
   struct regcache *regcache
     = get_thread_arch_regcache (current_inferior ()->process_target (),
 				ptid, target_gdbarch ());
@@ -872,7 +862,7 @@ ps_err_e
 ps_lsetregs (struct ps_prochandle *ph, lwpid_t lwpid,
 	     const prgregset_t gregset)
 {
-  ptid_t ptid = ptid_t (inferior_ptid.pid (), lwpid, 0);
+  ptid_t ptid = ptid_t (current_inferior ()->pid, lwpid, 0);
   struct regcache *regcache
     = get_thread_arch_regcache (current_inferior ()->process_target (),
 				ptid, target_gdbarch ());
@@ -925,7 +915,7 @@ ps_err_e
 ps_lgetfpregs (struct ps_prochandle *ph, lwpid_t lwpid,
 	       prfpregset_t *fpregset)
 {
-  ptid_t ptid = ptid_t (inferior_ptid.pid (), lwpid, 0);
+  ptid_t ptid = ptid_t (current_inferior ()->pid, lwpid, 0);
   struct regcache *regcache
     = get_thread_arch_regcache (current_inferior ()->process_target (),
 				ptid, target_gdbarch ());
@@ -942,7 +932,7 @@ ps_err_e
 ps_lsetfpregs (struct ps_prochandle *ph, lwpid_t lwpid,
 	       const prfpregset_t * fpregset)
 {
-  ptid_t ptid = ptid_t (inferior_ptid.pid (), lwpid, 0);
+  ptid_t ptid = ptid_t (current_inferior ()->pid, lwpid, 0);
   struct regcache *regcache
     = get_thread_arch_regcache (current_inferior ()->process_target (),
 				ptid, target_gdbarch ());
@@ -1012,7 +1002,7 @@ sol_update_thread_list_callback (const td_thrhandle_t *th, void *ignored)
   if (retval != TD_OK)
     return -1;
 
-  ptid_t ptid = ptid_t (inferior_ptid.pid (), 0, ti.ti_tid);
+  ptid_t ptid = ptid_t (current_inferior ()->pid, 0, ti.ti_tid);
   thread_info *thr = find_thread_ptid (current_inferior (), ptid);
   if (thr == NULL || thr->state == THREAD_EXITED)
     {

base-commit: a8a566853a0fc7f57159e55436ff6f395e499568
prerequisite-patch-id: 5d757f55a3e4e55e9b9e70684280f64ea4176356
-- 
2.14.5

Re: Unbreaking gdb on Solaris post-multitarget [PR 25939]

[Rainer Orth]

Hi Pedro,

> On 6/18/20 3:55 PM, Pedro Alves via Gdb-patches wrote:
>> On 6/17/20 3:45 PM, Rainer Orth wrote:
>>> [Thread debugging using libthread_db enabled]
>>> [New Thread 1 (LWP 1)]
>>> Breakpoint 1 at 0x401036: file hello.c, line 6.
>>> bottom-gdb.gdb:3: Error in sourced command file:
>>> procfs: couldn't find pid 0 in procinfo list.
>> 
>> I see what this is.  This is procfs_target::wait relying on
>> inferior_ptid.  Since the multi-target series, inferior_ptid
>> is null_ptid before we call target_wait:
>> 
>> static ptid_t
>> do_target_wait_1 (inferior *inf, ptid_t ptid,
>> 		  target_waitstatus *status, int options)
>> {
>>   ptid_t event_ptid;
>>   struct thread_info *tp;
>> 
>>   /* We know that we are looking for an event in the target of inferior
>>      INF, but we don't know which thread the event might come from.  As
>>      such we want to make sure that INFERIOR_PTID is reset so that none of
>>      the wait code relies on it - doing so is always a mistake.  */
>>   switch_to_inferior_no_thread (inf);
>> 
>> 
>> I'm working on a patch.

I'd identified procfs_target::wait as the cause of the
find_procinfo_or_die call with pid = 0 by adding a sleep call in the
latter and attaching gdb to look where I am, but hadn't gotten further yet.

> Here it is.  This works for me on a Solaris 11.3 (virtual and slow...) machine.

I'd meant to suggest trying gcc211 in the GCC compile farm, but the
machine was quite busy when I looked/tried, lacked some vital software
(bison, patch, makeinfo, make only as gmake, gcc 5.5.0 only which may or
may not work).  I've built those myself and managed a gdb build, too,
but the experience wasn't too pretty: not what I'd want to work with as
a developer on a foreign platform meant to support gcc/gdb/binutils...

> Debugging GDB itself works for me, and I've checked that the gdb.base/break.exp
> testcase passes cleanly, at least.
>
> Your push_target fix is still necessary, FAOD.

Should I push it as is (with an appropriate description, of course) or
would the code change need a comment, too?

> Could you give it a try?

I did so now, both on amd64-pc-solaris2.11 (Solaris 11.4), and
sparcv9-sun-solaris2.11 (Solaris 11.3, gcc211 above).  

gdb basically works again, but compared to the pre-multi-target results
I have still a considerable number of regressions:

before:

# of expected passes            62928
# of unexpected failures        1841
# of unexpected successes       4
# of expected failures          49
# of unknown successes          6

now:

# of expected passes            63768
# of unexpected failures        2411
# of expected failures          52
# of unknown successes          1

Of course there's months of gdb development between the two, but e.g. I
see

    268 /vol/src/gnu/gdb/hg/master/local/gdb/thread.c:336: internal-error: thread_info::thread_info(inferior*, ptid_t): Assertion `inf_ != NULL' failed.
    119 /vol/src/gnu/gdb/hg/master/local/gdb/thread.c:86: internal-error: thread_info* inferior_thread(): Assertion `tp' failed.
     88 /vol/src/gnu/gdb/hg/master/local/gdb/inline-frame.c:384: internal-error: void skip_inline_frames(thread_info*, bpstat): Assertion `find_inline_frame_stacore LWP 1 In:     ' failed.

compared to previous

    487 /vol/src/gnu/gdb/hg/master/reghunt/gdb/thread.c:93: internal-error: thread_info* inferior_thread(): Assertion `tp' failed.
     72 /vol/src/gnu/gdb/hg/master/reghunt/gdb/inline-frame.c:367: internal-error: void skip_inline_frames(thread_info*, bpstat): Assertion `find_inline_frame_state (thread) == NULL' failed.

Some of those are definitively regressions, although it's difficult to
say with the flaky nature of several tests on Solaris.

Whatever the case, it looks like I have months of work ahead ;-)

Thanks a lot for fixing this.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: Unbreaking gdb on Solaris post-multitarget [PR 25939]

[Pedro Alves]

On 6/19/20 1:36 PM, Rainer Orth wrote:
>> On 6/18/20 3:55 PM, Pedro Alves via Gdb-patches wrote:

>> Your push_target fix is still necessary, FAOD.
> 
> Should I push it as is (with an appropriate description, of course) or
> would the code change need a comment, too?

It's fine without a comment.  I think you can remove the 
push_target call from procfs_init_inferior at the same time,
too, as that one becomes unnecessary.  Basically make the fix be
about moving the push_target call earlier.

> 
>> Could you give it a try?
> 
> I did so now, both on amd64-pc-solaris2.11 (Solaris 11.4), and
> sparcv9-sun-solaris2.11 (Solaris 11.3, gcc211 above).  
> 
> gdb basically works again, but compared to the pre-multi-target results
> I have still a considerable number of regressions:
> 
> before:
> 
> # of expected passes            62928
> # of unexpected failures        1841
> # of unexpected successes       4
> # of expected failures          49
> # of unknown successes          6
> 
> now:
> 
> # of expected passes            63768
> # of unexpected failures        2411
> # of expected failures          52
> # of unknown successes          1
> 
> Of course there's months of gdb development between the two, but e.g. I

Yeah, may not be regressions, but new FAILs instead.  The number of
expected passes is up too.

> Some of those are definitively regressions, although it's difficult to
> say with the flaky nature of several tests on Solaris.
> 
> Whatever the case, it looks like I have months of work ahead ;-)
> 
> Thanks a lot for fixing this.

You're welcome.

Thanks,
Pedro Alves

[COMMITTED PATCH][PR gdb/25939] Move push_target call earlier in procfs.c

[Rainer Orth]

[-- Attachment #1: Type: text/plain, Size: 3590 bytes --]

Hi Pedro,

> On 6/19/20 1:36 PM, Rainer Orth wrote:
>>> On 6/18/20 3:55 PM, Pedro Alves via Gdb-patches wrote:
>
>>> Your push_target fix is still necessary, FAOD.
>> 
>> Should I push it as is (with an appropriate description, of course) or
>> would the code change need a comment, too?
>
> It's fine without a comment.  I think you can remove the 
> push_target call from procfs_init_inferior at the same time,
> too, as that one becomes unnecessary.  Basically make the fix be
> about moving the push_target call earlier.

that works just fine.  Here's what I've committed after testing both
pathes together on amd64-pc-solaris2.11.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University


[PR gdb/25939] Move push_target call earlier in procfs.c

Since the multi-target patch, the run command fails on Solaris with an
assertion failure even for a trivial program:

$ ./gdb -D ./data-directory ./hello
GNU gdb (GDB) 10.0.50.20200106-git
[...]
Reading symbols from ./hello...
(gdb) run
Starting program: /vol/obj/gnu/gdb/gdb/reghunt/no-resync/122448/gdb/hello 
/vol/src/gnu/gdb/hg/master/reghunt/gdb/thread.c:336: internal-error:
thread_info::thread_info(inferior*, ptid_t): Assertion `inf_ != NULL'
failed.

Here's the start of the corresponding stack trace:

#0  internal_error (
    file=file@entry=0x966150
"/vol/src/gnu/gdb/hg/master/reghunt/gdb/thread.c", line=line@entry=336,
fmt=0x9ddb94 "%s: Assertion `%s' failed.")
    at /vol/src/gnu/gdb/hg/master/reghunt/gdb/gdbsupport/errors.c:51
#1  0x0000000000ef81f4 in thread_info::thread_info (this=0x1212020, 
    inf_=<optimized out>, ptid_=...)
    at /vol/src/gnu/gdb/hg/master/reghunt/gdb/thread.c:344
#2  0x0000000000ef82cd in new_thread (inf=inf@entry=0x0, ptid=...)
    at /vol/src/gnu/gdb/hg/master/reghunt/gdb/thread.c:239
#3  0x0000000000efac3c in add_thread_silent (
    targ=targ@entry=0x11b0940 <the_procfs_target>, ptid=...)
    at /vol/src/gnu/gdb/hg/master/reghunt/gdb/thread.c:304
#4  0x0000000000d90692 in procfs_target::create_inferior (
    this=0x11b0940 <the_procfs_target>, 
    exec_file=0x13dbef0
"/vol/obj/gnu/gdb/gdb/reghunt/no-resync/122448/gdb/hello", allargs="",
env=0x13c48f0, from_tty=<optimized out>)
    at /vol/src/gnu/gdb/hg/master/reghunt/gdb/gdbsupport/ptid.h:47
#5  0x0000000000c84e64 in run_command_1 (args=<optimized out>, from_tty=1, 
    run_how=run_how@entry=RUN_NORMAL)
    at /vol/gcc-9/include/c++/9.1.0/bits/basic_string.h:263
#6  0x0000000000c85007 in run_command (args=<optimized out>, 
    from_tty=<optimized out>)
    at /vol/src/gnu/gdb/hg/master/reghunt/gdb/infcmd.c:687

Looking closer, I found that in add_thread_silent as called from
procfs.c (procfs_target::create_inferior) find_inferior_ptid returns
NULL.  The all_inferiors (targ) iterator comes up empty.

Going from there, I see that in add_thread_silent

m_target_stack = {m_top = file_stratum, m_stack = {0x20190e0
<the_dummy_target>, 0x200b8c0 <exec_ops>, 0x0, 0x0, 0x0, 0x0, 0x0}}}

i.e. the_procfs_target is missing compared to the_amd64_linux_nat_target
on Linux/x86_64.

Moving the push_target call earlier allows debugging to get over the
initial assertion failure.  I run instead into

procfs: couldn't find pid 0 in procinfo list.

which is fixed by

	https://sourceware.org/pipermail/gdb-patches/2020-June/169674.html

Both patches tested together on amd64-pc-solaris2.11.

	PR gdb/25939
	* procfs.c (procfs_target::procfs_init_inferior): Move push_target
	call ...
	(procfs_target::create_inferior): ... here.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: sol2-procfs-push_target.patch --]
[-- Type: text/x-patch, Size: 842 bytes --]

# HG changeset patch
# Parent  34403039561e7d3e10eb37561e6b574370a182d5
Move push_target call earlier in procfs.c [PR25939]

diff --git a/gdb/procfs.c b/gdb/procfs.c
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -2781,11 +2781,6 @@ procfs_target::procfs_init_inferior (int
   int fail;
   int lwpid;
 
-  /* This routine called on the parent side (GDB side)
-     after GDB forks the inferior.  */
-  if (!target_is_pushed (this))
-    push_target (this);
-
   pi = create_procinfo (pid, 0);
   if (pi == NULL)
     perror (_("procfs: out of memory in 'init_inferior'"));
@@ -3006,6 +3001,9 @@ procfs_target::create_inferior (const ch
       shell_file = tryname;
     }
 
+  if (!target_is_pushed (this))
+    push_target (this);
+
   pid = fork_inferior (exec_file, allargs, env, procfs_set_exec_trap,
 		       NULL, NULL, shell_file, NULL);
 

Re: [COMMITTED PATCH][PR gdb/25939] Move push_target call earlier in procfs.c

[Pedro Alves]

On 6/21/20 5:37 PM, Rainer Orth wrote:
> Hi Pedro,
> 
>> On 6/19/20 1:36 PM, Rainer Orth wrote:
>>>> On 6/18/20 3:55 PM, Pedro Alves via Gdb-patches wrote:
>>
>>>> Your push_target fix is still necessary, FAOD.
>>>
>>> Should I push it as is (with an appropriate description, of course) or
>>> would the code change need a comment, too?
>>
>> It's fine without a comment.  I think you can remove the 
>> push_target call from procfs_init_inferior at the same time,
>> too, as that one becomes unnecessary.  Basically make the fix be
>> about moving the push_target call earlier.
> 
> that works just fine.  Here's what I've committed after testing both
> pathes together on amd64-pc-solaris2.11.

I've committed mine now.

Thanks,
Pedro Alves