Re: /srv/bzr/emacs/trunk r103781: * src/bytecode.c (Fbyte_code): CAR and CDR can GC.

Re: /srv/bzr/emacs/trunk r103781: * src/bytecode.c (Fbyte_code): CAR and CDR can GC.

[Daniel Colascione]

On 3/30/2011 11:04 AM, Stefan Monnier wrote:
> ------------------------------------------------------------
> revno: 103781
> committer: Stefan Monnier<monnier@iro.umontreal.ca>
> branch nick: trunk
> timestamp: Wed 2011-03-30 14:04:11 -0400
> message:
>    * src/bytecode.c (Fbyte_code): CAR and CDR can GC.


Wait, what?  How?  wrong_type_argument never returns.  If we run into a 
problem, we'll throw up to some higher context and never hit the 
AFTER_POTENTIAL_GC case. BEFORE_POTENTIAL_GC (if it's not a noop) sets 
the stack top field to point to the top of the stack, but because we 
never return from wrong_type_argument, we don't care whether the stack 
is lost.  v1 might be on the stack, so the only thing keeping it alive 
might be stack.top.

Now, Fsignal *does* run signal-hook-function, which of course might GC. 
  But that's done using call2, which will keep error_symbol and data 
alive even if we do GC, so a GC in this case shouldn't actually cause 
any problems.  Likewise for the debugging hooks.  So GC shouldn't 
actually cause a problem, IIUC.

What am I missing?

=== modified file 'src/ChangeLog'
--- a/src/ChangeLog	2011-03-30 13:35:37 +0000
+++ b/src/ChangeLog	2011-03-30 18:04:11 +0000
@@ -1,3 +1,7 @@
+2011-03-30  Stefan Monnier  <monnier@iro.umontreal.ca>
+
+	* bytecode.c (Fbyte_code): CAR and CDR can GC.
+
  2011-03-30  Zachary Kanfer  <zkanfer@gmail.com>  (tiny change)

  	* keyboard.c (Fexecute_extended_command): Do log the "suggest key

=== modified file 'src/bytecode.c'
--- a/src/bytecode.c	2011-03-17 02:18:00 +0000
+++ b/src/bytecode.c	2011-03-30 18:04:11 +0000
@@ -554,7 +554,16 @@
  	  {
  	    Lisp_Object v1;
  	    v1 = TOP;
-	    TOP = CAR (v1);
+	    if (CONSP (v1))
+	      TOP = XCAR (v1);
+	    else if (NILP (v1))
+	      TOP = Qnil;
+	    else
+	      {
+		BEFORE_POTENTIAL_GC ();
+		wrong_type_argument (Qlistp, v1);
+		AFTER_POTENTIAL_GC ();
+	      }
  	    break;
  	  }

@@ -580,7 +589,17 @@
  	  {
  	    Lisp_Object v1;
  	    v1 = TOP;
-	    TOP = CDR (v1);
+	    if (CONSP (v1))
+	      TOP = XCDR (v1);
+	    else if (NILP (v1))
+	      TOP = Qnil;
+	    else
+	      {
+		BEFORE_POTENTIAL_GC ();
+		wrong_type_argument (Qlistp, v1);
+		AFTER_POTENTIAL_GC ();
+	      }
+	    break;
  	    break;
  	  }

@@ -911,13 +930,13 @@
  	    v1 = POP;
  	    v2 = TOP;
  	    CHECK_NUMBER (v2);
-	    AFTER_POTENTIAL_GC ();
  	    op = XINT (v2);
  	    immediate_quit = 1;
  	    while (--op >= 0 && CONSP (v1))
  	      v1 = XCDR (v1);
  	    immediate_quit = 0;
  	    TOP = CAR (v1);
+	    AFTER_POTENTIAL_GC ();
  	    break;
  	  }

Re: /srv/bzr/emacs/trunk r103781: * src/bytecode.c (Fbyte_code): CAR and CDR can GC.

[Stefan Monnier]

> Wait, what?  How?  wrong_type_argument never returns.  If we run
> into a problem, we'll throw up to some higher context and never hit the
> AFTER_POTENTIAL_GC case.

That's true the AFTER_POTENTIAL_GC will never be executed.

> BEFORE_POTENTIAL_GC (if it's not a noop) sets the
> stack top field to point to the top of the stack, but because we never
> return from wrong_type_argument, we don't care whether the stack is lost.

But even if we never return to this code, the GC will still want to
trace through the byte_code_stacks and will need the `top' field
for that.

> What am I missing?

Without the BEFORE_POTENTIAL_GC you hit the following check in
mark_byte_stack:

      /* If STACK->top is null here, this means there's an opcode in
	 Fbyte_code that wasn't expected to GC, but did.  To find out
	 which opcode this is, record the value of `stack', and walk
	 up the stack in a debugger, stopping in frames of Fbyte_code.
	 The culprit is found in the frame of Fbyte_code where the
	 address of its local variable `stack' is equal to the
	 recorded value of `stack' here.  */
      eassert (stack->top);

You could argue that the check is conservative, but it's actually
a useful check and the BEFORE_POTENTIAL_GC I added is very cheap (`car'
and `cdr' very rarely signal errors and the cost of such signalling
dwarfs the cost of BEFORE_POTENTIAL_GC).  You could also argue that instead
of BEFORE_POTENTIAL_GC we should simply set .top to .bottom so that the
elements on the stack can be GC'd.  If so, I'd agree and would encourage
you to make a patch for it.
Oh, and BTW, currently BEFORE_POTENTIAL_GC and AFTER_POTENTIAL_GC are
usually no-ops because the byte-code stack is stack-allocated and the
stack is already conservatively scanned on most systems.  But we could
significantly speed up Elisp function calls if we manage the byte-code
stacks differently (bigger chunks rather than one per function
invacation), so BEFORE/AFTER_POTENTIAL_GC may lose their no-op status in
the future.


        Stefan