Windows native GDB event handling enhancement

Windows native GDB event handling enhancement

[Joel Brobecker]

Hello,

This patch series introduces two patches:

 * [RFA 1/2][master+8.3] (Windows) fix thr != nullptr assert failure in delete_thread_1
 * [RFA 2/2][master only] gdb/windows-nat.c: Get rid of main_thread_id global

I started working on those when I realized that GDB 8.3 showed
some crashes which seemed more frequent than the typical instability
we might have seen from time to time. And sure enough, I found
something.

It's split into two patches, although conceptually, they should be
one patch. The reason for that is that the change in its globality
is, in my opinion, potentially risky, because it is a bit of a jump
in the unknown, because I don't really know the reasons behind doing
the things the way we used to do them. What I know is that what
I propose seems to make better sense to me, and seems cleaner too;
but there might be something behind them I didn't know.

So I've extracted the part that avoids the crash into a first patch,
knowing that this part seems clear enough to me that the risk, if
any, should be at worst that we crash later on. In a couple of weeks
of nightly testing on various Windows versions, though, I haven't
seen it happen, so I think we're good. That way, this patch could
be reviewed on its own, and eventually find its way to the gdb-8.3-branch
as well.

Then the second patch, which complete the first one, could be poushed
to master. It's potentially more disruptive, which is why we give it
more time in master, and don't consider it for gdb-8.3-branch.

-- 
Joel

[RFA 2/2][master only] gdb/windows-nat.c: Get rid of main_thread_id global

[Joel Brobecker]

Hello,

This global is meant to point to the "main" thread of execution of
the program we are debugging. It is set when attaching to a process
or when receiving a CREATE_PROCESS_DEBUG_EVENT event. The theory at
the time was that this was also going to be the thread receiving
the EXIT_PROCESS_DEBUG_EVENT event.

Unfortunately, we have discovered since then that this is actually
not guaranteed. What this means in practice is that there is moderate
risk that main_thread_id refers to a thread which no longer exists.

This global is used in 3 situations:
  - OUTPUT_DEBUG_STRING_EVENT
  - LOAD_DLL_DEBUG_EVENT
  - UNLOAD_DLL_DEBUG_EVENT

It's not clear why we would need to use the main_thread_id in those cases
instead of using the thread ID provided by the kernel events itself.
So this patch implements this approach, which then allows us to delete
the main_thread_id global.

gdb/testsuite:

	* windows-nat.c (main_thread_id): Delete.
	(handle_output_debug_string): Replace main_thread_id by
	current_event.dwThreadId.
	(fake_create_process): Likewise.
	(get_windows_debug_event) <CREATE_PROCESS_DEBUG_EVENT>:
	Do not set main_thread_id.
	<LOAD_DLL_DEBUG_EVENT>: Replace main_thread_id by
	current_event.dwThreadId.
	<UNLOAD_DLL_DEBUG_EVENT>: Likewise.

Tested on x86-windows and x86_64-windows using AdaCore's testsuite.
I'd like to observe this patch in our nightly testing environment
for at least a week before pushing it to master here. But it doesn't
preclude us from reviewing the patch, IMO.

Thoughts? OK for master (after a week of AdaCore testing)?

Thanks
-- 
Joel

---
 gdb/windows-nat.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 039eb13..d188ed0 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -237,7 +237,6 @@ static DEBUG_EVENT current_event;	/* The current debug event from
 					   WaitForDebugEvent */
 static HANDLE current_process_handle;	/* Currently executing process */
 static windows_thread_info *current_thread;	/* Info on currently selected thread */
-static DWORD main_thread_id;		/* Thread ID of the main thread */
 
 /* Counts of things.  */
 static int exception_count = 0;
@@ -1026,7 +1025,7 @@ handle_output_debug_string (struct target_waitstatus *ourstatus)
 	  ourstatus->kind = TARGET_WAITKIND_STOPPED;
 	  retval = strtoul (p, &p, 0);
 	  if (!retval)
-	    retval = main_thread_id;
+	    retval = current_event.dwThreadId;
 	  else if ((x = (LPCVOID) (uintptr_t) strtoull (p, NULL, 0))
 		   && ReadProcessMemory (current_process_handle, x,
 					 &saved_context,
@@ -1402,13 +1401,12 @@ fake_create_process (void)
        (unsigned) GetLastError ());
       /*  We can not debug anything in that case.  */
     }
-  main_thread_id = current_event.dwThreadId;
   current_thread
     = windows_add_thread (ptid_t (current_event.dwProcessId, 0,
 				  current_event.dwThreadId),
 			  current_event.u.CreateThread.hThread,
 			  current_event.u.CreateThread.lpThreadLocalBase);
-  return main_thread_id;
+  return current_event.dwThreadId;
 }
 
 void
@@ -1604,7 +1602,6 @@ get_windows_debug_event (struct target_ops *ops,
 	break;
 
       current_process_handle = current_event.u.CreateProcessInfo.hProcess;
-      main_thread_id = current_event.dwThreadId;
       /* Add the main thread.  */
       th = windows_add_thread
         (ptid_t (current_event.dwProcessId, 0,
@@ -1648,7 +1645,7 @@ get_windows_debug_event (struct target_ops *ops,
       catch_errors (handle_load_dll);
       ourstatus->kind = TARGET_WAITKIND_LOADED;
       ourstatus->value.integer = 0;
-      thread_id = main_thread_id;
+      thread_id = current_event.dwThreadId;
       break;
 
     case UNLOAD_DLL_DEBUG_EVENT:
@@ -1661,7 +1658,7 @@ get_windows_debug_event (struct target_ops *ops,
       catch_errors (handle_unload_dll);
       ourstatus->kind = TARGET_WAITKIND_LOADED;
       ourstatus->value.integer = 0;
-      thread_id = main_thread_id;
+      thread_id = current_event.dwThreadId;
       break;
 
     case EXCEPTION_DEBUG_EVENT:
-- 

Re: [RFA 2/2][master only] gdb/windows-nat.c: Get rid of main_thread_id global

[Joel Brobecker]

> > This global is meant to point to the "main" thread of execution of
> > the program we are debugging. It is set when attaching to a process
> > or when receiving a CREATE_PROCESS_DEBUG_EVENT event. The theory at
> > the time was that this was also going to be the thread receiving
> > the EXIT_PROCESS_DEBUG_EVENT event.
> > 
> > Unfortunately, we have discovered since then that this is actually
> > not guaranteed. What this means in practice is that there is moderate
> > risk that main_thread_id refers to a thread which no longer exists.
> > 
> > This global is used in 3 situations:
> >   - OUTPUT_DEBUG_STRING_EVENT
> >   - LOAD_DLL_DEBUG_EVENT
> >   - UNLOAD_DLL_DEBUG_EVENT
> > 
> > It's not clear why we would need to use the main_thread_id in those cases
> > instead of using the thread ID provided by the kernel events itself.
> > So this patch implements this approach, which then allows us to delete
> > the main_thread_id global.
> > 
> > gdb/testsuite:
> > 
> > 	* windows-nat.c (main_thread_id): Delete.
> > 	(handle_output_debug_string): Replace main_thread_id by
> > 	current_event.dwThreadId.
> > 	(fake_create_process): Likewise.
> > 	(get_windows_debug_event) <CREATE_PROCESS_DEBUG_EVENT>:
> > 	Do not set main_thread_id.
> > 	<LOAD_DLL_DEBUG_EVENT>: Replace main_thread_id by
> > 	current_event.dwThreadId.
> > 	<UNLOAD_DLL_DEBUG_EVENT>: Likewise.
> 
> Thanks, but isn't this throwing the baby with the bathwater, though?
> AFAIK, the thread whose ID we get in the CREATE_PROCESS_DEBUG_EVENT
> _is_ indeed the main thread, at least that's my reading of everything
> I see about this on the Internet.  What might _not_ be true is that
> the main thread exits last.  In fact, I could write a legitimate
> Windows program whose main thread exited long before the process
> terminated (although doing that in a console application that uses
> 'main' would be somewhat tricky at best).  Perhaps that's what happens
> in the cases where you saw the problem.

It happens completely randomly, and in all cases, the program itself
has a main program which waits for all software threads to complete
before itself terminating. Not sure what Windows is doing under
the hood.

> AFAIU, the real problem with this global is that we delete the main
> thread when its thread-termination notification is received, and we
> also assume that EXIT_PROCESS_DEBUG_EVENT tells us that the main
> thread exited.  But if this is the problem, then we could do one of
> two things:
> 
>   . Keep the main_thread global, but use it just for avoiding the
>     announcement of the main thread.
> 
>   . Install some logic that would avoid trying to delete a thread if
>     its ID is equal to main_thread, when main_thread cannot be found
>     in the list of threads.
> 
> This would allow us to avoid the crashes, but still not announce the
> main thread as any other thread.

The crash is already avoided by patch #1, which does _not_ delete
the global.

The reason why I want to delete the global is because it references
a thread that the Windows kernel told us does not exist anymore,
and I fear that keeping it around will cause us to pass it back
to the Windows kernel, thus triggering an error. As far as I can
tell from our uses of it as part of pure event handling, I don't see
why we would need that global at all. So, rather than stay on shaky
grounds regarding that question, I prefer we bite the bullet and
try to do it right.

The second question is about the main thread creation/exit
notifications. At the moment, what we are relying on to emit or
silence the notification is the kind of event we received.
So, for CREATE_PROCESS_DEBUG_EVENT/EXIT_PROCESS_DEBUG_EVENT events,
we silence the notification. Unfortunately, we have now discovered
that does not work, because EXIT_PROCESS_DEBUG_EVENT doesn't
always point the same thread as CREATE_PROCESS_DEBUG_EVENT.
If I understand you correctly, you want to preserve the global
so we can avoid those notifications. In my opinion, doing that
just for the purpose of filtering one notification is simply
not worth the trouble of keeping a global around.  Generally speaking,
we tend to try to avoid globals as much as we can, as they are hard to
track, and makes the code harder to understand and maintain.
Modifying the logic to use the main_thread_id global instead of
the kind of event is a step backwards, in my opinion.

-- 
Joel

Re: [RFA 2/2][master only] gdb/windows-nat.c: Get rid of main_thread_id global

[Eli Zaretskii]

> Date: Wed, 17 Apr 2019 10:38:42 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org
> 
> It happens completely randomly, and in all cases, the program itself
> has a main program which waits for all software threads to complete
> before itself terminating. Not sure what Windows is doing under
> the hood.

At program termination time, all kinds of irregularities could happen,
especially if the program doesn't exit by exiting 'main' -- because
AFAIK doing the latter causes CRT to explicitly shut down all the
other threads.

But there's little doubt in my mind that the thread whose ID is
reported by CREATE_PROCESS_DEBUG_EVENT is the main thread.

> If I understand you correctly, you want to preserve the global
> so we can avoid those notifications. In my opinion, doing that
> just for the purpose of filtering one notification is simply
> not worth the trouble of keeping a global around.  Generally speaking,
> we tend to try to avoid globals as much as we can, as they are hard to
> track, and makes the code harder to understand and maintain.
> Modifying the logic to use the main_thread_id global instead of
> the kind of event is a step backwards, in my opinion.

If the only problem is having the global variable, we could mark the
main thread in some other way, that won't need the global.

But yes, I think not announcing the main thread is a nice feature, and
losing it due to the mess that happens at program exit (something to
which most users won't pay attention anyway) would be a pity.  I'd
even agree to announcing the exit of the main thread, if we cannot
come up with a better solution, if that could allow us avoid its
announcement at program startup.

The use case I have in mind is debugging a single-threaded program.
It is jarring enough, when debugging such a program, to see GDB
announcing threads started by the OS.  It would be nice if we could
avoid announcing the main thread.

Thanks.

Re: [RFA 2/2][master only] gdb/windows-nat.c: Get rid of main_thread_id global

[Joel Brobecker]

> > If I understand you correctly, you want to preserve the global
> > so we can avoid those notifications. In my opinion, doing that
> > just for the purpose of filtering one notification is simply
> > not worth the trouble of keeping a global around.  Generally speaking,
> > we tend to try to avoid globals as much as we can, as they are hard to
> > track, and makes the code harder to understand and maintain.
> > Modifying the logic to use the main_thread_id global instead of
> > the kind of event is a step backwards, in my opinion.
> 
> If the only problem is having the global variable, we could mark the
> main thread in some other way, that won't need the global.
> 
> But yes, I think not announcing the main thread is a nice feature, and
> losing it due to the mess that happens at program exit (something to
> which most users won't pay attention anyway) would be a pity.  I'd
> even agree to announcing the exit of the main thread, if we cannot
> come up with a better solution, if that could allow us avoid its
> announcement at program startup.
> 
> The use case I have in mind is debugging a single-threaded program.
> It is jarring enough, when debugging such a program, to see GDB
> announcing threads started by the OS.  It would be nice if we could
> avoid announcing the main thread.

Personally, I feel the other way, especially since we always get
all sorts of random temporary thread notifications polutting the
output, that I don't mind the extra notification. And that way,
I know when each thread is actually terminated, and with what
error code.

But if you and others feel differently, I can switch my approach:

  - patch #1 (master+8.3): Just fix the crash, and nothing more.

        This means that, on 8.3, thread creation and exit notifications
        can be slightly off; this is considered an acceptable known
        problem, and is not a regression.

        This is actually what we've put in AdaCore's GDB as a first
        step to see we had any issue after that, and we did not.

  - patch #2 (master only): Remove uses of the global

        But we keep the filtering of the "main" thread. We still
        the known problem that the thread-exit notifications might
        be silenced for the wrong thread. Not a regression.

  - patch #3 (master only): Tag the main thread, and silence
    the thread-exit notification of the thread that got tagged.
    Depending on the implementation, this patch would potentially
    be acceptable for 8.3, but might need extra work to apply.
    I think we've had enough problems with 8.3 as it is, I don't
    want to add more things to do, so I vote OUT for 8.3.

I don't think I have time for that before the weekend or next week,
though.

-- 
Joel

Re: [RFA 2/2][master only] gdb/windows-nat.c: Get rid of main_thread_id global

[Eli Zaretskii]

> Date: Wed, 17 Apr 2019 15:17:29 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org
> 
> Personally, I feel the other way, especially since we always get
> all sorts of random temporary thread notifications polutting the
> output, that I don't mind the extra notification. And that way,
> I know when each thread is actually terminated, and with what
> error code.
> 
> But if you and others feel differently, I can switch my approach:
> 
>   - patch #1 (master+8.3): Just fix the crash, and nothing more.
> 
>         This means that, on 8.3, thread creation and exit notifications
>         can be slightly off; this is considered an acceptable known
>         problem, and is not a regression.
> 
>         This is actually what we've put in AdaCore's GDB as a first
>         step to see we had any issue after that, and we did not.
> 
>   - patch #2 (master only): Remove uses of the global
> 
>         But we keep the filtering of the "main" thread. We still
>         the known problem that the thread-exit notifications might
>         be silenced for the wrong thread. Not a regression.
> 
>   - patch #3 (master only): Tag the main thread, and silence
>     the thread-exit notification of the thread that got tagged.
>     Depending on the implementation, this patch would potentially
>     be acceptable for 8.3, but might need extra work to apply.
>     I think we've had enough problems with 8.3 as it is, I don't
>     want to add more things to do, so I vote OUT for 8.3.
> 
> I don't think I have time for that before the weekend or next week,
> though.

Mine is just one opinion.  I think we should solicit opinions from
others before you invest all that effort.  If I'm the only one who is
bothered by your original proposal, I think you should go ahead and
push it.

Thanks.

Re: [RFA 2/2][master only] gdb/windows-nat.c: Get rid of main_thread_id global

[Joel Brobecker]

> > Personally, I feel the other way, especially since we always get
> > all sorts of random temporary thread notifications polutting the
> > output, that I don't mind the extra notification. And that way,
> > I know when each thread is actually terminated, and with what
> > error code.
> > 
> > But if you and others feel differently, I can switch my approach:
> > 
> >   - patch #1 (master+8.3): Just fix the crash, and nothing more.
> > 
> >         This means that, on 8.3, thread creation and exit notifications
> >         can be slightly off; this is considered an acceptable known
> >         problem, and is not a regression.
> > 
> >         This is actually what we've put in AdaCore's GDB as a first
> >         step to see we had any issue after that, and we did not.
> > 
> >   - patch #2 (master only): Remove uses of the global
> > 
> >         But we keep the filtering of the "main" thread. We still
> >         the known problem that the thread-exit notifications might
> >         be silenced for the wrong thread. Not a regression.
> > 
> >   - patch #3 (master only): Tag the main thread, and silence
> >     the thread-exit notification of the thread that got tagged.
> >     Depending on the implementation, this patch would potentially
> >     be acceptable for 8.3, but might need extra work to apply.
> >     I think we've had enough problems with 8.3 as it is, I don't
> >     want to add more things to do, so I vote OUT for 8.3.
> > 
> > I don't think I have time for that before the weekend or next week,
> > though.
> 
> Mine is just one opinion.  I think we should solicit opinions from
> others before you invest all that effort.  If I'm the only one who is
> bothered by your original proposal, I think you should go ahead and
> push it.

Since I think you use the debugger on Windows more than I personally do,
I'll go the other way. Unless someone else tips the balance the other way,
I'll go with your opinion over mine ;-).

-- 
Joel

Re: [RFA 2/2][master only] gdb/windows-nat.c: Get rid of main_thread_id global

[Pedro Alves]

(I should have read this thread before replying on the other one.)

On 4/17/19 11:17 PM, Joel Brobecker wrote:
> 
>   - patch #3 (master only): Tag the main thread, and silence
>     the thread-exit notification of the thread that got tagged.
>     Depending on the implementation, this patch would potentially
>     be acceptable for 8.3, but might need extra work to apply.
>     I think we've had enough problems with 8.3 as it is, I don't
>     want to add more things to do, so I vote OUT for 8.3.

This doesn't seem right to me, given that the main thread may
legitimately exit before other threads.  If this approach is taken,
maybe tweak it a bit:

silence the thread-exit notification of the thread that got tagged
_iff there are no other threads in the process_.

Thanks,
Pedro Alves

Re: [RFA 2/2][master only] gdb/windows-nat.c: Get rid of main_thread_id global

[Joel Brobecker]

> >   - patch #3 (master only): Tag the main thread, and silence
> >     the thread-exit notification of the thread that got tagged.
> >     Depending on the implementation, this patch would potentially
> >     be acceptable for 8.3, but might need extra work to apply.
> >     I think we've had enough problems with 8.3 as it is, I don't
> >     want to add more things to do, so I vote OUT for 8.3.
> 
> This doesn't seem right to me, given that the main thread may
> legitimately exit before other threads.  If this approach is taken,
> maybe tweak it a bit:
> 
> silence the thread-exit notification of the thread that got tagged
> _iff there are no other threads in the process_.

Wouldn't that be confusing? What would happen in that case is that
we end up getting a a thread-exit notification with a thread ID
for which there hasn't been a new-thread notification. Without
knowing the implementation details of how GDB decides to silence
or not thread notifications, people are going to ask themselves
where the thread that exited came from, and why we didn't see
a new-thread notification for it before.

To decide which approach we want to take - what would be the issue
if we decided to silence the main thread's exit notification while
some other threads might still be alive? Assuming Pedro is making
a valid point, I think we should go back to pre-8.3 behavior, and
show all thread create/exit notifications, including for the main
thread.


-- 
Joel

Re: [RFA 2/2][master only] gdb/windows-nat.c: Get rid of main_thread_id global

[Pedro Alves]

On 4/19/19 9:43 PM, Joel Brobecker wrote:
>>>   - patch #3 (master only): Tag the main thread, and silence
>>>     the thread-exit notification of the thread that got tagged.
>>>     Depending on the implementation, this patch would potentially
>>>     be acceptable for 8.3, but might need extra work to apply.
>>>     I think we've had enough problems with 8.3 as it is, I don't
>>>     want to add more things to do, so I vote OUT for 8.3.
>>
>> This doesn't seem right to me, given that the main thread may
>> legitimately exit before other threads.  If this approach is taken,
>> maybe tweak it a bit:
>>
>> silence the thread-exit notification of the thread that got tagged
>> _iff there are no other threads in the process_.
> 
> Wouldn't that be confusing? What would happen in that case is that
> we end up getting a a thread-exit notification with a thread ID
> for which there hasn't been a new-thread notification. Without
> knowing the implementation details of how GDB decides to silence
> or not thread notifications, people are going to ask themselves
> where the thread that exited came from, and why we didn't see
> a new-thread notification for it before.
> 
> To decide which approach we want to take - what would be the issue
> if we decided to silence the main thread's exit notification while
> some other threads might still be alive?

In my view, it's OK to not let the user know the main thread is
exiting when it's the last thread in the process because the process
is exiting right away.  But if the main thread is not the last
thread, then the process is going to stay around.  And in that case,
if a user wants thread exit notifications, I think it just makes sense
to let the user know a thread exited, more so since the main thread of
a program tends to be its most "important" thread.

I don't see the asymmetry (silent at creation, not silent at destruction)
in this particular case as surprising -- when the program is
starting up, there's only one thread, and I don't think users are
surprised to find that after running to main there's already one
thread in the thread list.

I also think that if we included the GDB thread id (and name) in thread creation/exited
notifications, similar to the output of inferior start/exit notifications, then users
would be even less surprised with the asymmetry, as users could clearly tell that
it's the main thread that is exiting:

Currently:

 [Thread 48192.0xd100 exited with code 1]

With gdb thread id and name:

[Thread 1.1 (48192.0xd100 "the thread's name") exited with code 1]

I think that would be a good overall change, regardless.

> Assuming Pedro is making
> a valid point, I think we should go back to pre-8.3 behavior, and
> show all thread create/exit notifications, including for the main
> thread.

Thanks,
Pedro Alves

Re: [RFA 2/2][master only] gdb/windows-nat.c: Get rid of main_thread_id global

[André Pönitz]

On Mon, Apr 22, 2019 at 03:24:46PM +0100, Pedro Alves wrote:
> I don't see the asymmetry (silent at creation, not silent at destruction)
> in this particular case as surprising [...]

It's not surprising, but it's inconsistent and causes extra work in
cases one wants to mechanically examine such output.

Andre'

Re: [RFA 2/2][master only] gdb/windows-nat.c: Get rid of main_thread_id global

[Pedro Alves]

On 4/22/19 4:23 PM, André Pönitz wrote:
> On Mon, Apr 22, 2019 at 03:24:46PM +0100, Pedro Alves wrote:
>> I don't see the asymmetry (silent at creation, not silent at destruction)
>> in this particular case as surprising [...]
> 
> It's not surprising, but it's inconsistent 
> and causes extra work in
> cases one wants to mechanically examine such output.

I'm curious about what extra work you're thinking of.  Can't think of
anything significant.

But do note that this discussion does not affect MI.

So what do you propose?  My proposal kept these two ideas in mind:

- be quiet wrt to threads with single-threaded programs.
- inform the user the main thread exited while other threads stay running.

Thanks,
Pedro Alves

Re: [RFA 2/2][master only] gdb/windows-nat.c: Get rid of main_thread_id global

[André Pönitz]

On Mon, Apr 22, 2019 at 06:28:55PM +0100, Pedro Alves wrote:
> On 4/22/19 4:23 PM, André Pönitz wrote:
> > On Mon, Apr 22, 2019 at 03:24:46PM +0100, Pedro Alves wrote:
> >> I don't see the asymmetry (silent at creation, not silent at destruction)
> >> in this particular case as surprising [...]
> > 
> > It's not surprising, but it's inconsistent 
> > and causes extra work in
> > cases one wants to mechanically examine such output.
> 
> I'm curious about what extra work you're thinking of.

As far as I understand it, any (non-MI) user wanting to keep track of
running threads would need to find out which events are "not surprising"
(for which version of GDB...) and mentally (or in code) "generate"
the "missing" events. That's a kind of "work". 

> Can't think of anything significant.

It's not significant, but less surprising to me personally, i.e.
violating the principle of least astonishment.
 
> But do note that this discussion does not affect MI.

I understand that, and for thread related notifications MI is sufficient
for my frontend purposes, but in general MI does not cover all functionality
of CLI and already this discrepancy is a surprise to some degree. I'd
rather see the differences go away over time, not to increase.

> So what do you propose?  My proposal kept these two ideas in mind:
> 
> - be quiet wrt to threads with single-threaded programs.

TBH, I don't see the necessity of this requirement. That would be two
lines of noise per run. 

> - inform the user the main thread exited while other threads stay running.

Also here, I don't see enough gain in one suppressed line of output
to compensate for the inconsistency, this time even with a CLI user hat on.

Andre'

Re: [RFA 2/2][master only] gdb/windows-nat.c: Get rid of main_thread_id global

[Eli Zaretskii]

> Date: Mon, 22 Apr 2019 23:46:07 +0200
> From: André Pönitz <apoenitz@t-online.de>
> Cc: Joel Brobecker <brobecker@adacore.com>, Eli Zaretskii <eliz@gnu.org>,
> 	gdb-patches@sourceware.org
> 
> > - be quiet wrt to threads with single-threaded programs.
> 
> TBH, I don't see the necessity of this requirement. That would be two
> lines of noise per run. 

It violates the same principle of least astonishment, in the important
use case of a program that has only one application thread, the main
thread.

And please also keep in mind that we are not talking about introducing
a new feature, we are talking about preserving an existing feature, in
a way that will avoid crashing GDB in some rare cases.  It sounds
wrong to me to lose a feature because its implementation causes
problems in a rare use case; we should instead fix the implementation.

> > - inform the user the main thread exited while other threads stay running.
> 
> Also here, I don't see enough gain in one suppressed line of output
> to compensate for the inconsistency, this time even with a CLI user hat on.

Forgive me for asking, but do you frequently debug with GDB on
Windows?  Because this issue is specific to native Windows debugging,
it won't affect any other platforms.  On Windows, we already have
threads popping up and exiting that are started by the OS beyond our
control, which is already quite jarring even to me, certainly to
someone who doesn't debug day in and day out.  Having the main thread
of my program add more noise is not insignificant, because ideally I
don't want to see any of my application threads shown unless my
program really started a thread.

Thanks.

Re: [RFA 2/2][master only] gdb/windows-nat.c: Get rid of main_thread_id global

[André Pönitz]

On Tue, Apr 23, 2019 at 08:54:04AM +0300, Eli Zaretskii wrote:
> > Also here, I don't see enough gain in one suppressed line of output
> > to compensate for the inconsistency, this time even with a CLI user hat on.
> 
> Forgive me for asking, but do you frequently debug with GDB on
> Windows? 

Not frequently, and if so, only for testing purposes. 

But I do get regular feedback from a handful downstream users.

> Because this issue is specific to native Windows debugging,
> it won't affect any other platforms.

The general pattern (subjective inconsistency in GDB output - and 
input when I think about it) is, fortunately or not, cross-platform.

> On Windows, we already have
> threads popping up and exiting that are started by the OS beyond our
> control, which is already quite jarring even to me,

[It's not just you in this case...]

> certainly to someone who doesn't debug day in and day out.  Having
> the main thread of my program add more noise is not insignificant,
> because ideally I don't want to see any of my application threads
> shown unless my program really started a thread.

To be honest, I have still difficulties to find a mental model on what
irks a typical Windows users. On one hand, they can apparently stand
developing on that platform, on the other hand, two lines of extra
output, even issued for consistency reasons, appear to be a show
stopper. 

Andre'

Re: [RFA 2/2][master only] gdb/windows-nat.c: Get rid of main_thread_id global

[Eli Zaretskii]

> Date: Wed, 24 Apr 2019 22:13:22 +0200
> From: André Pönitz <apoenitz@t-online.de>
> Cc: palves@redhat.com, brobecker@adacore.com, gdb-patches@sourceware.org
> 
> To be honest, I have still difficulties to find a mental model on what
> irks a typical Windows users. On one hand, they can apparently stand
> developing on that platform, on the other hand, two lines of extra
> output, even issued for consistency reasons, appear to be a show
> stopper.

That was sarcasm, I guess?

In any case, I'm not sure why you think developing for Windows in the
relevant use cases is something that needs some super-natural
endurance capabilities: I develop using Emacs, GCC, Binutils, GDB,
Grep, Find, and all the other tools you are familiar with.  It's very
similar to developing on a Posix platform.  GDB doesn't support debug
info in formats emitted by Windows compilers, it only supports
GCC-emitted info, AFAIK.

And two lines of extra output you don't expect are not a show-stopper,
but they do make you stop and think for a moment whether this is
something your code did.

Re: [RFA 2/2][master only] gdb/windows-nat.c: Get rid of main_thread_id global

[Pedro Alves]

On 4/22/19 10:46 PM, André Pönitz wrote:
> On Mon, Apr 22, 2019 at 06:28:55PM +0100, Pedro Alves wrote:
>> On 4/22/19 4:23 PM, André Pönitz wrote:
>>> On Mon, Apr 22, 2019 at 03:24:46PM +0100, Pedro Alves wrote:
>>>> I don't see the asymmetry (silent at creation, not silent at destruction)
>>>> in this particular case as surprising [...]
>>>
>>> It's not surprising, but it's inconsistent 
>>> and causes extra work in
>>> cases one wants to mechanically examine such output.
>>
>> I'm curious about what extra work you're thinking of.
> 
> As far as I understand it, any (non-MI) user wanting to keep track of
> running threads would need to find out which events are "not surprising"
> (for which version of GDB...) and mentally (or in code) "generate"
> the "missing" events. That's a kind of "work". 

gdb on GNU/Linux already works like I'm suggesting, and has been doing it
for a few years, and I don't think I recall ever hearing complaints about it:

Simple single-threaded "hello world" program:

 (gdb) r
 Starting program: /home/pedro/brno/pedro/gdb/tests/main 
 [Inferior 1 (process 13298) exited normally]
 (gdb) 

Multi-threaded program that exits normally (gdb.threads/schedlock.exp testcase):

 (gdb) r
 Starting program: build/gdb/testsuite/outputs/gdb.threads/schedlock/schedlock 
 [Thread debugging using libthread_db enabled]
 Using host libthread_db library "/lib64/libthread_db.so.1".
 [New Thread 0x7ffff74b8700 (LWP 13984)]            <<< extra thread
 [Thread 0x7ffff74b8700 (LWP 13984) exited]         <<< extra thread exits
 [Inferior 1 (process 13980) exited normally]
 (gdb) 

Threaded program that has the leader thread exit with pthread_exit while
another thread keeps running (gdb.threads/leader-exit.exp testcase):

 (gdb) r
 Starting program: build/gdb/testsuite/outputs/gdb.threads/leader-exit/leader-exit 
 [Thread debugging using libthread_db enabled]
 Using host libthread_db library "/lib64/libthread_db.so.1".
 [New Thread 0x7ffff74b8700 (LWP 13230)]               <<<< extra thread
 [Thread 0x7ffff7fb6740 (LWP 13226) exited]            <<<< leader thread exits (notice LWP == process ID)
 [Inferior 1 (process 13226) exited normally]
 (gdb) 

Note there's some time between the leader exiting, and the process exiting.

If the output were changed like I was suggesting, we'd have something like this,
which I think would be much clearer:

 (gdb) r
 Starting program: build/gdb/testsuite/outputs/gdb.threads/schedlock/schedlock 
 [Thread debugging using libthread_db enabled]
 Using host libthread_db library "/lib64/libthread_db.so.1".
 [New Thread 1.2 (0x7ffff74b8700 (LWP 13984))]
 [Thread 1.2 (0x7ffff74b8700 (LWP 13984)) exited]
 [Inferior 1 (process 13980) exited normally]
 (gdb)

And:

 (gdb) r
 Starting program: build/gdb/testsuite/outputs/gdb.threads/leader-exit/leader-exit 
 [Thread debugging using libthread_db enabled]
 Using host libthread_db library "/lib64/libthread_db.so.1".
 [New Thread 1.2 (0x7ffff74b8700 (LWP 13230))]
 [Thread 1.1 (0x7ffff7fb6740 (LWP 13226)) exited]
 [Inferior 1 (process 13226) exited normally]
 (gdb) 

We're going on a tangent, but maybe even say "Thread ... appeared" instead
of "New Thread", for better visual alignment, like:

 (gdb) r
 Starting program: build/gdb/testsuite/outputs/gdb.threads/schedlock/schedlock 
 [Thread debugging using libthread_db enabled]
 Using host libthread_db library "/lib64/libthread_db.so.1".
 [Thread 1.2 (0x7ffff74b8700 (LWP 13984)) appeared]
 [Thread 1.2 (0x7ffff74b8700 (LWP 13984)) exited]
 [Inferior 1 (process 13980) exited normally]
 (gdb)

> 
>> Can't think of anything significant.
> 
> It's not significant, but less surprising to me personally, i.e.
> violating the principle of least astonishment.
>  
>> But do note that this discussion does not affect MI.
> 
> I understand that, and for thread related notifications MI is sufficient
> for my frontend purposes, but in general MI does not cover all functionality
> of CLI and already this discrepancy is a surprise to some degree. I'd
> rather see the differences go away over time, not to increase.
> 
>> So what do you propose?  My proposal kept these two ideas in mind:
>>
>> - be quiet wrt to threads with single-threaded programs.
> 
> TBH, I don't see the necessity of this requirement. That would be two
> lines of noise per run. 

I think I had originally proposed that when I switched gdb's model
to "always a thread", but got pushed back.  And I do think the pushback made
sense, particularly when you consider remote debugging targets that don't even
have a concept of threads.

I kind of convinced myself by considering bare metal targets, but also considering
further "execution unit" levels/layers -- say gdb adds native support for fibers, and
the model is that the initial thread has an initial fiber too -- i.e., there's always
a fiber, like there's always a thread.  Would we want to automatically announce the
initial fiber as soon as the process starts, even if the process doesn't make
use of fibers?

 (gdb) r
 Starting program: ...
 [New Thread 1.1 ...]
 [New Fiber 1.1 ...]
 .....
 [Fiber 1.1 ... exited]
 [Thread 1.1 exited]
 [Inferior 1 (process 13226) exited normally]
 (gdb) 

Or, would we rather be quiet about fibers until something creates a second one,
similarly to how we are quiet about threads until a second thread is created?

And fibers would be one kind of unit of execution.  There may be others.
What to do then?

> 
>> - inform the user the main thread exited while other threads stay running.
> 
> Also here, I don't see enough gain in one suppressed line of output
> to compensate for the inconsistency, this time even with a CLI user hat on.
Thanks,
Pedro Alves

[RFA 1/2][master+8.3] (Windows) fix thr != nullptr assert failure in delete_thread_1

[Joel Brobecker]

Hello,

We have observed that GDB would randomly trip the the following
assertion failure when debugging on Windows. When allowing
the program to run until the inferior exits, we occasionally see:

     (gdb) cont
     Continuing.
     [Thread 48192.0xd100 exited with code 1]
     [Thread 48192.0x10ad8 exited with code 1]
     [Thread 48192.0x36e28 exited with code 0]
     [Thread 48192.0x52be4 exited with code 0]
     [Thread 48192.0x5aa40 exited with code 0]
     ../../src/gdb/thread.c:453: internal-error: void delete_thread_1(thread_inf
o*, bool): Assertion `thr != nullptr' failed.

Running the same scenario with some additional traces enabled...

    (gdb) set verbose
    (gdb) set debugevents

... allows us to understand what the issue is. To understand, we need
to first look at the events received when starting the program, and
in particular which threads got created how. First, we get a
CREATE_PROCESS_DEBUG_EVENT for tid=0x442a8:

    gdb: kernel event for pid=317536 tid=0x442a8 code=CREATE_PROCESS_DEBUG_EVENT)

Shortly after, we get some CREATE_THREAD_DEBUG_EVENT events,
one of them being for tid=0x4010c:

    gdb: kernel event for pid=317536 tid=0x4010c code=CREATE_THREAD_DEBUG_EVENT)
Fast forward a bit of debugging, and we do a "cont" as above,
at which point the programs reaches the end, and the system reports
"exit" events. The first interesting one is the following:

    gdb: kernel event for pid=317536 tid=0x442a8 code=EXIT_THREAD_DEBUG_EVENT)

This is reporting a thread-exit event for a thread whose tid
is the TID of what we call the "main thread". That's the thread
that was created when we received the CREATE_PROCESS_DEBUG_EVENT
notification, and whose TID is actually stored in a global variable
called main_thread_id. This is not something we expected, as
the assumption we made was that the main thread would exit last,
and we would be notified of it via an EXIT_PROCESS_DEBUG_EVENT.
But apparently, this is not always true, at least on Windows Server
2012 and 2016 where this issue has been observed happening randomly.

The consequence of the above notification is that we call
windows_delete_thread for that thread, which removes it from
our list of known threads.

And a little bit later, then we then get the EXIT_PROCESS_DEBUG_EVENT,
and we can see that the associated tid is not the main_thread_id,
but rather the tid of one of the threads that was created during
the lifetime of the program, in this case tid=0x4010c:

    gdb: kernel event for pid=317536 tid=0x4010c code=EXIT_PROCESS_DEBUG_EVENT)

And the debug trace printed right after shows why we're crashing:

    [Deleting Thread 317536.0x442a8]

We are trying to delete the thread whose tid=0x442a8, which is
the main_thread_id! As we have already deleted that thread before,
the search for it returns a nullptr, which then trips the assertion
check in delete_thread_1.

This commit fixes the immediate issue; but it still ignores the open
question of what to do with the main_thread_id global, particularly
after that thread has been removed from our list of threads. The bigger
question is kept separate from this patch, because this smaller patch
seems to work well enough on its own in practice, and make the patch
less complex, thus a possible candidate for backporting to a release
branch (thinking of the gdb-8.3-branch, in this case).

Another change that this commit makes is re-instate the thread create/
exit notifications for what we thought was the main thread. We added
some filtering recently to not show those notifications. Otherwise,
with what we've discovered, it is possible that the list of threads
reported by the "new thread" notifications does not match the list
of threads from the "thread exit" notifications.

gdb/ChangeLog:

	* windows-nat.c (windows_add_thread): Remove parameter "main_thread_p".
	Adjust function accordingly.  Update all callers.
	(windows_delete_thread): Likewise.
	(get_windows_debug_event) <EXIT_PROCESS_DEBUG_EVENT>: Use
        current_event.dwThreadId instead of main_thread_id.

Tested on both x86-windows and x86_64-windows using AdaCore's testsuite.
The portion of the patch that uses current_event.dwThreadId instead of
main_thread_id has undergood about a couple of weeks of nightly testing
on various Windows versions.

OK to master?
OK for 8.3?

Thanks!

---
 gdb/windows-nat.c | 58 ++++++++++++++++++++++++-------------------------------
 1 file changed, 25 insertions(+), 33 deletions(-)

diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 5009418..039eb13 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -430,12 +430,10 @@ thread_rec (DWORD id, int get_context)
 
    PTID is the ptid of the thread to be added.
    H is its Windows handle.
-   TLB is its thread local base.
-   MAIN_THREAD_P should be true if the thread to be added is
-   the main thread, false otherwise.  */
+   TLB is its thread local base.  */
 
 static windows_thread_info *
-windows_add_thread (ptid_t ptid, HANDLE h, void *tlb, bool main_thread_p)
+windows_add_thread (ptid_t ptid, HANDLE h, void *tlb)
 {
   windows_thread_info *th;
   DWORD id;
@@ -456,13 +454,18 @@ windows_add_thread (ptid_t ptid, HANDLE h, void *tlb, bool main_thread_p)
 
   /* Add this new thread to the list of threads.
 
-     To be consistent with what's done on other platforms, we add
-     the main thread silently (in reality, this thread is really
-     more of a process to the user than a thread).  */
-  if (main_thread_p)
-    add_thread_silent (ptid);
-  else
-    add_thread (ptid);
+     On many platforms, we add the main program's thread silently.
+     But unfortunately, it does not seem to identify such thread
+     on Windows. We used to think that the main program's thread
+     is the thread reported by the CREATE_PROCESS_DEBUG_EVENT, but
+     we've seen since then that this thread sometimes exits before
+     other threads in the program -- when that happens, what we see
+     is that the CREATE_PROCESS_DEBUG_EVENT thread gets reported
+     as exited via an EXIT_THREAD_DEBUG_EVENT, and another thread
+     gets used in the EXIT_PROCESS_DEBUG_EVENT notification instead.
+
+     So, we just add all threads verbosely.  */
+  add_thread (ptid);
 
   /* Set the debug registers for the new thread if they are used.  */
   if (debug_registers_used)
@@ -503,12 +506,10 @@ windows_init_thread_list (void)
 /* Delete a thread from the list of threads.
 
    PTID is the ptid of the thread to be deleted.
-   EXIT_CODE is the thread's exit code.
-   MAIN_THREAD_P should be true if the thread to be deleted is
-   the main thread, false otherwise.  */
+   EXIT_CODE is the thread's exit code.  */
 
 static void
-windows_delete_thread (ptid_t ptid, DWORD exit_code, bool main_thread_p)
+windows_delete_thread (ptid_t ptid, DWORD exit_code)
 {
   windows_thread_info *th;
   DWORD id;
@@ -517,16 +518,11 @@ windows_delete_thread (ptid_t ptid, DWORD exit_code, bool main_thread_p)
 
   id = ptid.tid ();
 
-  /* Emit a notification about the thread being deleted.
-
-     Note that no notification was printed when the main thread
-     was created, and thus, unless in verbose mode, we should be
-     symetrical, and avoid that notification for the main thread
-     here as well.  */
+  /* Emit a notification about the thread being deleted.  */
 
   if (info_verbose)
     printf_unfiltered ("[Deleting %s]\n", target_pid_to_str (ptid).c_str ());
-  else if (print_thread_events && !main_thread_p)
+  else if (print_thread_events)
     printf_unfiltered (_("[%s exited with code %u]\n"),
 		       target_pid_to_str (ptid).c_str (),
 		       (unsigned) exit_code);
@@ -1411,8 +1407,7 @@ fake_create_process (void)
     = windows_add_thread (ptid_t (current_event.dwProcessId, 0,
 				  current_event.dwThreadId),
 			  current_event.u.CreateThread.hThread,
-			  current_event.u.CreateThread.lpThreadLocalBase,
-			  true /* main_thread_p */);
+			  current_event.u.CreateThread.lpThreadLocalBase);
   return main_thread_id;
 }
 
@@ -1584,8 +1579,7 @@ get_windows_debug_event (struct target_ops *ops,
       th = windows_add_thread
         (ptid_t (current_event.dwProcessId, 0, current_event.dwThreadId),
 	 current_event.u.CreateThread.hThread,
-	 current_event.u.CreateThread.lpThreadLocalBase,
-	 false /* main_thread_p */);
+	 current_event.u.CreateThread.lpThreadLocalBase);
 
       break;
 
@@ -1596,8 +1590,7 @@ get_windows_debug_event (struct target_ops *ops,
 		     "EXIT_THREAD_DEBUG_EVENT"));
       windows_delete_thread (ptid_t (current_event.dwProcessId, 0,
 				     current_event.dwThreadId),
-			     current_event.u.ExitThread.dwExitCode,
-			     false /* main_thread_p */);
+			     current_event.u.ExitThread.dwExitCode);
       th = &dummy_thread_info;
       break;
 
@@ -1617,8 +1610,7 @@ get_windows_debug_event (struct target_ops *ops,
         (ptid_t (current_event.dwProcessId, 0,
 		 current_event.dwThreadId),
 	 current_event.u.CreateProcessInfo.hThread,
-	 current_event.u.CreateProcessInfo.lpThreadLocalBase,
-	 true /* main_thread_p */);
+	 current_event.u.CreateProcessInfo.lpThreadLocalBase);
       thread_id = current_event.dwThreadId;
       break;
 
@@ -1637,11 +1629,11 @@ get_windows_debug_event (struct target_ops *ops,
       else if (saw_create == 1)
 	{
 	  windows_delete_thread (ptid_t (current_event.dwProcessId, 0,
-					 main_thread_id),
-				 0, true /* main_thread_p */);
+					 current_event.dwThreadId),
+				 0);
 	  ourstatus->kind = TARGET_WAITKIND_EXITED;
 	  ourstatus->value.integer = current_event.u.ExitProcess.dwExitCode;
-	  thread_id = main_thread_id;
+	  thread_id = current_event.dwThreadId;
 	}
       break;
 
-- 
2.1.4

Re: [RFA 1/2][master+8.3] (Windows) fix thr != nullptr assert failure in delete_thread_1

[Pedro Alves]

On 4/16/19 11:33 PM, Joel Brobecker wrote:
> Hello,
> 
> We have observed that GDB would randomly trip the the following

"the the"

> assertion failure when debugging on Windows. When allowing
> the program to run until the inferior exits, we occasionally see:
> 
>      (gdb) cont
>      Continuing.
>      [Thread 48192.0xd100 exited with code 1]
>      [Thread 48192.0x10ad8 exited with code 1]
>      [Thread 48192.0x36e28 exited with code 0]
>      [Thread 48192.0x52be4 exited with code 0]
>      [Thread 48192.0x5aa40 exited with code 0]
>      ../../src/gdb/thread.c:453: internal-error: void delete_thread_1(thread_inf
> o*, bool): Assertion `thr != nullptr' failed.
> 
> Running the same scenario with some additional traces enabled...
> 
>     (gdb) set verbose
>     (gdb) set debugevents
> 
> ... allows us to understand what the issue is. To understand, we need
> to first look at the events received when starting the program, and
> in particular which threads got created how. First, we get a
> CREATE_PROCESS_DEBUG_EVENT for tid=0x442a8:
> 
>     gdb: kernel event for pid=317536 tid=0x442a8 code=CREATE_PROCESS_DEBUG_EVENT)
> 
> Shortly after, we get some CREATE_THREAD_DEBUG_EVENT events,
> one of them being for tid=0x4010c:
> 
>     gdb: kernel event for pid=317536 tid=0x4010c code=CREATE_THREAD_DEBUG_EVENT)
> Fast forward a bit of debugging, and we do a "cont" as above,
> at which point the programs reaches the end, and the system reports
> "exit" events. The first interesting one is the following:
> 
>     gdb: kernel event for pid=317536 tid=0x442a8 code=EXIT_THREAD_DEBUG_EVENT)
> 
> This is reporting a thread-exit event for a thread whose tid
> is the TID of what we call the "main thread". That's the thread
> that was created when we received the CREATE_PROCESS_DEBUG_EVENT
> notification, and whose TID is actually stored in a global variable
> called main_thread_id. This is not something we expected, as
> the assumption we made was that the main thread would exit last,
> and we would be notified of it via an EXIT_PROCESS_DEBUG_EVENT.
> But apparently, this is not always true, at least on Windows Server
> 2012 and 2016 where this issue has been observed happening randomly.
> 
> The consequence of the above notification is that we call
> windows_delete_thread for that thread, which removes it from
> our list of known threads.
> 
> And a little bit later, then we then get the EXIT_PROCESS_DEBUG_EVENT,
> and we can see that the associated tid is not the main_thread_id,
> but rather the tid of one of the threads that was created during
> the lifetime of the program, in this case tid=0x4010c:
> 
>     gdb: kernel event for pid=317536 tid=0x4010c code=EXIT_PROCESS_DEBUG_EVENT)
> 
> And the debug trace printed right after shows why we're crashing:
> 
>     [Deleting Thread 317536.0x442a8]
> 
> We are trying to delete the thread whose tid=0x442a8, which is
> the main_thread_id! As we have already deleted that thread before,
> the search for it returns a nullptr, which then trips the assertion
> check in delete_thread_1.
> 
> This commit fixes the immediate issue; but it still ignores the open
> question of what to do with the main_thread_id global, particularly
> after that thread has been removed from our list of threads. The bigger
> question is kept separate from this patch, because this smaller patch
> seems to work well enough on its own in practice, and make the patch
> less complex, thus a possible candidate for backporting to a release
> branch (thinking of the gdb-8.3-branch, in this case).
> 
> Another change that this commit makes is re-instate the thread create/
> exit notifications for what we thought was the main thread. We added
> some filtering recently to not show those notifications. Otherwise,
> with what we've discovered, it is possible that the list of threads
> reported by the "new thread" notifications does not match the list
> of threads from the "thread exit" notifications.
> 
> gdb/ChangeLog:
> 
> 	* windows-nat.c (windows_add_thread): Remove parameter "main_thread_p".
> 	Adjust function accordingly.  Update all callers.
> 	(windows_delete_thread): Likewise.
> 	(get_windows_debug_event) <EXIT_PROCESS_DEBUG_EVENT>: Use
>         current_event.dwThreadId instead of main_thread_id.
> 
> Tested on both x86-windows and x86_64-windows using AdaCore's testsuite.
> The portion of the patch that uses current_event.dwThreadId instead of
> main_thread_id has undergood about a couple of weeks of nightly testing
> on various Windows versions.

"undergood"?

> 
> OK to master?
> OK for 8.3?
> 
> Thanks!
> 
> ---
>  gdb/windows-nat.c | 58 ++++++++++++++++++++++++-------------------------------
>  1 file changed, 25 insertions(+), 33 deletions(-)
> 
> diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
> index 5009418..039eb13 100644
> --- a/gdb/windows-nat.c
> +++ b/gdb/windows-nat.c
> @@ -430,12 +430,10 @@ thread_rec (DWORD id, int get_context)
>  
>     PTID is the ptid of the thread to be added.
>     H is its Windows handle.
> -   TLB is its thread local base.
> -   MAIN_THREAD_P should be true if the thread to be added is
> -   the main thread, false otherwise.  */
> +   TLB is its thread local base.  */
>  
>  static windows_thread_info *
> -windows_add_thread (ptid_t ptid, HANDLE h, void *tlb, bool main_thread_p)
> +windows_add_thread (ptid_t ptid, HANDLE h, void *tlb)
>  {
>    windows_thread_info *th;
>    DWORD id;
> @@ -456,13 +454,18 @@ windows_add_thread (ptid_t ptid, HANDLE h, void *tlb, bool main_thread_p)
>  
>    /* Add this new thread to the list of threads.
>  
> -     To be consistent with what's done on other platforms, we add
> -     the main thread silently (in reality, this thread is really
> -     more of a process to the user than a thread).  */
> -  if (main_thread_p)
> -    add_thread_silent (ptid);
> -  else
> -    add_thread (ptid);
> +     On many platforms, we add the main program's thread silently.
> +     But unfortunately, it does not seem to identify such thread

Not clear what "it" is referring to here.  Maybe you
meant -- "it does not seem possible" ?

> +     on Windows. We used to think that the main program's thread

Double space after periods.

> +     is the thread reported by the CREATE_PROCESS_DEBUG_EVENT, but
> +     we've seen since then that this thread sometimes exits before
> +     other threads in the program -- when that happens, what we see
> +     is that the CREATE_PROCESS_DEBUG_EVENT thread gets reported
> +     as exited via an EXIT_THREAD_DEBUG_EVENT, and another thread
> +     gets used in the EXIT_PROCESS_DEBUG_EVENT notification instead.

I'd think that the thread reported in the CREATE_PROCESS_DEBUG_EVENT
event _is_ always the main thread, but that then it's possible for that
thread to _not_ be the last to exit the process.  Seems more logical to
me than what the comments above, and the commit log, suggest.

Note that <https://docs.microsoft.com/en-us/windows/desktop/api/minwinbase/ns-minwinbase-create_process_debug_info>
says that the CREATE_PROCESS_DEBUG_INFO structure includes a field for
the _initial_ thread: current_event.u.CreateProcessInfo.hThread.
The initial thread is what we call the main thread.

We're reading that handle when creating the gdb thread:

      main_thread_id = current_event.dwThreadId;
      /* Add the main thread.  */
      th = windows_add_thread
        (ptid_t (current_event.dwProcessId, 0,
		 current_event.dwThreadId),
	 current_event.u.CreateProcessInfo.hThread,
	 current_event.u.CreateProcessInfo.lpThreadLocalBase,

And I'd be very surprised if that handle (and thus the thread id
reported by the event), wasn't always the initial thread.

Note also that
<https://docs.microsoft.com/en-us/windows/desktop/api/minwinbase/ns-minwinbase-exit_process_debug_info>
shows that the EXIT_PROCESS_DEBUG_INFO structure does not have an associated
thread handle at all.  This seems to add a bit more strength to the hypothesis that it's the
assumption that the main thread is always the last to exit that is what's incorrect,
not that we can't determine which is the main/initial thread.

From <https://docs.microsoft.com/en-us/windows/desktop/procthread/terminating-a-thread>,
we see:

  "If the thread is the only active thread in the process, the process is terminated."

which again suggests that there's nothing special about the initial thread wrt to
process termination.

Did you try writing a small test program with a couple threads, that has
the main thread call ExitThread, or better, TerminateThread, while the other
thread still runs?  I wouldn't be surprised if that reproduced the scenario
in question.

> @@ -1637,11 +1629,11 @@ get_windows_debug_event (struct target_ops *ops,
>        else if (saw_create == 1)
>  	{
>  	  windows_delete_thread (ptid_t (current_event.dwProcessId, 0,
> -					 main_thread_id),
> -				 0, true /* main_thread_p */);
> +					 current_event.dwThreadId),
> +				 0);
>  	  ourstatus->kind = TARGET_WAITKIND_EXITED;
>  	  ourstatus->value.integer = current_event.u.ExitProcess.dwExitCode;
> -	  thread_id = main_thread_id;
> +	  thread_id = current_event.dwThreadId;
>  	}
>        break;

This looks right.

Thanks,
Pedro Alves

Re: [RFA 1/2][master+8.3] (Windows) fix thr != nullptr assert failure in delete_thread_1

[Pedro Alves]

On 4/18/19 3:52 PM, Pedro Alves wrote:

>> @@ -1637,11 +1629,11 @@ get_windows_debug_event (struct target_ops *ops,
>>        else if (saw_create == 1)
>>  	{
>>  	  windows_delete_thread (ptid_t (current_event.dwProcessId, 0,
>> -					 main_thread_id),
>> -				 0, true /* main_thread_p */);
>> +					 current_event.dwThreadId),
>> +				 0);
>>  	  ourstatus->kind = TARGET_WAITKIND_EXITED;
>>  	  ourstatus->value.integer = current_event.u.ExitProcess.dwExitCode;
>> -	  thread_id = main_thread_id;
>> +	  thread_id = current_event.dwThreadId;
>>  	}
>>        break;
> 
> This looks right.

I forgot to add: reading this made me wonder -- what about gdbserver?
I looked, and while gdbserver/win32-low.c had an equivalent of main_thread_id,
it doesn't use it for EXIT_PROCESS_DEBUG_EVENT either.

Thanks,
Pedro Alves

[v2] Windows native GDB event handling enhancement

[Joel Brobecker]

Hello,

This is v2 of the patch series, which now consists of 2 patches:

 * [RFA v2 1/2] (Windows) fix thr != nullptr assert failure in

   Compared to v1, it has been reduced to the essence of the fix,
   which is to avoid using main_thread_id during the exit-process
   event; and that's it.

 * [RFA v2 2/2] gdb/windows-nat.c: Get rid of main_thread_id global

   That one shold be unchanged.

The part that re-instated the thread notifications for the main thread
(at create-process event) and for the last thread (at exit-process
event) has been excised from v1 of patch #1.  I think this is the best
compromise based on the discussions so far.

I will personally miss the silenced notifications, but for those
like me, we can enable the windows event debug logs.

The reason why I kept the patches split in two is so that I can apply
patch #1 on the gdb-8.3 branch as well. Patch #2 has been behaving
well as far as AdaCore nightly testing is concerned, but it's only
been a few weeks since we implemented it, and I think it deserves
more time in master before being in a release.

The patches were tested on both 32bit and 64bit native windows,
using AdaCore testsuite. OK to push?

Thanks,
-- 
Joel

[RFA v2 2/2][master only] gdb/windows-nat.c: Get rid of main_thread_id global

[Joel Brobecker]

This global is meant to point to the "main" thread of execution of
the program we are debugging. It is set when attaching to a process
or when receiving a CREATE_PROCESS_DEBUG_EVENT event. The theory at
the time was that this was also going to be the thread receiving
the EXIT_PROCESS_DEBUG_EVENT event.

Unfortunately, we have discovered since then that this is actually
not guaranteed. What this means in practice is that there is moderate
risk that main_thread_id refers to a thread which no longer exists.

This global is used in 3 situations:
  - OUTPUT_DEBUG_STRING_EVENT
  - LOAD_DLL_DEBUG_EVENT
  - UNLOAD_DLL_DEBUG_EVENT

It's not clear why we would need to use the main_thread_id in those cases
instead of using the thread ID provided by the kernel events itself.
So this patch implements this approach, which then allows us to delete
the main_thread_id global.

gdb/testsuite:

	* windows-nat.c (main_thread_id): Delete.
	(handle_output_debug_string): Replace main_thread_id by
	current_event.dwThreadId.
	(fake_create_process): Likewise.
	(get_windows_debug_event) <CREATE_PROCESS_DEBUG_EVENT>:
	Do not set main_thread_id.
	<LOAD_DLL_DEBUG_EVENT>: Replace main_thread_id by
	current_event.dwThreadId.
	<UNLOAD_DLL_DEBUG_EVENT>: Likewise.
---
 gdb/windows-nat.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 9f3242c..ae4e3d5 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -237,7 +237,6 @@ static DEBUG_EVENT current_event;	/* The current debug event from
 					   WaitForDebugEvent */
 static HANDLE current_process_handle;	/* Currently executing process */
 static windows_thread_info *current_thread;	/* Info on currently selected thread */
-static DWORD main_thread_id;		/* Thread ID of the main thread */
 
 /* Counts of things.  */
 static int exception_count = 0;
@@ -1030,7 +1029,7 @@ handle_output_debug_string (struct target_waitstatus *ourstatus)
 	  ourstatus->kind = TARGET_WAITKIND_STOPPED;
 	  retval = strtoul (p, &p, 0);
 	  if (!retval)
-	    retval = main_thread_id;
+	    retval = current_event.dwThreadId;
 	  else if ((x = (LPCVOID) (uintptr_t) strtoull (p, NULL, 0))
 		   && ReadProcessMemory (current_process_handle, x,
 					 &saved_context,
@@ -1406,14 +1405,13 @@ fake_create_process (void)
        (unsigned) GetLastError ());
       /*  We can not debug anything in that case.  */
     }
-  main_thread_id = current_event.dwThreadId;
   current_thread
     = windows_add_thread (ptid_t (current_event.dwProcessId, 0,
 				  current_event.dwThreadId),
 			  current_event.u.CreateThread.hThread,
 			  current_event.u.CreateThread.lpThreadLocalBase,
 			  true /* main_thread_p */);
-  return main_thread_id;
+  return current_event.dwThreadId;
 }
 
 void
@@ -1611,7 +1609,6 @@ get_windows_debug_event (struct target_ops *ops,
 	break;
 
       current_process_handle = current_event.u.CreateProcessInfo.hProcess;
-      main_thread_id = current_event.dwThreadId;
       /* Add the main thread.  */
       th = windows_add_thread
         (ptid_t (current_event.dwProcessId, 0,
@@ -1656,7 +1653,7 @@ get_windows_debug_event (struct target_ops *ops,
       catch_errors (handle_load_dll);
       ourstatus->kind = TARGET_WAITKIND_LOADED;
       ourstatus->value.integer = 0;
-      thread_id = main_thread_id;
+      thread_id = current_event.dwThreadId;
       break;
 
     case UNLOAD_DLL_DEBUG_EVENT:
@@ -1669,7 +1666,7 @@ get_windows_debug_event (struct target_ops *ops,
       catch_errors (handle_unload_dll);
       ourstatus->kind = TARGET_WAITKIND_LOADED;
       ourstatus->value.integer = 0;
-      thread_id = main_thread_id;
+      thread_id = current_event.dwThreadId;
       break;
 
     case EXCEPTION_DEBUG_EVENT:
-- 
2.1.4

[RFA v2 1/2][master+8.3] (Windows) fix thr != nullptr assert failure in delete_thread_1

[Joel Brobecker]

We have observed that GDB would randomly trip the following
assertion failure when debugging on Windows. When allowing
the program to run until the inferior exits, we occasionally see:

     (gdb) cont
     Continuing.
     [Thread 48192.0xd100 exited with code 1]
     [Thread 48192.0x10ad8 exited with code 1]
     [Thread 48192.0x36e28 exited with code 0]
     [Thread 48192.0x52be4 exited with code 0]
     [Thread 48192.0x5aa40 exited with code 0]
     ../../src/gdb/thread.c:453: internal-error: void delete_thread_1(thread_inf
o*, bool): Assertion `thr != nullptr' failed.

Running the same scenario with some additional traces enabled...

    (gdb) set verbose
    (gdb) set debugevents

... allows us to understand what the issue is. To understand, we need
to first look at the events received when starting the program, and
in particular which threads got created how. First, we get a
CREATE_PROCESS_DEBUG_EVENT for tid=0x442a8:

    gdb: kernel event for pid=317536 tid=0x442a8 code=CREATE_PROCESS_DEBUG_EVENT)

Shortly after, we get some CREATE_THREAD_DEBUG_EVENT events,
one of them being for tid=0x4010c:

    gdb: kernel event for pid=317536 tid=0x4010c code=CREATE_THREAD_DEBUG_EVENT)
Fast forward a bit of debugging, and we do a "cont" as above,
at which point the programs reaches the end, and the system reports
"exit" events. The first interesting one is the following:

    gdb: kernel event for pid=317536 tid=0x442a8 code=EXIT_THREAD_DEBUG_EVENT)

This is reporting a thread-exit event for a thread whose tid
is the TID of what we call the "main thread". That's the thread
that was created when we received the CREATE_PROCESS_DEBUG_EVENT
notification, and whose TID is actually stored in a global variable
named main_thread_id. This is not something we expected, as
the assumption we made was that the main thread would exit last,
and we would be notified of it via an EXIT_PROCESS_DEBUG_EVENT.
But apparently, this is not always true, at least on Windows Server
2012 and 2016 where this issue has been observed happening randomly.

The consequence of the above notification is that we call
windows_delete_thread for that thread, which removes it from
our list of known threads.

And a little bit later, then we then get the EXIT_PROCESS_DEBUG_EVENT,
and we can see that the associated tid is not the main_thread_id,
but rather the tid of one of the threads that was created during
the lifetime of the program, in this case tid=0x4010c:

    gdb: kernel event for pid=317536 tid=0x4010c code=EXIT_PROCESS_DEBUG_EVENT)

And the debug trace printed right after shows why we're crashing:

    [Deleting Thread 317536.0x442a8]

We are trying to delete the thread whose tid=0x442a8, which is
the main_thread_id! As we have already deleted that thread before,
the search for it returns a nullptr, which then trips the assertion
check in delete_thread_1.

This commit fixes this issue. It ignores the open question of
what to do with the main_thread_id global, particularly after
that thread has been removed from our list of threads. This will
be dealt with as a separate patch, to allow cherry-picking
this patch into a release branch.

For now, we fix the code so as to avoid this crash.

gdb/ChangeLog:

	* windows-nat.c (get_windows_debug_event) <EXIT_PROCESS_DEBUG_EVENT>:
	Use current_event.dwThreadId instead of main_thread_id.
---
 gdb/windows-nat.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 5009418..9f3242c 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -1637,11 +1637,11 @@ get_windows_debug_event (struct target_ops *ops,
       else if (saw_create == 1)
 	{
 	  windows_delete_thread (ptid_t (current_event.dwProcessId, 0,
-					 main_thread_id),
+					 current_event.dwThreadId),
 				 0, true /* main_thread_p */);
 	  ourstatus->kind = TARGET_WAITKIND_EXITED;
 	  ourstatus->value.integer = current_event.u.ExitProcess.dwExitCode;
-	  thread_id = main_thread_id;
+	  thread_id = current_event.dwThreadId;
 	}
       break;
 
-- 
2.1.4

Re: [v2] Windows native GDB event handling enhancement

[Pedro Alves]

On 4/28/19 5:58 PM, Joel Brobecker wrote:
> Hello,
> 
> This is v2 of the patch series, which now consists of 2 patches:
> 
>  * [RFA v2 1/2] (Windows) fix thr != nullptr assert failure in
> 
>    Compared to v1, it has been reduced to the essence of the fix,
>    which is to avoid using main_thread_id during the exit-process
>    event; and that's it.
> 
>  * [RFA v2 2/2] gdb/windows-nat.c: Get rid of main_thread_id global
> 
>    That one shold be unchanged.
> 
> The part that re-instated the thread notifications for the main thread
> (at create-process event) and for the last thread (at exit-process
> event) has been excised from v1 of patch #1.  I think this is the best
> compromise based on the discussions so far.
> 
> I will personally miss the silenced notifications, but for those
> like me, we can enable the windows event debug logs.
> 
> The reason why I kept the patches split in two is so that I can apply
> patch #1 on the gdb-8.3 branch as well. Patch #2 has been behaving
> well as far as AdaCore nightly testing is concerned, but it's only
> been a few weeks since we implemented it, and I think it deserves
> more time in master before being in a release.
> 
> The patches were tested on both 32bit and 64bit native windows,
> using AdaCore testsuite. OK to push?
LGTM.

Thanks,
Pedro Alves

pushed(master+8.3): [v2] Windows native GDB event handling enhancement

[Joel Brobecker]

> LGTM.

Thanks Pedro.

Both patches now pushed to master; patch #1 also pushed to 8.3.

-- 
Joel