all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* Re: [Emacs-diffs] master 0a5b6e2: Fix aborts in GC under GC_CHECK_MARKED_OBJECTS
       [not found] ` <20161221201813.A13CA2201BA@vcs.savannah.gnu.org>
@ 2016-12-21 20:29   ` Stefan Monnier
  2016-12-21 20:40     ` Eli Zaretskii
  2016-12-22  4:22   ` Daniel Colascione
  1 sibling, 1 reply; 9+ messages in thread
From: Stefan Monnier @ 2016-12-21 20:29 UTC (permalink / raw)
  To: emacs-devel; +Cc: Eli Zaretskii

>     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



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Emacs-diffs] master 0a5b6e2: Fix aborts in GC under GC_CHECK_MARKED_OBJECTS
  2016-12-21 20:29   ` [Emacs-diffs] master 0a5b6e2: Fix aborts in GC under GC_CHECK_MARKED_OBJECTS Stefan Monnier
@ 2016-12-21 20:40     ` Eli Zaretskii
  2016-12-21 22:52       ` Stefan Monnier
  0 siblings, 1 reply; 9+ messages in thread
From: Eli Zaretskii @ 2016-12-21 20:40 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

> 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".



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Emacs-diffs] master 0a5b6e2: Fix aborts in GC under GC_CHECK_MARKED_OBJECTS
  2016-12-21 20:40     ` Eli Zaretskii
@ 2016-12-21 22:52       ` Stefan Monnier
  2016-12-22  3:35         ` Eli Zaretskii
  0 siblings, 1 reply; 9+ messages in thread
From: Stefan Monnier @ 2016-12-21 22:52 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: emacs-devel

>> 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



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Emacs-diffs] master 0a5b6e2: Fix aborts in GC under GC_CHECK_MARKED_OBJECTS
  2016-12-21 22:52       ` Stefan Monnier
@ 2016-12-22  3:35         ` Eli Zaretskii
  2016-12-22  3:45           ` Stefan Monnier
  2016-12-22  4:18           ` Daniel Colascione
  0 siblings, 2 replies; 9+ messages in thread
From: Eli Zaretskii @ 2016-12-22  3:35 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

> 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?



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Emacs-diffs] master 0a5b6e2: Fix aborts in GC under GC_CHECK_MARKED_OBJECTS
  2016-12-22  3:35         ` Eli Zaretskii
@ 2016-12-22  3:45           ` Stefan Monnier
  2016-12-22 16:17             ` Eli Zaretskii
  2016-12-22  4:18           ` Daniel Colascione
  1 sibling, 1 reply; 9+ messages in thread
From: Stefan Monnier @ 2016-12-22  3:45 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: emacs-devel

>> 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



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Emacs-diffs] master 0a5b6e2: Fix aborts in GC under GC_CHECK_MARKED_OBJECTS
  2016-12-22  3:35         ` Eli Zaretskii
  2016-12-22  3:45           ` Stefan Monnier
@ 2016-12-22  4:18           ` Daniel Colascione
  1 sibling, 0 replies; 9+ messages in thread
From: Daniel Colascione @ 2016-12-22  4:18 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: Stefan Monnier, emacs-devel

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.



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Emacs-diffs] master 0a5b6e2: Fix aborts in GC under GC_CHECK_MARKED_OBJECTS
       [not found] ` <20161221201813.A13CA2201BA@vcs.savannah.gnu.org>
  2016-12-21 20:29   ` [Emacs-diffs] master 0a5b6e2: Fix aborts in GC under GC_CHECK_MARKED_OBJECTS Stefan Monnier
@ 2016-12-22  4:22   ` Daniel Colascione
  2016-12-22 16:14     ` Eli Zaretskii
  1 sibling, 1 reply; 9+ messages in thread
From: Daniel Colascione @ 2016-12-22  4:22 UTC (permalink / raw)
  To: emacs-devel; +Cc: Eli Zaretskii

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.



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Emacs-diffs] master 0a5b6e2: Fix aborts in GC under GC_CHECK_MARKED_OBJECTS
  2016-12-22  4:22   ` Daniel Colascione
@ 2016-12-22 16:14     ` Eli Zaretskii
  0 siblings, 0 replies; 9+ messages in thread
From: Eli Zaretskii @ 2016-12-22 16:14 UTC (permalink / raw)
  To: Daniel Colascione; +Cc: emacs-devel

> 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.



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Emacs-diffs] master 0a5b6e2: Fix aborts in GC under GC_CHECK_MARKED_OBJECTS
  2016-12-22  3:45           ` Stefan Monnier
@ 2016-12-22 16:17             ` Eli Zaretskii
  0 siblings, 0 replies; 9+ messages in thread
From: Eli Zaretskii @ 2016-12-22 16:17 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

> 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.



^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2016-12-22 16:17 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20161221201813.3130.28327@vcs.savannah.gnu.org>
     [not found] ` <20161221201813.A13CA2201BA@vcs.savannah.gnu.org>
2016-12-21 20:29   ` [Emacs-diffs] master 0a5b6e2: Fix aborts in GC under GC_CHECK_MARKED_OBJECTS Stefan Monnier
2016-12-21 20:40     ` Eli Zaretskii
2016-12-21 22:52       ` Stefan Monnier
2016-12-22  3:35         ` Eli Zaretskii
2016-12-22  3:45           ` Stefan Monnier
2016-12-22 16:17             ` Eli Zaretskii
2016-12-22  4:18           ` Daniel Colascione
2016-12-22  4:22   ` Daniel Colascione
2016-12-22 16:14     ` Eli Zaretskii

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.