[Cygwin] Fix for: detaching crashes the inferior.

[Cygwin] Fix for: detaching crashes the inferior.

[Pedro Alves]

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

Hi all,

Detaching on Cygwin often crashes the inferior, either
when gdb attaches to the inferior, or when gdb launches
the inferior as a child.

Initially I thought that somehow the FLAGS_TRACE_BIT
would be set in the context, and after detaching there would
be no debugger to catch the exception, but it turns out that
it isn't the case - I couldn't find a case where FLAGS_TRACE_BIT
was set.  But, surprisingly doing a SetThreadContext with
CONTEXT_CONTROL makes the crashes go away - even if you don't
change the contents of the context.  I'm not 100% sure why, but,
it fixes it.  If MSFT sends me the Windows sources, I'll look
deeper :)

No regressions on i686-pc-cygwin on XP Pro.

OK?

Cheers,
Pedro Alves





[-- Attachment #2: win32_detach.diff --]
[-- Type: text/x-diff, Size: 1436 bytes --]

2007-04-08  Pedro Alves  <pedro_alves@portugalmail.pt>

	* win32-nat.c (win32_detach): Reset the CONTEXT_CONTROL part of
	the main thread's context and clear the FLAGS_TRACE_BIT
	before detaching.

---
 gdb/win32-nat.c |   18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

Index: src/gdb/win32-nat.c
===================================================================
--- src.orig/gdb/win32-nat.c	2007-04-08 12:11:22.000000000 +0100
+++ src/gdb/win32-nat.c	2007-04-08 12:11:54.000000000 +0100
@@ -1775,7 +1775,23 @@ win32_detach (char *args, int from_tty)
 
   if (has_detach_ability ())
     {
-      delete_command (NULL, 0);
+      thread_info *th;
+
+      /* Don't leave breakpoints in the inferior.  */
+      remove_breakpoints ();
+
+      /* We also need to reset the thread's context,
+	 else we'll get frequent inferior crashes.  */
+      th = thread_rec (current_event.dwThreadId, FALSE);
+      th->context.ContextFlags = CONTEXT_CONTROL;
+      win32_fetch_inferior_registers (PS_REGNUM);
+      /* Doing a SetThreadContext with CONTEXT_CONTROL seems to be
+	 enough, but let's clear FLAGS_TRACE_BIT just in case
+	 somewhere else it gets set.   */
+      th->context.EFlags &= ~FLAG_TRACE_BIT;
+      CHECK (SetThreadContext (th->h, &th->context));
+      th->context.ContextFlags = 0;
+
       win32_continue (DBG_CONTINUE, -1);
       if (!DebugActiveProcessStop (current_event.dwProcessId))
 	{

Re: [Cygwin] Fix for: detaching crashes the inferior.

[Pedro Alves]

Pedro Alves wrote:
> Hi all,
>
> Detaching on Cygwin often crashes the inferior, either
> when gdb attaches to the inferior, or when gdb launches
> the inferior as a child.
>
> Initially I thought that somehow the FLAGS_TRACE_BIT
> would be set in the context, and after detaching there would
> be no debugger to catch the exception, but it turns out that
> it isn't the case - I couldn't find a case where FLAGS_TRACE_BIT
> was set.  But, surprisingly doing a SetThreadContext with
> CONTEXT_CONTROL makes the crashes go away - even if you don't
> change the contents of the context.  I'm not 100% sure why, but,
> it fixes it.  If MSFT sends me the Windows sources, I'll look
> deeper :)

I'm seeing something I missed.  There may be a more correct fix.
Ignore this patch for now.

Cheers,
Pedro Alves

Re: [Cygwin] Fix for: detaching crashes the inferior.

[Pedro Alves]

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

Pedro Alves wrote:
> Pedro Alves wrote:
>> Hi all,
>>
>> Detaching on Cygwin often crashes the inferior, either
>> when gdb attaches to the inferior, or when gdb launches
>> the inferior as a child.
>>

Here is a new version.

The real reason for the crashes is that we should be setting
the context back to the inferior, as gdb's copy contains
an adjusted PC.  With win32_continue we would resume the
inferior at the wrong address.  win32_resume takes care of
that, so the simple fix is to use it when detaching.

The current code uses delete_command to remove breakpoints, but
that leaves the internal breakpoints behind - not something
we want.  Is there a case where we can get to to_detach
without remove_breakpoints being called?  I don't think there
is - we always get here through normal_stop, right?
If there isn't, I'll just remove the call to it I'm
introducing in the patch.

Cheers,
Pedro Alves




[-- Attachment #2: win32_detach.diff --]
[-- Type: text/x-diff, Size: 998 bytes --]

2007-04-09  Pedro Alves  <pedro_alves@portugalmail.pt>

	* win32-nat.c (win32_detach): Remove breakpoints with
	remove_breakpoints instead of delete_command.  Resume inferior
	with win32_resume instead of win32_continue.

---
 gdb/win32-nat.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Index: src/gdb/win32-nat.c
===================================================================
--- src.orig/gdb/win32-nat.c	2007-04-08 12:11:22.000000000 +0100
+++ src/gdb/win32-nat.c	2007-04-09 02:48:50.000000000 +0100
@@ -1775,8 +1775,13 @@ win32_detach (char *args, int from_tty)
 
   if (has_detach_ability ())
     {
-      delete_command (NULL, 0);
-      win32_continue (DBG_CONTINUE, -1);
+      ptid_t ptid = {-1};
+
+      /* Don't leave breakpoints in the inferior.  */
+      remove_breakpoints ();
+
+      win32_resume (ptid, 0, TARGET_SIGNAL_0);
+
       if (!DebugActiveProcessStop (current_event.dwProcessId))
 	{
 	  error (_("Can't detach process %lu (error %lu)"),

Re: [Cygwin] Fix for: detaching crashes the inferior.

[Eli Zaretskii]

> Date: Mon, 09 Apr 2007 03:06:20 +0100
> From: Pedro Alves <pedro_alves@portugalmail.pt>
> 
> Is there a case where we can get to to_detach
> without remove_breakpoints being called?  I don't think there
> is - we always get here through normal_stop, right?

If you think this must be so, but are unsure, you can add a flag that
is turned off in remove_breakpoints and turned on when a breakpoint is
set.  Then, in to_detach, if the flag is on, you can scream bloody
murder (and even abort(), if you think it's fatal).  Once users start
using the modified version, you will soon know whether the assumption
was wrong...

Re: [Cygwin] Fix for: detaching crashes the inferior.

[Pedro Alves]

On 4/9/07, Eli Zaretskii <eliz@gnu.org> wrote:
> > Date: Mon, 09 Apr 2007 03:06:20 +0100
> > From: Pedro Alves <pedro_alves@portugalmail.pt>
> >
> > Is there a case where we can get to to_detach
> > without remove_breakpoints being called?  I don't think there
> > is - we always get here through normal_stop, right?
>
> If you think this must be so, but are unsure, you can add a flag that
> is turned off in remove_breakpoints and turned on when a breakpoint is
> set.  Then, in to_detach, if the flag is on, you can scream bloody
> murder (and even abort(), if you think it's fatal).  Once users start
> using the modified version, you will soon know whether the assumption
> was wrong...
>

:) Thanks for the hint, I'll try to do that.

I think that gdb by design always removes the breakpoints from the
inferior when it stops, but it would be nice if someone would confirm
or unconfirm this.

Cheers,
Pedro Alves

Re: [Cygwin] Fix for: detaching crashes the inferior.

[Daniel Jacobowitz]

On Tue, Apr 10, 2007 at 11:33:15AM +0100, Pedro Alves wrote:
> I think that gdb by design always removes the breakpoints from the
> inferior when it stops, but it would be nice if someone would confirm
> or unconfirm this.

You are correct.  I think we won't be able to keep doing that forever,
but if we stop, it shouldn't become to_detach's responsibility to
handle it.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [Cygwin] Fix for: detaching crashes the inferior.

[Michael Snyder]

On Tue, 2007-04-10 at 11:33 +0100, Pedro Alves wrote:
> On 4/9/07, Eli Zaretskii <eliz@gnu.org> wrote:
> > > Date: Mon, 09 Apr 2007 03:06:20 +0100
> > > From: Pedro Alves <pedro_alves@portugalmail.pt>
> > >
> > > Is there a case where we can get to to_detach
> > > without remove_breakpoints being called?  I don't think there
> > > is - we always get here through normal_stop, right?
> >
> > If you think this must be so, but are unsure, you can add a flag that
> > is turned off in remove_breakpoints and turned on when a breakpoint is
> > set.  Then, in to_detach, if the flag is on, you can scream bloody
> > murder (and even abort(), if you think it's fatal).  Once users start
> > using the modified version, you will soon know whether the assumption
> > was wrong...
> >
> 
> :) Thanks for the hint, I'll try to do that.
> 
> I think that gdb by design always removes the breakpoints from the
> inferior when it stops, but it would be nice if someone would confirm
> or unconfirm this.

GDB by design always removes the beakpoints from the 
inferior when it stops.  

;-)

Re: [Cygwin] Fix for: detaching crashes the inferior.

[Eli Zaretskii]

> Date: Tue, 10 Apr 2007 11:33:15 +0100
> From: "Pedro Alves" <pedro_alves@portugalmail.pt>
> Cc: gdb-patches@sourceware.org
> 
> I think that gdb by design always removes the breakpoints from the
> inferior when it stops, but it would be nice if someone would confirm
> or unconfirm this.

I think you are right, but you wanted to be 110% sure...

Re: [Cygwin] Fix for: detaching crashes the inferior.

[Christopher Faylor]

On Mon, Apr 09, 2007 at 03:06:20AM +0100, Pedro Alves wrote:
>Pedro Alves wrote:
>>Pedro Alves wrote:
>>>Hi all,
>>>
>>>Detaching on Cygwin often crashes the inferior, either
>>>when gdb attaches to the inferior, or when gdb launches
>>>the inferior as a child.
>>>
>
>Here is a new version.
>
>The real reason for the crashes is that we should be setting
>the context back to the inferior, as gdb's copy contains
>an adjusted PC.  With win32_continue we would resume the
>inferior at the wrong address.  win32_resume takes care of
>that, so the simple fix is to use it when detaching.
>
>The current code uses delete_command to remove breakpoints, but
>that leaves the internal breakpoints behind - not something
>we want.  Is there a case where we can get to to_detach
>without remove_breakpoints being called?  I don't think there
>is - we always get here through normal_stop, right?
>If there isn't, I'll just remove the call to it I'm
>introducing in the patch.

Are you sure you aren't seeing problems when you attach/detach more than
once?  I know that there is someplace that isn't getting cleared in that
scenario.  I have thought that I fixed that problem a few times but it
still seems to be there in the latest cygwin release.

Anyway, I'd like Corinna to comment on this if possible since that
particular piece of code that you're patching is hers, I believe.

cgf

Re: [Cygwin] Fix for: detaching crashes the inferior.

[Corinna Vinschen]

On Apr 10 01:15, Christopher Faylor wrote:
> On Mon, Apr 09, 2007 at 03:06:20AM +0100, Pedro Alves wrote:
> >The real reason for the crashes is that we should be setting
> >the context back to the inferior, as gdb's copy contains
> >an adjusted PC.  With win32_continue we would resume the
> >inferior at the wrong address.  win32_resume takes care of
> >that, so the simple fix is to use it when detaching.
> >[...]
> Are you sure you aren't seeing problems when you attach/detach more than
> once?  I know that there is someplace that isn't getting cleared in that
> scenario.  I have thought that I fixed that problem a few times but it
> still seems to be there in the latest cygwin release.

What "someplace" are you referring to, Chris?  Maybe that can be solved,
too, while we're at it.

> Anyway, I'd like Corinna to comment on this if possible since that
> particular piece of code that you're patching is hers, I believe.

Calling win32_resume seems actually to be the better choice here.  The
patch looks ok to me.


Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

Re: [Cygwin] Fix for: detaching crashes the inferior.

[Pedro Alves]

On 4/10/07, Corinna Vinschen wrote:
> On Apr 10 01:15, Christopher Faylor wrote:

> > Anyway, I'd like Corinna to comment on this if possible since that
> > particular piece of code that you're patching is hers, I believe.
>
> Calling win32_resume seems actually to be the better choice here.  The
> patch looks ok to me.
>

Thanks Corinna,

I'll wait for Chris' OK before commiting.

Cheers,
Pedro Alves

Re: [Cygwin] Fix for: detaching crashes the inferior.

[Pedro Alves]

On 4/10/07, Christopher Faylor wrote:

> Are you sure you aren't seeing problems when you attach/detach more than
> once?  I know that there is someplace that isn't getting cleared in that
> scenario.  I have thought that I fixed that problem a few times but it
> still seems to be there in the latest cygwin release.
>

I tried that many times, with the following test program:

$ cat main.c
#include <stdio.h>
#include <windows.h>

int
main(int argc, char** argv)
{
  int count = 0;
  while (1)
    {
      printf ("count = %d\n", count++);
      fflush (stdout);
      Sleep (1000);
    }
}

I attached with gdb --pid <pid>, set a break on line 9, continue,
breakpoint is hit, hit next a couple of times, detach, reattach without
restarting gdb, continue till the breakpoint, step a couple times,
remove breakpoint, detach, restart gdb, attach, do the same, but
letting gdb start the inferior as a child, redo the testing a couple of
times, and nothing bad happens...

... except that once, I saw for this warning:
(gdb) attach 2396
Warning: Failed to get SE_DEBUG_NAME privilege
This can cause attach to fail on Windows NT/2K/XP
Attaching to program `/cygdrive/d/gdb-20070402/build/main.exe', process 2396
Loaded symbols for /cygdrive/c/WINNT/system32/ntdll.dll
Loaded symbols for /cygdrive/c/WINNT/system32/kernel32.dll
Loaded symbols for /usr/bin/cygwin1.dll
Loaded symbols for /cygdrive/c/WINNT/system32/advapi32.dll
Loaded symbols for /cygdrive/c/WINNT/system32/rpcrt4.dll
Loaded symbols for /cygdrive/c/WINNT/system32/secur32.dll
Loaded symbols for /cygdrive/c/WINNT/system32/user32.dll
Loaded symbols for /cygdrive/c/WINNT/system32/gdi32.dll
[Switching to thread 2396.0xe3c]

Have no idea if that is really a problem, but it should be a
separate patch.

Cheers,
Pedro Alves

Re: [Cygwin] Fix for: detaching crashes the inferior.

[Christopher Faylor]

On Tue, Apr 10, 2007 at 11:45:39AM +0100, Pedro Alves wrote:
>On 4/10/07, Christopher Faylor wrote:
>
>>Are you sure you aren't seeing problems when you attach/detach more than
>>once?  I know that there is someplace that isn't getting cleared in that
>>scenario.  I have thought that I fixed that problem a few times but it
>>still seems to be there in the latest cygwin release.
>>
>
>I tried that many times, with the following test program:
>
>$ cat main.c
>#include <stdio.h>
>#include <windows.h>
>
>int
>main(int argc, char** argv)
>{
> int count = 0;
> while (1)
>   {
>     printf ("count = %d\n", count++);
>     fflush (stdout);
>     Sleep (1000);
>   }
>}
>
>I attached with gdb --pid <pid>, set a break on line 9, continue,
>breakpoint is hit, hit next a couple of times, detach, reattach without
>restarting gdb, continue till the breakpoint, step a couple times,
>remove breakpoint, detach, restart gdb, attach, do the same, but
>letting gdb start the inferior as a child, redo the testing a couple of
>times, and nothing bad happens...

I'm not necessarily talking about attaching and restarting the same
program multiple times.  I don't know the exact scenario when I see the
problem (or I probably would have fixed it) but I think it is possibly
when attaching after doing a normal "gdb program" or maybe just killing
and rerunning after attaching.

Like I said, I don't know for sure but I wasn't asking you to fix the
problem.  Go ahead and check in what you've got.

cgf

Re: [Cygwin] Fix for: detaching crashes the inferior.

[Pedro Alves]

Pedro Alves wrote:
>	* win32-nat.c (win32_detach): Remove delete_command call.
>	Resume inferior with win32_resume instead of win32_continue.

Christopher Faylor wrote:
> Go ahead and check in what you've got.
>   

Thanks.  Checked in.

Cheers,
Pedro Alves

Re: [Cygwin] Fix for: detaching crashes the inferior.

[Pedro Alves]

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


>> Pedro Alves wrote:
>>
>> The current code uses delete_command to remove breakpoints, but
>> that leaves the internal breakpoints behind - not something
>> we want.  Is there a case where we can get to to_detach
>> without remove_breakpoints being called?  I don't think there
>> is - we always get here through normal_stop, right?
>> If there isn't, I'll just remove the call to it I'm
>> introducing in the patch.
>>     

Here is the updated patch that removes the call to delete_command.

OK?

Cheers,
Pedro Alves


[-- Attachment #2: win32_detach.diff --]
[-- Type: text/x-diff, Size: 857 bytes --]

2007-04-10  Pedro Alves  <pedro_alves@portugalmail.pt>

	* win32-nat.c (win32_detach): Remove delete_command call.
	Resume inferior with win32_resume instead of win32_continue.

---
 gdb/win32-nat.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Index: src/gdb/win32-nat.c
===================================================================
--- src.orig/gdb/win32-nat.c	2007-04-08 12:11:22.000000000 +0100
+++ src/gdb/win32-nat.c	2007-04-10 21:31:02.000000000 +0100
@@ -1775,8 +1775,9 @@ win32_detach (char *args, int from_tty)
 
   if (has_detach_ability ())
     {
-      delete_command (NULL, 0);
-      win32_continue (DBG_CONTINUE, -1);
+      ptid_t ptid = {-1};
+      win32_resume (ptid, 0, TARGET_SIGNAL_0);
+
       if (!DebugActiveProcessStop (current_event.dwProcessId))
 	{
 	  error (_("Can't detach process %lu (error %lu)"),

Re: [Cygwin] Fix for: detaching crashes the inferior.

[Corinna Vinschen]

On Apr 10 22:39, Pedro Alves wrote:
> 
> >>Pedro Alves wrote:
> >>
> >>The current code uses delete_command to remove breakpoints, but
> >>that leaves the internal breakpoints behind - not something
> >>we want.  Is there a case where we can get to to_detach
> >>without remove_breakpoints being called?  I don't think there
> >>is - we always get here through normal_stop, right?
> >>If there isn't, I'll just remove the call to it I'm
> >>introducing in the patch.
> >>    
> 
> Here is the updated patch that removes the call to delete_command.
> 
> OK?

Ok with me, but Chris has the final word.


Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat