* 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 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 >> + 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 &j!z޶ן{yb֫rnr