gdb: don't use inferior_ptid in linux_nat_wait_1

Message ID 20200801222432.7404-1-simon.marchi@efficios.com
State New
Headers show
Series
  • gdb: don't use inferior_ptid in linux_nat_wait_1
Related show

Commit Message

Hannes Domani via Gdb-patches Aug. 1, 2020, 10:24 p.m.
From: Simon Marchi <simon.marchi@polymtl.ca>


target_ops::wait implementations should not rely on the value of
inferior_ptid on entry.  While looking at another wait-related patch, I
noticed that the code in linux_nat_wait_1, checking for a newly created
process, did just that.  This patch fixes it.  Note that I didn't see
any bug, this "fix" is simply to make the function respect the
target_ops::wait contract.

Instead of checking inferior_ptid, check for the passed in `ptid`
value.

During startup, linux_nat_wait_1 gets called a few times with the
pid-only ptid, while startup_inferior waits for the expected number of
exec events.  For this reason, I needed to add a `find_lwp_pid` call to
ensure that the actions of changing the main thread's ptid, and adding
the initial lwp, were done only once for a given process.

This was not needed before, since thread_change_ptid, through the
thread_ptid_changed observer, ends up changing inferior_ptid.  So the
second time around, inferior_ptid was not a pid-only ptid.

That find_lwp_pid won't add much overhead, as it will only be called
when the ptid is a pid-only ptid.  And AFAIK, that only happens during
inferior startup.

An alternative to that `find_lwp_pid` call might be to make
startup_inferior realize that the main thread has changed ptid, and make
it wait for the new ptid.  But that doesn't look easy to do.

Regtested on amd64/Linux.

gdb/ChangeLog:

	* linux-nat.c (linux_nat_wait_1): Don't use inferior_ptid when
        checking for initial lwp.

Change-Id: I8f1d5c766f5cb2a29c948bc75fa4582d7130c23f
---
 gdb/linux-nat.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

-- 
2.28.0

Comments

Tom Tromey Aug. 4, 2020, 6:04 p.m. | #1
Simon> gdb/ChangeLog:

Simon> 	* linux-nat.c (linux_nat_wait_1): Don't use inferior_ptid when
Simon>         checking for initial lwp.

This seems fine to me.
Thanks.

Tom
Simon Marchi Aug. 4, 2020, 6:49 p.m. | #2
On 2020-08-04 2:04 p.m., Tom Tromey wrote:
> Simon> gdb/ChangeLog:

> 

> Simon> 	* linux-nat.c (linux_nat_wait_1): Don't use inferior_ptid when

> Simon>         checking for initial lwp.

> 

> This seems fine to me.

> Thanks.


Thanks for checking.  I'll wait to see if Pedro has an opinion about this, just
in case.

Simon
Simon Marchi Sept. 14, 2020, 3:52 p.m. | #3
On 2020-08-04 2:49 p.m., Simon Marchi wrote:
> On 2020-08-04 2:04 p.m., Tom Tromey wrote:

>> Simon> gdb/ChangeLog:

>>

>> Simon> 	* linux-nat.c (linux_nat_wait_1): Don't use inferior_ptid when

>> Simon>         checking for initial lwp.

>>

>> This seems fine to me.

>> Thanks.

> 

> Thanks for checking.  I'll wait to see if Pedro has an opinion about this, just

> in case.

> 

> Simon

> 


I pushed this patch.

Simon

Patch

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 20b03bc2ba9..75c6d219d6a 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -3277,14 +3277,13 @@  linux_nat_wait_1 (ptid_t ptid, struct target_waitstatus *ourstatus,
   /* The first time we get here after starting a new inferior, we may
      not have added it to the LWP list yet - this is the earliest
      moment at which we know its PID.  */
-  if (inferior_ptid.is_pid ())
+  if (ptid.is_pid () && find_lwp_pid (ptid) == nullptr)
     {
-      /* Upgrade the main thread's ptid.  */
-      thread_change_ptid (linux_target, inferior_ptid,
-			  ptid_t (inferior_ptid.pid (),
-				  inferior_ptid.pid (), 0));
+      ptid_t lwp_ptid (ptid.pid (), ptid.pid ());
 
-      lp = add_initial_lwp (inferior_ptid);
+      /* Upgrade the main thread's ptid.  */
+      thread_change_ptid (linux_target, ptid, lwp_ptid);
+      lp = add_initial_lwp (lwp_ptid);
       lp->resumed = 1;
     }