* [PATCH 0/8] Fix removing inferiors from within "thread apply" commands
@ 2017-04-11 23:51 Pedro Alves
2017-04-11 23:51 ` [PATCH 3/8] C++fy thread_apply_all_command Pedro Alves
` (7 more replies)
0 siblings, 8 replies; 25+ messages in thread
From: Pedro Alves @ 2017-04-11 23:51 UTC (permalink / raw)
To: gdb-patches
It took a bit more work than I envisioned, but ...
... this series aims at fixing the issues mentioned here:
https://sourceware.org/ml/gdb-patches/2017-04/msg00137.html
Those are addressed by the last patch in this series (patch 8).
That patch however exposed a couple other latent problems, which are
fixed by the first two patches in this series.
Patch 3 ("C++fy thread_apply_all_command"), I wrote because I first
thought we'd also have to bump the refcounts of all inferiors in
"thread apply all". That's actually not necessary, but still that's a
useful patch for getting rid of cleanups, so I included it here.
Patch 4 increases coverage of a regression test for a related bug. I
had noticed that we weren't making sure that GDB reverts back to no
thread selected in that test. And patch 3 made me realize that that
test also didn't exercise "thread apply $some_thread", which goes via
a different path, so I fixed it too.
Patches 5-7 are preparatory/cleanup work for patch 8.
Tested on x86_64 Fedora 23, native and gdbserver.
Pedro Alves (8):
watch_command_1: Fix dangling frame access
Fix follow-fork latent bug
C++fy thread_apply_all_command
Improve coverage of the PR threads/13217 regression test
GC inferior.c:init_inferior_list
Make inferior a class with cdtors, and use new/delete
Make inferior::detaching a bool, and introduce
scoped_restore::release()
Fix removing inferiors from within "thread apply" commands
gdb/Makefile.in | 6 +-
gdb/breakpoint.c | 34 +++--
gdb/common/scoped_restore.h | 36 +++--
gdb/inferior.c | 52 ++-----
gdb/inferior.h | 110 +++++++++------
gdb/infrun.c | 13 +-
gdb/testsuite/gdb.threads/threadapply.exp | 166 +++++++++++++++++++++-
gdb/testsuite/lib/gdb.exp | 19 +++
gdb/thread.c | 222 ++++++++++++++++--------------
gdb/unittests/scoped_restore-selftests.c | 110 +++++++++++++++
10 files changed, 556 insertions(+), 212 deletions(-)
create mode 100644 gdb/unittests/scoped_restore-selftests.c
--
2.5.5
^ permalink raw reply [flat|nested] 25+ messages in thread* [PATCH 3/8] C++fy thread_apply_all_command 2017-04-11 23:51 [PATCH 0/8] Fix removing inferiors from within "thread apply" commands Pedro Alves @ 2017-04-11 23:51 ` Pedro Alves 2017-04-13 10:06 ` Yao Qi 2017-04-11 23:51 ` [PATCH 7/8] Make inferior::detaching a bool, and introduce scoped_restore::release() Pedro Alves ` (6 subsequent siblings) 7 siblings, 1 reply; 25+ messages in thread From: Pedro Alves @ 2017-04-11 23:51 UTC (permalink / raw) To: gdb-patches This eliminates a couple cleanups. gdb/ChangeLog: yyyy-mm-dd Pedro Alves <palves@redhat.com> * thread.c: Include <algorithm>. (thread_array_cleanup): Delete. (scoped_inc_dec_ref): New class. (live_threads_count): New function. (set_thread_refcount): Delete. (tp_array_compar_ascending): Now a bool. (tp_array_compar): Convert to a std::sort comparison function. (thread_apply_all_command): Use std::vector and scoped_inc_dec_ref and live_threads_count. --- gdb/thread.c | 137 +++++++++++++++++++++++++++++++---------------------------- 1 file changed, 73 insertions(+), 64 deletions(-) diff --git a/gdb/thread.c b/gdb/thread.c index abfce71..88fd521 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -44,6 +44,7 @@ #include "cli/cli-utils.h" #include "thread-fsm.h" #include "tid-parse.h" +#include <algorithm> /* Definition of struct thread_info exported to gdbthread.h. */ @@ -69,16 +70,27 @@ static void info_threads_command (char *, int); static void thread_apply_command (char *, int); static void restore_current_thread (ptid_t); -/* Data to cleanup thread array. */ +/* RAII type used to increase / decrease the refcount of each thread + in a given list of threads. */ -struct thread_array_cleanup +class scoped_inc_dec_ref { - /* Array of thread pointers used to set - reference count. */ - struct thread_info **tp_array; +public: + explicit scoped_inc_dec_ref (const std::vector<thread_info *> &thrds) + : m_thrds (thrds) + { + for (thread_info *thr : m_thrds) + thr->incref (); + } - /* Thread count in the array. */ - int count; + ~scoped_inc_dec_ref () + { + for (thread_info *thr : m_thrds) + thr->decref (); + } + +private: + const std::vector<thread_info *> &m_thrds; }; @@ -565,6 +577,20 @@ thread_count (void) return result; } +/* Return the number of non-exited threads in the thread list. */ + +static int +live_threads_count (void) +{ + int result = 0; + struct thread_info *tp; + + ALL_NON_EXITED_THREADS (tp) + ++result; + + return result; +} + int valid_global_thread_id (int global_id) { @@ -1605,19 +1631,6 @@ restore_current_thread_cleanup_dtor (void *arg) xfree (old); } -/* Set the thread reference count. */ - -static void -set_thread_refcount (void *data) -{ - int k; - struct thread_array_cleanup *ta_cleanup - = (struct thread_array_cleanup *) data; - - for (k = 0; k != ta_cleanup->count; k++) - ta_cleanup->tp_array[k]->decref (); -} - struct cleanup * make_cleanup_restore_current_thread (void) { @@ -1693,30 +1706,30 @@ print_thread_id (struct thread_info *thr) return s; } -/* If non-zero tp_array_compar should sort in ascending order, otherwise in - descending order. */ +/* If true, tp_array_compar should sort in ascending order, otherwise + in descending order. */ -static int tp_array_compar_ascending; +static bool tp_array_compar_ascending; /* Sort an array for struct thread_info pointers by thread ID (first by inferior number, and then by per-inferior thread number). The order is determined by TP_ARRAY_COMPAR_ASCENDING. */ -static int -tp_array_compar (const void *ap_voidp, const void *bp_voidp) +static bool +tp_array_compar (const thread_info *a, const thread_info *b) { - const struct thread_info *a = *(const struct thread_info * const *) ap_voidp; - const struct thread_info *b = *(const struct thread_info * const *) bp_voidp; - if (a->inf->num != b->inf->num) { - return (((a->inf->num > b->inf->num) - (a->inf->num < b->inf->num)) - * (tp_array_compar_ascending ? +1 : -1)); + if (tp_array_compar_ascending) + return a->inf->num < b->inf->num; + else + return a->inf->num > b->inf->num; } - return (((a->per_inf_num > b->per_inf_num) - - (a->per_inf_num < b->per_inf_num)) - * (tp_array_compar_ascending ? +1 : -1)); + if (tp_array_compar_ascending) + return (a->per_inf_num < b->per_inf_num); + else + return (a->per_inf_num > b->per_inf_num); } /* Apply a GDB command to a list of threads. List syntax is a whitespace @@ -1732,15 +1745,13 @@ thread_apply_all_command (char *cmd, int from_tty) { struct cleanup *old_chain; char *saved_cmd; - int tc; - struct thread_array_cleanup ta_cleanup; - tp_array_compar_ascending = 0; + tp_array_compar_ascending = false; if (cmd != NULL && check_for_argument (&cmd, "-ascending", strlen ("-ascending"))) { cmd = skip_spaces (cmd); - tp_array_compar_ascending = 1; + tp_array_compar_ascending = true; } if (cmd == NULL || *cmd == '\000') @@ -1755,42 +1766,40 @@ thread_apply_all_command (char *cmd, int from_tty) saved_cmd = xstrdup (cmd); make_cleanup (xfree, saved_cmd); - /* Note this includes exited threads. */ - tc = thread_count (); + int tc = live_threads_count (); if (tc != 0) { - struct thread_info **tp_array; - struct thread_info *tp; - int i = 0, k; + /* Save a copy of the thread list and increment each thread's + refcount while executing the command in the context of each + thread, in case the command is one that wipes threads. E.g., + detach, kill, disconnect, etc., or even normally continuing + over an inferior or thread exit. */ + std::vector<thread_info *> thr_list_cpy; + thr_list_cpy.reserve (tc); - /* Save a copy of the thread_list in case we execute detach - command. */ - tp_array = XNEWVEC (struct thread_info *, tc); - make_cleanup (xfree, tp_array); + { + thread_info *tp; - ALL_NON_EXITED_THREADS (tp) - { - tp_array[i] = tp; - tp->incref (); - i++; - } - /* Because we skipped exited threads, we may end up with fewer - threads in the array than the total count of threads. */ - gdb_assert (i <= tc); + ALL_NON_EXITED_THREADS (tp) + { + thr_list_cpy.push_back (tp); + } + + gdb_assert (thr_list_cpy.size () == tc); + } - if (i != 0) - qsort (tp_array, i, sizeof (*tp_array), tp_array_compar); + /* Increment the refcounts, and restore them back on scope + exit. */ + scoped_inc_dec_ref inc_dec_ref (thr_list_cpy); - ta_cleanup.tp_array = tp_array; - ta_cleanup.count = i; - make_cleanup (set_thread_refcount, &ta_cleanup); + std::sort (thr_list_cpy.begin (), thr_list_cpy.end (), tp_array_compar); - for (k = 0; k != i; k++) - if (thread_alive (tp_array[k])) + for (thread_info *thr : thr_list_cpy) + if (thread_alive (thr)) { - switch_to_thread (tp_array[k]->ptid); + switch_to_thread (thr->ptid); printf_filtered (_("\nThread %s (%s):\n"), - print_thread_id (tp_array[k]), + print_thread_id (thr), target_pid_to_str (inferior_ptid)); execute_command (cmd, from_tty); -- 2.5.5 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/8] C++fy thread_apply_all_command 2017-04-11 23:51 ` [PATCH 3/8] C++fy thread_apply_all_command Pedro Alves @ 2017-04-13 10:06 ` Yao Qi 0 siblings, 0 replies; 25+ messages in thread From: Yao Qi @ 2017-04-13 10:06 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches Pedro Alves <palves@redhat.com> writes: > yyyy-mm-dd Pedro Alves <palves@redhat.com> > > * thread.c: Include <algorithm>. > (thread_array_cleanup): Delete. > (scoped_inc_dec_ref): New class. > (live_threads_count): New function. > (set_thread_refcount): Delete. > (tp_array_compar_ascending): Now a bool. > (tp_array_compar): Convert to a std::sort comparison function. > (thread_apply_all_command): Use std::vector and scoped_inc_dec_ref > and live_threads_count. LGTM. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 7/8] Make inferior::detaching a bool, and introduce scoped_restore::release() 2017-04-11 23:51 [PATCH 0/8] Fix removing inferiors from within "thread apply" commands Pedro Alves 2017-04-11 23:51 ` [PATCH 3/8] C++fy thread_apply_all_command Pedro Alves @ 2017-04-11 23:51 ` Pedro Alves 2017-04-11 23:51 ` [PATCH 6/8] Make inferior a class with cdtors, and use new/delete Pedro Alves ` (5 subsequent siblings) 7 siblings, 0 replies; 25+ messages in thread From: Pedro Alves @ 2017-04-11 23:51 UTC (permalink / raw) To: gdb-patches I left making inferior::detaching a bool to a separate patch, because doing that makes a make_cleanup_restore_integer call in infrun.c:prepare_for_detach no longer compile (passing a 'bool *' when an 'int *' is expected). Since we want to get rid of cleanups anyway, I looked at converting that to a scoped_restore. However, prepare_for_detach wants to discard the cleanup on success, and scoped_restore doesn't have an equivalent for that. So I added one -- I called it "release()" because it seems like a natural fit in the way standard components call similarly-spirited methods, and, it's also what the proposal for a generic scope guard calls it too, AFAICS: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4189.pdf I've added some scoped_guard unit tests, while at it. gdb/ChangeLog: yyyy-mm-dd Pedro Alves <palves@redhat.com> * Makefile.in (SUBDIR_UNITTESTS_SRCS): Add unittests/scoped_restore-selftests.c. (SUBDIR_UNITTESTS_OBS): Add scoped_restore-selftests.o. * common/scoped_restore.h (scoped_restore_base): Make "class". (scoped_restore_base::release): New public method. (scoped_restore_base::scoped_restore_base): New protected ctor. (scoped_restore_base::m_saved_var): New protected field. (scoped_restore_tmpl::scoped_restore_tmpl(T*)): Initialize the scoped_restore_base base class instead of m_saved_var directly. (scoped_restore_tmpl::scoped_restore_tmpl(T*, T2)): Likewise. (scoped_restore_tmpl::scoped_restore_tmpl(const scoped_restore_tmpl<T>&)): Likewise. (scoped_restore_tmpl::~scoped_restore_tmpl): Use the saved_var method. (scoped_restore_tmpl::saved_var): New method. (scoped_restore_tmpl::m_saved_var): Delete. * inferior.h (inferior::detaching): Now a bool. * infrun.c (prepare_for_detach): Use a scoped_restore instead of a cleanup. * unittests/scoped_restore-selftests.c: New file. --- gdb/Makefile.in | 6 +- gdb/common/scoped_restore.h | 36 +++++++--- gdb/inferior.h | 2 +- gdb/infrun.c | 8 +-- gdb/unittests/scoped_restore-selftests.c | 110 +++++++++++++++++++++++++++++++ 5 files changed, 144 insertions(+), 18 deletions(-) create mode 100644 gdb/unittests/scoped_restore-selftests.c diff --git a/gdb/Makefile.in b/gdb/Makefile.in index 23e4bed..aac1ed9 100644 --- a/gdb/Makefile.in +++ b/gdb/Makefile.in @@ -526,12 +526,14 @@ SUBDIR_PYTHON_CFLAGS = SUBDIR_UNITTESTS_SRCS = \ unittests/function-view-selftests.c \ unittests/offset-type-selftests.c \ - unittests/ptid-selftests.c + unittests/ptid-selftests.c \ + unittests/scoped_restore-selftests.c SUBDIR_UNITTESTS_OBS = \ function-view-selftests.o \ offset-type-selftests.o \ - ptid-selftests.o + ptid-selftests.o \ + scoped_restore-selftests.o # Opcodes currently live in one of two places. Either they are in the # opcode library, typically ../opcodes, or they are in a header file diff --git a/gdb/common/scoped_restore.h b/gdb/common/scoped_restore.h index ae7a49f..a638a06 100644 --- a/gdb/common/scoped_restore.h +++ b/gdb/common/scoped_restore.h @@ -21,8 +21,23 @@ #define SCOPED_RESTORE_H /* Base class for scoped_restore_tmpl. */ -struct scoped_restore_base +class scoped_restore_base { +public: + /* This informs the (scoped_restore_impl<T>) dtor that you no longer + want the original value restored. */ + void release () const + { m_saved_var = NULL; } + +protected: + scoped_restore_base (void *saved_var) + : m_saved_var (saved_var) + {} + + /* The type-erased saved variable. This is here so that clients can + call release() on a "scoped_restore" local, which is a typedef to + a scoped_restore_base. See below. */ + mutable void *m_saved_var; }; /* A convenience typedef. Users of make_scoped_restore declare the @@ -40,7 +55,7 @@ class scoped_restore_tmpl : public scoped_restore_base of *VAR. *VAR will be restored when this scoped_restore object is destroyed. */ scoped_restore_tmpl (T *var) - : m_saved_var (var), + : scoped_restore_base (var), m_saved_value (*var) { } @@ -52,14 +67,14 @@ class scoped_restore_tmpl : public scoped_restore_base E.g.: T='base'; T2='derived'. */ template <typename T2> scoped_restore_tmpl (T *var, T2 value) - : m_saved_var (var), + : scoped_restore_base (var), m_saved_value (*var) { *var = value; } scoped_restore_tmpl (const scoped_restore_tmpl<T> &other) - : m_saved_var (other.m_saved_var), + : scoped_restore_base {other.m_saved_var}, m_saved_value (other.m_saved_value) { other.m_saved_var = NULL; @@ -67,18 +82,19 @@ class scoped_restore_tmpl : public scoped_restore_base ~scoped_restore_tmpl () { - if (m_saved_var != NULL) - *m_saved_var = m_saved_value; + if (saved_var () != NULL) + *saved_var () = m_saved_value; } - private: +private: + /* Return a pointer to the saved variable with its type + restored. */ + T *saved_var () + { return static_cast<T *> (m_saved_var); } /* No need for this. It is intentionally not defined anywhere. */ scoped_restore_tmpl &operator= (const scoped_restore_tmpl &); - /* The saved variable. */ - mutable T *m_saved_var; - /* The saved value. */ const T m_saved_value; }; diff --git a/gdb/inferior.h b/gdb/inferior.h index 2638ac7..44f8157 100644 --- a/gdb/inferior.h +++ b/gdb/inferior.h @@ -387,7 +387,7 @@ public: bool waiting_for_vfork_done = false; /* True if we're in the process of detaching from this inferior. */ - int detaching = 0; + bool detaching = false; /* What is left to do for an execution command after any thread of this inferior stops. For continuations associated with a diff --git a/gdb/infrun.c b/gdb/infrun.c index c7298a3..fcafdc1 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -3630,7 +3630,6 @@ prepare_for_detach (void) { struct inferior *inf = current_inferior (); ptid_t pid_ptid = pid_to_ptid (inf->pid); - struct cleanup *old_chain_1; struct displaced_step_inferior_state *displaced; displaced = get_displaced_stepping_state (inf->pid); @@ -3644,8 +3643,7 @@ prepare_for_detach (void) fprintf_unfiltered (gdb_stdlog, "displaced-stepping in-process while detaching"); - old_chain_1 = make_cleanup_restore_integer (&inf->detaching); - inf->detaching = 1; + scoped_restore restore_detaching = make_scoped_restore (&inf->detaching, true); while (!ptid_equal (displaced->step_ptid, null_ptid)) { @@ -3685,12 +3683,12 @@ prepare_for_detach (void) inferior, so this must mean the process is gone. */ if (!ecs->wait_some_more) { - discard_cleanups (old_chain_1); + restore_detaching.release (); error (_("Program exited while detaching")); } } - discard_cleanups (old_chain_1); + restore_detaching.release (); } /* Wait for control to return from inferior to debugger. diff --git a/gdb/unittests/scoped_restore-selftests.c b/gdb/unittests/scoped_restore-selftests.c new file mode 100644 index 0000000..37842f9 --- /dev/null +++ b/gdb/unittests/scoped_restore-selftests.c @@ -0,0 +1,110 @@ +/* Self tests for scoped_restore for GDB, the GNU debugger. + + Copyright (C) 2017 Free Software Foundation, Inc. + + This file is part of GDB. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +#include "defs.h" +#include "selftest.h" +#include "common/scoped_restore.h" + +namespace selftests { +namespace scoped_restore_tests { + +struct Base {}; +struct Derived : Base {}; + +static int global; + +/* Check that we can return a scoped_restore from a function. Below + we'll make sure this does the right thing. */ +static scoped_restore_tmpl<int> +make_scoped_restore_global (int value) +{ + return make_scoped_restore (&global, value); +} + +static void +run_tests () +{ + /* Test that single-argument make_scoped_restore restores the + original value on scope exit. */ + { + int integer = 0; + { + scoped_restore restore = make_scoped_restore (&integer); + gdb_assert (integer == 0); + integer = 1; + } + SELF_CHECK (integer == 0); + } + + /* Same, with two-argument make_scoped_restore. */ + { + int integer = 0; + { + scoped_restore restore = make_scoped_restore (&integer, 1); + SELF_CHECK (integer == 1); + } + SELF_CHECK (integer == 0); + } + + /* Test releasing a scoped_restore. */ + { + int integer = 0; + { + scoped_restore restore = make_scoped_restore (&integer, 1); + SELF_CHECK (integer == 1); + restore.release (); + } + /* The overridden value should persist. */ + SELF_CHECK (integer == 1); + } + + /* Test creating a scoped_restore with a value of a type convertible + to T. */ + { + Base *base = nullptr; + Derived derived; + { + scoped_restore restore = make_scoped_restore (&base, &derived); + + SELF_CHECK (base == &derived); + } + SELF_CHECK (base == nullptr); + } + + /* Test calling a function that returns a scoped restore. Makes + sure that if the compiler emits a call to the copy ctor, that we + still do the right thing. */ + { + { + SELF_CHECK (global == 0); + scoped_restore restore = make_scoped_restore_global (1); + SELF_CHECK (global == 1); + } + SELF_CHECK (global == 0); + } +} + +} /* namespace scoped_restore */ +} /* namespace selftests */ + +void +_initialize_scoped_restore_selftests () +{ + register_self_test (selftests::scoped_restore_tests::run_tests); +} -- 2.5.5 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 6/8] Make inferior a class with cdtors, and use new/delete 2017-04-11 23:51 [PATCH 0/8] Fix removing inferiors from within "thread apply" commands Pedro Alves 2017-04-11 23:51 ` [PATCH 3/8] C++fy thread_apply_all_command Pedro Alves 2017-04-11 23:51 ` [PATCH 7/8] Make inferior::detaching a bool, and introduce scoped_restore::release() Pedro Alves @ 2017-04-11 23:51 ` Pedro Alves 2017-04-13 10:24 ` Yao Qi 2017-04-11 23:51 ` [PATCH 8/8] Fix removing inferiors from within "thread apply" commands Pedro Alves ` (4 subsequent siblings) 7 siblings, 1 reply; 25+ messages in thread From: Pedro Alves @ 2017-04-11 23:51 UTC (permalink / raw) To: gdb-patches A following patch in this series will add a private field to struct inferior, making it a non-POD, which requires allocating/destroying inferiors with new/delete, etc. Note: this commit makes all boolean fields of inferior be "bool", except the "detaching" field. That'll require more work, so I split it to a separate patch. gdb/ChangeLog: yyyy-mm-dd Pedro Alves <palves@redhat.com> * inferior.c (free_inferior): Convert to ... (inferior::~inferior): ... this dtor. (inferior::inferior): New ctor, factored out from ... (add_inferior_silent): ... here. Allocate the inferior with a new expression. (delete_inferior): Call delete instead of free_inferior. * inferior.h (gdb_environ, continuation): Forward declare. (inferior): Now a class. Add in-class initialization to all members. Make boolean fields bool, except 'detaching'. (inferior::inferior): New explicit ctor. (inferior::~inferior): New. --- gdb/inferior.c | 35 ++++++++++++++---------------- gdb/inferior.h | 67 +++++++++++++++++++++++++++++++--------------------------- 2 files changed, 52 insertions(+), 50 deletions(-) diff --git a/gdb/inferior.c b/gdb/inferior.c index 54e9967..327590a 100644 --- a/gdb/inferior.c +++ b/gdb/inferior.c @@ -92,9 +92,10 @@ save_current_inferior (void) return old_chain; } -static void -free_inferior (struct inferior *inf) +inferior::~inferior () { + inferior *inf = this; + discard_all_inferior_continuations (inf); inferior_free_data (inf); xfree (inf->args); @@ -102,38 +103,34 @@ free_inferior (struct inferior *inf) free_environ (inf->environment); target_desc_info_free (inf->tdesc_info); xfree (inf->priv); - xfree (inf); +} + +inferior::inferior (int pid_) + : num (++highest_inferior_num), + pid (pid_), + environment (make_environ ()), + registry_data () +{ + init_environ (this->environment); + inferior_alloc_data (this); } struct inferior * add_inferior_silent (int pid) { - struct inferior *inf; - - inf = XNEW (struct inferior); - memset (inf, 0, sizeof (*inf)); - inf->pid = pid; - - inf->control.stop_soon = NO_STOP_QUIETLY; - - inf->num = ++highest_inferior_num; + inferior *inf = new inferior (pid); if (inferior_list == NULL) inferior_list = inf; else { - struct inferior *last; + inferior *last; for (last = inferior_list; last->next != NULL; last = last->next) ; last->next = inf; } - inf->environment = make_environ (); - init_environ (inf->environment); - - inferior_alloc_data (inf); - observer_notify_inferior_added (inf); if (pid != 0) @@ -207,7 +204,7 @@ delete_inferior (struct inferior *todel) if (program_space_empty_p (inf->pspace)) delete_program_space (inf->pspace); - free_inferior (inf); + delete inf; } /* If SILENT then be quiet -- don't announce a inferior exit, or the diff --git a/gdb/inferior.h b/gdb/inferior.h index 29ec7be..2638ac7 100644 --- a/gdb/inferior.h +++ b/gdb/inferior.h @@ -30,6 +30,8 @@ struct regcache; struct ui_out; struct terminal_info; struct target_desc_info; +struct gdb_environ; +struct continuation; /* For bpstat. */ #include "breakpoint.h" @@ -305,111 +307,114 @@ struct inferior_control_state target process ids. Each inferior may in turn have multiple threads running in it. */ -struct inferior +class inferior { +public: + explicit inferior (int pid); + ~inferior (); + /* Pointer to next inferior in singly-linked list of inferiors. */ - struct inferior *next; + struct inferior *next = NULL; /* Convenient handle (GDB inferior id). Unique across all inferiors. */ - int num; + int num = 0; /* Actual target inferior id, usually, a process id. This matches the ptid_t.pid member of threads of this inferior. */ - int pid; + int pid = 0; /* True if the PID was actually faked by GDB. */ - int fake_pid_p; + bool fake_pid_p = false; /* The highest thread number this inferior ever had. */ - int highest_thread_num; + int highest_thread_num = 0; /* State of GDB control of inferior process execution. See `struct inferior_control_state'. */ - struct inferior_control_state control; + inferior_control_state control {NO_STOP_QUIETLY}; /* True if this was an auto-created inferior, e.g. created from following a fork; false, if this inferior was manually added by the user, and we should not attempt to prune it automatically. */ - int removable; + bool removable = false; /* The address space bound to this inferior. */ - struct address_space *aspace; + struct address_space *aspace = NULL; /* The program space bound to this inferior. */ - struct program_space *pspace; + struct program_space *pspace = NULL; /* The arguments string to use when running. */ - char *args; + char *args = NULL; /* The size of elements in argv. */ - int argc; + int argc = 0; /* The vector version of arguments. If ARGC is nonzero, then we must compute ARGS from this (via the target). This is always coming from main's argv and therefore should never be freed. */ - char **argv; + char **argv = NULL; /* The name of terminal device to use for I/O. */ - char *terminal; + char *terminal = NULL; /* Environment to use for running inferior, in format described in environ.h. */ - struct gdb_environ *environment; + gdb_environ *environment = NULL; - /* Nonzero if this child process was attached rather than - forked. */ - int attach_flag; + /* True if this child process was attached rather than forked. */ + bool attach_flag = false; /* If this inferior is a vfork child, then this is the pointer to its vfork parent, if GDB is still attached to it. */ - struct inferior *vfork_parent; + inferior *vfork_parent = NULL; /* If this process is a vfork parent, this is the pointer to the child. Since a vfork parent is left frozen by the kernel until the child execs or exits, a process can only have one vfork child at a given time. */ - struct inferior *vfork_child; + inferior *vfork_child = NULL; /* True if this inferior should be detached when it's vfork sibling exits or execs. */ - int pending_detach; + bool pending_detach = false; /* True if this inferior is a vfork parent waiting for a vfork child not under our control to be done with the shared memory region, either by exiting or execing. */ - int waiting_for_vfork_done; + bool waiting_for_vfork_done = false; /* True if we're in the process of detaching from this inferior. */ - int detaching; + int detaching = 0; /* What is left to do for an execution command after any thread of this inferior stops. For continuations associated with a specific thread, see `struct thread_info'. */ - struct continuation *continuations; + continuation *continuations = NULL; /* True if setup_inferior wasn't called for this inferior yet. Until that is done, we must not access inferior memory or registers, as we haven't determined the target architecture/description. */ - int needs_setup; + bool needs_setup = false; /* Private data used by the target vector implementation. */ - struct private_inferior *priv; + private_inferior *priv = NULL; /* HAS_EXIT_CODE is true if the inferior exited with an exit code. In this case, the EXIT_CODE field is also valid. */ - int has_exit_code; - LONGEST exit_code; + bool has_exit_code = false; + LONGEST exit_code = 0; /* Default flags to pass to the symbol reading functions. These are used whenever a new objfile is created. */ - symfile_add_flags symfile_flags; + symfile_add_flags symfile_flags = 0; /* Info about an inferior's target description (if it's fetched; the user supplied description's filename, if any; etc.). */ - struct target_desc_info *tdesc_info; + target_desc_info *tdesc_info = NULL; /* The architecture associated with the inferior through the connection to the target. @@ -422,7 +427,7 @@ struct inferior per-thread/per-frame/per-objfile properties, accesses to per-inferior/target properties should be made through this gdbarch. */ - struct gdbarch *gdbarch; + struct gdbarch *gdbarch = NULL; /* Per inferior data-pointers required by other GDB modules. */ REGISTRY_FIELDS; -- 2.5.5 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 6/8] Make inferior a class with cdtors, and use new/delete 2017-04-11 23:51 ` [PATCH 6/8] Make inferior a class with cdtors, and use new/delete Pedro Alves @ 2017-04-13 10:24 ` Yao Qi 0 siblings, 0 replies; 25+ messages in thread From: Yao Qi @ 2017-04-13 10:24 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches Pedro Alves <palves@redhat.com> writes: > gdb/ChangeLog: > yyyy-mm-dd Pedro Alves <palves@redhat.com> > > * inferior.c (free_inferior): Convert to ... > (inferior::~inferior): ... this dtor. > (inferior::inferior): New ctor, factored out from ... > (add_inferior_silent): ... here. Allocate the inferior with a new > expression. > (delete_inferior): Call delete instead of free_inferior. > * inferior.h (gdb_environ, continuation): Forward declare. > (inferior): Now a class. Add in-class initialization to all > members. Make boolean fields bool, except 'detaching'. > (inferior::inferior): New explicit ctor. > (inferior::~inferior): New. LGTM. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 8/8] Fix removing inferiors from within "thread apply" commands 2017-04-11 23:51 [PATCH 0/8] Fix removing inferiors from within "thread apply" commands Pedro Alves ` (2 preceding siblings ...) 2017-04-11 23:51 ` [PATCH 6/8] Make inferior a class with cdtors, and use new/delete Pedro Alves @ 2017-04-11 23:51 ` Pedro Alves 2017-04-13 11:33 ` Yao Qi 2017-04-11 23:51 ` [PATCH 2/8] Fix follow-fork latent bug Pedro Alves ` (3 subsequent siblings) 7 siblings, 1 reply; 25+ messages in thread From: Pedro Alves @ 2017-04-11 23:51 UTC (permalink / raw) To: gdb-patches This patch fixes an internal error exposed by a test that does something like: define kill-and-remove kill inferiors 2 remove-inferiors 2 end # Start one inferior. start # Start another inferior. add-inferior 2 inferior 2 start # Kill and remove inferior 1 while inferior 2 is selected. thread apply 1.1 kill-and-remove The internal error looks like this: Thread 1.1 (Thread 0x7ffff7fc2700 (LWP 20677)): [Switching to inferior 1 [process 20677] (gdb/testsuite/outputs/gdb.threads/threadapply/threadapply)] [Switching to thread 1.1 (Thread 0x7ffff7fc2700 (LWP 20677))] #0 main () at src/gdb/testsuite/gdb.threads/threadapply.c:38 38 for (i = 0; i < NUM; i++) src/gdb/inferior.c:66: internal-error: void set_current_inferior(inferior*): Assertion `inf != NULL' failed. A problem internal to GDB has been detected, further debugging may prove unreliable. Quit this debugging session? (y or n) FAIL: gdb.threads/threadapply.exp: kill_and_remove_inferior: try kill-and-remove: thread apply 1.1 kill-and-remove (GDB internal error) There are several problems around this area of the code. One is that in do_restore_current_thread_cleanup, we do a look up of inferior by ptid, which can find the wrong inferior if the previously selected inferior exited and some other inferior was started with a reused pid (rare, but still...). The other problem is that the "remove-inferiors" command rejects attempts to remove the current inferior, but when we get to "remove-inferiors" in a "thread apply THR remove-inferiors 2" command, the current inferior is the inferior of thread THR, not the previously selected inferior, so if the previously selected inferior was inferior 2, that command still manages to wipe it, and then gdb restores the old selected inferior, which is now a dangling pointer... So the fix here is: - Make make_cleanup_restore_current_thread store a pointer to the previously selected inferior directly, and use it directly instead of doing ptid look ups. - Add a refcount to inferiors, very similar to thread_info's refcount, that is incremented/decremented by make_cleanup_restore_current_thread, and checked before deleting an inferior. gdb/ChangeLog: 2017-04-11 Pedro Alves <palves@redhat.com> * inferior.c (prune_inferiors, remove_inferior_command): Skip inferiors marked not-deletable instead of comparing to the currente inferior. * inferior.h (struct inferior): Forward declare. (current_inferior, set_current_inferior): Move declaration to before struct inferior's definition, and fix comment. (inferior::deletable, inferior::incref, inferior::decref): New methods. (m_refcount): New private field. * thread.c (switch_to_to_thread): New function. (switch_to_thread (thread_info *)): New function, factored out from ... (switch_to_thread (ptid_t)): ... this. (restore_current_thread): Delete. (current_thread_cleanup): Remove 'inf_id' and 'was_removable' fields, and add 'inf' field. (do_restore_current_thread_cleanup): Check whether old->inf is alive instead of looking up an inferior by ptid. Use switch_to_thread and switch_to_no_thread. Assert that the current inferior matches inferior_ptid. (restore_current_thread_cleanup_dtor): Use old->inf directly instead of lookup up an inferior by id. Decref the inferior. Don't restore 'removable'. (make_cleanup_restore_current_thread): Same the inferior pointer in old, instead of the inferior number. Incref the inferior. Don't save/clear 'removable'. gdb/testsuite/ChangeLog: 2017-04-11 Pedro Alves <palves@redhat.com> * gdb.threads/threadapply.exp (kill_and_remove_inferior): New procedure. (top level): Call it. * lib/gdb.exp (gdb_define_cmd): New procedure. --- gdb/inferior.c | 5 +- gdb/inferior.h | 40 +++++++-- gdb/testsuite/gdb.threads/threadapply.exp | 135 ++++++++++++++++++++++++++++++ gdb/testsuite/lib/gdb.exp | 19 +++++ gdb/thread.c | 85 ++++++++++--------- 5 files changed, 237 insertions(+), 47 deletions(-) diff --git a/gdb/inferior.c b/gdb/inferior.c index 327590a..c918a5b 100644 --- a/gdb/inferior.c +++ b/gdb/inferior.c @@ -483,13 +483,12 @@ void prune_inferiors (void) { struct inferior *ss, **ss_link; - struct inferior *current = current_inferior (); ss = inferior_list; ss_link = &inferior_list; while (ss) { - if (ss == current + if (!ss->deletable () || !ss->removable || ss->pid != 0) { @@ -774,7 +773,7 @@ remove_inferior_command (char *args, int from_tty) continue; } - if (inf == current_inferior ()) + if (!inf->deletable ()) { warning (_("Can not remove current inferior %d."), num); continue; diff --git a/gdb/inferior.h b/gdb/inferior.h index 44f8157..89acb83 100644 --- a/gdb/inferior.h +++ b/gdb/inferior.h @@ -32,6 +32,7 @@ struct terminal_info; struct target_desc_info; struct gdb_environ; struct continuation; +struct inferior; /* For bpstat. */ #include "breakpoint.h" @@ -298,6 +299,11 @@ struct inferior_control_state enum stop_kind stop_soon; }; +/* Return a pointer to the current inferior. */ +extern inferior *current_inferior (); + +extern void set_current_inferior (inferior *); + /* GDB represents the state of each program execution with an object called an inferior. An inferior typically corresponds to a process but is more general and applies also to targets that do not have a @@ -313,9 +319,31 @@ public: explicit inferior (int pid); ~inferior (); + /* Returns true if we can delete this inferior. We can't delete it + if it is the current inferior, or if there's code out there that + relies on it existing (m_refcount > 0). */ + bool deletable () const + { + return m_refcount == 0 && this != current_inferior (); + } + /* Pointer to next inferior in singly-linked list of inferiors. */ struct inferior *next = NULL; + /* Increase the refcount. */ + void incref () + { + gdb_assert (m_refcount >= 0); + m_refcount++; + } + + /* Decrease the refcount. */ + void decref () + { + m_refcount--; + gdb_assert (m_refcount >= 0); + } + /* Convenient handle (GDB inferior id). Unique across all inferiors. */ int num = 0; @@ -431,6 +459,12 @@ public: /* Per inferior data-pointers required by other GDB modules. */ REGISTRY_FIELDS; + +private: + /* If this is > 0, then it means there's code out there that relies + on this inferior existing. Don't allow deleting it in that + case. */ + int m_refcount = 0; }; /* Keep a registry of per-inferior data-pointers required by other GDB @@ -517,12 +551,6 @@ extern int number_of_live_inferiors (void); (not cores, not executables, real live processes). */ extern int have_live_inferiors (void); -/* Return a pointer to the current inferior. It is an error to call - this if there is no current inferior. */ -extern struct inferior *current_inferior (void); - -extern void set_current_inferior (struct inferior *); - extern struct cleanup *save_current_inferior (void); /* Traverse all inferiors. */ diff --git a/gdb/testsuite/gdb.threads/threadapply.exp b/gdb/testsuite/gdb.threads/threadapply.exp index 959e8b9..39687da 100644 --- a/gdb/testsuite/gdb.threads/threadapply.exp +++ b/gdb/testsuite/gdb.threads/threadapply.exp @@ -93,3 +93,138 @@ proc thr_apply_detach {thread_set} { foreach thread_set {"all" "1.1 1.2 1.3"} { thr_apply_detach $thread_set } + +# Test killing and removing inferiors from a command run via "thread +# apply THREAD_SET". THREAD_SET can be either "1.1", or "all". GDB +# used to mistakenly allow deleting the previously-selected inferior, +# in some cases, leading to crashes. + +proc kill_and_remove_inferior {thread_set} { + global binfile + global gdb_prompt + + # The test starts multiple inferiors, therefore non-extended + # remote is not supported. + if [target_info exists use_gdb_stub] { + unsupported "using gdb stub" + return + } + + set any "\[^\r\n\]*" + set ws "\[ \t\]\+" + + clean_restart ${binfile} + + with_test_prefix "start inferior 1" { + runto_main + } + + # Add and start inferior number NUM. + proc add_and_start_inferior {num} { + global binfile + + # Start another inferior. + gdb_test "add-inferior" "Added inferior $num.*" \ + "add empty inferior $num" + gdb_test "inferior $num" "Switching to inferior $num.*" \ + "switch to inferior $num" + gdb_test "file ${binfile}" ".*" "load file in inferior $num" + + with_test_prefix "start inferior $num" { + runto_main + } + } + + # Start another inferior. + add_and_start_inferior 2 + + # And yet another. + add_and_start_inferior 3 + + gdb_test "thread 2.1" "Switching to thread 2.1 .*" + + # Try removing an active inferior via a "thread apply" command. + # Should fail/warn. + with_test_prefix "try remove" { + + gdb_define_cmd "remove" { + "remove-inferiors 3" + } + + # Inferior 3 is still alive, so can't remove it. + gdb_test "thread apply $thread_set remove" \ + "warning: Can not remove active inferior 3.*" + # Check that GDB restored the selected thread. + gdb_test "thread" "Current thread is 2.1 .*" + + gdb_test "info inferiors" \ + [multi_line \ + "${ws}1${ws}process ${any}" \ + "\\\* 2${ws}process ${any}" \ + "${ws}3${ws}process ${any}" \ + ] + } + + # Kill and try to remove inferior 2 while inferior 2 is selected. + # Removing the inferior should fail/warn. + with_test_prefix "try kill-and-remove" { + + # The "inferior 1" command works around PR gdb/19318 ("kill + # inferior N" shouldn't switch to inferior N). + gdb_define_cmd "kill-and-remove" { + "kill inferiors 2" + "inferior 1" + "remove-inferiors 2" + } + + # Note that when threads=1.1, this makes sure we're really + # testing failure to remove the inferior the user had selected + # before the "thread apply" command, instead of testing + # refusal to remove the currently-iterated inferior. + gdb_test "thread apply $thread_set kill-and-remove" \ + "warning: Can not remove current inferior 2.*" + gdb_test "thread" "No thread selected" \ + "switched to no thread selected" + + gdb_test "info inferiors" \ + [multi_line \ + "${ws}1${ws}process ${any}" \ + "\\\* 2${ws}<null>${any}" \ + "${ws}3${ws}process ${any}" \ + ] + } + + # Try removing (the now dead) inferior 2 while inferior 1 is + # selected. This should succeed. + with_test_prefix "try remove 2" { + + gdb_test "thread 1.1" "Switching to thread 1.1 .*" + + gdb_define_cmd "remove-again" { + "remove-inferiors 2" + } + + set test "thread apply $thread_set remove-again" + gdb_test_multiple $test $test { + -re "warning: Can not remove.*$gdb_prompt $" { + fail $test + } + -re "$gdb_prompt $" { + pass $test + } + } + gdb_test "thread" "Current thread is 1.1 .*" + # Check that only inferiors 1 and 3 are around. + gdb_test "info inferiors" \ + [multi_line \ + "\\\* 1${ws}process ${any}" \ + "${ws}3${ws}process ${any}" \ + ] + } +} + +# Test both "all" and a thread list, because those are implemented as +# different commands in GDB. +foreach_with_prefix thread_set {"all" "1.1"} { + kill_and_remove_inferior $thread_set +} diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp index ea77361..6633d24 100644 --- a/gdb/testsuite/lib/gdb.exp +++ b/gdb/testsuite/lib/gdb.exp @@ -6070,5 +6070,24 @@ proc dejagnu_version { } { return $dg_ver } +# Define user-defined command COMMAND using the COMMAND_LIST as the +# command's definition. The terminating "end" is added automatically. + +proc gdb_define_cmd {command command_list} { + global gdb_prompt + + set input [multi_line_input {*}$command_list "end"] + set test "define $command" + + gdb_test_multiple "define $command" $test { + -re "End with" { + gdb_test_multiple $input $test { + -re "\r\n$gdb_prompt " { + } + } + } + } +} + # Always load compatibility stuff. load_lib future.exp diff --git a/gdb/thread.c b/gdb/thread.c index 88fd521..f48d871 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -68,7 +68,6 @@ static void thread_apply_all_command (char *, int); static int thread_alive (struct thread_info *); static void info_threads_command (char *, int); static void thread_apply_command (char *, int); -static void restore_current_thread (ptid_t); /* RAII type used to increase / decrease the refcount of each thread in a given list of threads. */ @@ -1469,45 +1468,56 @@ switch_to_thread_no_regs (struct thread_info *thread) stop_pc = ~(CORE_ADDR) 0; } -/* Switch from one thread to another. */ +/* Switch to no thread selected. */ -void -switch_to_thread (ptid_t ptid) +static void +switch_to_no_thread () { - /* Switch the program space as well, if we can infer it from the now - current thread. Otherwise, it's up to the caller to select the - space it wants. */ - if (ptid != null_ptid) - { - struct inferior *inf; + if (inferior_ptid == null_ptid) + return; - inf = find_inferior_ptid (ptid); - gdb_assert (inf != NULL); - set_current_program_space (inf->pspace); - set_current_inferior (inf); - } + inferior_ptid = null_ptid; + reinit_frame_cache (); + stop_pc = ~(CORE_ADDR) 0; +} - if (ptid == inferior_ptid) +/* Switch from one thread to another. */ + +static void +switch_to_thread (thread_info *thr) +{ + gdb_assert (thr != NULL); + + if (inferior_ptid == thr->ptid) return; - inferior_ptid = ptid; + /* Switch the current program space and current inferior as + well. */ + set_current_program_space (thr->inf->pspace); + set_current_inferior (thr->inf); + + inferior_ptid = thr->ptid; reinit_frame_cache (); /* We don't check for is_stopped, because we're called at times while in the TARGET_RUNNING state, e.g., while handling an internal event. */ - if (inferior_ptid != null_ptid - && !is_exited (ptid) - && !is_executing (ptid)) - stop_pc = regcache_read_pc (get_thread_regcache (ptid)); + if (thr->state != THREAD_EXITED + && !thr->executing) + stop_pc = regcache_read_pc (get_thread_regcache (thr->ptid)); else stop_pc = ~(CORE_ADDR) 0; } -static void -restore_current_thread (ptid_t ptid) +/* See gdbthread.h. */ + +void +switch_to_thread (ptid_t ptid) { - switch_to_thread (ptid); + if (ptid == null_ptid) + switch_to_no_thread (); + else + switch_to_thread (find_thread_ptid (ptid)); } static void @@ -1578,8 +1588,7 @@ struct current_thread_cleanup struct frame_id selected_frame_id; int selected_frame_level; int was_stopped; - int inf_id; - int was_removable; + inferior *inf; }; static void @@ -1595,12 +1604,12 @@ do_restore_current_thread_cleanup (void *arg) /* If the previously selected thread belonged to a process that has in the mean time exited (or killed, detached, etc.), then don't revert back to it, but instead simply drop back to no thread selected. */ - && find_inferior_ptid (old->thread->ptid) != NULL) - restore_current_thread (old->thread->ptid); + && old->inf->pid != 0) + switch_to_thread (old->thread); else { - restore_current_thread (null_ptid); - set_current_inferior (find_inferior_id (old->inf_id)); + switch_to_no_thread (); + set_current_inferior (old->inf); } /* The running state of the originally selected thread may have @@ -1613,21 +1622,22 @@ do_restore_current_thread_cleanup (void *arg) && target_has_memory) restore_selected_frame (old->selected_frame_id, old->selected_frame_level); + + if (inferior_ptid == null_ptid) + gdb_assert (current_inferior ()->pid == 0); + else + gdb_assert (current_inferior ()->pid == inferior_ptid.pid ()); } static void restore_current_thread_cleanup_dtor (void *arg) { struct current_thread_cleanup *old = (struct current_thread_cleanup *) arg; - struct thread_info *tp; - struct inferior *inf; if (old->thread != NULL) old->thread->decref (); - inf = find_inferior_id (old->inf_id); - if (inf != NULL) - inf->removable = old->was_removable; + old->inf->decref (); xfree (old); } @@ -1637,8 +1647,7 @@ make_cleanup_restore_current_thread (void) struct current_thread_cleanup *old = XNEW (struct current_thread_cleanup); old->thread = NULL; - old->inf_id = current_inferior ()->num; - old->was_removable = current_inferior ()->removable; + old->inf = current_inferior (); if (inferior_ptid != null_ptid) { @@ -1670,7 +1679,7 @@ make_cleanup_restore_current_thread (void) old->thread = tp; } - current_inferior ()->removable = 0; + old->inf->incref (); return make_cleanup_dtor (do_restore_current_thread_cleanup, old, restore_current_thread_cleanup_dtor); -- 2.5.5 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 8/8] Fix removing inferiors from within "thread apply" commands 2017-04-11 23:51 ` [PATCH 8/8] Fix removing inferiors from within "thread apply" commands Pedro Alves @ 2017-04-13 11:33 ` Yao Qi 2017-04-13 12:22 ` Pedro Alves 0 siblings, 1 reply; 25+ messages in thread From: Yao Qi @ 2017-04-13 11:33 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches Pedro Alves <palves@redhat.com> writes: > @@ -313,9 +319,31 @@ public: > explicit inferior (int pid); > ~inferior (); > > + /* Returns true if we can delete this inferior. We can't delete it > + if it is the current inferior, or if there's code out there that > + relies on it existing (m_refcount > 0). */ > + bool deletable () const > + { > + return m_refcount == 0 && this != current_inferior (); > + } > + > /* Pointer to next inferior in singly-linked list of inferiors. */ > struct inferior *next = NULL; > > + /* Increase the refcount. */ > + void incref () > + { > + gdb_assert (m_refcount >= 0); > + m_refcount++; > + } > + > + /* Decrease the refcount. */ > + void decref () > + { > + m_refcount--; > + gdb_assert (m_refcount >= 0); > + } > + > /* Convenient handle (GDB inferior id). Unique across all > inferiors. */ > int num = 0; > @@ -431,6 +459,12 @@ public: > > /* Per inferior data-pointers required by other GDB modules. */ > REGISTRY_FIELDS; > + > +private: > + /* If this is > 0, then it means there's code out there that relies > + on this inferior existing. Don't allow deleting it in that > + case. */ > + int m_refcount = 0; > }; Can we move them to a super class, so that both thread and inferior can extend it? > > -/* Switch from one thread to another. */ > +/* Switch to no thread selected. */ > > -void > -switch_to_thread (ptid_t ptid) > +static void > +switch_to_no_thread () > { > - /* Switch the program space as well, if we can infer it from the now > - current thread. Otherwise, it's up to the caller to select the > - space it wants. */ > - if (ptid != null_ptid) > - { > - struct inferior *inf; > + if (inferior_ptid == null_ptid) > + return; > > - inf = find_inferior_ptid (ptid); > - gdb_assert (inf != NULL); > - set_current_program_space (inf->pspace); > - set_current_inferior (inf); > - } > + inferior_ptid = null_ptid; > + reinit_frame_cache (); > + stop_pc = ~(CORE_ADDR) 0; > +} > > - if (ptid == inferior_ptid) > +/* Switch from one thread to another. */ > + > +static void > +switch_to_thread (thread_info *thr) > +{ > + gdb_assert (thr != NULL); > + > + if (inferior_ptid == thr->ptid) > return; > > - inferior_ptid = ptid; > + /* Switch the current program space and current inferior as > + well. */ > + set_current_program_space (thr->inf->pspace); > + set_current_inferior (thr->inf); > + > + inferior_ptid = thr->ptid; Can these code be replaced by switch_to_thread_no_regs? -- Yao (齐尧) ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 8/8] Fix removing inferiors from within "thread apply" commands 2017-04-13 11:33 ` Yao Qi @ 2017-04-13 12:22 ` Pedro Alves 2017-04-13 14:49 ` Pedro Alves 2017-04-13 15:31 ` Yao Qi 0 siblings, 2 replies; 25+ messages in thread From: Pedro Alves @ 2017-04-13 12:22 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches On 04/13/2017 12:33 PM, Yao Qi wrote: > Pedro Alves <palves@redhat.com> writes: > >> @@ -313,9 +319,31 @@ public: >> explicit inferior (int pid); >> ~inferior (); >> >> + /* Returns true if we can delete this inferior. We can't delete it >> + if it is the current inferior, or if there's code out there that >> + relies on it existing (m_refcount > 0). */ >> + bool deletable () const >> + { >> + return m_refcount == 0 && this != current_inferior (); >> + } >> + >> /* Pointer to next inferior in singly-linked list of inferiors. */ >> struct inferior *next = NULL; >> >> + /* Increase the refcount. */ >> + void incref () >> + { >> + gdb_assert (m_refcount >= 0); >> + m_refcount++; >> + } >> + >> + /* Decrease the refcount. */ >> + void decref () >> + { >> + m_refcount--; >> + gdb_assert (m_refcount >= 0); >> + } >> + >> /* Convenient handle (GDB inferior id). Unique across all >> inferiors. */ >> int num = 0; >> @@ -431,6 +459,12 @@ public: >> >> /* Per inferior data-pointers required by other GDB modules. */ >> REGISTRY_FIELDS; >> + >> +private: >> + /* If this is > 0, then it means there's code out there that relies >> + on this inferior existing. Don't allow deleting it in that >> + case. */ >> + int m_refcount = 0; >> }; > > Can we move them to a super class, so that both thread and inferior can > extend it? Just to be clear, would the class include the incref()/decref() methods + m_refcount, plus some refcount() accessor method, or more than that? Let me give it a try. >> +static void >> +switch_to_thread (thread_info *thr) >> +{ >> + gdb_assert (thr != NULL); >> + >> + if (inferior_ptid == thr->ptid) >> return; >> >> - inferior_ptid = ptid; >> + /* Switch the current program space and current inferior as >> + well. */ >> + set_current_program_space (thr->inf->pspace); >> + set_current_inferior (thr->inf); >> + >> + inferior_ptid = thr->ptid; > > Can these code be replaced by switch_to_thread_no_regs? Looks like it. I'm testing the patch below on top. From c74e7b7b6eb29664e1afded656194266e95ee869 Mon Sep 17 00:00:00 2001 From: Pedro Alves <palves@redhat.com> Date: Thu, 13 Apr 2017 13:11:39 +0100 Subject: [PATCH] switch_to_thread_no_regs --- gdb/thread.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/gdb/thread.c b/gdb/thread.c index f48d871..2c38a9a 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -1457,10 +1457,8 @@ info_threads_command (char *arg, int from_tty) void switch_to_thread_no_regs (struct thread_info *thread) { - struct inferior *inf; + struct inferior *inf = thread->inf; - inf = find_inferior_ptid (thread->ptid); - gdb_assert (inf != NULL); set_current_program_space (inf->pspace); set_current_inferior (inf); @@ -1491,12 +1489,8 @@ switch_to_thread (thread_info *thr) if (inferior_ptid == thr->ptid) return; - /* Switch the current program space and current inferior as - well. */ - set_current_program_space (thr->inf->pspace); - set_current_inferior (thr->inf); + switch_to_thread_no_regs (thr); - inferior_ptid = thr->ptid; reinit_frame_cache (); /* We don't check for is_stopped, because we're called at times @@ -1505,8 +1499,6 @@ switch_to_thread (thread_info *thr) if (thr->state != THREAD_EXITED && !thr->executing) stop_pc = regcache_read_pc (get_thread_regcache (thr->ptid)); - else - stop_pc = ~(CORE_ADDR) 0; } /* See gdbthread.h. */ -- 2.5.5 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 8/8] Fix removing inferiors from within "thread apply" commands 2017-04-13 12:22 ` Pedro Alves @ 2017-04-13 14:49 ` Pedro Alves 2017-04-19 8:23 ` Yao Qi 2017-04-13 15:31 ` Yao Qi 1 sibling, 1 reply; 25+ messages in thread From: Pedro Alves @ 2017-04-13 14:49 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches On 04/13/2017 01:22 PM, Pedro Alves wrote: > On 04/13/2017 12:33 PM, Yao Qi wrote: >> Pedro Alves <palves@redhat.com> writes: >> >>> +private: >>> + /* If this is > 0, then it means there's code out there that relies >>> + on this inferior existing. Don't allow deleting it in that >>> + case. */ >>> + int m_refcount = 0; >>> }; >> >> Can we move them to a super class, so that both thread and inferior can >> extend it? > > Just to be clear, would the class include the incref()/decref() > methods + m_refcount, plus some refcount() accessor method, > or more than that? Let me give it a try. See patch below that implements this. I also added comments explaining the thread_info and inferior objects' refcount handling to hopefully make things clearer. I then realized that unlike thread_info objects, there's a "current inferior" pointer, and switching the current inferior always goes through a single function (set_current_inferior), so we can make being the current inferior be a strong reference accounted for in the inferior's refcount. This simplifies the inferior::deletable method's implementation which no longer compares this with current_inferior(). It's just a trade off, because now set_current_inferior and initialize_inferiors must manage the refcounts. The end result is just the same. Not sure which version is clearer -- this, or keeping inferior::deletable as it was (and reverting the set_current_inferior change). Maybe the other way is a little bit more efficient given deleting inferiors is way less frequently done than switching around the current inferior. OTOH maybe this way is clearer. If you have a preference, let me know. > Looks like it. I'm testing the patch below on top. It worked, so I squashed it in. Here's the new version. WDYT? I've pushed this to the users/palves/thread_apply-v2 branch. From 09f9f6b2d10719a213dbcae6e6ad73f5882a99b5 Mon Sep 17 00:00:00 2001 From: Pedro Alves <palves@redhat.com> Date: Thu, 13 Apr 2017 15:28:57 +0100 Subject: [PATCH] Fix removing inferiors from within "thread apply" commands This patch fixes an internal error exposed by a test that does something like: define kill-and-remove kill inferiors 2 remove-inferiors 2 end # Start one inferior. start # Start another inferior. add-inferior 2 inferior 2 start # Kill and remove inferior 1 while inferior 2 is selected. thread apply 1.1 kill-and-remove The internal error looks like this: Thread 1.1 (Thread 0x7ffff7fc2700 (LWP 20677)): [Switching to inferior 1 [process 20677] (gdb/testsuite/outputs/gdb.threads/threadapply/threadapply)] [Switching to thread 1.1 (Thread 0x7ffff7fc2700 (LWP 20677))] #0 main () at src/gdb/testsuite/gdb.threads/threadapply.c:38 38 for (i = 0; i < NUM; i++) src/gdb/inferior.c:66: internal-error: void set_current_inferior(inferior*): Assertion `inf != NULL' failed. A problem internal to GDB has been detected, further debugging may prove unreliable. Quit this debugging session? (y or n) FAIL: gdb.threads/threadapply.exp: kill_and_remove_inferior: try kill-and-remove: thread apply 1.1 kill-and-remove (GDB internal error) There are several problems around this area of the code. One is that in do_restore_current_thread_cleanup, we do a look up of inferior by ptid, which can find the wrong inferior if the previously selected inferior exited and some other inferior was started with a reused pid (rare, but still...). The other problem is that the "remove-inferiors" command rejects attempts to remove the current inferior, but when we get to "remove-inferiors" in a "thread apply THR remove-inferiors 2" command, the current inferior is the inferior of thread THR, not the previously selected inferior, so if the previously selected inferior was inferior 2, that command still manages to wipe it, and then gdb restores the old selected inferior, which is now a dangling pointer... So the fix here is: - Make make_cleanup_restore_current_thread store a pointer to the previously selected inferior directly, and use it directly instead of doing ptid look ups. - Add a refcount to inferiors, very similar to thread_info's refcount, that is incremented/decremented by make_cleanup_restore_current_thread, and checked before deleting an inferior. To avoid duplication, a new refcounted_object type is added, that both thread_info and inferior inherit from. gdb/ChangeLog: yyyy-mm-dd Pedro Alves <palves@redhat.com> * common/refcounted-object.h: New file. * gdbthread.h: Include "common/refcounted-object.h". (thread_info): Inherit from refcounted_object and add comments. (thread_info::incref, thread_info::decref) (thread_info::m_refcount): Delete. (thread_info::deletable): Use the refcounted_object::refcount() method. * inferior.c (current_inferior_): Add comment. (set_current_inferior): Increment/decrement refcounts. (prune_inferiors, remove_inferior_command): Skip inferiors marked not-deletable instead of comparing with the current inferior. (initialize_inferiors): Increment the initial inferior's refcount. * inferior.h (struct inferior): Forward declare. Include "common/refcounted-object.h". (current_inferior, set_current_inferior): Move declaration to before struct inferior's definition, and fix comment. (inferior): Inherit from refcounted_object. Add comments. * thread.c (switch_to_thread_no_regs): Reference the thread's inferior pointer directly instead of doing a ptid lookup. (switch_to_no_thread): New function. (switch_to_thread(thread_info *)): New function, factored out from ... (switch_to_thread(ptid_t)): ... this. (restore_current_thread): Delete. (current_thread_cleanup): Remove 'inf_id' and 'was_removable' fields, and add 'inf' field. (do_restore_current_thread_cleanup): Check whether old->inf is alive instead of looking up an inferior by ptid. Use switch_to_thread and switch_to_no_thread. Assert that the current inferior matches inferior_ptid. (restore_current_thread_cleanup_dtor): Use old->inf directly instead of lookup up an inferior by id. Decref the inferior. Don't restore 'removable'. (make_cleanup_restore_current_thread): Same the inferior pointer in old, instead of the inferior number. Incref the inferior. Don't save/clear 'removable'. gdb/testsuite/ChangeLog: yyyy-mm-dd Pedro Alves <palves@redhat.com> * gdb.threads/threadapply.exp (kill_and_remove_inferior): New procedure. (top level): Call it. * lib/gdb.exp (gdb_define_cmd): New procedure. --- gdb/common/refcounted-object.h | 55 ++++++++++++ gdb/gdbthread.h | 45 +++++----- gdb/inferior.c | 12 ++- gdb/inferior.h | 35 ++++++-- gdb/testsuite/gdb.threads/threadapply.exp | 135 ++++++++++++++++++++++++++++++ gdb/testsuite/lib/gdb.exp | 19 +++++ gdb/thread.c | 87 +++++++++---------- 7 files changed, 308 insertions(+), 80 deletions(-) create mode 100644 gdb/common/refcounted-object.h diff --git a/gdb/common/refcounted-object.h b/gdb/common/refcounted-object.h new file mode 100644 index 0000000..bca0327 --- /dev/null +++ b/gdb/common/refcounted-object.h @@ -0,0 +1,55 @@ +/* Base class of intrusively reference-counted objects. + Copyright (C) 2017 Free Software Foundation, Inc. + + This file is part of GDB. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +#ifndef REFCOUNTED_OBJECT_H +#define REFCOUNTED_OBJECT_H + +/* Base class of reference-countable objects. Incrementing and + decrementing the reference count is an external responsibility. */ + +class refcounted_object +{ +public: + refcounted_object () = default; + + /* Increase the refcount. */ + void incref () + { + gdb_assert (m_refcount >= 0); + m_refcount++; + } + + /* Decrease the refcount. */ + void decref () + { + m_refcount--; + gdb_assert (m_refcount >= 0); + } + + int refcount () const { return m_refcount; } + +private: + /* Disable copy. */ + refcounted_object (const refcounted_object &) = delete; + refcounted_object &operator=(const refcounted_object &) = delete; + + /* The reference count. */ + int m_refcount = 0; +}; + +#endif /* REFCOUNTED_OBJECT_H */ diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h index 4cd7390..33977ee 100644 --- a/gdb/gdbthread.h +++ b/gdb/gdbthread.h @@ -31,6 +31,7 @@ struct symtab; #include "common/vec.h" #include "target/waitstatus.h" #include "cli/cli-utils.h" +#include "common/refcounted-object.h" /* Frontend view of the thread state. Possible extensions: stepping, finishing, until(ling),... */ @@ -177,7 +178,24 @@ typedef struct value *value_ptr; DEF_VEC_P (value_ptr); typedef VEC (value_ptr) value_vec; -struct thread_info +/* Threads are intrusively refcounted objects. Being the + user-selected thread is normally considered an implicit strong + reference and is thus not accounted in the refcount, unlike + inferior objects. This is necessary, because there's no "current + thread" pointer. Instead the current thread is inferred from the + inferior_ptid global. However, when GDB needs to remember the + selected thread to later restore it, GDB bumps the thread object's + refcount, to prevent something deleting the thread object before + reverting back (e.g., due to a "kill" command. If the thread + meanwhile exits before being re-selected, then the thread object is + left listed in the thread list, but marked with state + THREAD_EXITED. (See make_cleanup_restore_current_thread and + delete_thread). All other thread references are considered weak + references. Placing a thread in the thread list is an implicit + strong reference, and is thus not accounted for in the thread's + refcount. */ + +class thread_info : public refcounted_object { public: explicit thread_info (inferior *inf, ptid_t ptid); @@ -186,22 +204,8 @@ public: bool deletable () const { /* If this is the current thread, or there's code out there that - relies on it existing (m_refcount > 0) we can't delete yet. */ - return (m_refcount == 0 && !ptid_equal (ptid, inferior_ptid)); - } - - /* Increase the refcount. */ - void incref () - { - gdb_assert (m_refcount >= 0); - m_refcount++; - } - - /* Decrease the refcount. */ - void decref () - { - m_refcount--; - gdb_assert (m_refcount >= 0); + relies on it existing (refcount > 0) we can't delete yet. */ + return (refcount () == 0 && !ptid_equal (ptid, inferior_ptid)); } struct thread_info *next = NULL; @@ -362,13 +366,6 @@ public: fields point to self. */ struct thread_info *step_over_prev = NULL; struct thread_info *step_over_next = NULL; - -private: - - /* If this is > 0, then it means there's code out there that relies - on this thread being listed. Don't delete it from the lists even - if we detect it exiting. */ - int m_refcount = 0; }; /* Create an empty thread list, or empty the existing one. */ diff --git a/gdb/inferior.c b/gdb/inferior.c index 327590a..e85859f 100644 --- a/gdb/inferior.c +++ b/gdb/inferior.c @@ -50,7 +50,9 @@ static int highest_inferior_num; `set print inferior-events'. */ static int print_inferior_events = 0; -/* The Current Inferior. */ +/* The Current Inferior. This is a strong reference. I.e., whenever + an inferior is the current inferior, its refcount is + incremented. */ static struct inferior *current_inferior_ = NULL; struct inferior* @@ -65,6 +67,8 @@ set_current_inferior (struct inferior *inf) /* There's always an inferior. */ gdb_assert (inf != NULL); + inf->incref (); + current_inferior_->decref (); current_inferior_ = inf; } @@ -483,13 +487,12 @@ void prune_inferiors (void) { struct inferior *ss, **ss_link; - struct inferior *current = current_inferior (); ss = inferior_list; ss_link = &inferior_list; while (ss) { - if (ss == current + if (!ss->deletable () || !ss->removable || ss->pid != 0) { @@ -774,7 +777,7 @@ remove_inferior_command (char *args, int from_tty) continue; } - if (inf == current_inferior ()) + if (!inf->deletable ()) { warning (_("Can not remove current inferior %d."), num); continue; @@ -1017,6 +1020,7 @@ initialize_inferiors (void) that. Do this after initialize_progspace, due to the current_program_space reference. */ current_inferior_ = add_inferior (0); + current_inferior_->incref (); current_inferior_->pspace = current_program_space; current_inferior_->aspace = current_program_space->aspace; /* The architecture will be initialized shortly, by diff --git a/gdb/inferior.h b/gdb/inferior.h index e39a80c..c6fb9d3 100644 --- a/gdb/inferior.h +++ b/gdb/inferior.h @@ -32,6 +32,7 @@ struct terminal_info; struct target_desc_info; struct gdb_environ; struct continuation; +struct inferior; /* For bpstat. */ #include "breakpoint.h" @@ -46,6 +47,7 @@ struct continuation; #include "registry.h" #include "symfile-add-flags.h" +#include "common/refcounted-object.h" struct infcall_suspend_state; struct infcall_control_state; @@ -298,6 +300,11 @@ struct inferior_control_state enum stop_kind stop_soon; }; +/* Return a pointer to the current inferior. */ +extern inferior *current_inferior (); + +extern void set_current_inferior (inferior *); + /* GDB represents the state of each program execution with an object called an inferior. An inferior typically corresponds to a process but is more general and applies also to targets that do not have a @@ -305,14 +312,30 @@ struct inferior_control_state inferior, as does each attachment to an existing process. Inferiors have unique internal identifiers that are different from target process ids. Each inferior may in turn have multiple - threads running in it. */ - -class inferior + threads running in it. + + Inferiors are intrusively refcounted objects. Unlike thread + objects, being the user-selected inferior is considered a strong + reference and is thus accounted for in the inferior object's + refcount (see set_current_inferior). When GDB needs to remember + the selected inferior to later restore it, GDB temporarily bumps + the inferior object's refcount, to prevent something deleting the + inferior object before reverting back (e.g., due to a + "remove-inferiors" command (see + make_cleanup_restore_current_thread). All other inferior + references are considered weak references. Inferiors are always + listed exactly once in the inferior list, so placing an inferior in + the inferior list is an implicit, not counted strong reference. */ + +class inferior : public refcounted_object { public: explicit inferior (int pid); ~inferior (); + /* Returns true if we can delete this inferior. */ + bool deletable () const { return refcount () == 0; } + /* Pointer to next inferior in singly-linked list of inferiors. */ struct inferior *next = NULL; @@ -517,12 +540,6 @@ extern int number_of_live_inferiors (void); (not cores, not executables, real live processes). */ extern int have_live_inferiors (void); -/* Return a pointer to the current inferior. It is an error to call - this if there is no current inferior. */ -extern struct inferior *current_inferior (void); - -extern void set_current_inferior (struct inferior *); - extern struct cleanup *save_current_inferior (void); /* Traverse all inferiors. */ diff --git a/gdb/testsuite/gdb.threads/threadapply.exp b/gdb/testsuite/gdb.threads/threadapply.exp index 959e8b9..39687da 100644 --- a/gdb/testsuite/gdb.threads/threadapply.exp +++ b/gdb/testsuite/gdb.threads/threadapply.exp @@ -93,3 +93,138 @@ proc thr_apply_detach {thread_set} { foreach thread_set {"all" "1.1 1.2 1.3"} { thr_apply_detach $thread_set } + +# Test killing and removing inferiors from a command run via "thread +# apply THREAD_SET". THREAD_SET can be either "1.1", or "all". GDB +# used to mistakenly allow deleting the previously-selected inferior, +# in some cases, leading to crashes. + +proc kill_and_remove_inferior {thread_set} { + global binfile + global gdb_prompt + + # The test starts multiple inferiors, therefore non-extended + # remote is not supported. + if [target_info exists use_gdb_stub] { + unsupported "using gdb stub" + return + } + + set any "\[^\r\n\]*" + set ws "\[ \t\]\+" + + clean_restart ${binfile} + + with_test_prefix "start inferior 1" { + runto_main + } + + # Add and start inferior number NUM. + proc add_and_start_inferior {num} { + global binfile + + # Start another inferior. + gdb_test "add-inferior" "Added inferior $num.*" \ + "add empty inferior $num" + gdb_test "inferior $num" "Switching to inferior $num.*" \ + "switch to inferior $num" + gdb_test "file ${binfile}" ".*" "load file in inferior $num" + + with_test_prefix "start inferior $num" { + runto_main + } + } + + # Start another inferior. + add_and_start_inferior 2 + + # And yet another. + add_and_start_inferior 3 + + gdb_test "thread 2.1" "Switching to thread 2.1 .*" + + # Try removing an active inferior via a "thread apply" command. + # Should fail/warn. + with_test_prefix "try remove" { + + gdb_define_cmd "remove" { + "remove-inferiors 3" + } + + # Inferior 3 is still alive, so can't remove it. + gdb_test "thread apply $thread_set remove" \ + "warning: Can not remove active inferior 3.*" + # Check that GDB restored the selected thread. + gdb_test "thread" "Current thread is 2.1 .*" + + gdb_test "info inferiors" \ + [multi_line \ + "${ws}1${ws}process ${any}" \ + "\\\* 2${ws}process ${any}" \ + "${ws}3${ws}process ${any}" \ + ] + } + + # Kill and try to remove inferior 2 while inferior 2 is selected. + # Removing the inferior should fail/warn. + with_test_prefix "try kill-and-remove" { + + # The "inferior 1" command works around PR gdb/19318 ("kill + # inferior N" shouldn't switch to inferior N). + gdb_define_cmd "kill-and-remove" { + "kill inferiors 2" + "inferior 1" + "remove-inferiors 2" + } + + # Note that when threads=1.1, this makes sure we're really + # testing failure to remove the inferior the user had selected + # before the "thread apply" command, instead of testing + # refusal to remove the currently-iterated inferior. + gdb_test "thread apply $thread_set kill-and-remove" \ + "warning: Can not remove current inferior 2.*" + gdb_test "thread" "No thread selected" \ + "switched to no thread selected" + + gdb_test "info inferiors" \ + [multi_line \ + "${ws}1${ws}process ${any}" \ + "\\\* 2${ws}<null>${any}" \ + "${ws}3${ws}process ${any}" \ + ] + } + + # Try removing (the now dead) inferior 2 while inferior 1 is + # selected. This should succeed. + with_test_prefix "try remove 2" { + + gdb_test "thread 1.1" "Switching to thread 1.1 .*" + + gdb_define_cmd "remove-again" { + "remove-inferiors 2" + } + + set test "thread apply $thread_set remove-again" + gdb_test_multiple $test $test { + -re "warning: Can not remove.*$gdb_prompt $" { + fail $test + } + -re "$gdb_prompt $" { + pass $test + } + } + gdb_test "thread" "Current thread is 1.1 .*" + # Check that only inferiors 1 and 3 are around. + gdb_test "info inferiors" \ + [multi_line \ + "\\\* 1${ws}process ${any}" \ + "${ws}3${ws}process ${any}" \ + ] + } +} + +# Test both "all" and a thread list, because those are implemented as +# different commands in GDB. +foreach_with_prefix thread_set {"all" "1.1"} { + kill_and_remove_inferior $thread_set +} diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp index ea77361..6633d24 100644 --- a/gdb/testsuite/lib/gdb.exp +++ b/gdb/testsuite/lib/gdb.exp @@ -6070,5 +6070,24 @@ proc dejagnu_version { } { return $dg_ver } +# Define user-defined command COMMAND using the COMMAND_LIST as the +# command's definition. The terminating "end" is added automatically. + +proc gdb_define_cmd {command command_list} { + global gdb_prompt + + set input [multi_line_input {*}$command_list "end"] + set test "define $command" + + gdb_test_multiple "define $command" $test { + -re "End with" { + gdb_test_multiple $input $test { + -re "\r\n$gdb_prompt " { + } + } + } + } +} + # Always load compatibility stuff. load_lib future.exp diff --git a/gdb/thread.c b/gdb/thread.c index 88fd521..2c38a9a 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -68,7 +68,6 @@ static void thread_apply_all_command (char *, int); static int thread_alive (struct thread_info *); static void info_threads_command (char *, int); static void thread_apply_command (char *, int); -static void restore_current_thread (ptid_t); /* RAII type used to increase / decrease the refcount of each thread in a given list of threads. */ @@ -1458,10 +1457,8 @@ info_threads_command (char *arg, int from_tty) void switch_to_thread_no_regs (struct thread_info *thread) { - struct inferior *inf; + struct inferior *inf = thread->inf; - inf = find_inferior_ptid (thread->ptid); - gdb_assert (inf != NULL); set_current_program_space (inf->pspace); set_current_inferior (inf); @@ -1469,45 +1466,50 @@ switch_to_thread_no_regs (struct thread_info *thread) stop_pc = ~(CORE_ADDR) 0; } -/* Switch from one thread to another. */ +/* Switch to no thread selected. */ -void -switch_to_thread (ptid_t ptid) +static void +switch_to_no_thread () { - /* Switch the program space as well, if we can infer it from the now - current thread. Otherwise, it's up to the caller to select the - space it wants. */ - if (ptid != null_ptid) - { - struct inferior *inf; + if (inferior_ptid == null_ptid) + return; - inf = find_inferior_ptid (ptid); - gdb_assert (inf != NULL); - set_current_program_space (inf->pspace); - set_current_inferior (inf); - } + inferior_ptid = null_ptid; + reinit_frame_cache (); + stop_pc = ~(CORE_ADDR) 0; +} + +/* Switch from one thread to another. */ + +static void +switch_to_thread (thread_info *thr) +{ + gdb_assert (thr != NULL); - if (ptid == inferior_ptid) + if (inferior_ptid == thr->ptid) return; - inferior_ptid = ptid; + switch_to_thread_no_regs (thr); + reinit_frame_cache (); /* We don't check for is_stopped, because we're called at times while in the TARGET_RUNNING state, e.g., while handling an internal event. */ - if (inferior_ptid != null_ptid - && !is_exited (ptid) - && !is_executing (ptid)) - stop_pc = regcache_read_pc (get_thread_regcache (ptid)); - else - stop_pc = ~(CORE_ADDR) 0; + if (thr->state != THREAD_EXITED + && !thr->executing) + stop_pc = regcache_read_pc (get_thread_regcache (thr->ptid)); } -static void -restore_current_thread (ptid_t ptid) +/* See gdbthread.h. */ + +void +switch_to_thread (ptid_t ptid) { - switch_to_thread (ptid); + if (ptid == null_ptid) + switch_to_no_thread (); + else + switch_to_thread (find_thread_ptid (ptid)); } static void @@ -1578,8 +1580,7 @@ struct current_thread_cleanup struct frame_id selected_frame_id; int selected_frame_level; int was_stopped; - int inf_id; - int was_removable; + inferior *inf; }; static void @@ -1595,12 +1596,12 @@ do_restore_current_thread_cleanup (void *arg) /* If the previously selected thread belonged to a process that has in the mean time exited (or killed, detached, etc.), then don't revert back to it, but instead simply drop back to no thread selected. */ - && find_inferior_ptid (old->thread->ptid) != NULL) - restore_current_thread (old->thread->ptid); + && old->inf->pid != 0) + switch_to_thread (old->thread); else { - restore_current_thread (null_ptid); - set_current_inferior (find_inferior_id (old->inf_id)); + switch_to_no_thread (); + set_current_inferior (old->inf); } /* The running state of the originally selected thread may have @@ -1613,21 +1614,22 @@ do_restore_current_thread_cleanup (void *arg) && target_has_memory) restore_selected_frame (old->selected_frame_id, old->selected_frame_level); + + if (inferior_ptid == null_ptid) + gdb_assert (current_inferior ()->pid == 0); + else + gdb_assert (current_inferior ()->pid == inferior_ptid.pid ()); } static void restore_current_thread_cleanup_dtor (void *arg) { struct current_thread_cleanup *old = (struct current_thread_cleanup *) arg; - struct thread_info *tp; - struct inferior *inf; if (old->thread != NULL) old->thread->decref (); - inf = find_inferior_id (old->inf_id); - if (inf != NULL) - inf->removable = old->was_removable; + old->inf->decref (); xfree (old); } @@ -1637,8 +1639,7 @@ make_cleanup_restore_current_thread (void) struct current_thread_cleanup *old = XNEW (struct current_thread_cleanup); old->thread = NULL; - old->inf_id = current_inferior ()->num; - old->was_removable = current_inferior ()->removable; + old->inf = current_inferior (); if (inferior_ptid != null_ptid) { @@ -1670,7 +1671,7 @@ make_cleanup_restore_current_thread (void) old->thread = tp; } - current_inferior ()->removable = 0; + old->inf->incref (); return make_cleanup_dtor (do_restore_current_thread_cleanup, old, restore_current_thread_cleanup_dtor); -- 2.5.5 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 8/8] Fix removing inferiors from within "thread apply" commands 2017-04-13 14:49 ` Pedro Alves @ 2017-04-19 8:23 ` Yao Qi 2017-04-19 12:15 ` Pedro Alves 0 siblings, 1 reply; 25+ messages in thread From: Yao Qi @ 2017-04-19 8:23 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches Pedro Alves <palves@redhat.com> writes: > See patch below that implements this. I also added comments > explaining the thread_info and inferior objects' refcount handling > to hopefully make things clearer. > Yes, they are helpful. > I then realized that unlike thread_info objects, there's a > "current inferior" pointer, and switching the current > inferior always goes through a single function (set_current_inferior), > so we can make being the current inferior be a strong reference > accounted for in the inferior's refcount. This simplifies the It is reasonable to me. > inferior::deletable method's implementation which no longer > compares this with current_inferior(). It's just a trade off, > because now set_current_inferior and initialize_inferiors > must manage the refcounts. The end result is just the same. > Not sure which version is clearer -- this, or keeping inferior::deletable > as it was (and reverting the set_current_inferior change). > Maybe the other way is a little bit more efficient given > deleting inferiors is way less frequently done than switching > around the current inferior. OTOH maybe this way is clearer. > If you have a preference, let me know. This version is good to me. > -struct thread_info > +/* Threads are intrusively refcounted objects. Being the > + user-selected thread is normally considered an implicit strong > + reference and is thus not accounted in the refcount, unlike > + inferior objects. This is necessary, because there's no "current > + thread" pointer. Instead the current thread is inferred from the > + inferior_ptid global. However, when GDB needs to remember the > + selected thread to later restore it, GDB bumps the thread object's > + refcount, to prevent something deleting the thread object before > + reverting back (e.g., due to a "kill" command. If the thread Missing ")"? -- Yao (齐尧) ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 8/8] Fix removing inferiors from within "thread apply" commands 2017-04-19 8:23 ` Yao Qi @ 2017-04-19 12:15 ` Pedro Alves 2017-04-19 12:20 ` Pedro Alves 0 siblings, 1 reply; 25+ messages in thread From: Pedro Alves @ 2017-04-19 12:15 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches On 04/19/2017 09:23 AM, Yao Qi wrote: > Pedro Alves <palves@redhat.com> writes: > >> See patch below that implements this. I also added comments >> explaining the thread_info and inferior objects' refcount handling >> to hopefully make things clearer. >> > > Yes, they are helpful. > >> I then realized that unlike thread_info objects, there's a >> "current inferior" pointer, and switching the current >> inferior always goes through a single function (set_current_inferior), >> so we can make being the current inferior be a strong reference >> accounted for in the inferior's refcount. This simplifies the > > It is reasonable to me. > >> inferior::deletable method's implementation which no longer >> compares this with current_inferior(). It's just a trade off, >> because now set_current_inferior and initialize_inferiors >> must manage the refcounts. The end result is just the same. >> Not sure which version is clearer -- this, or keeping inferior::deletable >> as it was (and reverting the set_current_inferior change). >> Maybe the other way is a little bit more efficient given >> deleting inferiors is way less frequently done than switching >> around the current inferior. OTOH maybe this way is clearer. >> If you have a preference, let me know. > > This version is good to me. Thanks. > >> -struct thread_info >> +/* Threads are intrusively refcounted objects. Being the >> + user-selected thread is normally considered an implicit strong >> + reference and is thus not accounted in the refcount, unlike >> + inferior objects. This is necessary, because there's no "current >> + thread" pointer. Instead the current thread is inferred from the >> + inferior_ptid global. However, when GDB needs to remember the >> + selected thread to later restore it, GDB bumps the thread object's >> + refcount, to prevent something deleting the thread object before >> + reverting back (e.g., due to a "kill" command. If the thread > > Missing ")"? Yes, I'll add it. (bah, looks like I forgot to refresh the patch before pushing. I'll really fix it in a follow up). Looks like I had missed re-testing against gdbserver when I added the assertion at the end of do_restore_current_thread_cleanup, the one that caught the latent bug in the fork handling: static void do_restore_current_thread_cleanup (void *arg) { ... if (inferior_ptid == null_ptid) gdb_assert (current_inferior ()->pid == 0); else gdb_assert (current_inferior ()->pid == inferior_ptid.pid ()); } This triggers with remote debugging: #0 0x00000000006711f3 in internal_error(char const*, int, char const*, ...) (file=0xaa73d8 "src/gdb/thread.c", line=1619, fmt=0xaa7340 "%s: Assertion `%s' failed.") at src/gdb/common/errors.c:54 #1 0x00000000007a0f98 in do_restore_current_thread_cleanup(void*) (arg=0x1944b00) at src/gdb/thread.c:1619 #2 0x00000000005f6d86 in do_my_cleanups(cleanup**, cleanup*) (pmy_chain=0xf197e0 <cleanup_chain>, old_chain=0xa34900 <sentinel_cleanup>) at src/gdb/common/cleanups.c:154 #3 0x00000000005f6de1 in do_cleanups(cleanup*) (old_chain=0xa34900 <sentinel_cleanup>) at src/gdb/common/cleanups.c:176 #4 0x00000000005d9fad in breakpoint_re_set() () at src/gdb/breakpoint.c:14569 #5 0x000000000076e368 in clear_symtab_users(enum_flags<symfile_add_flag>) (add_flags=...) at src/gdb/symfile.c:2943 #6 0x0000000000769e99 in finish_new_objfile(objfile*, symfile_add_flags) (objfile=0x195c6c0, add_flags=...) at src/gdb/symfile.c:1097 #7 0x000000000076a33b in symbol_file_add_with_addrs(bfd*, char const*, symfile_add_flags, section_addr_info*, objfile_flags, objfile*) (abfd=0x195a0a0, name=0x1957950 "target:gdb/gdbserver/gdbserver", add_flags=..., addrs=0x0, flags=..., parent=0x0) at src/gdb/symfile.c:1223 #8 0x000000000076a4be in symbol_file_add_from_bfd(bfd*, char const*, enum_flags<symfile_add_flag>, section_addr_info*, enum_flags<objfile_flag>, objfile*) (abfd=0x195a0a0, name=0x1957950 "target:gdb/gdbserver/gdbserver", add_flags=..., addrs=0x0, flags=..., parent=0x0) at src/gdb/symfile.c:1268 #9 0x000000000076a542 in symbol_file_add(char const*, enum_flags<symfile_add_flag>, section_addr_info*, enum_flags<objfile_flag>) (name=0x1957950 "target:gdb/gdbserver/gdbserver", add_flags=..., addrs=0x0, flags=...) at src/gdb/symfile.c:1281 #10 0x000000000076a657 in symbol_file_add_main_1(char const*, symfile_add_flags, objfile_flags) (args=0x1957950 "target:gdb/gdbserver/gdbserver", add_flags=..., flags=...) at src/gdb/symfile.c:1304 #11 0x000000000076a5c2 in symbol_file_add_main(char const*, enum_flags<symfile_add_flag>) (args=0x1957950 "target:gdb/gdbserver/gdbserver", add_flags=...) at src/gdb/symfile.c:1295 #12 0x000000000067e38d in try_open_exec_file(char const*, inferior*, enum_flags<symfile_add_flag>) (exec_file_host=0x1957950 "target:gdb/gdbserver/gdbserver", inf=0x11e8fe0, add_flags=...) at src/gdb/exec.c:184 #13 0x000000000067e62f in exec_file_locate_attach(int, int, int) (pid=28228, defer_bp_reset=0, from_tty=1) at src/gdb/exec.c:231 #14 0x000000000049c3c7 in remote_add_inferior(int, int, int, int) (fake_pid_p=0, pid=28228, attached=0, try_open_exec=1) at src/gdb/remote.c:1826 #15 0x000000000049c69f in remote_notice_new_inferior(ptid_t, int) (currthread=..., executing=0) at src/gdb/remote.c:1929 #16 0x000000000049ef43 in remote_update_thread_list(target_ops*) (ops=0xf2f6e0 <remote_ops>) at src/gdb/remote.c:3324 #17 0x00000000007868a5 in delegate_update_thread_list(target_ops*) (self=0xf2f6e0 <remote_ops>) at src/gdb/target-delegates.c:1487 #18 0x00000000007962cd in target_update_thread_list() () at src/gdb/target.c:3426 #19 0x00000000004a0c26 in remote_start_remote(int, target_ops*, int) (from_tty=1, target=0xf2f6e0 <remote_ops>, extended_p=0) at src/gdb/remote.c:4211 The problem starts around remote_add_inferior, where we create an inferior with a non-zero pid, and then leave inferior_ptid set to null_ptid. We can't fix that by simply making inferior_ptid point to the new inferior because we haven't added any thread to the inferior yet at this point. It'll be a bit complicated to untangle that, so I'll defer adding the assertion until some later time. Below's what I pushed (the same, except the assertion added). (I've pushed patch #7 as well.) From 3a3fd0fd2c4c87fdd588c51d879961a49e38f0c1 Mon Sep 17 00:00:00 2001 From: Pedro Alves <palves@redhat.com> Date: Wed, 19 Apr 2017 13:12:23 +0100 Subject: [PATCH] Fix removing inferiors from within "thread apply" commands This patch fixes an internal error exposed by a test that does something like: define kill-and-remove kill inferiors 2 remove-inferiors 2 end # Start one inferior. start # Start another inferior. add-inferior 2 inferior 2 start # Kill and remove inferior 1 while inferior 2 is selected. thread apply 1.1 kill-and-remove The internal error looks like this: Thread 1.1 (Thread 0x7ffff7fc2700 (LWP 20677)): [Switching to inferior 1 [process 20677] (gdb/testsuite/outputs/gdb.threads/threadapply/threadapply)] [Switching to thread 1.1 (Thread 0x7ffff7fc2700 (LWP 20677))] #0 main () at src/gdb/testsuite/gdb.threads/threadapply.c:38 38 for (i = 0; i < NUM; i++) src/gdb/inferior.c:66: internal-error: void set_current_inferior(inferior*): Assertion `inf != NULL' failed. A problem internal to GDB has been detected, further debugging may prove unreliable. Quit this debugging session? (y or n) FAIL: gdb.threads/threadapply.exp: kill_and_remove_inferior: try kill-and-remove: thread apply 1.1 kill-and-remove (GDB internal error) There are several problems around this area of the code. One is that in do_restore_current_thread_cleanup, we do a look up of inferior by ptid, which can find the wrong inferior if the previously selected inferior exited and some other inferior was started with a reused pid (rare, but still...). The other problem is that the "remove-inferiors" command rejects attempts to remove the current inferior, but when we get to "remove-inferiors" in a "thread apply THR remove-inferiors 2" command, the current inferior is the inferior of thread THR, not the previously selected inferior, so if the previously selected inferior was inferior 2, that command still manages to wipe it, and then gdb restores the old selected inferior, which is now a dangling pointer... So the fix here is: - Make make_cleanup_restore_current_thread store a pointer to the previously selected inferior directly, and use it directly instead of doing ptid look ups. - Add a refcount to inferiors, very similar to thread_info's refcount, that is incremented/decremented by make_cleanup_restore_current_thread, and checked before deleting an inferior. To avoid duplication, a new refcounted_object type is added, that both thread_info and inferior inherit from. gdb/ChangeLog: 2017-04-19 Pedro Alves <palves@redhat.com> * common/refcounted-object.h: New file. * gdbthread.h: Include "common/refcounted-object.h". (thread_info): Inherit from refcounted_object and add comments. (thread_info::incref, thread_info::decref) (thread_info::m_refcount): Delete. (thread_info::deletable): Use the refcounted_object::refcount() method. * inferior.c (current_inferior_): Add comment. (set_current_inferior): Increment/decrement refcounts. (prune_inferiors, remove_inferior_command): Skip inferiors marked not-deletable instead of comparing with the current inferior. (initialize_inferiors): Increment the initial inferior's refcount. * inferior.h (struct inferior): Forward declare. Include "common/refcounted-object.h". (current_inferior, set_current_inferior): Move declaration to before struct inferior's definition, and fix comment. (inferior): Inherit from refcounted_object. Add comments. * thread.c (switch_to_thread_no_regs): Reference the thread's inferior pointer directly instead of doing a ptid lookup. (switch_to_no_thread): New function. (switch_to_thread(thread_info *)): New function, factored out from ... (switch_to_thread(ptid_t)): ... this. (restore_current_thread): Delete. (current_thread_cleanup): Remove 'inf_id' and 'was_removable' fields, and add 'inf' field. (do_restore_current_thread_cleanup): Check whether old->inf is alive instead of looking up an inferior by ptid. Use switch_to_thread and switch_to_no_thread. (restore_current_thread_cleanup_dtor): Use old->inf directly instead of lookup up an inferior by id. Decref the inferior. Don't restore 'removable'. (make_cleanup_restore_current_thread): Same the inferior pointer in old, instead of the inferior number. Incref the inferior. Don't save/clear 'removable'. gdb/testsuite/ChangeLog: 2017-04-19 Pedro Alves <palves@redhat.com> * gdb.threads/threadapply.exp (kill_and_remove_inferior): New procedure. (top level): Call it. * lib/gdb.exp (gdb_define_cmd): New procedure. --- gdb/ChangeLog | 38 +++++++++ gdb/testsuite/ChangeLog | 7 ++ gdb/common/refcounted-object.h | 56 +++++++++++++ gdb/gdbthread.h | 45 +++++----- gdb/inferior.c | 12 ++- gdb/inferior.h | 35 ++++++-- gdb/testsuite/gdb.threads/threadapply.exp | 135 ++++++++++++++++++++++++++++++ gdb/testsuite/lib/gdb.exp | 19 +++++ gdb/thread.c | 82 +++++++++--------- 9 files changed, 349 insertions(+), 80 deletions(-) create mode 100644 gdb/common/refcounted-object.h diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 68c3c91..e260896 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,5 +1,43 @@ 2017-04-19 Pedro Alves <palves@redhat.com> + * common/refcounted-object.h: New file. + * gdbthread.h: Include "common/refcounted-object.h". + (thread_info): Inherit from refcounted_object and add comments. + (thread_info::incref, thread_info::decref) + (thread_info::m_refcount): Delete. + (thread_info::deletable): Use the refcounted_object::refcount() + method. + * inferior.c (current_inferior_): Add comment. + (set_current_inferior): Increment/decrement refcounts. + (prune_inferiors, remove_inferior_command): Skip inferiors marked + not-deletable instead of comparing with the current inferior. + (initialize_inferiors): Increment the initial inferior's refcount. + * inferior.h (struct inferior): Forward declare. + Include "common/refcounted-object.h". + (current_inferior, set_current_inferior): Move declaration to + before struct inferior's definition, and fix comment. + (inferior): Inherit from refcounted_object. Add comments. + * thread.c (switch_to_thread_no_regs): Reference the thread's + inferior pointer directly instead of doing a ptid lookup. + (switch_to_no_thread): New function. + (switch_to_thread(thread_info *)): New function, factored out + from ... + (switch_to_thread(ptid_t)): ... this. + (restore_current_thread): Delete. + (current_thread_cleanup): Remove 'inf_id' and 'was_removable' + fields, and add 'inf' field. + (do_restore_current_thread_cleanup): Check whether old->inf is + alive instead of looking up an inferior by ptid. Use + switch_to_thread and switch_to_no_thread. + (restore_current_thread_cleanup_dtor): Use old->inf directly + instead of lookup up an inferior by id. Decref the inferior. + Don't restore 'removable'. + (make_cleanup_restore_current_thread): Same the inferior pointer + in old, instead of the inferior number. Incref the inferior. + Don't save/clear 'removable'. + +2017-04-19 Pedro Alves <palves@redhat.com> + * Makefile.in (SUBDIR_UNITTESTS_SRCS): Add unittests/scoped_restore-selftests.c. (SUBDIR_UNITTESTS_OBS): Add scoped_restore-selftests.o. diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog index 42d6a8d..c4d5b79 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,3 +1,10 @@ +2017-04-19 Pedro Alves <palves@redhat.com> + + * gdb.threads/threadapply.exp (kill_and_remove_inferior): New + procedure. + (top level): Call it. + * lib/gdb.exp (gdb_define_cmd): New procedure. + 2017-04-12 Pedro Alves <palves@redhat.com> PR gdb/21323 diff --git a/gdb/common/refcounted-object.h b/gdb/common/refcounted-object.h new file mode 100644 index 0000000..9d0ac10 --- /dev/null +++ b/gdb/common/refcounted-object.h @@ -0,0 +1,56 @@ +/* Base class of intrusively reference-counted objects. + Copyright (C) 2017 Free Software Foundation, Inc. + + This file is part of GDB. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +#ifndef REFCOUNTED_OBJECT_H +#define REFCOUNTED_OBJECT_H + +/* Base class of intrusively reference-countable objects. + Incrementing and decrementing the reference count is an external + responsibility. */ + +class refcounted_object +{ +public: + refcounted_object () = default; + + /* Increase the refcount. */ + void incref () + { + gdb_assert (m_refcount >= 0); + m_refcount++; + } + + /* Decrease the refcount. */ + void decref () + { + m_refcount--; + gdb_assert (m_refcount >= 0); + } + + int refcount () const { return m_refcount; } + +private: + /* Disable copy. */ + refcounted_object (const refcounted_object &) = delete; + refcounted_object &operator=(const refcounted_object &) = delete; + + /* The reference count. */ + int m_refcount = 0; +}; + +#endif /* REFCOUNTED_OBJECT_H */ diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h index 4cd7390..33977ee 100644 --- a/gdb/gdbthread.h +++ b/gdb/gdbthread.h @@ -31,6 +31,7 @@ struct symtab; #include "common/vec.h" #include "target/waitstatus.h" #include "cli/cli-utils.h" +#include "common/refcounted-object.h" /* Frontend view of the thread state. Possible extensions: stepping, finishing, until(ling),... */ @@ -177,7 +178,24 @@ typedef struct value *value_ptr; DEF_VEC_P (value_ptr); typedef VEC (value_ptr) value_vec; -struct thread_info +/* Threads are intrusively refcounted objects. Being the + user-selected thread is normally considered an implicit strong + reference and is thus not accounted in the refcount, unlike + inferior objects. This is necessary, because there's no "current + thread" pointer. Instead the current thread is inferred from the + inferior_ptid global. However, when GDB needs to remember the + selected thread to later restore it, GDB bumps the thread object's + refcount, to prevent something deleting the thread object before + reverting back (e.g., due to a "kill" command. If the thread + meanwhile exits before being re-selected, then the thread object is + left listed in the thread list, but marked with state + THREAD_EXITED. (See make_cleanup_restore_current_thread and + delete_thread). All other thread references are considered weak + references. Placing a thread in the thread list is an implicit + strong reference, and is thus not accounted for in the thread's + refcount. */ + +class thread_info : public refcounted_object { public: explicit thread_info (inferior *inf, ptid_t ptid); @@ -186,22 +204,8 @@ public: bool deletable () const { /* If this is the current thread, or there's code out there that - relies on it existing (m_refcount > 0) we can't delete yet. */ - return (m_refcount == 0 && !ptid_equal (ptid, inferior_ptid)); - } - - /* Increase the refcount. */ - void incref () - { - gdb_assert (m_refcount >= 0); - m_refcount++; - } - - /* Decrease the refcount. */ - void decref () - { - m_refcount--; - gdb_assert (m_refcount >= 0); + relies on it existing (refcount > 0) we can't delete yet. */ + return (refcount () == 0 && !ptid_equal (ptid, inferior_ptid)); } struct thread_info *next = NULL; @@ -362,13 +366,6 @@ public: fields point to self. */ struct thread_info *step_over_prev = NULL; struct thread_info *step_over_next = NULL; - -private: - - /* If this is > 0, then it means there's code out there that relies - on this thread being listed. Don't delete it from the lists even - if we detect it exiting. */ - int m_refcount = 0; }; /* Create an empty thread list, or empty the existing one. */ diff --git a/gdb/inferior.c b/gdb/inferior.c index 327590a..e85859f 100644 --- a/gdb/inferior.c +++ b/gdb/inferior.c @@ -50,7 +50,9 @@ static int highest_inferior_num; `set print inferior-events'. */ static int print_inferior_events = 0; -/* The Current Inferior. */ +/* The Current Inferior. This is a strong reference. I.e., whenever + an inferior is the current inferior, its refcount is + incremented. */ static struct inferior *current_inferior_ = NULL; struct inferior* @@ -65,6 +67,8 @@ set_current_inferior (struct inferior *inf) /* There's always an inferior. */ gdb_assert (inf != NULL); + inf->incref (); + current_inferior_->decref (); current_inferior_ = inf; } @@ -483,13 +487,12 @@ void prune_inferiors (void) { struct inferior *ss, **ss_link; - struct inferior *current = current_inferior (); ss = inferior_list; ss_link = &inferior_list; while (ss) { - if (ss == current + if (!ss->deletable () || !ss->removable || ss->pid != 0) { @@ -774,7 +777,7 @@ remove_inferior_command (char *args, int from_tty) continue; } - if (inf == current_inferior ()) + if (!inf->deletable ()) { warning (_("Can not remove current inferior %d."), num); continue; @@ -1017,6 +1020,7 @@ initialize_inferiors (void) that. Do this after initialize_progspace, due to the current_program_space reference. */ current_inferior_ = add_inferior (0); + current_inferior_->incref (); current_inferior_->pspace = current_program_space; current_inferior_->aspace = current_program_space->aspace; /* The architecture will be initialized shortly, by diff --git a/gdb/inferior.h b/gdb/inferior.h index e39a80c..c6fb9d3 100644 --- a/gdb/inferior.h +++ b/gdb/inferior.h @@ -32,6 +32,7 @@ struct terminal_info; struct target_desc_info; struct gdb_environ; struct continuation; +struct inferior; /* For bpstat. */ #include "breakpoint.h" @@ -46,6 +47,7 @@ struct continuation; #include "registry.h" #include "symfile-add-flags.h" +#include "common/refcounted-object.h" struct infcall_suspend_state; struct infcall_control_state; @@ -298,6 +300,11 @@ struct inferior_control_state enum stop_kind stop_soon; }; +/* Return a pointer to the current inferior. */ +extern inferior *current_inferior (); + +extern void set_current_inferior (inferior *); + /* GDB represents the state of each program execution with an object called an inferior. An inferior typically corresponds to a process but is more general and applies also to targets that do not have a @@ -305,14 +312,30 @@ struct inferior_control_state inferior, as does each attachment to an existing process. Inferiors have unique internal identifiers that are different from target process ids. Each inferior may in turn have multiple - threads running in it. */ - -class inferior + threads running in it. + + Inferiors are intrusively refcounted objects. Unlike thread + objects, being the user-selected inferior is considered a strong + reference and is thus accounted for in the inferior object's + refcount (see set_current_inferior). When GDB needs to remember + the selected inferior to later restore it, GDB temporarily bumps + the inferior object's refcount, to prevent something deleting the + inferior object before reverting back (e.g., due to a + "remove-inferiors" command (see + make_cleanup_restore_current_thread). All other inferior + references are considered weak references. Inferiors are always + listed exactly once in the inferior list, so placing an inferior in + the inferior list is an implicit, not counted strong reference. */ + +class inferior : public refcounted_object { public: explicit inferior (int pid); ~inferior (); + /* Returns true if we can delete this inferior. */ + bool deletable () const { return refcount () == 0; } + /* Pointer to next inferior in singly-linked list of inferiors. */ struct inferior *next = NULL; @@ -517,12 +540,6 @@ extern int number_of_live_inferiors (void); (not cores, not executables, real live processes). */ extern int have_live_inferiors (void); -/* Return a pointer to the current inferior. It is an error to call - this if there is no current inferior. */ -extern struct inferior *current_inferior (void); - -extern void set_current_inferior (struct inferior *); - extern struct cleanup *save_current_inferior (void); /* Traverse all inferiors. */ diff --git a/gdb/testsuite/gdb.threads/threadapply.exp b/gdb/testsuite/gdb.threads/threadapply.exp index 959e8b9..39687da 100644 --- a/gdb/testsuite/gdb.threads/threadapply.exp +++ b/gdb/testsuite/gdb.threads/threadapply.exp @@ -93,3 +93,138 @@ proc thr_apply_detach {thread_set} { foreach thread_set {"all" "1.1 1.2 1.3"} { thr_apply_detach $thread_set } + +# Test killing and removing inferiors from a command run via "thread +# apply THREAD_SET". THREAD_SET can be either "1.1", or "all". GDB +# used to mistakenly allow deleting the previously-selected inferior, +# in some cases, leading to crashes. + +proc kill_and_remove_inferior {thread_set} { + global binfile + global gdb_prompt + + # The test starts multiple inferiors, therefore non-extended + # remote is not supported. + if [target_info exists use_gdb_stub] { + unsupported "using gdb stub" + return + } + + set any "\[^\r\n\]*" + set ws "\[ \t\]\+" + + clean_restart ${binfile} + + with_test_prefix "start inferior 1" { + runto_main + } + + # Add and start inferior number NUM. + proc add_and_start_inferior {num} { + global binfile + + # Start another inferior. + gdb_test "add-inferior" "Added inferior $num.*" \ + "add empty inferior $num" + gdb_test "inferior $num" "Switching to inferior $num.*" \ + "switch to inferior $num" + gdb_test "file ${binfile}" ".*" "load file in inferior $num" + + with_test_prefix "start inferior $num" { + runto_main + } + } + + # Start another inferior. + add_and_start_inferior 2 + + # And yet another. + add_and_start_inferior 3 + + gdb_test "thread 2.1" "Switching to thread 2.1 .*" + + # Try removing an active inferior via a "thread apply" command. + # Should fail/warn. + with_test_prefix "try remove" { + + gdb_define_cmd "remove" { + "remove-inferiors 3" + } + + # Inferior 3 is still alive, so can't remove it. + gdb_test "thread apply $thread_set remove" \ + "warning: Can not remove active inferior 3.*" + # Check that GDB restored the selected thread. + gdb_test "thread" "Current thread is 2.1 .*" + + gdb_test "info inferiors" \ + [multi_line \ + "${ws}1${ws}process ${any}" \ + "\\\* 2${ws}process ${any}" \ + "${ws}3${ws}process ${any}" \ + ] + } + + # Kill and try to remove inferior 2 while inferior 2 is selected. + # Removing the inferior should fail/warn. + with_test_prefix "try kill-and-remove" { + + # The "inferior 1" command works around PR gdb/19318 ("kill + # inferior N" shouldn't switch to inferior N). + gdb_define_cmd "kill-and-remove" { + "kill inferiors 2" + "inferior 1" + "remove-inferiors 2" + } + + # Note that when threads=1.1, this makes sure we're really + # testing failure to remove the inferior the user had selected + # before the "thread apply" command, instead of testing + # refusal to remove the currently-iterated inferior. + gdb_test "thread apply $thread_set kill-and-remove" \ + "warning: Can not remove current inferior 2.*" + gdb_test "thread" "No thread selected" \ + "switched to no thread selected" + + gdb_test "info inferiors" \ + [multi_line \ + "${ws}1${ws}process ${any}" \ + "\\\* 2${ws}<null>${any}" \ + "${ws}3${ws}process ${any}" \ + ] + } + + # Try removing (the now dead) inferior 2 while inferior 1 is + # selected. This should succeed. + with_test_prefix "try remove 2" { + + gdb_test "thread 1.1" "Switching to thread 1.1 .*" + + gdb_define_cmd "remove-again" { + "remove-inferiors 2" + } + + set test "thread apply $thread_set remove-again" + gdb_test_multiple $test $test { + -re "warning: Can not remove.*$gdb_prompt $" { + fail $test + } + -re "$gdb_prompt $" { + pass $test + } + } + gdb_test "thread" "Current thread is 1.1 .*" + # Check that only inferiors 1 and 3 are around. + gdb_test "info inferiors" \ + [multi_line \ + "\\\* 1${ws}process ${any}" \ + "${ws}3${ws}process ${any}" \ + ] + } +} + +# Test both "all" and a thread list, because those are implemented as +# different commands in GDB. +foreach_with_prefix thread_set {"all" "1.1"} { + kill_and_remove_inferior $thread_set +} diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp index ea77361..6633d24 100644 --- a/gdb/testsuite/lib/gdb.exp +++ b/gdb/testsuite/lib/gdb.exp @@ -6070,5 +6070,24 @@ proc dejagnu_version { } { return $dg_ver } +# Define user-defined command COMMAND using the COMMAND_LIST as the +# command's definition. The terminating "end" is added automatically. + +proc gdb_define_cmd {command command_list} { + global gdb_prompt + + set input [multi_line_input {*}$command_list "end"] + set test "define $command" + + gdb_test_multiple "define $command" $test { + -re "End with" { + gdb_test_multiple $input $test { + -re "\r\n$gdb_prompt " { + } + } + } + } +} + # Always load compatibility stuff. load_lib future.exp diff --git a/gdb/thread.c b/gdb/thread.c index 88fd521..e4113c2 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -68,7 +68,6 @@ static void thread_apply_all_command (char *, int); static int thread_alive (struct thread_info *); static void info_threads_command (char *, int); static void thread_apply_command (char *, int); -static void restore_current_thread (ptid_t); /* RAII type used to increase / decrease the refcount of each thread in a given list of threads. */ @@ -1458,10 +1457,8 @@ info_threads_command (char *arg, int from_tty) void switch_to_thread_no_regs (struct thread_info *thread) { - struct inferior *inf; + struct inferior *inf = thread->inf; - inf = find_inferior_ptid (thread->ptid); - gdb_assert (inf != NULL); set_current_program_space (inf->pspace); set_current_inferior (inf); @@ -1469,45 +1466,50 @@ switch_to_thread_no_regs (struct thread_info *thread) stop_pc = ~(CORE_ADDR) 0; } -/* Switch from one thread to another. */ +/* Switch to no thread selected. */ -void -switch_to_thread (ptid_t ptid) +static void +switch_to_no_thread () { - /* Switch the program space as well, if we can infer it from the now - current thread. Otherwise, it's up to the caller to select the - space it wants. */ - if (ptid != null_ptid) - { - struct inferior *inf; + if (inferior_ptid == null_ptid) + return; - inf = find_inferior_ptid (ptid); - gdb_assert (inf != NULL); - set_current_program_space (inf->pspace); - set_current_inferior (inf); - } + inferior_ptid = null_ptid; + reinit_frame_cache (); + stop_pc = ~(CORE_ADDR) 0; +} + +/* Switch from one thread to another. */ - if (ptid == inferior_ptid) +static void +switch_to_thread (thread_info *thr) +{ + gdb_assert (thr != NULL); + + if (inferior_ptid == thr->ptid) return; - inferior_ptid = ptid; + switch_to_thread_no_regs (thr); + reinit_frame_cache (); /* We don't check for is_stopped, because we're called at times while in the TARGET_RUNNING state, e.g., while handling an internal event. */ - if (inferior_ptid != null_ptid - && !is_exited (ptid) - && !is_executing (ptid)) - stop_pc = regcache_read_pc (get_thread_regcache (ptid)); - else - stop_pc = ~(CORE_ADDR) 0; + if (thr->state != THREAD_EXITED + && !thr->executing) + stop_pc = regcache_read_pc (get_thread_regcache (thr->ptid)); } -static void -restore_current_thread (ptid_t ptid) +/* See gdbthread.h. */ + +void +switch_to_thread (ptid_t ptid) { - switch_to_thread (ptid); + if (ptid == null_ptid) + switch_to_no_thread (); + else + switch_to_thread (find_thread_ptid (ptid)); } static void @@ -1578,8 +1580,7 @@ struct current_thread_cleanup struct frame_id selected_frame_id; int selected_frame_level; int was_stopped; - int inf_id; - int was_removable; + inferior *inf; }; static void @@ -1595,12 +1596,12 @@ do_restore_current_thread_cleanup (void *arg) /* If the previously selected thread belonged to a process that has in the mean time exited (or killed, detached, etc.), then don't revert back to it, but instead simply drop back to no thread selected. */ - && find_inferior_ptid (old->thread->ptid) != NULL) - restore_current_thread (old->thread->ptid); + && old->inf->pid != 0) + switch_to_thread (old->thread); else { - restore_current_thread (null_ptid); - set_current_inferior (find_inferior_id (old->inf_id)); + switch_to_no_thread (); + set_current_inferior (old->inf); } /* The running state of the originally selected thread may have @@ -1619,15 +1620,11 @@ static void restore_current_thread_cleanup_dtor (void *arg) { struct current_thread_cleanup *old = (struct current_thread_cleanup *) arg; - struct thread_info *tp; - struct inferior *inf; if (old->thread != NULL) old->thread->decref (); - inf = find_inferior_id (old->inf_id); - if (inf != NULL) - inf->removable = old->was_removable; + old->inf->decref (); xfree (old); } @@ -1637,8 +1634,7 @@ make_cleanup_restore_current_thread (void) struct current_thread_cleanup *old = XNEW (struct current_thread_cleanup); old->thread = NULL; - old->inf_id = current_inferior ()->num; - old->was_removable = current_inferior ()->removable; + old->inf = current_inferior (); if (inferior_ptid != null_ptid) { @@ -1670,7 +1666,7 @@ make_cleanup_restore_current_thread (void) old->thread = tp; } - current_inferior ()->removable = 0; + old->inf->incref (); return make_cleanup_dtor (do_restore_current_thread_cleanup, old, restore_current_thread_cleanup_dtor); -- 2.5.5 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 8/8] Fix removing inferiors from within "thread apply" commands 2017-04-19 12:15 ` Pedro Alves @ 2017-04-19 12:20 ` Pedro Alves 0 siblings, 0 replies; 25+ messages in thread From: Pedro Alves @ 2017-04-19 12:20 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches On 04/19/2017 01:15 PM, Pedro Alves wrote: >>> -struct thread_info >>> >> +/* Threads are intrusively refcounted objects. Being the >>> >> + user-selected thread is normally considered an implicit strong >>> >> + reference and is thus not accounted in the refcount, unlike >>> >> + inferior objects. This is necessary, because there's no "current >>> >> + thread" pointer. Instead the current thread is inferred from the >>> >> + inferior_ptid global. However, when GDB needs to remember the >>> >> + selected thread to later restore it, GDB bumps the thread object's >>> >> + refcount, to prevent something deleting the thread object before >>> >> + reverting back (e.g., due to a "kill" command. If the thread >> > >> > Missing ")"? > Yes, I'll add it. (bah, looks like I forgot to refresh the patch > before pushing. I'll really fix it in a follow up). Like so. Pushed. From a6c21d4a553de184562fd8409a5bcd3f2cc2561a Mon Sep 17 00:00:00 2001 From: Pedro Alves <palves@redhat.com> Date: Wed, 19 Apr 2017 13:16:05 +0100 Subject: [PATCH] gdbthread.h: Fix comment typo gdb/ChangeLog: 2017-04-19 Pedro Alves <palves@redhat.com> * gdbthread.h (thread): Add missing closing parenthesis in comment. --- gdb/ChangeLog | 5 +++++ gdb/gdbthread.h | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index e260896..b30e63f 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,5 +1,10 @@ 2017-04-19 Pedro Alves <palves@redhat.com> + * gdbthread.h (thread): Add missing closing parenthesis in + comment. + +2017-04-19 Pedro Alves <palves@redhat.com> + * common/refcounted-object.h: New file. * gdbthread.h: Include "common/refcounted-object.h". (thread_info): Inherit from refcounted_object and add comments. diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h index 33977ee..68427e9 100644 --- a/gdb/gdbthread.h +++ b/gdb/gdbthread.h @@ -186,7 +186,7 @@ typedef VEC (value_ptr) value_vec; inferior_ptid global. However, when GDB needs to remember the selected thread to later restore it, GDB bumps the thread object's refcount, to prevent something deleting the thread object before - reverting back (e.g., due to a "kill" command. If the thread + reverting back (e.g., due to a "kill" command). If the thread meanwhile exits before being re-selected, then the thread object is left listed in the thread list, but marked with state THREAD_EXITED. (See make_cleanup_restore_current_thread and -- 2.5.5 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 8/8] Fix removing inferiors from within "thread apply" commands 2017-04-13 12:22 ` Pedro Alves 2017-04-13 14:49 ` Pedro Alves @ 2017-04-13 15:31 ` Yao Qi 1 sibling, 0 replies; 25+ messages in thread From: Yao Qi @ 2017-04-13 15:31 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches Pedro Alves <palves@redhat.com> writes: > Just to be clear, would the class include the incref()/decref() > methods + m_refcount, plus some refcount() accessor method, Yes, that is what I meant. > or more than that? Let me give it a try. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 2/8] Fix follow-fork latent bug 2017-04-11 23:51 [PATCH 0/8] Fix removing inferiors from within "thread apply" commands Pedro Alves ` (3 preceding siblings ...) 2017-04-11 23:51 ` [PATCH 8/8] Fix removing inferiors from within "thread apply" commands Pedro Alves @ 2017-04-11 23:51 ` Pedro Alves 2017-04-13 9:58 ` Yao Qi 2017-04-11 23:51 ` [PATCH 1/8] watch_command_1: Fix dangling frame access Pedro Alves ` (2 subsequent siblings) 7 siblings, 1 reply; 25+ messages in thread From: Pedro Alves @ 2017-04-11 23:51 UTC (permalink / raw) To: gdb-patches A later patch in the series adds an assertion to switch_to_thread that the resulting inferior_ptid always matches the "current_inferior()" inferior. This exposed a latent bug in the follow-fork code, where we're building the fork child inferior. We're switching inferior_ptid, but not the current inferior object... gdb/ChangeLog: yyyy-mm-dd Pedro Alves <palves@redhat.com> * infrun.c (follow_fork_inferior): Also switch the current inferior. --- gdb/infrun.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/gdb/infrun.c b/gdb/infrun.c index b5eb4ab..c7298a3 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -498,11 +498,11 @@ holding the child stopped. Try \"set detach-on-fork\" or \ child_inf->gdbarch = parent_inf->gdbarch; copy_inferior_target_desc_info (child_inf, parent_inf); - old_chain = save_inferior_ptid (); - save_current_program_space (); + old_chain = save_current_space_and_thread (); inferior_ptid = child_ptid; add_thread (inferior_ptid); + set_current_inferior (child_inf); child_inf->symfile_flags = SYMFILE_NO_READ; /* If this is a vfork child, then the address-space is @@ -631,6 +631,7 @@ holding the child stopped. Try \"set detach-on-fork\" or \ inferior_ptid = child_ptid; add_thread (inferior_ptid); + set_current_inferior (child_inf); /* If this is a vfork child, then the address-space is shared with the parent. If we detached from the parent, then we can -- 2.5.5 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/8] Fix follow-fork latent bug 2017-04-11 23:51 ` [PATCH 2/8] Fix follow-fork latent bug Pedro Alves @ 2017-04-13 9:58 ` Yao Qi 2017-04-13 12:09 ` Pedro Alves 0 siblings, 1 reply; 25+ messages in thread From: Yao Qi @ 2017-04-13 9:58 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches Pedro Alves <palves@redhat.com> writes: > - old_chain = save_inferior_ptid (); > - save_current_program_space (); > + old_chain = save_current_space_and_thread (); > > inferior_ptid = child_ptid; > add_thread (inferior_ptid); > + set_current_inferior (child_inf); > child_inf->symfile_flags = SYMFILE_NO_READ; Can we set up child thread_info inferior and pspace first, and then, call switch_to_thread_no_regs to switch them in one go? > > /* If this is a vfork child, then the address-space is > @@ -631,6 +631,7 @@ holding the child stopped. Try \"set detach-on-fork\" or \ > > inferior_ptid = child_ptid; > add_thread (inferior_ptid); > + set_current_inferior (child_inf); -- Yao (齐尧) ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/8] Fix follow-fork latent bug 2017-04-13 9:58 ` Yao Qi @ 2017-04-13 12:09 ` Pedro Alves 2017-04-13 15:33 ` Yao Qi 0 siblings, 1 reply; 25+ messages in thread From: Pedro Alves @ 2017-04-13 12:09 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches On 04/13/2017 10:58 AM, Yao Qi wrote: > Pedro Alves <palves@redhat.com> writes: > >> - old_chain = save_inferior_ptid (); >> - save_current_program_space (); >> + old_chain = save_current_space_and_thread (); >> >> inferior_ptid = child_ptid; >> add_thread (inferior_ptid); >> + set_current_inferior (child_inf); >> child_inf->symfile_flags = SYMFILE_NO_READ; > > Can we set up child thread_info inferior and pspace first, and then, > call switch_to_thread_no_regs to switch them in one go? Although it seemingly works, and I tried this patch on top of the series: diff --git c/gdb/infrun.c w/gdb/infrun.c index fcafdc1..3c5f583 100644 --- c/gdb/infrun.c +++ w/gdb/infrun.c @@ -500,9 +500,7 @@ holding the child stopped. Try \"set detach-on-fork\" or \ old_chain = save_current_space_and_thread (); - inferior_ptid = child_ptid; - add_thread (inferior_ptid); - set_current_inferior (child_inf); + add_thread (child_ptid); child_inf->symfile_flags = SYMFILE_NO_READ; /* If this is a vfork child, then the address-space is @@ -629,9 +627,7 @@ holding the child stopped. Try \"set detach-on-fork\" or \ this new thread, before cloning the program space, and informing the solib layer about this new process. */ - inferior_ptid = child_ptid; - add_thread (inferior_ptid); - set_current_inferior (child_inf); + add_thread (child_ptid); /* If this is a vfork child, then the address-space is shared with the parent. If we detached from the parent, then we can @@ -657,6 +653,8 @@ holding the child stopped. Try \"set detach-on-fork\" or \ the core, this wouldn't be required. */ solib_create_inferior_hook (0); } + + switch_to_thread (child_ptid); } return target_follow_fork (follow_child, detach_fork); I don't think I'd like to do that (at least not now), because it doesn't look like clone_program_space and solib_create_inferior_hook really work correctly if called without switching the current inferior to point to the child. For example, I set a breakpoint on current_inferior, active while GDB is running clone_program_space, and with "set detach-on-fork off" for example, we get many hits which currently (it looks to me incorrectly) refer to the parent inferior instead of the child inferior that is loading symbols. Here's a sample: (top-gdb) bt #0 0x00000000006c2baa in current_inferior() () at src/gdb/inferior.c:59 #1 0x00000000006a5026 in set_target_gdbarch(gdbarch*) (new_gdbarch=0x19aba30) at src/gdb/gdbarch.c:5438 #2 0x00000000005b21ac in set_gdbarch_from_file(bfd*) (abfd=0x1193540) at src/gdb/arch-utils.c:620 #3 0x000000000067f2a8 in exec_file_attach(char const*, int) (filename=0x1894a30, from_tty=0) at src/gdb/exec.c:383 #4 0x0000000000729985 in clone_program_space(program_space*, program_space*) (dest=0x1c8d3a0, src=0x11e8de0) at src/gdb/progspace.c:193 #5 0x00000000006c571a in follow_fork_inferior(int, int) (follow_child=0, detach_fork=0) at infrun.c:527 (top-gdb) bt #0 0x00000000006c2baa in current_inferior() () at src/gdb/inferior.c:59 #1 0x00000000005b66f5 in invalidate_auxv_cache() () at src/gdb/auxv.c:345 #2 0x000000000070f705 in observer_executable_changed_notification_stub(void const*, void const*) (data=0x5b66ec <invalidate_auxv_cache()>, args_data=0x7fffffffd028) at ./observer.inc:364 #3 0x000000000070ef72 in generic_observer_notify(observer_list*, void const*) (subject=0x11085e0, args=0x7fffffffd028) at src/gdb/observer.c:167 #4 0x000000000070f796 in observer_notify_executable_changed() () at ./observer.inc:387 #5 0x000000000067f316 in exec_file_attach(char const*, int) (filename=0x1894a30 "gdb/testsuite/outputs/gdb.base/foll-fork/foll-fork", from_tty=0) at src/gdb/exec.c:399 #6 0x0000000000729985 in clone_program_space(program_space*, program_space*) (dest=0x1c8d3a0, src=0x11e8de0) at src/gdb/progspace.c:193 I'd rather not have to dig into this area too deeply at this point. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/8] Fix follow-fork latent bug 2017-04-13 12:09 ` Pedro Alves @ 2017-04-13 15:33 ` Yao Qi 0 siblings, 0 replies; 25+ messages in thread From: Yao Qi @ 2017-04-13 15:33 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches Pedro Alves <palves@redhat.com> writes: > I'd rather not have to dig into this area too deeply at this point. OK, that is fine by me. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 1/8] watch_command_1: Fix dangling frame access 2017-04-11 23:51 [PATCH 0/8] Fix removing inferiors from within "thread apply" commands Pedro Alves ` (4 preceding siblings ...) 2017-04-11 23:51 ` [PATCH 2/8] Fix follow-fork latent bug Pedro Alves @ 2017-04-11 23:51 ` Pedro Alves 2017-04-13 9:34 ` Yao Qi 2017-04-11 23:56 ` [PATCH 4/8] Improve coverage of the PR threads/13217 regression test Pedro Alves 2017-04-11 23:56 ` [PATCH 5/8] GC inferior.c:init_inferior_list Pedro Alves 7 siblings, 1 reply; 25+ messages in thread From: Pedro Alves @ 2017-04-11 23:51 UTC (permalink / raw) To: gdb-patches While working on some changes to switch_to_thread, I inadvertently make switch_to_thread call reinit_frame_cache more frequently, even when the thread didn't change. This exposed a latent bug in watch_command_1, where we're referencing a frame after creating/inserting breakpoints, which potentially calls reinit_frame_cache if it needs to install breakpoints with a different thread selected. Handle this similarly to how it's already handled in other similar cases. I.e., save any frame-related information we might need before creating a breakpoint. gdb/ChangeLog: yyyy-mm-dd Pedro Alves <palves@redhat.com> * breakpoint.c (watch_command_1): Save watchpoint-frame info before calling create_internal_breakpoint. --- gdb/breakpoint.c | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 3925ec6..b2f6d6e 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -11100,7 +11100,6 @@ watch_command_1 (const char *arg, int accessflag, int from_tty, const struct block *exp_valid_block = NULL, *cond_exp_valid_block = NULL; struct value *val, *mark, *result; int saved_bitpos = 0, saved_bitsize = 0; - struct frame_info *frame; const char *exp_start = NULL; const char *exp_end = NULL; const char *tok, *end_tok; @@ -11278,35 +11277,44 @@ watch_command_1 (const char *arg, int accessflag, int from_tty, if (*tok) error (_("Junk at end of command.")); - frame = block_innermost_frame (exp_valid_block); + frame_info *wp_frame = block_innermost_frame (exp_valid_block); + + /* Save this because create_internal_breakpoint below invalidates + 'wp_frame'. */ + frame_id watchpoint_frame = get_frame_id (wp_frame); /* If the expression is "local", then set up a "watchpoint scope" breakpoint at the point where we've left the scope of the watchpoint expression. Create the scope breakpoint before the watchpoint, so that we will encounter it first in bpstat_stop_status. */ - if (exp_valid_block && frame) + if (exp_valid_block && wp_frame) { - if (frame_id_p (frame_unwind_caller_id (frame))) + struct frame_id caller_frame_id = frame_unwind_caller_id (wp_frame); + + if (frame_id_p (caller_frame_id)) { + gdbarch *caller_arch = frame_unwind_caller_arch (wp_frame); + CORE_ADDR caller_pc = frame_unwind_caller_pc (wp_frame); + scope_breakpoint - = create_internal_breakpoint (frame_unwind_caller_arch (frame), - frame_unwind_caller_pc (frame), + = create_internal_breakpoint (caller_arch, caller_pc, bp_watchpoint_scope, &momentary_breakpoint_ops); + /* create_internal_breakpoint could invalidate WP_FRAME. */ + wp_frame = NULL; + scope_breakpoint->enable_state = bp_enabled; /* Automatically delete the breakpoint when it hits. */ scope_breakpoint->disposition = disp_del; /* Only break in the proper frame (help with recursion). */ - scope_breakpoint->frame_id = frame_unwind_caller_id (frame); + scope_breakpoint->frame_id = caller_frame_id; /* Set the address at which we will stop. */ - scope_breakpoint->loc->gdbarch - = frame_unwind_caller_arch (frame); - scope_breakpoint->loc->requested_address - = frame_unwind_caller_pc (frame); + scope_breakpoint->loc->gdbarch = caller_arch; + scope_breakpoint->loc->requested_address = caller_pc; scope_breakpoint->loc->address = adjust_breakpoint_address (scope_breakpoint->loc->gdbarch, scope_breakpoint->loc->requested_address, @@ -11378,9 +11386,9 @@ watch_command_1 (const char *arg, int accessflag, int from_tty, else b->cond_string = 0; - if (frame) + if (frame_id_p (watchpoint_frame)) { - w->watchpoint_frame = get_frame_id (frame); + w->watchpoint_frame = watchpoint_frame; w->watchpoint_thread = inferior_ptid; } else -- 2.5.5 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/8] watch_command_1: Fix dangling frame access 2017-04-11 23:51 ` [PATCH 1/8] watch_command_1: Fix dangling frame access Pedro Alves @ 2017-04-13 9:34 ` Yao Qi 2017-04-13 15:26 ` Pedro Alves 0 siblings, 1 reply; 25+ messages in thread From: Yao Qi @ 2017-04-13 9:34 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches Pedro Alves <palves@redhat.com> writes: > - if (exp_valid_block && frame) > + if (exp_valid_block && wp_frame) if (exp_valid_block != NULL && wp_frame != NULL) Patch is good to me. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/8] watch_command_1: Fix dangling frame access 2017-04-13 9:34 ` Yao Qi @ 2017-04-13 15:26 ` Pedro Alves 0 siblings, 0 replies; 25+ messages in thread From: Pedro Alves @ 2017-04-13 15:26 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches On 04/13/2017 10:34 AM, Yao Qi wrote: > Pedro Alves <palves@redhat.com> writes: > >> - if (exp_valid_block && frame) >> + if (exp_valid_block && wp_frame) > > if (exp_valid_block != NULL && wp_frame != NULL) > > Patch is good to me. Thanks! I did that change, and pushed patches 1-6, up until the one that add cdtors to struct inferior. I went ahead because last my non-POD + memset detector flagged struct inferior, and also because I saw that Sergio's C++fy environ patch was adding yet another non-POD member to struct inferior. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 4/8] Improve coverage of the PR threads/13217 regression test 2017-04-11 23:51 [PATCH 0/8] Fix removing inferiors from within "thread apply" commands Pedro Alves ` (5 preceding siblings ...) 2017-04-11 23:51 ` [PATCH 1/8] watch_command_1: Fix dangling frame access Pedro Alves @ 2017-04-11 23:56 ` Pedro Alves 2017-04-13 10:09 ` Yao Qi 2017-04-11 23:56 ` [PATCH 5/8] GC inferior.c:init_inferior_list Pedro Alves 7 siblings, 1 reply; 25+ messages in thread From: Pedro Alves @ 2017-04-11 23:56 UTC (permalink / raw) To: gdb-patches - Make sure we end up with no thread selected after the detach. - Test both "thread apply all" and "thread apply $some_threads", for completeness. gdb/ChangeLog: yyyy-mm-dd Pedro Alves <palves@redhat.com> PR threads/13217 * gdb.threads/threadapply.exp (thr_apply_detach): New procedure. (top level): Call it twice, with different thread sets. --- gdb/testsuite/gdb.threads/threadapply.exp | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/gdb/testsuite/gdb.threads/threadapply.exp b/gdb/testsuite/gdb.threads/threadapply.exp index 789b283..959e8b9 100644 --- a/gdb/testsuite/gdb.threads/threadapply.exp +++ b/gdb/testsuite/gdb.threads/threadapply.exp @@ -63,4 +63,33 @@ gdb_test "step" "thread_function.*" "step to the thread_function" gdb_test "up" ".*in main.*" "go up in the stack frame" gdb_test "thread apply all print 1" "Thread ..*\\\$\[0-9]+ = 1.*Thread ..*\\\$\[0-9]+ = 1.*Thread ..*\\\$\[0-9]+ = 1.*Thread ..*\\\$\[0-9]+ = 1.*Thread ..*\\\$\[0-9]+ = 1.*Thread ..*\\\$\[0-9]+ = 1" "run a simple print command on all threads" gdb_test "down" "#0.*thread_function.*" "go down and check selected frame" -gdb_test "thread apply all detach" "Thread .*" + +# Make sure that GDB doesn't crash when the previously selected thread +# exits due to the command run via thread apply. Regression test for +# PR threads/13217. + +proc thr_apply_detach {thread_set} { + with_test_prefix "thread apply $thread_set" { + global binfile + global break_line + + clean_restart ${binfile} + + if ![runto_main] { + fail "can't run to main" + return -1 + } + + gdb_breakpoint "$break_line" + gdb_continue_to_breakpoint "all threads started" + + gdb_test "thread apply $thread_set detach" "Thread .*" + gdb_test "thread" "No thread selected" "switched to no thread selected" + } +} + +# Test both "all" and a thread list, because those are implemented as +# different commands in GDB. +foreach thread_set {"all" "1.1 1.2 1.3"} { + thr_apply_detach $thread_set +} -- 2.5.5 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 4/8] Improve coverage of the PR threads/13217 regression test 2017-04-11 23:56 ` [PATCH 4/8] Improve coverage of the PR threads/13217 regression test Pedro Alves @ 2017-04-13 10:09 ` Yao Qi 0 siblings, 0 replies; 25+ messages in thread From: Yao Qi @ 2017-04-13 10:09 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches Pedro Alves <palves@redhat.com> writes: > gdb/ChangeLog: gdb/testsuite/ChangeLog: > yyyy-mm-dd Pedro Alves <palves@redhat.com> > > PR threads/13217 > * gdb.threads/threadapply.exp (thr_apply_detach): New procedure. > (top level): Call it twice, with different thread sets. Patch is good to me. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 5/8] GC inferior.c:init_inferior_list 2017-04-11 23:51 [PATCH 0/8] Fix removing inferiors from within "thread apply" commands Pedro Alves ` (6 preceding siblings ...) 2017-04-11 23:56 ` [PATCH 4/8] Improve coverage of the PR threads/13217 regression test Pedro Alves @ 2017-04-11 23:56 ` Pedro Alves 2017-04-13 10:10 ` Yao Qi 7 siblings, 1 reply; 25+ messages in thread From: Pedro Alves @ 2017-04-11 23:56 UTC (permalink / raw) To: gdb-patches Not used anywhere. This was actually never used. It came in because I originally created inferior.c by copying thread.c, and doing s/thread/inferior/g, and missed that nothing needs this. :-) gdb/ChangeLog: yyyy-mm-dd Pedro Alves <palves@redhat.com> * inferior.c (init_inferior_list): Delete. * inferior.h (init_inferior_list): Delete. --- gdb/inferior.c | 18 ------------------ gdb/inferior.h | 3 --- 2 files changed, 21 deletions(-) diff --git a/gdb/inferior.c b/gdb/inferior.c index 4ae265e..54e9967 100644 --- a/gdb/inferior.c +++ b/gdb/inferior.c @@ -105,24 +105,6 @@ free_inferior (struct inferior *inf) xfree (inf); } -void -init_inferior_list (void) -{ - struct inferior *inf, *infnext; - - highest_inferior_num = 0; - if (!inferior_list) - return; - - for (inf = inferior_list; inf; inf = infnext) - { - infnext = inf->next; - free_inferior (inf); - } - - inferior_list = NULL; -} - struct inferior * add_inferior_silent (int pid) { diff --git a/gdb/inferior.h b/gdb/inferior.h index 7c0ddf3..29ec7be 100644 --- a/gdb/inferior.h +++ b/gdb/inferior.h @@ -433,9 +433,6 @@ struct inferior DECLARE_REGISTRY (inferior); -/* Create an empty inferior list, or empty the existing one. */ -extern void init_inferior_list (void); - /* Add an inferior to the inferior list, print a message that a new inferior is found, and return the pointer to the new inferior. Caller may use this pointer to initialize the private inferior -- 2.5.5 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 5/8] GC inferior.c:init_inferior_list 2017-04-11 23:56 ` [PATCH 5/8] GC inferior.c:init_inferior_list Pedro Alves @ 2017-04-13 10:10 ` Yao Qi 0 siblings, 0 replies; 25+ messages in thread From: Yao Qi @ 2017-04-13 10:10 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches Pedro Alves <palves@redhat.com> writes: > gdb/ChangeLog: > yyyy-mm-dd Pedro Alves <palves@redhat.com> > > * inferior.c (init_inferior_list): Delete. > * inferior.h (init_inferior_list): Delete. It is obvious. Patch is good to me. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2017-04-19 12:20 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-11 23:51 [PATCH 0/8] Fix removing inferiors from within "thread apply" commands Pedro Alves 2017-04-11 23:51 ` [PATCH 3/8] C++fy thread_apply_all_command Pedro Alves 2017-04-13 10:06 ` Yao Qi 2017-04-11 23:51 ` [PATCH 7/8] Make inferior::detaching a bool, and introduce scoped_restore::release() Pedro Alves 2017-04-11 23:51 ` [PATCH 6/8] Make inferior a class with cdtors, and use new/delete Pedro Alves 2017-04-13 10:24 ` Yao Qi 2017-04-11 23:51 ` [PATCH 8/8] Fix removing inferiors from within "thread apply" commands Pedro Alves 2017-04-13 11:33 ` Yao Qi 2017-04-13 12:22 ` Pedro Alves 2017-04-13 14:49 ` Pedro Alves 2017-04-19 8:23 ` Yao Qi 2017-04-19 12:15 ` Pedro Alves 2017-04-19 12:20 ` Pedro Alves 2017-04-13 15:31 ` Yao Qi 2017-04-11 23:51 ` [PATCH 2/8] Fix follow-fork latent bug Pedro Alves 2017-04-13 9:58 ` Yao Qi 2017-04-13 12:09 ` Pedro Alves 2017-04-13 15:33 ` Yao Qi 2017-04-11 23:51 ` [PATCH 1/8] watch_command_1: Fix dangling frame access Pedro Alves 2017-04-13 9:34 ` Yao Qi 2017-04-13 15:26 ` Pedro Alves 2017-04-11 23:56 ` [PATCH 4/8] Improve coverage of the PR threads/13217 regression test Pedro Alves 2017-04-13 10:09 ` Yao Qi 2017-04-11 23:56 ` [PATCH 5/8] GC inferior.c:init_inferior_list Pedro Alves 2017-04-13 10:10 ` Yao Qi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox