Re: [PATCH v1.1 01/23] Preserve selected thread in all-stop w/ background execution

Aktemur, Tankut Baris
2019-10-17T10:21:14Z

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 5778 bytes --]

* On Thursday, October 17, 2019 1:55 AM, Pedro Alves wrote:
>
> On 10/9/19 10:36 AM, Aktemur, Tankut Baris wrote:
> > * On September 7, 2019 1:28 AM, Pedro Alves wrote:
> >>
> >> diff --git a/gdb/infrun.c b/gdb/infrun.c
> >> index a9588f896a..9c888aa72f 100644
> >> --- a/gdb/infrun.c
> >> +++ b/gdb/infrun.c
> >> @@ -3048,6 +3048,11 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
> >>
> >> finish_state.release ();
> >>
> >> + /* If we've switched threads above, switch back to the previously
> >> + current thread. We don't want the user to see a different
> >> + selected thread. */
> >> + switch_to_thread (cur_thr);
> >> +
> >> /* Tell the event loop to wait for it to stop. If the target
> >> supports asynchronous execution, it'll do this from within
> >> target_resume. */
> >> @@ -3702,14 +3707,11 @@ fetch_inferior_event (void *client_data)
> >> set_current_traceframe (-1);
> >> }
> >>
> >> - gdb::optional<scoped_restore_current_thread> maybe_restore_thread;
> >> -
> >> - if (non_stop)
> >> - /* In non-stop mode, the user/frontend should not notice a thread
> >> - switch due to internal events. Make sure we reverse to the
> >> - user selected thread and frame after handling the event and
> >> - running any breakpoint commands. */
> >> - maybe_restore_thread.emplace ();
> >> + /* The user/frontend should not notice a thread switch due to
> >> + internal events. Make sure we revert to the user selected
> >> + thread and frame after handling the event and running any
> >> + breakpoint commands. */
> >> + scoped_restore_current_thread restore_thread;
> >>
> >
> > Because this increases the refcount of the current thread, in case the
> > fetched inferior event denotes a thread exit, the thread will not be deleted
> > right away. A non-deleted but exited thread stays in the inferior's thread
> > list. This, in turn, causes the "init_thread_list" call in inferior.c to
> > be skipped. As a side effect, a regression is observed in
> >
> > gdb.arch/i386-mpx-simple_segv.exp
>
> Thanks for spotting this. I don't have MPX on my machine, so I'm not
> exactly sure the sequence of events that lead to a failure in that test,
> but I found a way to reproduce a related problem without MPX, and wrote
> a test based on it.
>
> >
> > IMHO, the 'any_thread_p' predicate should be updated. This predicate is used
> > in two places (one in 'inferior.c' and the other in 'mi/mi-main.c'). Both
> > uses, I believe, are in fact interested in whether there are any non-exited
> > threads. I'd suggest updating 'any_thread_p' to 'any_non_exited_thread_p'.
> >
>
> I'm really not sure about that. Exited threads have a thread number still,
> as long as they're on the list. Calling init_thread_list resets the
> global thread counter, meaning that you could end up with multiple threads
> with the same number, until the exited threads are purged.
>
> I think instead a delete_exited_threads call in inferior_appeared
> is better.

Thanks, this makes sense.

>
> >> overlay_cache_invalid = 1;
> >> /* Flush target cache before starting to handle each event. Target
> >> @@ -3786,6 +3788,19 @@ fetch_inferior_event (void *client_data)
> >> inferior_event_handler (INF_EXEC_COMPLETE, NULL);
> >> cmd_done = 1;
> >> }
> >> +
> >> + /* If we got a TARGET_WAITKIND_NO_RESUMED event, then the
> >> + previously selected thread is gone. We have two
> >> + choices - switch to no thread selected, or restore the
> >> + previously selected thread (now exited). We chose the
> >> + later, just because that's what GDB used to do. After
> >> + this, "info threads" says "The current thread <Thread
> >> + ID 2> has terminated." instead of "No thread
> >> + selected.". */
> >> + if (!non_stop
> >> + && cmd_done
> >> + && ecs->ws.kind != TARGET_WAITKIND_NO_RESUMED)
> >> + restore_thread.dont_restore ();
> >> }
> >> }
> >>
> >
> > The comment and the code seem to contradict each other. The comment says
> > "if we got a TARGET_WAITKIND_NO_RESUMED" whereas the condition is
> >
> > ecs->ws.kind != TARGET_WAITKIND_NO_RESUMED
>
> I don't think they're contradicting, actually.
>
> When the 'if' condition is true, we won't restore the selected thread.
> So we if we got a TARGET_WAITKIND_NO_RESUMED, we restore the previous
> selected thread. For all other event kinds, we won't restore.
>

Right, sorry, my bad.

> >
> > Should TARGET_WAITKIND_THREAD_EXITED, TARGET_WAITKIND_EXITED, and
> > TARGET_WAITKIND_SIGNALLED be also included in the condition? They also mean
> > that the thread is gone, right?
>
> Not necessarily. With checkpoint, gdb automatically switches to
> another checkpoint on TARGET_WAITKIND_EXITED/TARGET_WAITKIND_SIGNALLED:
>
> (gdb) checkpoint
> checkpoint 1: fork returned pid 10741.
> (gdb) c
> Continuing.
> [Inferior 1 (process 10737) exited normally]
> [Switching to process 10741]
> (gdb) info threads
> Id Target Id Frame
> * 1 process 10741 "main" main () at main.cc:21
> (gdb)
>
> Here's an updated patch. I believe it should fix the MPX issue too.

Yes, the MPX test no longer regresses with this updated patch.

Regards,
-Baris

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
\x16º&Öéj×!zÊÞ¶êç×Ÿ{ßyb²Ö«r\x18\x1dn–­r\x17¬