Re: master 5252c45: Initialise unread buffer

Re: master 5252c45: Initialise unread buffer

[Lars Ingebrigtsen]

Mattias Engdegård <mattiase@savannah.gnu.org> writes:

>     The reader has an extra 1-char unread buffer that was incorrectly
>     initialised to 0, which means that the first character read would
>     always be NUL.  As this is often the code that looks for the
>     lexical-binding cookie, the first loaded source module would be
>     treated as dynamically bound.  During bootstrapping this is loadup.el
>     and so its local variables got dumped into the global environment.

Wow.  How did you manage to debug this?  :-)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master 5252c45: Initialise unread buffer

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Mattias Engdegård <mattiase@savannah.gnu.org> writes:
>
>>     The reader has an extra 1-char unread buffer that was incorrectly
>>     initialised to 0, which means that the first character read would
>>     always be NUL.  As this is often the code that looks for the
>>     lexical-binding cookie, the first loaded source module would be
>>     treated as dynamically bound.  During bootstrapping this is loadup.el
>>     and so its local variables got dumped into the global environment.
>
> Wow.  How did you manage to debug this?  :-)

But it seems like this leads to random breakages:

Debugger entered--Lisp error: (error "Invalid byte opcode: op=0, ptr=6")
  signal(error ("Invalid byte opcode: op=0, ptr=6"))
  apply(signal (error ("Invalid byte opcode: op=0, ptr=6")))
  (setq value-198 (apply fn-196 args-197))

This is when evalling the body of this test:

(ert-deftest eval-tests-accept-empty-optional-rest ()
  "Check that Emacs accepts empty &optional and &rest arglists.
Bug#24912."
  (dolist (args '((&optional) (&rest) (&optional &rest)
                  (&optional &rest a) (&optional a &rest)))
    (let ((fun `(lambda ,args 'ok)))
      (ert-info ("eval")
        (should (eq (funcall (eval fun t)) 'ok)))
      (ert-info ("byte comp check")
        (byte-compile-check-lambda-list args))
      (ert-info ("bytecomp")
        (let ((byte-compile-debug t))
          (should (eq (funcall (byte-compile fun)) 'ok))))))

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master 5252c45: Initialise unread buffer

[Mattias Engdegård]

19 sep. 2021 kl. 16.56 skrev Lars Ingebrigtsen <larsi@gnus.org>:

> Debugger entered--Lisp error: (error "Invalid byte opcode: op=0, ptr=6")

Sorry about that. I'll take a look.
If I don't find the error soon enough, or if there are actual errors in operation, the change will be reverted.

Re: master 5252c45: Initialise unread buffer

[Lars Ingebrigtsen]

Mattias Engdegård <mattiase@acm.org> writes:

> 19 sep. 2021 kl. 16.56 skrev Lars Ingebrigtsen <larsi@gnus.org>:
>
>> Debugger entered--Lisp error: (error "Invalid byte opcode: op=0, ptr=6")
>
> Sorry about that. I'll take a look.

I'm not absolutely sure that it's your change that's causing this weird
error, but it seemed the most likely.  :-/  And reverting back to the
checkout before yours made the problem go away.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master 5252c45: Initialise unread buffer

[Mattias Engdegård]

19 sep. 2021 kl. 17.41 skrev Lars Ingebrigtsen <larsi@gnus.org>:

> I'm not absolutely sure that it's your change that's causing this weird
> error, but it seemed the most likely.  :-/  And reverting back to the
> checkout before yours made the problem go away.

It's more fun actually. Thanks to the fixed reader bug, the lexical-binding cookie is now honoured in loadup.el. That causes Emacs to be dumped with `lexical-binding` set to t which as a consequence becomes the new default.

It's a testament to the sterling work by Stefan, Stefan, you, and everybody else that Emacs seems to work almost flawlessly anyway, but as it really wasn't our intention to do the switch now I've pushed an explicit binding of lexical-binding around the actual dumping so we're now back to normal.

What about the failing eval-tests then? They check, among other things, that

 (lambda (&rest) 'ok)

can be interpreted, byte-compiled, and returns `ok` when called. This fails with lexical binding enabled but works with dynamic binding, so when the default was inadvertently switched to lexical, the test started failing.

But wait, you protest. eval-tests.el is marked with -*- lexical-binding: t -*-. Surely? No?

Actually, any code in `ert-deftest` uses dynamic binding (until the accidental switch), regardless of the file cookie. Not many people know that.

So what is this bug that is still present? Let's take a look:

(byte-compile (lambda (&rest) 'ok))
=>
#[128 "\300\207" [ok] 1 "..."]

Let's pick it apart:
- The number 128 means that there are no positional args but a single &rest argument.
- The "\300\207" are the two byte-ops
   constant ok   ; push `ok` on the stack
   return        ; return the value on top of the stack
- The number 1 is the maximum stack size required.

When byte-code execution starts, the arguments are pushed on the stack -- in this case, a single nil for the &rest. The first byte-op then pushes the constant `ok`. But the stack is only sized for one element so that overwrites what follows, which is a copy of the byte-code. The smashed ops are then executed: if we are lucky we get an error, if unlucky an Emacs abort or crash.

I'm not sure why the byte-code interpreter makes a copy of the byte code for each function call. It certainly doesn't help the lamentably high function call overhead a bit. Most likely it's programmer, not code, efficiency that was optimised: since GC can move string data, the pc would need to be rebased each time that can occur, and that might need to be done in a lot of places. We should probably do something about that.

If the argument is named, then we get:

(byte-compile (lambda (&rest _x) 'ok))
=>
#[128 "\300\207" [ok] 2 "..."]

Note the max depth 2 here which gives a sufficiently big stack.

There's a number of things to fix here, but this message is too long already. Thanks for your interest! There are books for sale in the foyer.

Re: master 5252c45: Initialise unread buffer

[tomas]

[-- Attachment #1: Type: text/plain, Size: 397 bytes --]

On Mon, Sep 20, 2021 at 12:16:24PM +0200, Mattias Engdegård wrote:

[...]

> There's a number of things to fix here, but this message is too long already.

This from a random bystander: thanks for the fascinating
(and very instructive) account!

> Thanks for your interest! There are books for sale in the foyer.

I'll take that one on Emacs Lisp bytecodes, please :)

Cheers
 - t

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: master 5252c45: Initialise unread buffer

[Lars Ingebrigtsen]

Mattias Engdegård <mattiase@acm.org> writes:

> It's more fun actually. Thanks to the fixed reader bug, the
> lexical-binding cookie is now honoured in loadup.el. That causes Emacs
> to be dumped with `lexical-binding` set to t which as a consequence
> becomes the new default.

Heh.  The reader bug was a case of a bug leading to desirable behaviour
(accidentally).  They're the worst.

> Actually, any code in `ert-deftest` uses dynamic binding (until the
> accidental switch), regardless of the file cookie. Not many people
> know that.

I did not know that.

> I'm not sure why the byte-code interpreter makes a copy of the byte
> code for each function call. It certainly doesn't help the lamentably
> high function call overhead a bit.

Huh.

> There's a number of things to fix here, but this message is too long
> already. Thanks for your interest! There are books for sale in the
> foyer.

:-)

Amazing debugging.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master 5252c45: Initialise unread buffer

[Stefan Monnier]

> Actually, any code in `ert-deftest` uses dynamic binding (until the
> accidental switch), regardless of the file cookie. Not many people
> know that.

That's a bug.  Could you make a proper bug report for it?


        Stefan