From patchwork Sun Oct 19 22:56:26 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Doug Evans X-Patchwork-Id: 3287 Received: (qmail 28245 invoked by alias); 19 Oct 2014 22:57:48 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 28232 invoked by uid 89); 19 Oct 2014 22:57:47 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL, BAYES_00, FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-pd0-f171.google.com Received: from mail-pd0-f171.google.com (HELO mail-pd0-f171.google.com) (209.85.192.171) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Sun, 19 Oct 2014 22:57:46 +0000 Received: by mail-pd0-f171.google.com with SMTP id ft15so3800708pdb.16 for ; Sun, 19 Oct 2014 15:57:44 -0700 (PDT) X-Received: by 10.70.51.195 with SMTP id m3mr23297127pdo.27.1413759464569; Sun, 19 Oct 2014 15:57:44 -0700 (PDT) Received: from seba.sebabeach.org.gmail.com (173-13-178-50-sfba.hfc.comcastbusiness.net. [173.13.178.50]) by mx.google.com with ESMTPSA id pw10sm7349980pbc.93.2014.10.19.15.57.13 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sun, 19 Oct 2014 15:57:14 -0700 (PDT) From: Doug Evans To: Pedro Alves 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> Date: Sun, 19 Oct 2014 15:56:26 -0700 In-Reply-To: <542ED988.8050407@redhat.com> (Pedro Alves's message of "Fri, 03 Oct 2014 18:14:48 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 X-IsSubscribed: yes Pedro Alves writes: >> The most succinct way of expressing what I'm looking for that I can >> think of is that I want to understand this code, and I don't. >> There are several instances of the !have_inferiors() check, I picked >> this one because it seemed like as good a choice as any. > > init_thread_list does two things: > > - wipes all threads. > - resets the thread id counter back to 0, so the next > thread is gdb thread number 1. > > The main point of calling init_thread_list if there are > no live inferiors, is to reset the thread id counter. > That is so that with "run; kill; run", you end up with > the main thread being gdb thread number 1, in both > runs. That part I can understand. > Now, if there are other live inferiors, then we shouldn't wipe > their threads. And as gdb thread ids are global currently, > not private per-inferior/process, then we obviously shouldn't > reset the thread id counter either. This I can grok too. 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 (); + /* 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). + 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); highest_thread_num = 0; - if (!thread_list) - return; + /* Take the opportunity to update this since we know it's zero. */ + threads_executing = 0; +} + +void +init_thread_list (void) +{ + struct thread_info *tp, *tpnext; for (tp = thread_list; tp; tp = tpnext) { @@ -212,7 +227,8 @@ init_thread_list (void) } thread_list = NULL; - threads_executing = 0; + + reset_thread_numbering (); } /* Allocate a new thread with target id PTID and add it to the thread