[RFA] windows-nat.c Cygwin save_context fix

[RFA] windows-nat.c Cygwin save_context fix

[Pierre Muller]

  Cygwin has a way to give a "fake" context for 
an exception using a OUTPUT_DEBUG_STRING_EVENT.

  While trying to debug some crashes inside cygwin dll,
I realized the the saved_context code has a problem.

  The saved context was correctly written to the thread_info
struct, but later overwritten by a call to GetThreadContext.
 
  After the cygwin special output_debug_string was correctly
converted into a context stored in saved_context.
  The next call to do_windows_fetch_inferior_registers
then copied this context to the thread_info struct,
set the reload_context field to zero.

  But a later call to thread_rec() with get_context=1
can reset reload_context to one, if suspended field
is zero (this only happens if it is not the main thread).

  My patch fixes the problem by explicitly calling
SuspendThread for the threaded of the saved_context
if suspended is still zero at that point.


Pierre Muller
Pascal language support maintainer for GDB



2009-09-21  Pierre Muller  <muller@ics.u-strasbg.fr>

	* src/gdb/windows-nat.c (saved_threadid): New variable.
	(do_windows_fetch_inferior_registers): Check for correct thread id
	and force call to SuspendThread if needed.
	(handle_output_debug_string): Set saved_threadid.

Index: src/gdb/windows-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/windows-nat.c,v
retrieving revision 1.196
diff -u -p -r1.196 windows-nat.c
--- src/gdb/windows-nat.c	2 Jul 2009 17:21:07 -0000	1.196
+++ src/gdb/windows-nat.c	21 Sep 2009 14:53:38 -0000
@@ -97,6 +98,7 @@ static CORE_ADDR cygwin_load_start;
 static CORE_ADDR cygwin_load_end;
 #endif
 
+static int saved_threadid;
 static int have_saved_context;	/* True if we've saved context from a cygwin
signal. */
 static CONTEXT saved_context;	/* Containes the saved context from a cygwin
signal. */
 
@@ -381,11 +383,23 @@ do_windows_fetch_inferior_registers (str
   if (current_thread->reload_context)
     {
 #ifdef __COPY_CONTEXT_SIZE
-      if (have_saved_context)
+      if (have_saved_context && current_thread->id == saved_threadid)
 	{
 	  /* Lie about where the program actually is stopped since cygwin
has informed us that
 	     we should consider the signal to have occurred at another
location which is stored
 	     in "saved_context. */
+	  if (!current_thread->suspended)
+	  /* Force suspend to avoid resetting reload_context
+	     later in get_thread.  */ 
+	    {
+	      if (SuspendThread (current_thread->h) == (DWORD) -1)
+		{
+		  DWORD err = GetLastError ();
+		  warning (_("SuspendThread failed. (winerr %d)"),
+		  	   (int) err);
+		}
+	      current_thread->suspended = 1;
+	    }
 	  memcpy (&current_thread->context, &saved_context,
__COPY_CONTEXT_SIZE);
 	  have_saved_context = 0;
 	}
@@ -863,6 +877,7 @@ handle_output_debug_string (struct targe
 					 &saved_context,
__COPY_CONTEXT_SIZE, &n)
 		   && n == __COPY_CONTEXT_SIZE)
 	    have_saved_context = 1;
+	  saved_threadid = retval;
 	  current_event.dwThreadId = retval;
 	}
     }

Re: [RFA] windows-nat.c Cygwin save_context fix

[Christopher Faylor]

On Mon, Sep 21, 2009 at 05:14:57PM +0200, Pierre Muller wrote:
>  Cygwin has a way to give a "fake" context for 
>an exception using a OUTPUT_DEBUG_STRING_EVENT.
>
>  While trying to debug some crashes inside cygwin dll,
>I realized the the saved_context code has a problem.
>
>  The saved context was correctly written to the thread_info
>struct, but later overwritten by a call to GetThreadContext.
> 
>  After the cygwin special output_debug_string was correctly
>converted into a context stored in saved_context.
>  The next call to do_windows_fetch_inferior_registers
>then copied this context to the thread_info struct,
>set the reload_context field to zero.
>
>  But a later call to thread_rec() with get_context=1
>can reset reload_context to one, if suspended field
>is zero (this only happens if it is not the main thread).
>
>  My patch fixes the problem by explicitly calling
>SuspendThread for the threaded of the saved_context
>if suspended is still zero at that point.
>
>
>Pierre Muller
>Pascal language support maintainer for GDB
>
>
>
>2009-09-21  Pierre Muller  <muller@ics.u-strasbg.fr>
>
>	* src/gdb/windows-nat.c (saved_threadid): New variable.
>	(do_windows_fetch_inferior_registers): Check for correct thread id
>	and force call to SuspendThread if needed.
>	(handle_output_debug_string): Set saved_threadid.

I REALLY hate SuspendThread and am not likely to accept a patch which
uses it as a solution.

I don't really understand the scenario that you are talking about since
I (obviously?) have no problems debugging SEGVs in cygwin.  However,
would just removing the "have_saved_context = 0;" from
do_windows_fetch_inferior_registers fix the problem?

cgf

RE: [RFA] windows-nat.c Cygwin save_context fix

[Pierre Muller]

> -----Message d'origine-----
> De : Christopher Faylor [mailto:cgf-use-the-mailinglist-
> please@sourceware.org]
> Envoyé : Tuesday, September 22, 2009 12:27 AM
> À : gdb-patches@sourceware.org; Pierre Muller
> Objet : Re: [RFA] windows-nat.c Cygwin save_context fix
> 
> On Mon, Sep 21, 2009 at 05:14:57PM +0200, Pierre Muller wrote:
> >  Cygwin has a way to give a "fake" context for
> >an exception using a OUTPUT_DEBUG_STRING_EVENT.
> >
> >  While trying to debug some crashes inside cygwin dll,
> >I realized the the saved_context code has a problem.
> >
> >  The saved context was correctly written to the thread_info
> >struct, but later overwritten by a call to GetThreadContext.
> >
> >  After the cygwin special output_debug_string was correctly
> >converted into a context stored in saved_context.
> >  The next call to do_windows_fetch_inferior_registers
> >then copied this context to the thread_info struct,
> >set the reload_context field to zero.
> >
> >  But a later call to thread_rec() with get_context=1
> >can reset reload_context to one, if suspended field
> >is zero (this only happens if it is not the main thread).
> >
> >  My patch fixes the problem by explicitly calling
> >SuspendThread for the threaded of the saved_context
> >if suspended is still zero at that point.
> >
> >
> >Pierre Muller
> >Pascal language support maintainer for GDB
> >
> >
> >
> >2009-09-21  Pierre Muller  <muller@ics.u-strasbg.fr>
> >
> >	* src/gdb/windows-nat.c (saved_threadid): New variable.
> >	(do_windows_fetch_inferior_registers): Check for correct thread
> id
> >	and force call to SuspendThread if needed.
> >	(handle_output_debug_string): Set saved_threadid.
> 
> I REALLY hate SuspendThread and am not likely to accept a patch which
> uses it as a solution.

  I don't think that we really need to call SuspendThread,
in fact, I suspect the best would be to set suspended
to -1 as for main_threadid. I will try to check out if this also works.

> I don't really understand the scenario that you are talking about since
> I (obviously?) have no problems debugging SEGVs in cygwin.  However,

  I was trying to debug a home compiled expect 
linked to 8.5.7 tcl library, that was call abort,
and the subsequent cygwin stackdump was segfaulting.

> would just removing the "have_saved_context = 0;" from
> do_windows_fetch_inferior_registers fix the problem?

But them we need to be really careful about to where
we should reset it to zero, otherwise we might end up 
by still copying saved_context, even after having reran the debuggee, no?

Pierre

[RFA-v2] windows-nat.c Cygwin saved_context fix

[Pierre Muller]

After testing,
setting suspended field to -1
seems to work equally well for me.

  Here is an updated patch:


2009-09-22  Pierre Muller  <muller@ics.u-strasbg.fr>

	* windows-nat.c (saved_threadid): New variable.
	(do_windows_fetch_inferior_registers): Check for correct thread id
	and set suspended field to -1 if it is zero.
	(handle_output_debug_string): Set saved_threadid.

Index: src/gdb/windows-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/windows-nat.c,v
retrieving revision 1.197
diff -u -p -r1.197 windows-nat.c
--- src/gdb/windows-nat.c	21 Sep 2009 22:37:59 -0000	1.197
+++ src/gdb/windows-nat.c	21 Sep 2009 22:53:11 -0000
@@ -97,6 +97,7 @@ static CORE_ADDR cygwin_load_start;
 static CORE_ADDR cygwin_load_end;
 #endif
 
+static int saved_threadid;
 static int have_saved_context;	/* True if we've saved context from a cygwin
signal. */
 static CONTEXT saved_context;	/* Containes the saved context from a cygwin
signal. */
 
@@ -381,11 +382,15 @@ do_windows_fetch_inferior_registers (str
   if (current_thread->reload_context)
     {
 #ifdef __COPY_CONTEXT_SIZE
-      if (have_saved_context)
+      if (have_saved_context && current_thread->id == saved_threadid)
 	{
 	  /* Lie about where the program actually is stopped since cygwin
has informed us that
 	     we should consider the signal to have occurred at another
location which is stored
 	     in "saved_context. */
+	  if (!current_thread->suspended)
+	  /* Set suspended to -1 to avoid resetting reload_context
+	     later in get_thread.  */ 
+	    current_thread->suspended = -1;
 	  memcpy (&current_thread->context, &saved_context,
__COPY_CONTEXT_SIZE);
 	  have_saved_context = 0;
 	}
@@ -863,6 +868,7 @@ handle_output_debug_string (struct targe
 					 &saved_context,
__COPY_CONTEXT_SIZE, &n)
 		   && n == __COPY_CONTEXT_SIZE)
 	    have_saved_context = 1;
+	  saved_threadid = retval;
 	  current_event.dwThreadId = retval;
 	}
     }

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Pierre Muller
> Envoyé : Tuesday, September 22, 2009 12:36 AM
> À : gdb-patches@sourceware.org
> Objet : RE: [RFA] windows-nat.c Cygwin save_context fix
> 
> 
> 
> > -----Message d'origine-----
> > De : Christopher Faylor [mailto:cgf-use-the-mailinglist-
> > please@sourceware.org]
> > Envoyé : Tuesday, September 22, 2009 12:27 AM
> > À : gdb-patches@sourceware.org; Pierre Muller
> > Objet : Re: [RFA] windows-nat.c Cygwin save_context fix
> >
> > On Mon, Sep 21, 2009 at 05:14:57PM +0200, Pierre Muller wrote:
> > >  Cygwin has a way to give a "fake" context for
> > >an exception using a OUTPUT_DEBUG_STRING_EVENT.
> > >
> > >  While trying to debug some crashes inside cygwin dll,
> > >I realized the the saved_context code has a problem.
> > >
> > >  The saved context was correctly written to the thread_info
> > >struct, but later overwritten by a call to GetThreadContext.
> > >
> > >  After the cygwin special output_debug_string was correctly
> > >converted into a context stored in saved_context.
> > >  The next call to do_windows_fetch_inferior_registers
> > >then copied this context to the thread_info struct,
> > >set the reload_context field to zero.
> > >
> > >  But a later call to thread_rec() with get_context=1
> > >can reset reload_context to one, if suspended field
> > >is zero (this only happens if it is not the main thread).
> > >
> > >  My patch fixes the problem by explicitly calling
> > >SuspendThread for the threaded of the saved_context
> > >if suspended is still zero at that point.
> > >
> > >
> > >Pierre Muller
> > >Pascal language support maintainer for GDB
> > >
> > >
> > >
> > >2009-09-21  Pierre Muller  <muller@ics.u-strasbg.fr>
> > >
> > >	* src/gdb/windows-nat.c (saved_threadid): New variable.
> > >	(do_windows_fetch_inferior_registers): Check for correct thread
> > id
> > >	and force call to SuspendThread if needed.
> > >	(handle_output_debug_string): Set saved_threadid.
> >
> > I REALLY hate SuspendThread and am not likely to accept a patch which
> > uses it as a solution.
> 
>   I don't think that we really need to call SuspendThread,
> in fact, I suspect the best would be to set suspended
> to -1 as for main_threadid. I will try to check out if this also works.
> 
> > I don't really understand the scenario that you are talking about
> since
> > I (obviously?) have no problems debugging SEGVs in cygwin.  However,
> 
>   I was trying to debug a home compiled expect
> linked to 8.5.7 tcl library, that was call abort,
> and the subsequent cygwin stackdump was segfaulting.
> 
> > would just removing the "have_saved_context = 0;" from
> > do_windows_fetch_inferior_registers fix the problem?
> 
> But them we need to be really careful about to where
> we should reset it to zero, otherwise we might end up
> by still copying saved_context, even after having reran the debuggee,
> no?
> 
> Pierre
> 

Re: [RFA] windows-nat.c Cygwin save_context fix

[Christopher Faylor]

On Tue, Sep 22, 2009 at 12:35:38AM +0200, Pierre Muller wrote:
>cgf wrote:
>>would just removing the "have_saved_context = 0;" from
>>do_windows_fetch_inferior_registers fix the problem?
>
>But them we need to be really careful about to where we should reset it
>to zero, otherwise we might end up by still copying saved_context, even
>after having reran the debuggee, no?

No.  It's reset every time there is a debug event.

cgf

Re: [RFA-v2] windows-nat.c Cygwin saved_context fix

[Christopher Faylor]

On Tue, Sep 22, 2009 at 01:01:13AM +0200, Pierre Muller wrote:
>After testing,
>setting suspended field to -1
>seems to work equally well for me.
>
>  Here is an updated patch:
>
>
>2009-09-22  Pierre Muller  <muller@ics.u-strasbg.fr>
>
>	* windows-nat.c (saved_threadid): New variable.
>	(do_windows_fetch_inferior_registers): Check for correct thread id
>	and set suspended field to -1 if it is zero.
>	(handle_output_debug_string): Set saved_threadid.

Please try my previous suggestion of removing the have_saved_context = 0;

cgf

RE: [RFA-v2] windows-nat.c Cygwin saved_context fix

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Christopher Faylor
> Envoyé : Tuesday, September 22, 2009 3:32 PM
> À : gdb-patches@sourceware.org; Pierre Muller
> Objet : Re: [RFA-v2] windows-nat.c Cygwin saved_context fix
> 
> On Tue, Sep 22, 2009 at 01:01:13AM +0200, Pierre Muller wrote:
> >After testing,
> >setting suspended field to -1
> >seems to work equally well for me.
> >
> >  Here is an updated patch:
> >
> >
> >2009-09-22  Pierre Muller  <muller@ics.u-strasbg.fr>
> >
> >	* windows-nat.c (saved_threadid): New variable.
> >	(do_windows_fetch_inferior_registers): Check for correct thread
> id
> >	and set suspended field to -1 if it is zero.
> >	(handle_output_debug_string): Set saved_threadid.
> 
> Please try my previous suggestion of removing the have_saved_context =
> 0;

Christopher, 
I will try to explain what happens if 
you just remove the 'have_saved_context = 0'.

  The first time you call do_windows_fetch_inferior_registers
the copied context does copy the saved_context to the
context struct inside *current_thread.
  So far, so good...

  If there is any call to thread_rec with get_context=1,
this will set current_thread->reload_context to 1 again,
because current_thread->suspended is still 0.
  This means that the next call to do_windows_fetch_inferior_registers
will again write saved_context to current_thread->context.
  This still looks good, but it will fail
if we try to change any of these registers,
  for instance (gdb) set $ebp = 0
windows_store_inferior_registers callsthread_rec with get_context=1
which again sets current_thread->reload_context to 1.
Then the current_thread->context.Ebp gets changed to 0,
but set_command later calls a cascade of frame functions
that end up by refetching $eip, at which point
current_thread->context.Ebp  gets restored to the value in 
saved_context.
  
  Finally the change of $ebp register failed.
This can be avoid with the second patch I sent,
that sets suspended to -1.

Pierre