diff options
| -rw-r--r-- | gdb/ChangeLog | 38 | ||||
| -rw-r--r-- | gdb/common/refcounted-object.h | 56 | ||||
| -rw-r--r-- | gdb/gdbthread.h | 45 | ||||
| -rw-r--r-- | gdb/inferior.c | 12 | ||||
| -rw-r--r-- | gdb/inferior.h | 35 | ||||
| -rw-r--r-- | gdb/testsuite/ChangeLog | 7 | ||||
| -rw-r--r-- | gdb/testsuite/gdb.threads/threadapply.exp | 135 | ||||
| -rw-r--r-- | gdb/testsuite/lib/gdb.exp | 19 | ||||
| -rw-r--r-- | gdb/thread.c | 82 |
9 files changed, 349 insertions, 80 deletions
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/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/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/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); |