[PATCH] gdbserver: When attaching, add process before lwps

[PATCH] gdbserver: When attaching, add process before lwps

[Alan Hayward]

The recent BP/WP changes for AArch64 swapping the order in add_lwp()
so that the process was added before the lwp. This was due to the lwp
creation requiring the process data.

This also needs changing in linux_attach().

Fixes gdb.server/ext-attach.exp on Aarch64.

(This regression was hidden due to the racy nature of the gdb.server
tests - now they are no longer racy it'll be easier to spot. Also
checked X86).

gdb/gdbserver/ChangeLog:

2019-01-25  Alan Hayward  <alan.hayward@arm.com>

	* linux-low.c (linux_attach): Add process before lwp.
---
 gdb/gdbserver/linux-low.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 17cce24d76..1ab2cfb1eb 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -1188,6 +1188,8 @@ linux_attach (unsigned long pid)
   ptid_t ptid = ptid_t (pid, pid, 0);
   int err;
 
+  proc = linux_add_process (pid, 1);
+
   /* Attach to PID.  We will check for other threads
      soon.  */
   err = linux_attach_lwp (ptid);
@@ -1198,8 +1200,6 @@ linux_attach (unsigned long pid)
       error ("Cannot attach to process %ld: %s", pid, reason.c_str ());
     }
 
-  proc = linux_add_process (pid, 1);
-
   /* Don't ignore the initial SIGSTOP if we just attached to this
      process.  It will be collected by wait shortly.  */
   initial_thread = find_thread_ptid (ptid_t (pid, pid, 0));
-- 
2.17.2 (Apple Git-113)

Re: [PATCH] gdbserver: When attaching, add process before lwps

[Pedro Alves]

On 01/25/2019 10:48 AM, Alan Hayward wrote:
> The recent BP/WP changes for AArch64 swapping the order in add_lwp()
> so that the process was added before the lwp. This was due to the lwp
> creation requiring the process data.
> 
> This also needs changing in linux_attach().
> 
> Fixes gdb.server/ext-attach.exp on Aarch64.
> 
> (This regression was hidden due to the racy nature of the gdb.server
> tests - now they are no longer racy it'll be easier to spot. Also
> checked X86).
> 
> gdb/gdbserver/ChangeLog:
> 
> 2019-01-25  Alan Hayward  <alan.hayward@arm.com>
> 
> 	* linux-low.c (linux_attach): Add process before lwp.

> ---
>  gdb/gdbserver/linux-low.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index 17cce24d76..1ab2cfb1eb 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -1188,6 +1188,8 @@ linux_attach (unsigned long pid)
>    ptid_t ptid = ptid_t (pid, pid, 0);
>    int err;
>  
> +  proc = linux_add_process (pid, 1);
> +
>    /* Attach to PID.  We will check for other threads
>       soon.  */
>    err = linux_attach_lwp (ptid);
> @@ -1198,8 +1200,6 @@ linux_attach (unsigned long pid)
>        error ("Cannot attach to process %ld: %s", pid, reason.c_str ());
>      }
>  
> -  proc = linux_add_process (pid, 1);
> -

This fails to consider the error conditions.  That error call
visible above throws an exception.

- If GDBserver is already attached to the process, this will
  create another process_info object for the already-attached
  process, and leave it behind.

  For this case, it would seem better to check whether we're
  already attached to a given PID in common code, around
  server.c:attach_inferior.

- If GDBserver isn't already attached, but the attach fails,
  this will likewise leave a stale process_info object behind.

Thanks,
Pedro Alves

Re: [PATCH] gdbserver: When attaching, add process before lwps

[Alan Hayward]

> On 25 Jan 2019, at 18:23, Pedro Alves <palves@redhat.com> wrote:
> 
> On 01/25/2019 10:48 AM, Alan Hayward wrote:
>> The recent BP/WP changes for AArch64 swapping the order in add_lwp()
>> so that the process was added before the lwp. This was due to the lwp
>> creation requiring the process data.
>> 
>> This also needs changing in linux_attach().
>> 
>> Fixes gdb.server/ext-attach.exp on Aarch64.
>> 
>> (This regression was hidden due to the racy nature of the gdb.server
>> tests - now they are no longer racy it'll be easier to spot. Also
>> checked X86).
>> 
>> gdb/gdbserver/ChangeLog:
>> 
>> 2019-01-25  Alan Hayward  <alan.hayward@arm.com>
>> 
>> 	* linux-low.c (linux_attach): Add process before lwp.
> 
>> ---
>> gdb/gdbserver/linux-low.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
>> index 17cce24d76..1ab2cfb1eb 100644
>> --- a/gdb/gdbserver/linux-low.c
>> +++ b/gdb/gdbserver/linux-low.c
>> @@ -1188,6 +1188,8 @@ linux_attach (unsigned long pid)
>>   ptid_t ptid = ptid_t (pid, pid, 0);
>>   int err;
>> 
>> +  proc = linux_add_process (pid, 1);
>> +
>>   /* Attach to PID.  We will check for other threads
>>      soon.  */
>>   err = linux_attach_lwp (ptid);
>> @@ -1198,8 +1200,6 @@ linux_attach (unsigned long pid)
>>       error ("Cannot attach to process %ld: %s", pid, reason.c_str ());
>>     }
>> 
>> -  proc = linux_add_process (pid, 1);
>> -
> 
> This fails to consider the error conditions.  That error call
> visible above throws an exception.
> 
> - If GDBserver is already attached to the process, this will
>  create another process_info object for the already-attached
>  process, and leave it behind.
> 
>  For this case, it would seem better to check whether we're
>  already attached to a given PID in common code, around
>  server.c:attach_inferior.

Assuming you are suggesting adding a check if the process object
exists in gdbserver, rather than adding a new target func which
then calls out to /proc.

> 
> - If GDBserver isn't already attached, but the attach fails,
>  this will likewise leave a stale process_info object behind.
> 

I think I’m probably missing something here, but if the attach
fails for any reason then gdbserver will error and exit - so it
won’t matter if the object is not deleted?


New patch with the above changes. Ok?


    gdb/gdbserver/ChangeLog:

    2019-02-01  Alan Hayward  <alan.hayward@arm.com>

            * linux-low.c (linux_attach): Add process before lwp.
            * server.c (attach_inferior): Check if already attached.

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 44016d2310..8c5a51f23c 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -1188,18 +1188,19 @@ linux_attach (unsigned long pid)
   ptid_t ptid = ptid_t (pid, pid, 0);
   int err;

+  proc = linux_add_process (pid, 1);
+
   /* Attach to PID.  We will check for other threads
      soon.  */
   err = linux_attach_lwp (ptid);
   if (err != 0)
     {
-      std::string reason = linux_ptrace_attach_fail_reason_string (ptid, err);
+      remove_process (proc);

+      std::string reason = linux_ptrace_attach_fail_reason_string (ptid, err);
       error ("Cannot attach to process %ld: %s", pid, reason.c_str ());
     }

-  proc = linux_add_process (pid, 1);
-
   /* Don't ignore the initial SIGSTOP if we just attached to this
      process.  It will be collected by wait shortly.  */
   initial_thread = find_thread_ptid (ptid_t (pid, pid, 0));
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index f9bfdd7307..e960c10d40 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -295,6 +295,9 @@ attach_inferior (int pid)
   /* myattach should return -1 if attaching is unsupported,
      0 if it succeeded, and call error() otherwise.  */

+  if (find_process_pid (pid) != nullptr)
+    error ("Already attached to process %d\n", pid);
+
   if (myattach (pid) != 0)
     return -1;

Re: [PATCH] gdbserver: When attaching, add process before lwps

[Pedro Alves]

On 02/01/2019 11:20 AM, Alan Hayward wrote:
> 
> 
>> On 25 Jan 2019, at 18:23, Pedro Alves <palves@redhat.com> wrote:
>>
>> On 01/25/2019 10:48 AM, Alan Hayward wrote:
>>> The recent BP/WP changes for AArch64 swapping the order in add_lwp()
>>> so that the process was added before the lwp. This was due to the lwp
>>> creation requiring the process data.
>>>
>>> This also needs changing in linux_attach().
>>>
>>> Fixes gdb.server/ext-attach.exp on Aarch64.
>>>
>>> (This regression was hidden due to the racy nature of the gdb.server
>>> tests - now they are no longer racy it'll be easier to spot. Also
>>> checked X86).
>>>
>>> gdb/gdbserver/ChangeLog:
>>>
>>> 2019-01-25  Alan Hayward  <alan.hayward@arm.com>
>>>
>>> 	* linux-low.c (linux_attach): Add process before lwp.
>>
>>> ---
>>> gdb/gdbserver/linux-low.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
>>> index 17cce24d76..1ab2cfb1eb 100644
>>> --- a/gdb/gdbserver/linux-low.c
>>> +++ b/gdb/gdbserver/linux-low.c
>>> @@ -1188,6 +1188,8 @@ linux_attach (unsigned long pid)
>>>   ptid_t ptid = ptid_t (pid, pid, 0);
>>>   int err;
>>>
>>> +  proc = linux_add_process (pid, 1);
>>> +
>>>   /* Attach to PID.  We will check for other threads
>>>      soon.  */
>>>   err = linux_attach_lwp (ptid);
>>> @@ -1198,8 +1200,6 @@ linux_attach (unsigned long pid)
>>>       error ("Cannot attach to process %ld: %s", pid, reason.c_str ());
>>>     }
>>>
>>> -  proc = linux_add_process (pid, 1);
>>> -
>>
>> This fails to consider the error conditions.  That error call
>> visible above throws an exception.
>>
>> - If GDBserver is already attached to the process, this will
>>  create another process_info object for the already-attached
>>  process, and leave it behind.
>>
>>  For this case, it would seem better to check whether we're
>>  already attached to a given PID in common code, around
>>  server.c:attach_inferior.
> 
> Assuming you are suggesting adding a check if the process object
> exists in gdbserver, rather than adding a new target func which
> then calls out to /proc.

Yes.  I don't really understand what the call to /proc would do.

> 
>>
>> - If GDBserver isn't already attached, but the attach fails,
>>  this will likewise leave a stale process_info object behind.
>>
> 
> I think I’m probably missing something here, but if the attach
> fails for any reason then gdbserver will error and exit - so it
> won’t matter if the object is not deleted?

Try "gdbserver --multi" + "target extended-remote".
With that, gdbserver won't exit when a process exits.  The
connection stays open, ready for a "run" or "attach" command
to spawn new processes on the server side.

> 
> 
> New patch with the above changes. Ok?

Can you add a test for that error case to gdb.base/attach.exp, please?  

 (gdb) attach PID
 (gdb) add-iferior
 (gdb) inferior 2
 (gdb) attach PID  # should fail.
 # restart gdb

The restart gdb step checks whether gdbserver crashes / or misbehaves
while detaching from the process.

Thanks,
-- 
Thanks,
Pedro Alves

Re: [PATCH] gdbserver: When attaching, add process before lwps

[Pedro Alves]

On 02/01/2019 12:30 PM, Pedro Alves wrote:

> The restart gdb step checks whether gdbserver crashes / or misbehaves
> while detaching from the process.

Forgot to say, make sure to test with

 --target_board=native-extended-gdbserver

Thanks,
Pedro Alves