From patchwork Fri Oct 16 13:40:47 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Aleksandar Ristovski X-Patchwork-Id: 9186 Received: (qmail 21599 invoked by alias); 16 Oct 2015 13:40:51 -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 21588 invoked by uid 89); 16 Oct 2015 13:40:50 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW, SPF_SOFTFAIL autolearn=no version=3.3.2 X-HELO: smtp-a01.blackberry.com Received: from smtp-a01.blackberry.com (HELO smtp-a01.blackberry.com) (208.65.78.90) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 16 Oct 2015 13:40:49 +0000 Received: from mhs101cnc.rim.net ([10.65.141.79]) by mhs210cnc-app.rim.net with ESMTP; 16 Oct 2015 09:40:47 -0400 Received: from unknown (HELO [10.222.109.89]) ([10.65.140.253]) by mhs101cnc.rim.net with ESMTP; 16 Oct 2015 13:40:47 +0000 Subject: Re: [PATCH] gdbserver: Reset current_thread when its process is removed. To: Pedro Alves , "gdb-patches@sourceware.org" References: <1444919808-22088-1-git-send-email-aristovski@qnx.com> <561FD2B2.6070509@redhat.com> From: Aleksandar Ristovski X-Enigmail-Draft-Status: N1110 Message-ID: <5620FE5F.4000006@qnx.com> Date: Fri, 16 Oct 2015 09:40:47 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <561FD2B2.6070509@redhat.com> On 15-10-15 12:22 PM, Pedro Alves wrote: > On 10/15/2015 03:36 PM, Aleksandar Ristovski wrote: >> In case of running gdbserver with --multi, if a nat file >> removes a process, current_thread may remain set to now >> freed 'process' entry. >> >> This may lead to wrong operation or a crash. >> >> gdb/gdbserver/ChangeLog: >> * inferiors.c (remove_process): Reset current_thread if its >> associated process gets removed. >> --- >> gdb/gdbserver/inferiors.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/gdb/gdbserver/inferiors.c b/gdb/gdbserver/inferiors.c >> index 21f45fa..4688a44 100644 >> --- a/gdb/gdbserver/inferiors.c >> +++ b/gdb/gdbserver/inferiors.c >> @@ -291,6 +291,11 @@ remove_process (struct process_info *process) >> { >> clear_symbol_cache (&process->symbol_cache); >> free_all_breakpoints (process); >> + if (current_thread && get_thread_process (current_thread) == process) >> + { >> + remove_thread (current_thread); >> + current_thread = NULL; >> + } > > This seems very papering-over-something-else. > > - I could argue that current_thread = NULL would be better done > inside remove_thread than here, since what you say would happen > as well if the current thread is removed, even without removing > the process. Agree. > > - And then, if we remove the current thread, why not remove others? > True again. > AFAICS, other targets remove threads from within target_mourn, as that > way they have a chance of clearing auxiliary info associated with > the threads (the inferior_target_data()), and then some call > clear_inferiors, which also clears current_thread. clear_inferiors wouldn't work if we want to keep other inferiors. > >> remove_inferior (&all_processes, &process->entry); >> free (process); >> } >> Ok, if this is intended use, to first remove threads then it will work. I still think resetting current_thread in remove_thread is in order. Assert in remove_process would help people like me. Attached is another version as a proposal, though now not strictly necessary. The patch did not show any regressions testing on a native-gdbserver target board. (GNU/Linux x86_64) Thanks, Aleksandar From 51f95e4541852b0d6a10cf931bdeeada1a9dd99c Mon Sep 17 00:00:00 2001 From: Aleksandar Ristovski Date: Thu, 15 Oct 2015 09:23:48 -0400 Subject: [PATCH] gdbserver: Reset current_thread when the thread is removed. Reset current_thread and make sure 'remove_process' is used after all associated threads have been removed first. gdb/gdbserver/ChangeLog: * inferiors.c (thread_pid_matches_callback): New function. (find_thread_process): New function. (remove_thread): Reset current_thread. (remove_process): Assert threads have been removed first. --- gdb/gdbserver/inferiors.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/gdb/gdbserver/inferiors.c b/gdb/gdbserver/inferiors.c index 21f45fa..fe47a72 100644 --- a/gdb/gdbserver/inferiors.c +++ b/gdb/gdbserver/inferiors.c @@ -141,6 +141,21 @@ find_thread_ptid (ptid_t ptid) return (struct thread_info *) find_inferior_id (&all_threads, ptid); } +static int +thread_pid_matches_callback (struct inferior_list_entry *entry, void *args) +{ + return (ptid_get_pid (entry->id) == *(pid_t *)args); +} + +static struct thread_info * +find_thread_process (const struct process_info *const process) +{ + pid_t pid = ptid_get_pid (ptid_of (process)); + + return (struct thread_info *) + find_inferior (&all_threads, thread_pid_matches_callback, &pid); +} + ptid_t gdb_id_to_thread_id (ptid_t gdb_id) { @@ -166,6 +181,8 @@ remove_thread (struct thread_info *thread) discard_queued_stop_replies (ptid_of (thread)); remove_inferior (&all_threads, (struct inferior_list_entry *) thread); free_one_thread (&thread->entry); + if (current_thread == thread) + current_thread = NULL; } /* Return a pointer to the first inferior in LIST, or NULL if there isn't one. @@ -291,6 +308,7 @@ remove_process (struct process_info *process) { clear_symbol_cache (&process->symbol_cache); free_all_breakpoints (process); + gdb_assert (find_thread_process (process) == NULL); remove_inferior (&all_processes, &process->entry); free (process); } -- 1.9.1