Re: [Emacs-diffs] master 0a5b6e2: Fix aborts in GC under GC_CHECK_MARKED_OBJECTS

Re: [Emacs-diffs] master 0a5b6e2: Fix aborts in GC under GC_CHECK_MARKED_OBJECTS

[Stefan Monnier]

>     for thread objects.  They are marked via the all_threads list, and
>     therefore don't need to be inserted into the red-black tree, so
>     mem_find will never find them.

Hmm... when are they removed from all_threads?
If they're removed from all_threads when the thread terminates, then
there can still be references to the thread object stored elsewhere in
the heap.


        Stefan

Re: [Emacs-diffs] master 0a5b6e2: Fix aborts in GC under GC_CHECK_MARKED_OBJECTS

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: Eli Zaretskii <eliz@gnu.org>
> Date: Wed, 21 Dec 2016 15:29:12 -0500
> 
> >     for thread objects.  They are marked via the all_threads list, and
> >     therefore don't need to be inserted into the red-black tree, so
> >     mem_find will never find them.
> 
> Hmm... when are they removed from all_threads?

When the thread exits, see run_thread.

> If they're removed from all_threads when the thread terminates, then
> there can still be references to the thread object stored elsewhere in
> the heap.

They are all unlinked in run_thread, when the thread exits, AFAICT.
Or maybe I don't understand what you mean by "references".

Re: [Emacs-diffs] master 0a5b6e2: Fix aborts in GC under GC_CHECK_MARKED_OBJECTS

[Stefan Monnier]

>> If they're removed from all_threads when the thread terminates, then
>> there can still be references to the thread object stored elsewhere in
>> the heap.
> They are all unlinked in run_thread, when the thread exits, AFAICT.
> Or maybe I don't understand what you mean by "references".

AFAIK `make-thread` returns a thread object, which you can store
anywhere you like.  So even if you remove the thread object from
all_threads, it may still be referenced from all kinds of other place.


        Stefan

Re: [Emacs-diffs] master 0a5b6e2: Fix aborts in GC under GC_CHECK_MARKED_OBJECTS

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Wed, 21 Dec 2016 17:52:47 -0500
> 
> AFAIK `make-thread` returns a thread object, which you can store
> anywhere you like.  So even if you remove the thread object from
> all_threads, it may still be referenced from all kinds of other place.

And why is that a problem, both in general and wrt to the change I
installed that only matters if GC_CHECK_MARKED_OBJECTS is used?

Re: [Emacs-diffs] master 0a5b6e2: Fix aborts in GC under GC_CHECK_MARKED_OBJECTS

[Stefan Monnier]

>> AFAIK `make-thread` returns a thread object, which you can store
>> anywhere you like.  So even if you remove the thread object from
>> all_threads, it may still be referenced from all kinds of other place.
> And why is that a problem, both in general and wrt to the change I
> installed that only matters if GC_CHECK_MARKED_OBJECTS is used?

That there might be on the stack a Lisp_Object variable which holds
a Lisp_Vectorlike references which is really a reference to a thread
object, so the conservative GC would need to be able to recognize them,
AFAICT.  So your code doesn't make things worse, but I think that the
problem detected by GC_CHECK_MARKED_OBJECTS needs to be fixed by adding
thread objects to the red-black tree.


        Stefan

Re: [Emacs-diffs] master 0a5b6e2: Fix aborts in GC under GC_CHECK_MARKED_OBJECTS

[Daniel Colascione]

On Thu, Dec 22 2016, Eli Zaretskii wrote:
>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> Cc: emacs-devel@gnu.org
>> Date: Wed, 21 Dec 2016 17:52:47 -0500
>> 
>> AFAIK `make-thread` returns a thread object, which you can store
>> anywhere you like.  So even if you remove the thread object from
>> all_threads, it may still be referenced from all kinds of other place.
>
> And why is that a problem, both in general and wrt to the change I
> installed that only matters if GC_CHECK_MARKED_OBJECTS is used?

It's a problem because if you don't add thread objects to the tree,
conservative GC doesn't keep them alive and subsequent access to thread
objects can access freed memory.  It's perfectly legitimate to hold a
reference to a dead thread --- just like it's perfectly legitimate to
hold a reference to a killed buffer.

Re: [Emacs-diffs] master 0a5b6e2: Fix aborts in GC under GC_CHECK_MARKED_OBJECTS

[Daniel Colascione]

On Wed, Dec 21 2016, Eli Zaretskii wrote:
> branch: master
> commit 0a5b6e28f91ff46231a768737170e39172297257
> Author: Eli Zaretskii <eliz@gnu.org>
> Commit: Eli Zaretskii <eliz@gnu.org>
>
>     Fix aborts in GC under GC_CHECK_MARKED_OBJECTS
>     
>     * src/alloc.c (mark_object) [GC_CHECK_MARKED_OBJECTS]: Don't abort
>     for thread objects.  They are marked via the all_threads list, and
>     therefore don't need to be inserted into the red-black tree, so
>     mem_find will never find them.  Reported by Daniel Colascione
>     <dancol@dancol.org> in
>     http://lists.gnu.org/archive/html/emacs-devel/2016-12/msg00817.html.
> ---
>  src/alloc.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/src/alloc.c b/src/alloc.c
> index f2b7682..e979f36 100644
> --- a/src/alloc.c
> +++ b/src/alloc.c
> @@ -6406,7 +6406,7 @@ mark_object (Lisp_Object arg)
>  
>  #ifdef GC_CHECK_MARKED_OBJECTS
>  	m = mem_find (po);
> -	if (m == MEM_NIL && !SUBRP (obj))
> +	if (m == MEM_NIL && !SUBRP (obj) && !THREADP (obj))

This code is incorrect.  The only special case is po == &primary_thread.
As I mentioned in another message, all _other_ thread objects live in
dynamic storage and need the usual treatment of thread objects.
The reason we have SUBRP in the clause above is that *all* subr objects
live in static, not dynamic, storage.

Re: [Emacs-diffs] master 0a5b6e2: Fix aborts in GC under GC_CHECK_MARKED_OBJECTS

[Eli Zaretskii]

> From: Daniel Colascione <dancol@dancol.org>
> Cc: Eli Zaretskii <eliz@gnu.org>
> Date: Wed, 21 Dec 2016 20:22:40 -0800
> 
> >  #ifdef GC_CHECK_MARKED_OBJECTS
> >  	m = mem_find (po);
> > -	if (m == MEM_NIL && !SUBRP (obj))
> > +	if (m == MEM_NIL && !SUBRP (obj) && !THREADP (obj))
> 
> This code is incorrect.  The only special case is po == &primary_thread.
> As I mentioned in another message, all _other_ thread objects live in
> dynamic storage and need the usual treatment of thread objects.

Thanks, fixed.

Re: [Emacs-diffs] master 0a5b6e2: Fix aborts in GC under GC_CHECK_MARKED_OBJECTS

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Wed, 21 Dec 2016 22:45:43 -0500
> 
> That there might be on the stack a Lisp_Object variable which holds
> a Lisp_Vectorlike references which is really a reference to a thread
> object, so the conservative GC would need to be able to recognize them,
> AFAICT.  So your code doesn't make things worse, but I think that the
> problem detected by GC_CHECK_MARKED_OBJECTS needs to be fixed by adding
> thread objects to the red-black tree.

I think they already are in the tree: the thread object is a
Lisp_Vectorlike allocated from some block in vector_blocks, and all
the blocks there are already inserted into the tree.  Other than that,
the few members of thread_state structure are just "normal" Lisp
objects, so they are also in the tree.