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" <bug-gnu-emacs@gnu.org>]

"Andrea Corallo" <acorallo@gnu.org> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> Cc: 74771@debbugs.gnu.org
>>> From: Andrea Corallo <acorallo@gnu.org>
>>> Date: Wed, 11 Dec 2024 17:29:34 -0500
>>>
>>> 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
>>> >
>>> > ;;;   -*- 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)))
>>>
>>> Hi Eric,
>>>
>>> thanks for the report, I'll look at this in the coming days.
>>
>> Any progress here?
>
> Not so far, I'm on holiday this days so I don't have much time for
> coding, it's in my todo list tho.

No activity in a while, so I tried reproducing this: bug's still there.

Can you repeat for me what

(assume #s(comp-mvar (t) nil nil nil nil nil) (not #s(comp-mvar (foobles) nil nil nil nil nil)))

is supposed to mean?

To me, it appears to mean "the value of the first mvar is different from
the value of the second mvar, which is of type foobles".  This doesn't
say anything about the type of the first mvar, assuming there is more
than one value of the foobles type; if this is the correct
interpretation, we need to stop copying the typeset in this case:

diff --git a/lisp/emacs-lisp/comp-cstr.el b/lisp/emacs-lisp/comp-cstr.el
index 3d46cc8c6ae..5d87ff75703 100644
--- a/lisp/emacs-lisp/comp-cstr.el
+++ b/lisp/emacs-lisp/comp-cstr.el
@@ -1176,14 +1176,11 @@ comp-cstr-value-negation
   "Negate values in SRC setting the result in DST.
 DST is returned."
   (with-comp-cstr-accessors
-    (if (or (valset src) (range src))
-        (setf (typeset dst) ()
-              (valset dst) (valset src)
-              (range dst) (range src)
-              (neg dst) (not (neg src)))
-      (setf (typeset dst) (typeset src)
-            (valset dst) ()
-            (range dst) ()))
+    (and (or (valset src) (range src))
+         (setf (typeset dst) ()
+               (valset dst) (valset src)
+               (range dst) (range src)
+               (neg dst) (not (neg src))))
     dst))
 
 (defun comp-cstr-negation-make (src)


I see two other possibilities:

1. it means the lhs mvar is of type "foobles", but not identical to the
second mvar.  This is what comp-cstr-negation-make currently assumes.
In this case, comp--add-cond-cstrs should not be emitting this
pseudo-insn for the case in which the lhs mvar is known not to be of
type "foobles", but nothing else is known about its value:

diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
index 269eae315e4..d5c512fbdc3 100644
--- a/lisp/emacs-lisp/comp.el
+++ b/lisp/emacs-lisp/comp.el
@@ -2033,11 +2033,7 @@ comp--add-cond-cstrs
        (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))
+                         nil))
       (`((set ,(and (pred comp-mvar-p) cmp-res)
               (,(pred comp--call-op-p)
                ,(and (or (pred comp--equality-fun-p)

2. it means the lhs mvar is not of type "foobles".  In this case,
comp-cstr-value-negation should make the lhs mvar have a negated cstr
with type "foobles":

diff --git a/lisp/emacs-lisp/comp-cstr.el b/lisp/emacs-lisp/comp-cstr.el
index 3d46cc8c6ae..03a00123f64 100644
--- a/lisp/emacs-lisp/comp-cstr.el
+++ b/lisp/emacs-lisp/comp-cstr.el
@@ -1183,7 +1183,8 @@ comp-cstr-value-negation
               (neg dst) (not (neg src)))
       (setf (typeset dst) (typeset src)
             (valset dst) ()
-            (range dst) ()))
+            (range dst) ()
+            (neg dst) (not (neg src))))
     dst))
 
 (defun comp-cstr-negation-make (src)

Note that if (2) is intended, that is a really strange interpretation of
what "not" means: it's negating a cstr (a set of values), not an mvar (a
specific value), so why is the argument an mvar?  We could make this
change, and then try to recover whichever optimizations that disables:

diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
index 269eae315e4..bcc0628235a 100644
--- a/lisp/emacs-lisp/comp.el
+++ b/lisp/emacs-lisp/comp.el
@@ -2035,7 +2035,7 @@ comp--add-cond-cstrs
                          (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--type-to-cstr (comp-cstr-cl-tag mvar-tag))
                          (comp--add-cond-cstrs-target-block b bb1)
                          t))
       (`((set ,(and (pred comp-mvar-p) cmp-res)

In any case, please consider documenting the pseudo-insns.
not-eq-to-mvar and not-matching-cstr are two very different things!

Pip