From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29291 invoked by alias); 24 Oct 2014 16:54:04 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 29280 invoked by uid 89); 24 Oct 2014 16:54:03 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.7 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Fri, 24 Oct 2014 16:54:02 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s9OGrwbx018743 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Fri, 24 Oct 2014 12:53:58 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s9OGruSH006614; Fri, 24 Oct 2014 12:53:57 -0400 Message-ID: <544A8423.1010701@redhat.com> Date: Fri, 24 Oct 2014 16:54:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.1.1 MIME-Version: 1.0 To: Doug Evans CC: gdb-patches , Stan Shebs Subject: Re: Questions on commit 6c95b8df7fef5273da71c34775918c554aae0ea8 References: <54259976.9000907@redhat.com> <21547.2428.934404.571592@ruffy2.mtv.corp.google.com> <542ED988.8050407@redhat.com> In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2014-10/txt/msg00657.txt.bz2 On 10/19/2014 11:56 PM, Doug Evans wrote: > However, what if some code is not properly clearing a thread > from the list? (which may involve just tagging it as exited and leaving > gc'ing it until later) Such a bug will be hidden in the > !have_inferiors() case because we take a sledgehammer to the list. > To the reader it's not clear whether wiping the list is necessary, > or just follows along for the ride, so to speak, and is essentially > a no-op, because we're using init_thread_list to reset thread numbering. > > I would expect the attached patch to be a no-op. > I don't have an opinion on committing it. > I do have an opinion that the current code is confusing, > and this is one way to help clear it up. > > 2014-10-19 Doug Evans > > * thread.c (reset_thread_numbering): New function. > (init_thread_list): Call it. > * gdbthread.h (reset_thread_numbering): Declare. > * fork-child.c (fork_inferior): Call reset_thread_numbering instead of > init_thread_list. > * infcmd.c (kill_command): Ditto. > (detach_command): Ditto. > * remote.c (extended_remote_create_inferior): Ditto. > > diff --git a/gdb/fork-child.c b/gdb/fork-child.c > index 4c316b1..149eb22 100644 > --- a/gdb/fork-child.c > +++ b/gdb/fork-child.c > @@ -379,7 +379,10 @@ fork_inferior (char *exec_file_arg, char *allargs, char **env, > environ = save_our_env; > > if (!have_inferiors ()) > - init_thread_list (); > + { > + /* Reset thread numbering to begin back at 1. */ > + reset_thread_numbering (); > + } > > inf = current_inferior (); > > diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h > index fb47bae..4b3bf7f 100644 > --- a/gdb/gdbthread.h > +++ b/gdb/gdbthread.h > @@ -261,6 +261,11 @@ struct thread_info > struct btrace_thread_info btrace; > }; > > +/* Reset thread numbering so that they begin at 1 again. > + This can only be called when it is known that there are no current > + non-exited threads. A typical test is !have_inferiors(). */ > +extern void reset_thread_numbering (void); > + > /* Create an empty thread list, or empty the existing one. */ > extern void init_thread_list (void); > > diff --git a/gdb/infcmd.c b/gdb/infcmd.c > index 4415b31..0d06996 100644 > --- a/gdb/infcmd.c > +++ b/gdb/infcmd.c > @@ -2399,7 +2399,8 @@ kill_command (char *arg, int from_tty) > with their threads. */ > if (!have_inferiors ()) > { > - init_thread_list (); /* Destroy thread info. */ > + /* Reset thread numbering to begin back at 1. */ > + reset_thread_numbering (); > > /* Killing off the inferior can leave us with a core file. If > so, print the state we are left in. */ > @@ -2787,10 +2788,11 @@ detach_command (char *args, int from_tty) > if (!gdbarch_has_global_solist (target_gdbarch ())) > no_shared_libraries (NULL, from_tty); > > - /* If we still have inferiors to debug, then don't mess with their > - threads. */ > if (!have_inferiors ()) > - init_thread_list (); > + { > + /* No more inferiors, reset thread numbering back to 1. */ > + reset_thread_numbering (); > + } > > if (deprecated_detach_hook) > deprecated_detach_hook (); > diff --git a/gdb/remote.c b/gdb/remote.c > index 20f2988..6c43da8 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -8035,10 +8035,12 @@ extended_remote_create_inferior (struct target_ops *ops, > > if (!have_inferiors ()) > { > + /* Reset thread numbering to begin back at 1. */ > + reset_thread_numbering (); IMO this function name is clear enough that we wouldn't need to add that same comment to all callers. > + > /* Clean up from the last time we ran, before we mark the target > running again. This will mark breakpoints uninserted, and > get_offsets may insert breakpoints. */ > - init_thread_list (); > init_wait_for_inferior (); > } > > diff --git a/gdb/thread.c b/gdb/thread.c > index 5c94264..789ab7e 100644 > --- a/gdb/thread.c > +++ b/gdb/thread.c > @@ -195,15 +195,30 @@ free_thread (struct thread_info *tp) > xfree (tp); > } > > +/* See gdbthread.h. */ > + > void > -init_thread_list (void) > +reset_thread_numbering (void) > { > - struct thread_info *tp, *tpnext; > + struct thread_info *tp; > + > + /* While IWBN to assert thread_list is NULL, there could still be > + threads on it (e.g., if you detach from the current inferior). Not sure I understand this detach reference. Are you seeing exited threads after a detach from the current inferior? What are the steps you used? detach_inferior and exit_inferior are called with inferior_ptid pointing at null_ptid so that thread can be deleted. > + Instead verify all threads on the list have exited. They will be > + garbage-collected as we're able to. */ > + for (tp = thread_list; tp != NULL; tp = tp->next) > + gdb_assert (tp->state == THREAD_EXITED); Hmm, this doesn't make that much sense to me. Either we have an empty list, or we can't reset the thread numbers. Otherwise, the next thread we add might reuse the GDB ID of one of those exited threads, if it also reuses the target side ID (which can easily happen when the target fakes thread ids, e.g., remote.c:magic_null_ptid). Thinking about this further, I think the cases that may legitimately leave an exited thread in the list are related to thread's reference count having been incremented, either because a make_cleanup_restore_current_thread cleanup is in the cleanup chain, or the user did "thread apply all some-command-that-wipes-threads" -- detach/kill/etc., but also could also be the remote connection closing abruptly while running any command. do_restore_current_thread_cleanup won't restore the dead thread as selected thread, but I think the thread will still be in the list. This then suggest to me that what we need is a function that tries to garbage collect exited threads, and then call that without checking have_inferiors() at specific points We could start with the current places we call init_thread_list, and look for somewhere more central in a following step. prune_threads is close, but I don't think there's need to query anything off of the target. Resetting the thread numbering would still have to only be done under have_inferiors(), or, perhaps even clearer, only if the thread list is empty after the purging. IOW, say that the thread numbering always starts at 1 if we're adding a thread to an empty thread list. With this in mind, it might be simpler to instead reset to the thread numbering back to 1 in new_thread. WDYT? Thanks, Pedro Alves