Re: [RFC PATCH 3/3] gdb/nat/linux: Fix attaching to process when it has zombie threads

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

Pedro Alves <pedro@palves.net> writes:

> On 2024-03-21 23:11, Thiago Jung Bauermann wrote:
>> diff --git a/gdb/nat/linux-osdata.c b/gdb/nat/linux-osdata.c
>> index c254f2e4f05b..998279377433 100644
>> --- a/gdb/nat/linux-osdata.c
>> +++ b/gdb/nat/linux-osdata.c
>> @@ -112,6 +112,28 @@ linux_common_core_of_thread (ptid_t ptid)
>>    return core;
>>  }
>>
>> +/* See linux-osdata.h.  */
>> +
>> +std::optional<ULONGEST>
>> +linux_get_starttime (ptid_t ptid)
>
> Ditto re. moving this to linux-procfs.  This has nothing to do with "info osdata".

Done in v2.

>> index a82fb08b998e..1cdc687aa9cf 100644
>> --- a/gdb/nat/linux-osdata.h
>> +++ b/gdb/nat/linux-osdata.h
>> @@ -27,4 +27,8 @@ extern int linux_common_core_of_thread (ptid_t ptid);
>>  extern LONGEST linux_common_xfer_osdata (const char *annex, gdb_byte *readbuf,
>>  					 ULONGEST offset, ULONGEST len);
>>
>> +/* Get the start time of thread PTID.  */
>> +
>> +extern std::optional<ULONGEST> linux_get_starttime (ptid_t ptid);
>> +
>>  #endif /* NAT_LINUX_OSDATA_H */
>> diff --git a/gdb/nat/linux-procfs.c b/gdb/nat/linux-procfs.c
>> index b17e3120792e..b01bf36c0b53 100644
>> --- a/gdb/nat/linux-procfs.c
>> +++ b/gdb/nat/linux-procfs.c
>> @@ -17,10 +17,13 @@
>>     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>>
>>  #include "gdbsupport/common-defs.h"
>> +#include "linux-osdata.h"
>>  #include "linux-procfs.h"
>
> linux-procfs.h is the main header of this source file, so it should be first.
> But then again, I think this shouldn't be consuming things from linux-osdata, it
> should be the other way around.

Right. v2 doesn't need to include "linux-osdata.h" anymore.

>>  #include "gdbsupport/filestuff.h"
>>  #include <dirent.h>
>>  #include <sys/stat.h>
>> +#include <set>
>> +#include <utility>
>>
>>  /* Return the TGID of LWPID from /proc/pid/status.  Returns -1 if not
>>     found.  */
>> @@ -290,6 +293,10 @@ linux_proc_attach_tgid_threads (pid_t pid,
>>        return;
>>      }
>>
>> +  /* Keeps track of the LWPs we have already visited in /proc,
>> +     identified by their PID and starttime to detect PID reuse.  */
>> +  std::set<std::pair<unsigned long,ULONGEST>> visited_lwps;
>
> Missing space before ULONGEST.

Ah, indeed. Fixed in v2.

> AFAICT, you don't rely on order, so this could be an unordered_set?

True. Done in v2, but I had to add a hash function for
std::pair<unsigned long, ULONGEST>. I don't know anything about
constructing hashes, so I just went with what I saw elsewhere in GDB
(index_key_hasher in dwarf2/index-write.c) and XOR the hashes of each
element of the pair.

>> +
>>    /* Scan the task list for existing threads.  While we go through the
>>       threads, new threads may be spawned.  Cycle through the list of
>>       threads until we have done two iterations without finding new
>> @@ -308,6 +315,18 @@ linux_proc_attach_tgid_threads (pid_t pid,
>>  	  if (lwp != 0)
>>  	    {
>>  	      ptid_t ptid = ptid_t (pid, lwp);
>> +	      std::optional<ULONGEST> starttime = linux_get_starttime (ptid);
>> +
>> +	      if (starttime.has_value ())
>> +		{
>> +		  std::pair<unsigned long,ULONGEST> key (lwp, *starttime);
>
> Space before ULONGEST.

Fixed in v2.

>> +
>> +		  /* If we already visited this LWP, skip it this time.  */
>> +		  if (visited_lwps.find (key) != visited_lwps.cend ())
>> +		    continue;
>> +
>> +		  visited_lwps.insert (key);
>> +		}
>>
>>  	      if (attach_lwp (ptid))
>>  		new_threads_found = 1;

Thank you for the patch reviews.

--
Thiago