This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 2/2] Don't delete thread_info if refcount isn't zero
- From: Pedro Alves <palves at redhat dot com>
- To: Yao Qi <qiyaoltc at gmail dot com>, gdb-patches at sourceware dot org
- Date: Mon, 10 Apr 2017 16:57:27 +0100
- Subject: Re: [PATCH 2/2] Don't delete thread_info if refcount isn't zero
- Authentication-results: sourceware.org; auth=none
- Authentication-results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com
- Authentication-results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=palves at redhat dot com
- Dkim-filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 6B7C864D89
- Dmarc-filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 6B7C864D89
- References: <1f525e52-f547-63ac-0a31-e92686c9caf8@redhat.com> <1491426942-6306-1-git-send-email-yao.qi@linaro.org> <1491426942-6306-3-git-send-email-yao.qi@linaro.org> <9c8ed015-2eb0-9b5c-affc-b5ba59179a7f@redhat.com>
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