[commit/windows] Add thread ID in SuspendThread error warning message.

[commit/windows] Add thread ID in SuspendThread error warning message.

[Joel Brobecker]

From: brobecke <brobecke@metz.(none)>

Hello,

This patch adds the thread ID to a warning printed when a call to
SuspendThread fails. It will help investigate issues, particularly
when correlated with the various debug traces provided by the
windows-nat module.

For the record, the output has been changed from...

    warning: SuspendThread failed. (winerr 6)

... to ...

    warning: SuspendThread (tid=0x720) failed. (winerr 6)

gdb/ChangeLog:

        * window-nat.c (thread_rec): Add thread ID in SuspendThread
        warning message.

Tested on x86-windows, and checked in.

---
 gdb/ChangeLog     |    5 +++++
 gdb/windows-nat.c |    5 +++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 493da1d..2fb7ac8 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2013-06-11  Joel Brobecker  <brobecker@adacore.com>
+
+	* window-nat.c (thread_rec): Add thread ID in SuspendThread
+	warning message.
+
 2013-06-08  Pedro Alves  <pedro@codesourcery.com>
 	    Yao Qi  <yao@codesourcery.com>
 
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 8320d27..66c44eb 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -311,8 +311,9 @@ thread_rec (DWORD id, int get_context)
 		if (SuspendThread (th->h) == (DWORD) -1)
 		  {
 		    DWORD err = GetLastError ();
-		    warning (_("SuspendThread failed. (winerr %u)"),
-			     (unsigned) err);
+		    warning (_("SuspendThread (tid=0x%x) failed."
+			       " (winerr %d)"),
+			     (unsigned) id, (unsigned) err);
 		    return NULL;
 		  }
 		th->suspended = 1;
-- 
1.7.10.4

Re: [commit/windows] Add thread ID in SuspendThread error warning message.

[Pedro Alves]

On 06/11/2013 11:21 AM, Joel Brobecker wrote:
>  		    DWORD err = GetLastError ();
> -		    warning (_("SuspendThread failed. (winerr %u)"),
> -			     (unsigned) err);
> +		    warning (_("SuspendThread (tid=0x%x) failed."
> +			       " (winerr %d)"),

This reverted part of a previous change (winerr %u -> winerr %d).

> +			     (unsigned) id, (unsigned) err);
>  		    return NULL;

-- 
Pedro Alves

Re: [commit/windows] Add thread ID in SuspendThread error warning message.

[Joel Brobecker]

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

> On 06/11/2013 11:21 AM, Joel Brobecker wrote:
> >  		    DWORD err = GetLastError ();
> > -		    warning (_("SuspendThread failed. (winerr %u)"),
> > -			     (unsigned) err);
> > +		    warning (_("SuspendThread (tid=0x%x) failed."
> > +			       " (winerr %d)"),
> 
> This reverted part of a previous change (winerr %u -> winerr %d).

Gasp!!! Thanks for spotting that - I did do a word-by-word diff
with git, and yet missed it :-(.

> > +			     (unsigned) id, (unsigned) err);
> >  		    return NULL;

Fixed thusly. Re-checked with word-by-word diff, and hopefully
OK, this time.

gdb/ChangeLog:

        * windows-nat.c (thread_rec): Revert format used to print
        error code returned by SuspendThread from %d back to %u.

Thanks again,
-- 
Joel

[-- Attachment #2: windows-nat-thread-rec-winerr-format.diff --]
[-- Type: text/x-diff, Size: 1480 bytes --]

commit b6a022d0505c5ca87969b8e00077bb33adc33095
Author: Joel Brobecker <brobecker@adacore.com>
Date:   Tue Jun 11 12:56:34 2013 +0200

    [windows] Fix accidental change of %u -> %d in SuspendThread warning.
    
    While enhancing the warning printed in when SuspendThread fails,
    I accidently changed the format used to print the error code
    from %u to %d. This patch reverts it back.
    
    gdb/ChangeLog:
    
            * windows-nat.c (thread_rec): Revert format used to print
            error code returned by SuspendThread from %d back to %u.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index fc0508e..f8b9386 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
 2013-06-11  Joel Brobecker  <brobecker@adacore.com>
 
+	* windows-nat.c (thread_rec): Revert format used to print
+	error code returned by SuspendThread from %d back to %u.
+
+2013-06-11  Joel Brobecker  <brobecker@adacore.com>
+
 	* windows-nat.c (windows_continue): Add "0x" prefix for thread
 	ID in debug trace.
 	(get_windows_debug_event): Likewise, for all debug traces.
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index e0bb719..b30f425 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -312,7 +312,7 @@ thread_rec (DWORD id, int get_context)
 		  {
 		    DWORD err = GetLastError ();
 		    warning (_("SuspendThread (tid=0x%x) failed."
-			       " (winerr %d)"),
+			       " (winerr %u)"),
 			     (unsigned) id, (unsigned) err);
 		    return NULL;
 		  }

Re: [commit/windows] Add thread ID in SuspendThread error warning message.

[Pedro Alves]

On 06/11/2013 12:06 PM, Joel Brobecker wrote:

> Fixed thusly. Re-checked with word-by-word diff, and hopefully
> OK, this time.

Looks fine.  :-)

-- 
Pedro Alves

Re: [commit/windows] Add thread ID in SuspendThread error warning message.

[Eli Zaretskii]

> From: Joel Brobecker <brobecker@adacore.com>
> Date: Tue, 11 Jun 2013 12:21:46 +0200
> 
> For the record, the output has been changed from...
> 
>     warning: SuspendThread failed. (winerr 6)
> 
> ... to ...
> 
>     warning: SuspendThread (tid=0x720) failed. (winerr 6)

Thanks, I see these on Windows 7 (but not on XP) as well, and wonder
what they mean and what causes them.

Btw, wouldn't it be better to show the process ID as well, as in
0x1234.0x720, for consistency with how we show threads in other
contexts, like "New thread" etc?

Re: [commit/windows] Add thread ID in SuspendThread error warning message.

[Joel Brobecker]

> Thanks, I see these on Windows 7 (but not on XP) as well, and wonder
> what they mean and what causes them.

If you have a reproducer, it would be helpful to me. I wasn't
able to reproduce locally, and this trace would have helped,
but the person experiencing the problems no longer has the time
to re-do all the experiments for the time beeing. So the pressure
is less, but I am definitely curious to know what's causing this.

> Btw, wouldn't it be better to show the process ID as well, as in
> 0x1234.0x720, for consistency with how we show threads in other
> contexts, like "New thread" etc?

Might be a good idea indeed. Would you like to send a patch?
I've switched context to something that's going to take me
a day or two, so I'd rather not worry about that for now.
If not, I'll keep that in my TODO, and send a patch maybe next week.

-- 
Joel

Re: [commit/windows] Add thread ID in SuspendThread error warning message.

[Eli Zaretskii]

> Date: Tue, 11 Jun 2013 18:27:38 +0200
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org
> 
> > Thanks, I see these on Windows 7 (but not on XP) as well, and wonder
> > what they mean and what causes them.
> 
> If you have a reproducer, it would be helpful to me.

Sadly, no.  I tried to reproduce it, but it never happens when I want
it to, and I couldn't until now figure out what are the conditions for
that to happen.  I actually thought it was a bug in the program I was
debugging (Emacs), which made this warning even more important.

> > Btw, wouldn't it be better to show the process ID as well, as in
> > 0x1234.0x720, for consistency with how we show threads in other
> > contexts, like "New thread" etc?
> 
> Might be a good idea indeed. Would you like to send a patch?

I will try to find time.

Re: [commit/windows] Add thread ID in SuspendThread error warning message.

[Christopher Faylor]

On Tue, Jun 11, 2013 at 08:05:29PM +0300, Eli Zaretskii wrote:
>> Date: Tue, 11 Jun 2013 18:27:38 +0200
>> From: Joel Brobecker <brobecker@adacore.com>
>> Cc: gdb-patches@sourceware.org
>> 
>> > Thanks, I see these on Windows 7 (but not on XP) as well, and wonder
>> > what they mean and what causes them.
>> 
>> If you have a reproducer, it would be helpful to me.
>
>Sadly, no.  I tried to reproduce it, but it never happens when I want
>it to, and I couldn't until now figure out what are the conditions for
>that to happen.  I actually thought it was a bug in the program I was
>debugging (Emacs), which made this warning even more important.

FWIW, I've seen this from time to time and have convinced myself that
it's a red herring.  When it happens, it probably should just silently
continue.


cgf

Re: [commit/windows] Add thread ID in SuspendThread error warning message.

[Joel Brobecker]

> FWIW, I've seen this from time to time and have convinced myself that
> it's a red herring.  When it happens, it probably should just silently
> continue.

That's interesting. I have the following patch in AdaCore's tree
which I have been uhming and ahming about. Would it apply to your
situation as well?

-                   warning (_("SuspendThread failed. (winerr %u)"),
-                            (unsigned) err);
-                   return NULL;
+                   /* If SuspendThread failed with error 5 (access
+                      denied), then ignore the error.  It's unclear
+                      where this comes from and how to prevent it.
+                      But in the meantime, ignoring it seems to allow
+                      us to inspect the thread (including fetching
+                      registers) without apparent ill effect.  */
+                   if (err != 5)
+                     {
+                       warning (_("SuspendThread (tid=0x%x) failed."
+                                  " (winerr %d)"),
+                                (unsigned) id, (unsigned) err);
+                       return NULL;
+                     }

I think there are other situations as well were we emit a warning,
and where I've been considering the idea of downgrading them to
complaints, so that users don't unnecessarily get concerned. But
I wanted to investigate a little bit before doing so, and I've
never had the time :-(.

-- 
Joel

Re: [commit/windows] Add thread ID in SuspendThread error warning message.

[Christopher Faylor]

On Tue, Jun 18, 2013 at 04:43:49PM -0700, Joel Brobecker wrote:
>> FWIW, I've seen this from time to time and have convinced myself that
>> it's a red herring.  When it happens, it probably should just silently
>> continue.
>
>That's interesting. I have the following patch in AdaCore's tree
>which I have been uhming and ahming about. Would it apply to your
>situation as well?
>
>-                   warning (_("SuspendThread failed. (winerr %u)"),
>-                            (unsigned) err);
>-                   return NULL;
>+                   /* If SuspendThread failed with error 5 (access
>+                      denied), then ignore the error.  It's unclear
>+                      where this comes from and how to prevent it.
>+                      But in the meantime, ignoring it seems to allow
>+                      us to inspect the thread (including fetching
>+                      registers) without apparent ill effect.  */
>+                   if (err != 5)
>+                     {
>+                       warning (_("SuspendThread (tid=0x%x) failed."
>+                                  " (winerr %d)"),
>+                                (unsigned) id, (unsigned) err);
>+                       return NULL;
>+                     }

That's basically what I'm doing in the Cygwin release:

                if (SuspendThread (th->h) == (DWORD) -1)
                  {
                    DWORD err = GetLastError ();
-                   warning (_("SuspendThread failed. (winerr %u)"),
-                            (unsigned) err);
+                   /* Can get a ERROR_INVALID_HANDLE if the main thread has
+                      exited. */
+                   if (err != ERROR_INVALID_HANDLE)
+                     warning (_("SuspendThread(%p) failed. (winerr %u)"),
+                              (void *) th->h, (unsigned) err);
                    return NULL;
                  }
                th->suspended = 1;


>I think there are other situations as well were we emit a warning,
>and where I've been considering the idea of downgrading them to
>complaints, so that users don't unnecessarily get concerned. But
>I wanted to investigate a little bit before doing so, and I've
>never had the time :-(.

I think this probably shouldn't be a warning.  If there is an
issue then it will become clearer at some other point.

cgf

Re: [commit/windows] Add thread ID in SuspendThread error warning message.

[Joel Brobecker]

> >That's interesting. I have the following patch in AdaCore's tree
> >which I have been uhming and ahming about. Would it apply to your
> >situation as well?
> >
> >-                   warning (_("SuspendThread failed. (winerr %u)"),
> >-                            (unsigned) err);
> >-                   return NULL;
> >+                   /* If SuspendThread failed with error 5 (access
> >+                      denied), then ignore the error.  It's unclear
> >+                      where this comes from and how to prevent it.
> >+                      But in the meantime, ignoring it seems to allow
> >+                      us to inspect the thread (including fetching
> >+                      registers) without apparent ill effect.  */
> >+                   if (err != 5)
> >+                     {
> >+                       warning (_("SuspendThread (tid=0x%x) failed."
> >+                                  " (winerr %d)"),
> >+                                (unsigned) id, (unsigned) err);
> >+                       return NULL;
> >+                     }
> 
> That's basically what I'm doing in the Cygwin release:
> 
>                 if (SuspendThread (th->h) == (DWORD) -1)
>                   {
>                     DWORD err = GetLastError ();
> -                   warning (_("SuspendThread failed. (winerr %u)"),
> -                            (unsigned) err);
> +                   /* Can get a ERROR_INVALID_HANDLE if the main thread has
> +                      exited. */
> +                   if (err != ERROR_INVALID_HANDLE)
> +                     warning (_("SuspendThread(%p) failed. (winerr %u)"),
> +                              (void *) th->h, (unsigned) err);
>                     return NULL;
>                   }
>                 th->suspended = 1;

That's interesting again. If I read MSDN right, ERROR_INVALID_HANDLE
is 6, whereas I ignore the error for code 5 (access denied). I say
that it's interesting, because the report we had was for the error
you ignore.

Someone was able to reproduce this issue consistently, and I wish
they had more time to help me investigate this. If we had more
details on the two error cases, I would not mind checking in
a merge of both our changes.

-- 
Joel

Re: [commit/windows] Add thread ID in SuspendThread error warning message.

[Christopher Faylor]

On Mon, Jun 24, 2013 at 04:34:21PM -0700, Joel Brobecker wrote:
>> >That's interesting. I have the following patch in AdaCore's tree
>> >which I have been uhming and ahming about. Would it apply to your
>> >situation as well?
>> >
>> >-                   warning (_("SuspendThread failed. (winerr %u)"),
>> >-                            (unsigned) err);
>> >-                   return NULL;
>> >+                   /* If SuspendThread failed with error 5 (access
>> >+                      denied), then ignore the error.  It's unclear
>> >+                      where this comes from and how to prevent it.
>> >+                      But in the meantime, ignoring it seems to allow
>> >+                      us to inspect the thread (including fetching
>> >+                      registers) without apparent ill effect.  */
>> >+                   if (err != 5)
>> >+                     {
>> >+                       warning (_("SuspendThread (tid=0x%x) failed."
>> >+                                  " (winerr %d)"),
>> >+                                (unsigned) id, (unsigned) err);
>> >+                       return NULL;
>> >+                     }
>> 
>> That's basically what I'm doing in the Cygwin release:
>> 
>>                 if (SuspendThread (th->h) == (DWORD) -1)
>>                   {
>>                     DWORD err = GetLastError ();
>> -                   warning (_("SuspendThread failed. (winerr %u)"),
>> -                            (unsigned) err);
>> +                   /* Can get a ERROR_INVALID_HANDLE if the main thread has
>> +                      exited. */
>> +                   if (err != ERROR_INVALID_HANDLE)
>> +                     warning (_("SuspendThread(%p) failed. (winerr %u)"),
>> +                              (void *) th->h, (unsigned) err);
>>                     return NULL;
>>                   }
>>                 th->suspended = 1;
>
>That's interesting again. If I read MSDN right, ERROR_INVALID_HANDLE
>is 6, whereas I ignore the error for code 5 (access denied). I say
>that it's interesting, because the report we had was for the error
>you ignore.

I have only seen an invalid handle error and I think it makes sense
since it's possible that the thread has disappeared when this function
is called.  I thought it happened when the main thread exits before
some other thread.

>Someone was able to reproduce this issue consistently, and I wish
>they had more time to help me investigate this. If we had more
>details on the two error cases, I would not mind checking in
>a merge of both our changes.

I think adding the tid is a good idea so if you want to merge the
two that's fine with me.  I've been meaning to dust off my changes
and submit them for approval anyway so you'd save me that step.

cgf