Re: Emacs-diffs Digest, Vol 19, Issue 62

Re: Emacs-diffs Digest, Vol 19, Issue 62

[Kim F. Storm]

Richard Stallman <rms@gnu.org> writes:

> Why is this necessary?

I don't know ... anybody else have a clue?

The reason I added it is that I had SEVERE problems with emacs
misbehaving or crashing with the initial version of this patch.

One "misbehaviour" was that reading mail with Gnus suddenly would
signal not-a-list errors when going from one message to another.  I
guess that Gnus uses quite large lists to keep track of the messages I
have read -- and convert that information to/from text, so I would
guess it uses mapconcat or mapcar in that process, and thus triggers
the new code...

I then added the "dogc" stuff to the Lisp_Save_Value so that GC scans
the allocated memory.  (I'm on GNU/Linux which uses the MARK_STACK GC
method).

After adding the GC awareness, those crashes went away, so I thought I
better add GCPROs for the non-MARK_STACK case as well.

But you are right that it looks superfluous ... 

> 
>     diff -c emacs/src/fns.c:1.367 emacs/src/fns.c:1.368
>     *** emacs/src/fns.c:1.367	Tue Jun 22 13:57:00 2004
>     --- emacs/src/fns.c	Tue Jun 22 14:48:10 2004
>     ***************
>     *** 3020,3026 ****
>     --- 3020,3030 ----
> 	for (i = 1; i < nargs; i += 2)
> 	  args[i] = separator;
> 
>     +   GCPRO1 (*args);
>     +   gcpro1.nvars = nargs;
> 	ret = Fconcat (nargs, args);
>     +   UNGCPRO;
>     + 
> 	SAFE_FREE_LISP (nargs);
> 
> As far as I know, Feval can't be called from Fconcat, so GC
> cannot occur there.
> 
>     +   GCPRO1 (*args);
>     +   gcpro1.nvars = leni;
> 	ret = Flist (leni, args);
>     +   UNGCPRO;
>     + 
> 
> Same issue there.
> 
> 

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Emacs-diffs Digest, Vol 19, Issue 62

[Kim F. Storm]

storm@cua.dk (Kim F. Storm) writes:

> Richard Stallman <rms@gnu.org> writes:
> 
> > Why is this necessary?
> 
> I don't know ... anybody else have a clue?

Actually, thinking more about this, I now realize that the GCPROs are
indeed not needed in the non-MARK_STACK case.

If they were needed, they had to be added around the calls to mapcar1,
but mapcar1 already does the necessary GCPRO of the (partial) args
array.

Since the GCPROs does nothing in the MARK_STACK case, that
also why my "dogc" change DTRT for MARK_STACK-based GC.

I'll remove the GCPROs.

> 
> The reason I added it is that I had SEVERE problems with emacs
> misbehaving or crashing with the initial version of this patch.
> 
> One "misbehaviour" was that reading mail with Gnus suddenly would
> signal not-a-list errors when going from one message to another.  I
> guess that Gnus uses quite large lists to keep track of the messages I
> have read -- and convert that information to/from text, so I would
> guess it uses mapconcat or mapcar in that process, and thus triggers
> the new code...
> 
> I then added the "dogc" stuff to the Lisp_Save_Value so that GC scans
> the allocated memory.  (I'm on GNU/Linux which uses the MARK_STACK GC
> method).
> 
> After adding the GC awareness, those crashes went away, so I thought I
> better add GCPROs for the non-MARK_STACK case as well.
> 
> But you are right that it looks superfluous ... 
> 
> > 
> >     diff -c emacs/src/fns.c:1.367 emacs/src/fns.c:1.368
> >     *** emacs/src/fns.c:1.367	Tue Jun 22 13:57:00 2004
> >     --- emacs/src/fns.c	Tue Jun 22 14:48:10 2004
> >     ***************
> >     *** 3020,3026 ****
> >     --- 3020,3030 ----
> > 	for (i = 1; i < nargs; i += 2)
> > 	  args[i] = separator;
> > 
> >     +   GCPRO1 (*args);
> >     +   gcpro1.nvars = nargs;
> > 	ret = Fconcat (nargs, args);
> >     +   UNGCPRO;
> >     + 
> > 	SAFE_FREE_LISP (nargs);
> > 
> > As far as I know, Feval can't be called from Fconcat, so GC
> > cannot occur there.
> > 
> >     +   GCPRO1 (*args);
> >     +   gcpro1.nvars = leni;
> > 	ret = Flist (leni, args);
> >     +   UNGCPRO;
> >     + 
> > 
> > Same issue there.
> > 
> > 

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Emacs-diffs Digest, Vol 19, Issue 62

[Richard Stallman]

    The reason I added it is that I had SEVERE problems with emacs
    misbehaving or crashing with the initial version of this patch.

    One "misbehaviour" was that reading mail with Gnus suddenly would
    signal not-a-list errors when going from one message to another.  I
    guess that Gnus uses quite large lists to keep track of the messages I
    have read -- and convert that information to/from text, so I would
    guess it uses mapconcat or mapcar in that process, and thus triggers
    the new code...

Perhaps your other changes (adding dogc) were necessary to fix that
bug, but these two specific changes were not.

Re: Emacs-diffs Digest, Vol 19, Issue 62

[Andreas Schwab]

storm@cua.dk (Kim F. Storm) writes:

> Since the GCPROs does nothing in the MARK_STACK case, that
> also why my "dogc" change DTRT for MARK_STACK-based GC.

But is does not compile without MARK_STACK.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Emacs-diffs Digest, Vol 19, Issue 62

[Andreas Schwab]

Andreas Schwab <schwab@suse.de> writes:

> storm@cua.dk (Kim F. Storm) writes:
>
>> Since the GCPROs does nothing in the MARK_STACK case, that
>> also why my "dogc" change DTRT for MARK_STACK-based GC.
>
> But is does not compile without MARK_STACK.

Would you please fix that?

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Emacs-diffs Digest, Vol 19, Issue 62

[Kim F. Storm]

Andreas Schwab <schwab@suse.de> writes:

> Andreas Schwab <schwab@suse.de> writes:
> 
> > storm@cua.dk (Kim F. Storm) writes:
> >
> >> Since the GCPROs does nothing in the MARK_STACK case, that
> >> also why my "dogc" change DTRT for MARK_STACK-based GC.
> >
> > But is does not compile without MARK_STACK.
> 
> Would you please fix that?
> 

Sorry for the delay.

Does this patch work?

*** alloc.c	23 Jun 2004 10:30:52 +0200	1.346
--- alloc.c	12 Jul 2004 11:29:19 +0200	
***************
*** 5033,5038 ****
--- 5033,5039 ----
  	  break;
  
  	case Lisp_Misc_Save_Value:
+ #if GC_MARK_STACK
  	  {
  	    register struct Lisp_Save_Value *ptr = XSAVE_VALUE (obj);
  	    /* If DOGC is set, POINTER is the address of a memory
***************
*** 5045,5050 ****
--- 5046,5052 ----
  		  mark_maybe_object (*p);
  	      }
  	  }
+ #endif
  	  break;
  
  	case Lisp_Misc_Overlay:

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Emacs-diffs Digest, Vol 19, Issue 62

[Andreas Schwab]

storm@cua.dk (Kim F. Storm) writes:

> Does this patch work?
>
> *** alloc.c	23 Jun 2004 10:30:52 +0200	1.346
> --- alloc.c	12 Jul 2004 11:29:19 +0200	
> ***************
> *** 5033,5038 ****
> --- 5033,5039 ----
>   	  break;
>   
>   	case Lisp_Misc_Save_Value:
> + #if GC_MARK_STACK
>   	  {
>   	    register struct Lisp_Save_Value *ptr = XSAVE_VALUE (obj);
>   	    /* If DOGC is set, POINTER is the address of a memory
> ***************
> *** 5045,5050 ****
> --- 5046,5052 ----
>   		  mark_maybe_object (*p);

Do you actually need mark_maybe_object?  Wouldn't mark_object be ok, given
that any use of SAVE_ALLOCA_LISP has to initialize the array anyway?

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Emacs-diffs Digest, Vol 19, Issue 62

[Kim F. Storm]

Andreas Schwab <schwab@suse.de> writes:

> storm@cua.dk (Kim F. Storm) writes:
> 
> > Does this patch work?
> >
> > *** alloc.c	23 Jun 2004 10:30:52 +0200	1.346
> > --- alloc.c	12 Jul 2004 11:29:19 +0200	
> > ***************
> > *** 5033,5038 ****
> > --- 5033,5039 ----
> >   	  break;
> >   
> >   	case Lisp_Misc_Save_Value:
> > + #if GC_MARK_STACK
> >   	  {
> >   	    register struct Lisp_Save_Value *ptr = XSAVE_VALUE (obj);
> >   	    /* If DOGC is set, POINTER is the address of a memory
> > ***************
> > *** 5045,5050 ****
> > --- 5046,5052 ----
> >   		  mark_maybe_object (*p);
> 
> Do you actually need mark_maybe_object?  Wouldn't mark_object be ok, given
> that any use of SAVE_ALLOCA_LISP has to initialize the array anyway?

The array is _not_ initialized, it is filled in gradually, and GC may
occur in-between.  So mark_maybe_object is the right thing to use.

If you look at how traditional GCPRO is done at the places that use
those arrays, you will see that GC gradually modifies the gc data as
the array elements are filled in.

Since the array allocated by SAVE_ALLOCA_LISP is usually allocated on
the stack with alloca, it's elements are usually processed by
mark_maybe_object if MARK_STACK.  So it's the right thing to use
mark_maybe_object also when the array is allocated on the heap.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Emacs-diffs Digest, Vol 19, Issue 62

[Andreas Schwab]

storm@cua.dk (Kim F. Storm) writes:

> The array is _not_ initialized,

It is, all current uses do that (they have to due to non-MARK_STACK
architectures).  And keeping it like this is a good idea anyway.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Emacs-diffs Digest, Vol 19, Issue 62

[Kim F. Storm]

Andreas Schwab <schwab@suse.de> writes:

> storm@cua.dk (Kim F. Storm) writes:
> 
> > The array is _not_ initialized,
> 
> It is, all current uses do that (they have to due to non-MARK_STACK
> architectures).  And keeping it like this is a good idea anyway.

The way I read the code, the arrays are _not_ initialized.

And no, they don't have to be initialized due to non-MARK_STACK
architectures; the code that puts data into those arrays progressively
GCPROs only the initialized part of the array.

And IMO, it is not a good idea to waste cpu cycles initializing
(large) arrays that are subsequently filled with valid data.

But if you can show me the code that INITIALIZES those arrays, I will
believe you (and remove that code :-).

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Emacs-diffs Digest, Vol 19, Issue 62

[Kim F. Storm]

Andreas Schwab <schwab@suse.de> writes:

> storm@cua.dk (Kim F. Storm) writes:
> 
> > The array is _not_ initialized,
> 
> It is, all current uses do that (they have to due to non-MARK_STACK
> architectures).  And keeping it like this is a good idea anyway.
> 

As I said, it is not...

So what about testing the patch I sent you.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Emacs-diffs Digest, Vol 19, Issue 62

[Andreas Schwab]

storm@cua.dk (Kim F. Storm) writes:

> And no, they don't have to be initialized due to non-MARK_STACK
> architectures; the code that puts data into those arrays progressively
> GCPROs only the initialized part of the array.

You are right, I was mixing the uses of leni and nargs in mapconcat.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Emacs-diffs Digest, Vol 19, Issue 62

[Andreas Schwab]

storm@cua.dk (Kim F. Storm) writes:

> So what about testing the patch I sent you.

It's obviously correct, isn't it? :-)

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Emacs-diffs Digest, Vol 19, Issue 62

[Kim F. Storm]

Andreas Schwab <schwab@suse.de> writes:

> storm@cua.dk (Kim F. Storm) writes:
> 
> > So what about testing the patch I sent you.
> 
> It's obviously correct, isn't it? :-)
> 

I assume it is, but OTOH, I don't know how many times I've seen (and
written) obviously correct code that didn't work :-)

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk