bug#74771: Native compilation bug with struct predicates when lexical binding enabled (HEAD)

bug#74771: Native compilation bug with struct predicates when lexical binding enabled (HEAD)

[Eric Marsden]

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

Hi,

With the attached source file, Emacs miscompiles the struct predicate such
that a repeated call to the predicate on a non-struct object returns t.
This occurs with current HEAD on Linux/AMD64, but not on the Emacs 30.0.92
pretest. It does not occur when the lexical binding cookie is not present.

% /opt/emacs/bin/emacs -Q --batch --eval "(load (native-compile \"/tmp/bug.el\"))" -f run
Loading /home/emarsden/.emacs.d/eln-cache/31.0.50-c021c983/bug-59c4b27c-c70072f9.eln (native compiled elisp)...
Running in GNU Emacs 31.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.43, cairo version 1.18.2)
  of 2024-12-09
is? nil
is? t   ;; expecting nil
bar: 111


[-- Attachment #2: bug.el --]
[-- Type: text/x-emacs-lisp, Size: 409 bytes --]

;;;   -*- lexical-binding: t -*-
;;
;; /opt/emacs/bin/emacs -Q --batch -L . --eval "(load (native-compile \"/tmp/bug.el\"))" -f run

(require 'cl-lib)

(cl-defstruct foobles bar baz)

(defun bug (foo)
  (message "is? %s" (foobles-p foo))
  (message "is? %s" (foobles-p foo))
  (message "bar: %s" (foobles-bar foo)))

(defun run ()
  (message "Running in %s" (version))
  (let ((foo "foo"))
    (bug foo)))

bug#74771: Native compilation bug with struct predicates when lexical binding enabled (HEAD)

[Pip Cet via Bug reports for GNU Emacs, the Swiss army knife of text editors]

"Eric Marsden" <eric.marsden@risk-engineering.org> writes:

> Hi,
>
> With the attached source file, Emacs miscompiles the struct predicate such
> that a repeated call to the predicate on a non-struct object returns t.
> This occurs with current HEAD on Linux/AMD64, but not on the Emacs 30.0.92
> pretest. It does not occur when the lexical binding cookie is not present.
>
> % /opt/emacs/bin/emacs -Q --batch --eval "(load (native-compile \"/tmp/bug.el\"))" -f run
> Loading /home/emarsden/.emacs.d/eln-cache/31.0.50-c021c983/bug-59c4b27c-c70072f9.eln (native compiled elisp)...
> Running in GNU Emacs 31.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.43, cairo version 1.18.2)
>   of 2024-12-09
> is? nil
> is? t   ;; expecting nil
> bar: 111
>
> [2. text/x-emacs-lisp; bug.el]...

Can you try this patch?

diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
index 0d40f05bef1..c3e9a8be66d 100644
--- a/lisp/emacs-lisp/comp.el
+++ b/lisp/emacs-lisp/comp.el
@@ -2029,15 +2029,18 @@ comp--add-cond-cstrs
               (call symbol-value ,(and (pred comp-cstr-cl-tag-p) mvar-tag)))
          (set ,(and (pred comp-mvar-p) mvar-3)
               (call memq ,(and (pred comp-mvar-p) mvar-1) ,(and (pred comp-mvar-p) mvar-2)))
-         (cond-jump ,(and (pred comp-mvar-p) mvar-3) ,(pred comp-mvar-p) ,bb1 ,bb2))
-       (comp--emit-assume 'and mvar-tested
-                         (make--comp-mvar :type (comp-cstr-cl-tag mvar-tag))
-                         (comp--add-cond-cstrs-target-block b bb2)
-                         nil)
-       (comp--emit-assume 'and mvar-tested
-                         (make--comp-mvar :type (comp-cstr-cl-tag mvar-tag))
-                         (comp--add-cond-cstrs-target-block b bb1)
-                         t))
+         (cond-jump ,(and (pred comp-mvar-p) mvar-3) ,(and (pred comp-mvar-p) mvar-4) ,bb1 ,bb2))
+       (cond
+        ((and (comp-cstr-imm-vld-p mvar-4)
+              (eq (comp-cstr-imm mvar-4) t))
+         (comp--emit-assume 'and mvar-tested
+                            (make--comp-mvar :type (comp-cstr-cl-tag mvar-tag))
+                            (comp--add-cond-cstrs-target-block b bb2)
+                            nil)
+         (comp--emit-assume 'and mvar-tested
+                            (make--comp-mvar :type (comp-cstr-cl-tag mvar-tag))
+                            (comp--add-cond-cstrs-target-block b bb1)
+                            t))))
       (`((set ,(and (pred comp-mvar-p) cmp-res)
               (,(pred comp--call-op-p)
                ,(and (or (pred comp--equality-fun-p)

IIUC, the code blindly assumes that cond-jump would use t as its second
argument.  In your code, the second argument was nil, so the assumptions
were put into the wrong basic blocks.

It looks like there are quite a few such assumptions in comp.el. I think
we should fix them all to ensure that they test for truth, not
falsehood.  After that, we'll have to decide whether it's worth it
to optimize the negated cases.

Pip

bug#74771: Native compilation bug with struct predicates when lexical binding enabled (HEAD)

[Pip Cet via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Pip Cet <pipcet@protonmail.com> writes:

> IIUC, the code blindly assumes that cond-jump would use t as its second
> argument.  In your code, the second argument was nil, so the assumptions
> were put into the wrong basic blocks.

I did not understand correctly.  It seems cond-jump is still limited to
a nil second argument, which is an undocumented assumption that comp.el
continues to rely on.

I still think comp--emit-assume does the wrong thing when negating an
assumption, but we've been there before...

Pip

bug#74771: Native compilation bug with struct predicates when lexical binding enabled (HEAD)

[Eli Zaretskii]

> Cc: 74771@debbugs.gnu.org
> Date: Tue, 10 Dec 2024 22:12:03 +0000
> From:  Pip Cet via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Pip Cet <pipcet@protonmail.com> writes:
> 
> > IIUC, the code blindly assumes that cond-jump would use t as its second
> > argument.  In your code, the second argument was nil, so the assumptions
> > were put into the wrong basic blocks.
> 
> I did not understand correctly.  It seems cond-jump is still limited to
> a nil second argument, which is an undocumented assumption that comp.el
> continues to rely on.
> 
> I still think comp--emit-assume does the wrong thing when negating an
> assumption, but we've been there before...

Let's hear from Andrea (CC'ed) about this.

bug#74771: Native compilation bug with struct predicates when lexical binding enabled (HEAD)

[Pip Cet via Bug reports for GNU Emacs, the Swiss army knife of text editors]

"Eli Zaretskii" <eliz@gnu.org> writes:

>> Cc: 74771@debbugs.gnu.org
>> Date: Tue, 10 Dec 2024 22:12:03 +0000
>> From:  Pip Cet via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>>
>> Pip Cet <pipcet@protonmail.com> writes:
>>
>> > IIUC, the code blindly assumes that cond-jump would use t as its second
>> > argument.  In your code, the second argument was nil, so the assumptions
>> > were put into the wrong basic blocks.
>>
>> I did not understand correctly.  It seems cond-jump is still limited to
>> a nil second argument, which is an undocumented assumption that comp.el
>> continues to rely on.
>>
>> I still think comp--emit-assume does the wrong thing when negating an
>> assumption, but we've been there before...
>
> Let's hear from Andrea (CC'ed) about this.

You're right. I have a new theory, but as it boils down to something
that has been discussed before (whether "assume" pseudo-insns apply to
individual mvars or to all mvars sharing a slot - my understanding is it
has to be the latter, so we end up making invalid assumptions about the
new mvar in the slot based on things we know about the old, clobbered
mvar), it's best to let Andrea handle this one.  I'll just disable
nativecomp assume pseudo-insns for my builds as I don't want to run into
this bug.

Pip