GNU bug report logs - #74771
Native compilation bug with struct predicates when lexical binding enabled (HEAD)

Previous Next

Full log

Message #32 received at 74771 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: acorallo <at> gnu.org, Pip Cet <pipcet <at> protonmail.com>
Cc: eric.marsden <at> risk-engineering.org, 74771 <at> debbugs.gnu.org
Subject: Re: bug#74771: Native compilation bug with struct predicates when
 lexical binding enabled (HEAD)
Date: Sat, 22 Feb 2025 11:56:25 +0200

Ping! Can we make progress here, please?

> Date: Wed, 08 Jan 2025 11:51:07 +0000
> From: Pip Cet <pipcet <at> protonmail.com>
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 74771 <at> debbugs.gnu.org, eric.marsden <at> risk-engineering.org
> 
> "Andrea Corallo" <acorallo <at> gnu.org> writes:
> 
> > Eli Zaretskii <eliz <at> gnu.org> writes:
> >
> >>> Cc: 74771 <at> debbugs.gnu.org
> >>> From: Andrea Corallo <acorallo <at> gnu.org>
> >>> Date: Wed, 11 Dec 2024 17:29:34 -0500
> >>>
> >>> Eric Marsden <eric.marsden <at> 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
> 
> 

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.