bug#46670: 28.0.50; [feature/native-comp] possible miscompilation affecting lsp-mode

[Pip Cet <pipcet@gmail.com>]

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

On Sun, Feb 21, 2021 at 9:03 PM Andrea Corallo <akrl@sdf.org> wrote:
> Pip Cet <pipcet@gmail.com> writes:
>
> > On Sun, Feb 21, 2021 at 11:51 AM Pip Cet <pipcet@gmail.com> wrote:
> >> Does the attached patch help? Andrea, is my analysis correct?
> >
> > CCing Andrea.
> >
> > In more detail, some debugging showed we were trying to intersect a
> > "nil or t" constraint with a "sequence" constraint, the result being a
> > null constraint (t is not a sequence). So if (assume (and a b)) was
> > meant to imply the intersection of a and b, we're emitting it
> > incorrectly.
>
> Hi Pip,
>
> thanks for looking into this.

Thanks for your explanation!

> 'and' is meant to imply intersection, so yeah... as you guess there's
> some problem in the LIMPLE we generate :)

Thanks, I was on the wrong track there.

> The correct fix is to have `comp-add-cond-cstrs' rewrite the comparison
> sequence as something like:
>
> (set #(mvar nil X t) #(mvar 42082358 1 t))
> (set #(mvar 41121566 1 boolean) (call equal #(mvar 42082358 1 t) #(mvar 41665638 2 sequence)))
> (cond-jump #(mvar 41121566 1 boolean) #(mvar nil nil null) bb_2_cstrs_0 bb_1)
>
> Where X is a new slot we add to the frame.  We will reference this slot
> number in the assume instead of 1 so it does not get clobbered.

Okay, I think I understand the problem (we don't do classical SSA and
throw away the slot numbers), and your solution would avoid it, but it
seems needlessly complicated to me.

Why create a new slot that isn't used anywhere? The value of the stack
slot is clobbered by the (set ...), so we cannot emit any assumptions
about that stack slot based on previous values it held.

In fact, all we need to do is tell comp-cond-cstrs-target-mvar to
return nil more often, isn't it?

Pip

[-- Attachment #2: 0001-Don-t-lie-about-who-set-the-variable.patch --]
[-- Type: text/x-patch, Size: 961 bytes --]

From 2a400490851c530bd51f8a4453d0d3fb452ab561 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Sun, 21 Feb 2021 22:40:05 +0000
Subject: [PATCH] Don't lie about who set the variable.

---
 lisp/emacs-lisp/comp.el | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
index 4036080976546..194b1512cc2c4 100644
--- a/lisp/emacs-lisp/comp.el
+++ b/lisp/emacs-lisp/comp.el
@@ -2317,11 +2317,11 @@ comp-cond-cstrs-target-mvar
     (cl-loop
      with res = nil
      for insn in (comp-block-insns bb)
-     when (eq insn exit-insn)
-     do (cl-return (and (comp-mvar-p res) res))
      do (pcase insn
           (`(,(pred comp-assign-op-p) ,(pred targetp) ,rhs)
            (setf res rhs)))
+     when (eq insn exit-insn)
+     do (cl-return (and (comp-mvar-p res) res))
      finally (cl-assert nil))))
 
 (defun comp-add-cond-cstrs-target-block (curr-bb target-bb-sym)
-- 
2.30.0