[2/2] Don't delete thread_info if refcount isn't zero
Commit Message
I build GDB with asan, and run test case hook-stop.exp, and threadapply.exp,
I got the following asan error,
Comments
On 04/05/2017 10:15 PM, Yao Qi wrote:
> --- a/gdb/gdbthread.h
> +++ b/gdb/gdbthread.h
> @@ -183,6 +183,27 @@ public:
> explicit thread_info (inferior *inf, ptid_t ptid);
> ~thread_info ();
>
> + bool deletable ()
Could be const:
bool deletable () const
> + {
> + /* If this is the current thread, or there's code out there that
> + relies on it existing (refcount > 0) we can't delete yet. */
> + return (refcount == 0 && !ptid_equal (ptid, inferior_ptid));
> + }
> +
> + /* Increase the refcount. */
> + void inc_refcount ()
> + {
> + gdb_assert (refcount >= 0);
> + refcount++;
> + }
> +
> + /* Decrease the refcount. */
> + void dec_refcount ()
> + {
> + refcount--;
> + gdb_assert (refcount >= 0);
> + }
Nit: It's SO common to call this sort of methods "incref()" and
"decref()" that anything else looks a bit odd to me.
> 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 refcount = 0;
Since this is now private, I think we should give it an "m_" prefix.
> };
>
>
> +/* Set the TP's state as exited. */
> +
> +static void
> +set_thread_exited (struct thread_info *tp, int silent)
Drop "struct" ?
> static void
> do_restore_current_thread_cleanup (void *arg)
> {
> - struct thread_info *tp;
> struct current_thread_cleanup *old = (struct current_thread_cleanup *) arg;
>
> - tp = find_thread_ptid (old->inferior_ptid);
> -
> - /* If the previously selected thread belonged to a process that has
> - in the mean time been deleted (due to normal exit, detach, etc.),
> - then don't revert back to it, but instead simply drop back to no
> - thread selected. */
This comment still makes sense, with a small tweak -- saying "deleted"
has always been a bit misleading here:
/* 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. */
I'll be happy with restoring this comment alongside your new comment.
The patch will LGTM with the nits/comments up to here addressed.
The rest of the review comments below could be addressed separately
(by anyone, not necessarily you), I'm just putting them out as
I thought of them. I do think we should do it to avoid
hard-to-debug corner cases around find_inferior_ptid finding
an unrelated process that reused the old inferior's pid.
> - if (tp
> - && find_inferior_ptid (tp->ptid) != NULL)
> - restore_current_thread (old->inferior_ptid);
> + /* If an entry of thread_info was previously selected, it won't be
> + deleted because we've increased its refcount. The thread represented
> + by this thread_info entry may have already exited (due to normal exit,
> + detach, etc), so the thread_info.state is THREAD_EXITED. */
> + if (old->thread != NULL
> + && find_inferior_ptid (old->thread->ptid) != NULL)
> + restore_current_thread (old->thread->ptid);
Note this was a look up by inferior ptid, not by (stable) inferior number,
so we can genuinely find no inferior, even though the old inferior _object_
will always be around when we get here, given that we mark it non-removable
while the cleanup is installed [1]. Quite similar to increasing the
thread's refcount, really.
So this predicate would probably be better as:
if (old->thread != NULL
&& old->thread != THREAD_EXITED
&& find_inferior_id (old->inf_id)->pid != 0)
We could also store the inferior pointer in "old" directly,
instead of the id, sparing the inferior look up:
if (old->thread != NULL
&& old->thread != THREAD_EXITED
&& old->inf->pid != 0)
[1] - We should probably have 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 thread 2.1 / inferior 2 is selected.
thread apply 1.1 kill-and-remove
Thanks,
Pedro Alves
Pedro Alves <palves@redhat.com> writes:
>> - tp = find_thread_ptid (old->inferior_ptid);
>> -
>> - /* If the previously selected thread belonged to a process that has
>> - in the mean time been deleted (due to normal exit, detach, etc.),
>> - then don't revert back to it, but instead simply drop back to no
>> - thread selected. */
>
> This comment still makes sense, with a small tweak -- saying "deleted"
> has always been a bit misleading here:
>
> /* 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. */
>
> I'll be happy with restoring this comment alongside your new comment.
The comments describe something different from what the code does. With
my patch, in make_cleanup_restore_current_thread, if we can find
thread_info by inferior_ptid, saved it in old->thread and increase the
refcount, so that it won't be deleted, but it can be marked as exited.
Then, in do_restore_current_thread_cleanup,
+ if (old->thread != NULL
+ && find_inferior_ptid (old->thread->ptid) != NULL)
+ restore_current_thread (old->thread->ptid);
else
{
restore_current_thread (null_ptid);
the first line means we previously selected one existing thread, we
still switch to that thread, which may be already marked as exited. It
is nothing wrong as far as I can see.
On 04/07/2017 10:22 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
>
>>> - tp = find_thread_ptid (old->inferior_ptid);
>>> -
>>> - /* If the previously selected thread belonged to a process that has
>>> - in the mean time been deleted (due to normal exit, detach, etc.),
>>> - then don't revert back to it, but instead simply drop back to no
>>> - thread selected. */
>>
>> This comment still makes sense, with a small tweak -- saying "deleted"
>> has always been a bit misleading here:
>>
>> /* 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. */
>>
>> I'll be happy with restoring this comment alongside your new comment.
>
> The comments describe something different from what the code does.
It doesn't. See below.
> With
> my patch, in make_cleanup_restore_current_thread, if we can find
> thread_info by inferior_ptid, saved it in old->thread and increase the
> refcount, so that it won't be deleted, but it can be marked as exited.
> Then, in do_restore_current_thread_cleanup,
>
> + if (old->thread != NULL
> + && find_inferior_ptid (old->thread->ptid) != NULL)
> + restore_current_thread (old->thread->ptid);
> else
> {
> restore_current_thread (null_ptid);
>
> the first line means we previously selected one existing thread, we
> still switch to that thread, which may be already marked as exited. It
> is nothing wrong as far as I can see.
I'm not sure what you mean by "wrong". I'm saying that the old comment
still makes sense. That comment is talking about the
"find_inferior_ptid (old->thread->ptid) != NULL" line, which is a
lookup for an _inferior_ not a thread. Both the inferior and thread_info
object are still around, but the process may have exited/been detached
meanwhile, and consequently the inferior's "pid" field is now
zero. And in that case, we don't restore back the selected thread.
E.g., with:
start
thread apply all detach
#1 - the current thread (thread 1), has its refcount incremented.
#2 - detach gets rid of all threads of the process. The "thread 1"
object is left around, marked exited, because it was not
deletable.
#3 - when we try to restore the previous thread, its
_inferior_ has meanwhile gotten its "pid" field zeroed,
so a look up for an inferior with the pid of the previous
thread's "ptid" (which just looks at the pid) won't
find any inferior. (Unless it finds the wrong inferior
that happened to reuse the pid meanwhile.)
Hopefully that makes the rest of my comments in my previous
message clearer.
Thanks,
Pedro Alves
I'm taking a look at adding the test mentioned below, and managed to
trigger an internal error:
src/gdb/inferior.c:66: internal-error: void set_current_inferior(inferior*): Assertion `inf != NULL' failed.
A problem internal to GDB has been detected,KFAIL: gdb.threads/threadapply.exp: kill_and_remove: try kill-and-remove: thread apply 1.1 kill-and-remove (GDB internal error)
I'll poke a bit more at it.
Thanks,
Pedro Alves
On 04/06/2017 11:18 AM, Pedro Alves wrote:
> The rest of the review comments below could be addressed separately
> (by anyone, not necessarily you), I'm just putting them out as
> I thought of them. I do think we should do it to avoid
> hard-to-debug corner cases around find_inferior_ptid finding
> an unrelated process that reused the old inferior's pid.
>
>> - if (tp
>> - && find_inferior_ptid (tp->ptid) != NULL)
>> - restore_current_thread (old->inferior_ptid);
>> + /* If an entry of thread_info was previously selected, it won't be
>> + deleted because we've increased its refcount. The thread represented
>> + by this thread_info entry may have already exited (due to normal exit,
>> + detach, etc), so the thread_info.state is THREAD_EXITED. */
>> + if (old->thread != NULL
>> + && find_inferior_ptid (old->thread->ptid) != NULL)
>> + restore_current_thread (old->thread->ptid);
>
> Note this was a look up by inferior ptid, not by (stable) inferior number,
> so we can genuinely find no inferior, even though the old inferior _object_
> will always be around when we get here, given that we mark it non-removable
> while the cleanup is installed [1]. Quite similar to increasing the
> thread's refcount, really.
>
> So this predicate would probably be better as:
>
> if (old->thread != NULL
> && old->thread != THREAD_EXITED
> && find_inferior_id (old->inf_id)->pid != 0)
>
> We could also store the inferior pointer in "old" directly,
> instead of the id, sparing the inferior look up:
>
> if (old->thread != NULL
> && old->thread != THREAD_EXITED
> && old->inf->pid != 0)
>
>
> [1] - We should probably have 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 thread 2.1 / inferior 2 is selected.
> thread apply 1.1 kill-and-remove
=================================================================^M
^[[1m^[[31m==2291==ERROR: AddressSanitizer: heap-use-after-free on address 0x6160000999c4 at pc 0x000000826022 bp 0x7ffd28a8ff70 sp 0x7ffd28a8ff60^M
^[[1m^[[0m^[[1m^[[34mREAD of size 4 at 0x6160000999c4 thread T0^[[1m^[[0m^M
#0 0x826021 in release_stop_context_cleanup ../../binutils-gdb/gdb/infrun.c:8203^M
#1 0x72798a in do_my_cleanups ../../binutils-gdb/gdb/common/cleanups.c:154^M
#2 0x727a32 in do_cleanups(cleanup*) ../../binutils-gdb/gdb/common/cleanups.c:176^M
#3 0x826895 in normal_stop() ../../binutils-gdb/gdb/infrun.c:8381^M
#4 0x815208 in fetch_inferior_event(void*) ../../binutils-gdb/gdb/infrun.c:4011^M
#5 0x868aca in inferior_event_handler(inferior_event_type, void*) ../../binutils-gdb/gdb/inf-loop.c:44^M
....
^[[1m^[[32m0x6160000999c4 is located 68 bytes inside of 568-byte region [0x616000099980,0x616000099bb8)^M
^[[1m^[[0m^[[1m^[[35mfreed by thread T0 here:^[[1m^[[0m^M
#0 0x7fb0bc1312ca in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x982ca)^M
#1 0xb8c62f in xfree(void*) ../../binutils-gdb/gdb/common/common-utils.c:100^M
#2 0x83df67 in free_thread ../../binutils-gdb/gdb/thread.c:207^M
#3 0x83dfd2 in init_thread_list() ../../binutils-gdb/gdb/thread.c:223^M
#4 0x805494 in kill_command ../../binutils-gdb/gdb/infcmd.c:2595^M
....
Detaching from program: /home/yao.qi/SourceCode/gnu/build-with-asan/gdb/testsuite/outputs/gdb.threads/threadapply/threadapply, process 2399^M
=================================================================^M
^[[1m^[[31m==2387==ERROR: AddressSanitizer: heap-use-after-free on address 0x6160000a98c0 at pc 0x00000083fd28 bp 0x7ffd401c3110 sp 0x7ffd401c3100^M
^[[1m^[[0m^[[1m^[[34mREAD of size 4 at 0x6160000a98c0 thread T0^[[1m^[[0m^M
#0 0x83fd27 in thread_alive ../../binutils-gdb/gdb/thread.c:741^M
#1 0x844277 in thread_apply_all_command ../../binutils-gdb/gdb/thread.c:1804^M
....
^M
^[[1m^[[32m0x6160000a98c0 is located 64 bytes inside of 568-byte region [0x6160000a9880,0x6160000a9ab8)^M
^[[1m^[[0m^[[1m^[[35mfreed by thread T0 here:^[[1m^[[0m^M
#0 0x7f59a7e322ca in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x982ca)^M
#1 0xb8c62f in xfree(void*) ../../binutils-gdb/gdb/common/common-utils.c:100^M
#2 0x83df67 in free_thread ../../binutils-gdb/gdb/thread.c:207^M
#3 0x83dfd2 in init_thread_list() ../../binutils-gdb/gdb/thread.c:223^M
This patch fixes the issue by deleting thread_info object if it is
deletable, otherwise, mark it as exited (by set_thread_exited).
Function set_thread_exited is shared from delete_thread_1. This patch
also moves field "refcount" to private and methods inc_refcount and
dec_refcount. Additionally, we stop using "ptid_t" in
"struct current_thread_cleanup" to reference threads, instead we use
"thread_info" directly. Due to this change, we don't need
restore_current_thread_ptid_changed anymore.
gdb:
2017-04-05 Yao Qi <yao.qi@linaro.org>
PR gdb/19942
* gdbthread.h (thread_info::deletable): New method.
(thread_info::inc_refcount): New method.
(thread_info::dec_refcount): New method.
(thread_info::refcount): Move it to private.
* infrun.c (save_stop_context): Call inc_refcount.
(release_stop_context_cleanup): Likewise.
* thread.c (set_thread_exited): New function.
(init_thread_list): Delete "tp" only it is deletable, otherwise
call set_thread_exited.
(delete_thread_1): Call set_thread_exited.
(current_thread_cleanup) <inferior_pid>: Remove.
<thread>: New field.
(restore_current_thread_ptid_changed): Removed.
(do_restore_current_thread_cleanup): Adjust.
(restore_current_thread_cleanup_dtor): Don't call
find_thread_ptid.
(set_thread_refcount): Use dec_refcount.
(make_cleanup_restore_current_thread): Adjust.
(thread_apply_all_command): Call inc_refcount.
(_initialize_thread): Don't call
observer_attach_thread_ptid_changed.
---
gdb/gdbthread.h | 33 ++++++++++++++++---
gdb/infrun.c | 4 +--
gdb/thread.c | 98 +++++++++++++++++++++++++--------------------------------
3 files changed, 73 insertions(+), 62 deletions(-)
@@ -183,6 +183,27 @@ public:
explicit thread_info (inferior *inf, ptid_t ptid);
~thread_info ();
+ bool deletable ()
+ {
+ /* If this is the current thread, or there's code out there that
+ relies on it existing (refcount > 0) we can't delete yet. */
+ return (refcount == 0 && !ptid_equal (ptid, inferior_ptid));
+ }
+
+ /* Increase the refcount. */
+ void inc_refcount ()
+ {
+ gdb_assert (refcount >= 0);
+ refcount++;
+ }
+
+ /* Decrease the refcount. */
+ void dec_refcount ()
+ {
+ refcount--;
+ gdb_assert (refcount >= 0);
+ }
+
struct thread_info *next = NULL;
ptid_t ptid; /* "Actual process id";
In fact, this may be overloaded with
@@ -254,11 +275,6 @@ public:
but STATE will still be THREAD_RUNNING. */
enum thread_state state = THREAD_STOPPED;
- /* 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 refcount = 0;
-
/* State of GDB control of inferior thread execution.
See `struct thread_control_state'. */
thread_control_state control {};
@@ -346,6 +362,13 @@ 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 refcount = 0;
};
/* Create an empty thread list, or empty the existing one. */
@@ -8165,7 +8165,7 @@ save_stop_context (void)
/* Take a strong reference so that the thread can't be deleted
yet. */
sc->thread = inferior_thread ();
- sc->thread->refcount++;
+ sc->thread->inc_refcount();
}
else
sc->thread = NULL;
@@ -8182,7 +8182,7 @@ release_stop_context_cleanup (void *arg)
struct stop_context *sc = (struct stop_context *) arg;
if (sc->thread != NULL)
- sc->thread->refcount--;
+ sc->thread->dec_refcount ();
xfree (sc);
}
@@ -192,6 +192,27 @@ clear_thread_inferior_resources (struct thread_info *tp)
thread_cancel_execution_command (tp);
}
+/* Set the TP's state as exited. */
+
+static void
+set_thread_exited (struct thread_info *tp, int silent)
+{
+ /* Dead threads don't need to step-over. Remove from queue. */
+ if (tp->step_over_next != NULL)
+ thread_step_over_chain_remove (tp);
+
+ if (tp->state != THREAD_EXITED)
+ {
+ observer_notify_thread_exit (tp, silent);
+
+ /* Tag it as exited. */
+ tp->state = THREAD_EXITED;
+
+ /* Clear breakpoints, etc. associated with this thread. */
+ clear_thread_inferior_resources (tp);
+ }
+}
+
void
init_thread_list (void)
{
@@ -205,7 +226,10 @@ init_thread_list (void)
for (tp = thread_list; tp; tp = tpnext)
{
tpnext = tp->next;
- delete tp;
+ if (tp->deletable ())
+ delete tp;
+ else
+ set_thread_exited (tp, 1);
}
thread_list = NULL;
@@ -430,25 +454,9 @@ delete_thread_1 (ptid_t ptid, int silent)
if (!tp)
return;
- /* Dead threads don't need to step-over. Remove from queue. */
- if (tp->step_over_next != NULL)
- thread_step_over_chain_remove (tp);
+ set_thread_exited (tp, silent);
- if (tp->state != THREAD_EXITED)
- {
- observer_notify_thread_exit (tp, silent);
-
- /* Tag it as exited. */
- tp->state = THREAD_EXITED;
-
- /* Clear breakpoints, etc. associated with this thread. */
- clear_thread_inferior_resources (tp);
- }
-
- /* If this is the current thread, or there's code out there that
- relies on it existing (refcount > 0) we can't delete yet. */
- if (tp->refcount > 0
- || ptid_equal (tp->ptid, inferior_ptid))
+ if (!tp->deletable ())
{
/* Will be really deleted some other time. */
return;
@@ -1546,7 +1554,7 @@ struct current_thread_cleanup
'current_thread_cleanup_chain' below. */
struct current_thread_cleanup *next;
- ptid_t inferior_ptid;
+ thread_info *thread;
struct frame_id selected_frame_id;
int selected_frame_level;
int was_stopped;
@@ -1561,37 +1569,18 @@ struct current_thread_cleanup
succeeds. */
static struct current_thread_cleanup *current_thread_cleanup_chain;
-/* A thread_ptid_changed observer. Update all currently installed
- current_thread_cleanup cleanups that want to switch back to
- OLD_PTID to switch back to NEW_PTID instead. */
-
-static void
-restore_current_thread_ptid_changed (ptid_t old_ptid, ptid_t new_ptid)
-{
- struct current_thread_cleanup *it;
-
- for (it = current_thread_cleanup_chain; it != NULL; it = it->next)
- {
- if (ptid_equal (it->inferior_ptid, old_ptid))
- it->inferior_ptid = new_ptid;
- }
-}
-
static void
do_restore_current_thread_cleanup (void *arg)
{
- struct thread_info *tp;
struct current_thread_cleanup *old = (struct current_thread_cleanup *) arg;
- tp = find_thread_ptid (old->inferior_ptid);
-
- /* If the previously selected thread belonged to a process that has
- in the mean time been deleted (due to normal exit, detach, etc.),
- then don't revert back to it, but instead simply drop back to no
- thread selected. */
- if (tp
- && find_inferior_ptid (tp->ptid) != NULL)
- restore_current_thread (old->inferior_ptid);
+ /* If an entry of thread_info was previously selected, it won't be
+ deleted because we've increased its refcount. The thread represented
+ by this thread_info entry may have already exited (due to normal exit,
+ detach, etc), so the thread_info.state is THREAD_EXITED. */
+ if (old->thread != NULL
+ && find_inferior_ptid (old->thread->ptid) != NULL)
+ restore_current_thread (old->thread->ptid);
else
{
restore_current_thread (null_ptid);
@@ -1619,9 +1608,9 @@ restore_current_thread_cleanup_dtor (void *arg)
current_thread_cleanup_chain = current_thread_cleanup_chain->next;
- tp = find_thread_ptid (old->inferior_ptid);
- if (tp)
- tp->refcount--;
+ if (old->thread != NULL)
+ old->thread->dec_refcount ();
+
inf = find_inferior_id (old->inf_id);
if (inf != NULL)
inf->removable = old->was_removable;
@@ -1638,7 +1627,7 @@ set_thread_refcount (void *data)
= (struct thread_array_cleanup *) data;
for (k = 0; k != ta_cleanup->count; k++)
- ta_cleanup->tp_array[k]->refcount--;
+ ta_cleanup->tp_array[k]->dec_refcount ();
}
struct cleanup *
@@ -1646,7 +1635,7 @@ make_cleanup_restore_current_thread (void)
{
struct current_thread_cleanup *old = XNEW (struct current_thread_cleanup);
- old->inferior_ptid = inferior_ptid;
+ old->thread = NULL;
old->inf_id = current_inferior ()->num;
old->was_removable = current_inferior ()->removable;
@@ -1679,7 +1668,8 @@ make_cleanup_restore_current_thread (void)
struct thread_info *tp = find_thread_ptid (inferior_ptid);
if (tp)
- tp->refcount++;
+ tp->inc_refcount ();
+ old->thread = tp;
}
current_inferior ()->removable = 0;
@@ -1796,7 +1786,7 @@ thread_apply_all_command (char *cmd, int from_tty)
ALL_NON_EXITED_THREADS (tp)
{
tp_array[i] = tp;
- tp->refcount++;
+ tp->inc_refcount ();
i++;
}
/* Because we skipped exited threads, we may end up with fewer
@@ -2286,6 +2276,4 @@ Show printing of thread events (such as thread start and exit)."), NULL,
create_internalvar_type_lazy ("_thread", &thread_funcs, NULL);
create_internalvar_type_lazy ("_gthread", >hread_funcs, NULL);
-
- observer_attach_thread_ptid_changed (restore_current_thread_ptid_changed);
}