[patch] failed to attach to the same process after detaching in gdbserver multiprocess mode

[patch] failed to attach to the same process after detaching in gdbserver multiprocess mode

[Liang Cheng]

Reproduce step:
1. compile and link the test app (which contains an infinite loop)
2. run the app
3. start gdbserver in multiprocess mode (e.g. gdbserver --multi :5040)
3. attach the running process in gdb (e.g. target extended-remote
:5040; attach pid)
4. continue to run the app in gdb, pressing "c"
5. stop the execution of the app by pressing ctrl-c in gdb
4. detach
5. quit
7. attach the same process in gdb (e.g. target extended-remote :5040;
attach pid)

User would get something like below error in gdb
(gdb) attach 19372
Ignoring packet error, continuing...
Attached to process 19372
Ignoring packet error, continuing...
Ignoring packet error, continuing...
Ignoring packet error, continuing...
warning: Invalid remote reply: timeout

This issue does not happen in gdb-7.1.0.20100706.tar, but it happens
sometime after wards. The fix would reset cont_thread at detach.

Is the below patch OK? If it is, can anyone help to submit it as I
don't have account with write-access to the cvs?

Thanks,
Liang

licheng@licheng-linux:~/gdb-7.3/gdb-7.3.50.20110622/gdb/gdbserver$
diff -Naur linux-low.c.orig linux-low.c
--- linux-low.c.orig    2011-05-04 15:20:12.000000000 -0500
+++ linux-low.c 2011-06-23 15:05:26.633193953 -0500
@@ -837,6 +837,9 @@
   /* Since we presently can only stop all lwps of all processes, we
      need to unstop lwps of other processes.  */
   unstop_all_lwps (0, NULL);
+
+  /* reset cont_thread */
+  cont_thread = null_ptid;
   return 0;
 }

Re: [patch] failed to attach to the same process after detaching in gdbserver multiprocess mode

[Joel Brobecker]

Liang,

Thanks for sending these patches in.  I can't really approve your
patches since I don't have expertise in that area, but I can comment
on a couple of things:
  - It's great that you take the time to explain what the problem is;
  - Patches should have a ChangeLog entry to accompany them;
  - One nit: It's awesome that you take the time to add comments
    to document the code, and we don't use them often enough IMO.
    But I think that, in general, we want the comments in the code
    to say *why* you do what you do.  And oh, comments should start
    with a capital letter, and end with a period (and we always put
    2 spaces after period, even before a closing */

> +
> +  /* reset cont_thread */
> +  cont_thread = null_ptid;
>    return 0;

-- 
Joel

Re: [patch] failed to attach to the same process after detaching in gdbserver multiprocess mode

[Liang Cheng]

Thanks Joel, for your comments.

This problem is that Linux_wait_for_event_1 does not return in
linux_wait code path. So gdbserver stucks at linux-low.c::my_waitpid,
and does not return OK/response to gdb. Source code level: old gdb
resets lwp->stop_expected to 0 in linux-low.c::linux_attach if
!non_stop, but 'pedro@codesourcery.com' removes these lines in version
1.134 of linux-low.c. Instead, he replaced it with
'thread_last_resume_kind = resume_stop', which is supposed to work.
But as cross the different debug sessions on the same process (detach,
q; and attach again in multiple process debug mode), cont_thread is
saved and does not get reset at detaching. gdb server will use this
stale 'cont_thread' to make a decision: resume the stopped process in
the 2nd debug session. Thus, gdbserver does not get any chance of
returning the call from linux_wait, while gdb waits for its response,
until gdb tries 3 times of reading response.

The fix is to reset the cont_thread so that gdbserver won't use stale
states in multiple process mode.

Hope above explanation is clear. Adding ChangeLog and fixing the
format issue in comments.  Feel free to make further adjustment on the
comments or the log if necessary.

Thanks
Liang


2011-06-27  Liang Cheng  <liang.cheng555@gmail.com>

	* linux-low.c (linux_detach): Reset cont_thread.

--- linux-low.c.orig    2011-05-04 15:20:12.000000000 -0500
+++ linux-low.c 2011-06-27 11:45:36.223194361 -0500
@@ -837,6 +837,9 @@
   /* Since we presently can only stop all lwps of all processes, we
      need to unstop lwps of other processes.  */
   unstop_all_lwps (0, NULL);
+
+  /* Reset cont_thread.  */
+  cont_thread = null_ptid;
   return 0;
 }


On Mon, Jun 27, 2011 at 10:54 AM, Joel Brobecker <brobecker@adacore.com> wrote:
> Liang,
>
> Thanks for sending these patches in.  I can't really approve your
> patches since I don't have expertise in that area, but I can comment
> on a couple of things:
>  - It's great that you take the time to explain what the problem is;
>  - Patches should have a ChangeLog entry to accompany them;
>  - One nit: It's awesome that you take the time to add comments
>    to document the code, and we don't use them often enough IMO.
>    But I think that, in general, we want the comments in the code
>    to say *why* you do what you do.  And oh, comments should start
>    with a capital letter, and end with a period (and we always put
>    2 spaces after period, even before a closing */
>
>> +
>> +  /* reset cont_thread */
>> +  cont_thread = null_ptid;
>>    return 0;
>
> --
> Joel
>