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

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

[Mauricio Collares]

This was found by Anthony Cowley, who isolated the exact function in
lsp-mode that was misbehaving. I verified that I could reproduce this
findings, and then I removed surrounding context to obtain a minimized
testcase. If this fails to reproduce, it's entirely my fault.

Steps to reproduce:

1) Put this in minimized.el:

;;; -*- lexical-binding: t; -*-

(defun minimized--look-back (s)
  (and (equal (buffer-substring-no-properties (- (point) (length s)) (point))
              s)
       s))

(defun minimized-go ()
  (interactive)
  (message (minimized--look-back ".")))

(provide 'minimized)

2) Type "." in a buffer and then run minimized-go with the point after
the period. This prints back "." in the minibuffer if the code's
interpreted but not if it's native-compiled.

Note that removing the "lexical-binding: t" line makes the bug not
reproduce. Replacing "(- (point) (length s))" by "(1- (point))" also
makes the bug disappear.

Best,
Mauricio

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

[Pip Cet]

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

On Sun, Feb 21, 2021 at 12:14 AM Mauricio Collares
<mauricio@collares.org> wrote:
> This was found by Anthony Cowley, who isolated the exact function in
> lsp-mode that was misbehaving. I verified that I could reproduce this
> findings, and then I removed surrounding context to obtain a minimized
> testcase. If this fails to reproduce, it's entirely my fault.
>
> Steps to reproduce:
>
> 1) Put this in minimized.el:
>
> ;;; -*- lexical-binding: t; -*-
>
> (defun minimized--look-back (s)
>   (and (equal (buffer-substring-no-properties (- (point) (length s)) (point))
>               s)
>        s))
>
> (defun minimized-go ()
>   (interactive)
>   (message (minimized--look-back ".")))
>
> (provide 'minimized)
>
> 2) Type "." in a buffer and then run minimized-go with the point after
> the period. This prints back "." in the minibuffer if the code's
> interpreted but not if it's native-compiled.
>
> Note that removing the "lexical-binding: t" line makes the bug not
> reproduce. Replacing "(- (point) (length s))" by "(1- (point))" also
> makes the bug disappear.

I can reproduce this with this code:

(funcall
 (let ((comp-verbose 3) (comp-debug 3))
   (native-compile `(lambda (s) (and (equal
(buffer-substring-no-properties (- (point) (length s)) (point)) s)
s))))
 ")")

I think there's some confusion in comp-fwprop-insn between (and) as a
logical operator and (and) as a pcase operator. The latter means a
variable's constraint must be in the intersection of all argument
types, but the former only implies that the variable constraint is
somewhere in the union of the argument constraints [1].

Does the attached patch help? Andrea, is my analysis correct?

Pip

[1] - note that we emit (assume a (and b c)) for (setq a (and c b))
under some circumstances, so it would be incorrect to use only c's
constraint.

[-- Attachment #2: 0001-native-comp-Fix-constraint-for-assume-x-and-a-b-Bug-.patch --]
[-- Type: text/x-patch, Size: 902 bytes --]

From bd3a823b827c9394c11aae63dc3fa81098699296 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Sun, 21 Feb 2021 11:44:27 +0000
Subject: [PATCH] [native-comp] Fix constraint for (assume x (and a b))
 (Bug#46670)

* lisp/emacs-lisp/comp.el (comp-fwprop-insn): Use comp-cstr-union, not
comp-cstr-intersection.
---
 lisp/emacs-lisp/comp.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
index 4036080976546..965121657f601 100644
--- a/lisp/emacs-lisp/comp.el
+++ b/lisp/emacs-lisp/comp.el
@@ -3059,7 +3059,7 @@ comp-fwprop-insn
     (`(assume ,lval (,kind . ,operands))
      (cl-case kind
        (and
-        (apply #'comp-cstr-intersection lval operands))
+        (apply #'comp-cstr-union lval operands))
        (not
         ;; Prevent double negation!
         (unless (comp-cstr-neg (car operands))
-- 
2.30.0

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

[Pip Cet]

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.

Pip

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

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

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.

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

Just to mention we have also a number of tests in comp-tests.el to
checks that we predict the correct return value, applying the suggested
patch a number of these are not passing.  We'll want to add also a new
test there for this specific case when the issue is solved.

Here a slightly more reduced test-case I'm using here for the analysis
for which the compiler erroneously proves the return type is null.

(let ((comp-verbose 3))
  (native-compile
   '(λ (s)
      (and (equal (foo (length s)) s)
           s))))

Here is why, looking at LIMPLE coming in the final pass in bb_1 we emit:

(assume #(mvar 41121096 0 null) (and #(mvar 42082902 0 sequence) #(mvar 41121566 1 boolean)))

bb_1 is the basic block where 'equal' is verified so we want to enforce
that the result cstr of the call 'foo' is intersected with 's' because
they are equal.

Now the trouble is that while 's' here is represented correctly by the
mvar 42082902 the other operand of the and constraint is wrong.  Where
this is coming from then?

Inside the predecessor block bb_0 we have the compare and branch
sequence:

(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)

Here when we run the 'add-cstrs' pass `comp-add-cond-cstrs' is deciding
that 42082358 41665638 must be constrained and therefore is emitting the
assume.  Unfortunatelly because 42082358 and 41121566 are sharing the
same slot number when we do the next SSA rename the mvar referenced in
the assume will be one that is produced by 'equal' and not the one that
is consumed.

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.

Now I'm not 100% sure how trivial is to do that as no pass is ATM
changing the frame size.

I guess I'll try to write a patch tomorrow evening.

Thanks!

  Andrea

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

[Pip Cet]

[-- 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

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

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

Pip Cet <pipcet@gmail.com> writes:

> 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.

Correct, ATM the assumption is that we keep LIMPLE always as
"conventional SSA form".  This for a number of reasons but mainly it
greatly helps in maintaining the compiler simple.

I've experimented investing quite some effort into removing this
assumption but the result was definitely more complex and the produced
code considerably harder to debug.  The only advantage I could see in
the end was having a simpler and more elegant
`comp-cond-cstrs-target-mvar' due to the fact that was trivial to
implement a copy propagation pass), so I deemed was a good move to keep
always the conventional form.

> 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.

Yes but in this case (and probably others) we *have* to emit this
assumption.

The best option is to decide that negative slot numbers are not rendered
into libgccjit IR and we consider these virtual to solve these kind of
cases.  IIRC we already do something similar for the -1 index so this
concept has just to be generalized a bit.

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

Nope, the target mvar identified is the correct one, we just have ATM no
way to reference it reliably into the assume.  BTW applying your patch
is breaking quite some of the comp-tests-ret-type-spec-* tests :)

  Andrea

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

[Pip Cet]

On Mon, Feb 22, 2021 at 9:38 AM Andrea Corallo <akrl@sdf.org> wrote:
> Pip Cet <pipcet@gmail.com> writes:
> > 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.
>
> Correct, ATM the assumption is that we keep LIMPLE always as
> "conventional SSA form".  This for a number of reasons but mainly it
> greatly helps in maintaining the compiler simple.

"Conventional" meaning "not quite SSA"? I'm just trying to understand,
the decision seems correct to me, since we already ran stack slot
allocation in the byte compiler and we want to reuse those
assignments.

> > 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.
>
> Yes but in this case (and probably others) we *have* to emit this
> assumption.

Why? Are you saying the compiler requires (assume ...) insns for
correctness? If it does, I'm afraid that's a serious issue.

> The best option is to decide that negative slot numbers are not rendered
> into libgccjit IR and we consider these virtual to solve these kind of
> cases.  IIRC we already do something similar for the -1 index so this
> concept has just to be generalized a bit.

Again, that does seem very complicated, and if it improves
optimization we could probably improve it much more by modifying the
byte compiler to pop arguments in the caller rather than the callee.

> > In fact, all we need to do is tell comp-cond-cstrs-target-mvar to
> > return nil more often, isn't it?
>
> Nope, the target mvar identified is the correct one, we just have ATM no
> way to reference it reliably into the assume.

We don't have to, let's just not emit an assume about a variable that
we just introduced and that's never read?

> BTW applying your patch
> is breaking quite some of the comp-tests-ret-type-spec-* tests :)

Where do you keep those?

I see the same number of test failures with and without the patch,
running "make check". The failures seem unrelated, too...

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

[Pip Cet]

On Mon, Feb 22, 2021 at 10:04 AM Pip Cet <pipcet@gmail.com> wrote:

> > BTW applying your patch
> > is breaking quite some of the comp-tests-ret-type-spec-* tests :)
>
> Where do you keep those?

Oh, I see, they're written as though they tested comp.c.

At a quick glance, the test results aren't actually incorrect, they're
merely missed optimizations.

(Except for this one:

      ((defun comp-tests-ret-type-spec-f (x)
         (unless (symbolp x)
           x))
       (not symbol))

If I'm reading that correctly, it tests that (unless (symbolp x) x)
isn't a symbol, which it usually is)

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

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

Pip Cet <pipcet@gmail.com> writes:

> On Mon, Feb 22, 2021 at 10:04 AM Pip Cet <pipcet@gmail.com> wrote:
>
>> > BTW applying your patch
>> > is breaking quite some of the comp-tests-ret-type-spec-* tests :)
>>
>> Where do you keep those?
>
> Oh, I see, they're written as though they tested comp.c.
>
> At a quick glance, the test results aren't actually incorrect, they're
> merely missed optimizations.

Correct, note these is not only about potentially missed optimizations,
we expose the derived return type with `subr-type' and in the future we
might give it even more visibility (like using it it in the C-h f
output).

> (Except for this one:
>
>       ((defun comp-tests-ret-type-spec-f (x)
>          (unless (symbolp x)
>            x))
>        (not symbol))
>
> If I'm reading that correctly, it tests that (unless (symbolp x) x)
> isn't a symbol, which it usually is)

Yep, it verifies that this function has as inferred return type (not
symbol).

  Andrea

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

[Pip Cet]

On Mon, Feb 22, 2021 at 11:23 AM Andrea Corallo <akrl@sdf.org> wrote:
> Pip Cet <pipcet@gmail.com> writes:
> > On Mon, Feb 22, 2021 at 10:04 AM Pip Cet <pipcet@gmail.com> wrote:
> > (Except for this one:
> >
> >       ((defun comp-tests-ret-type-spec-f (x)
> >          (unless (symbolp x)
> >            x))
> >        (not symbol))
> >
> > If I'm reading that correctly, it tests that (unless (symbolp x) x)
> > isn't a symbol, which it usually is)
>
> Yep, it verifies that this function has as inferred return type (not
> symbol).

Which means the return value shouldn't ever be a symbol, right?
Because it's nil, which is a symbol, when (symbolp x). Am I missing
something here?

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

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

Pip Cet <pipcet@gmail.com> writes:

> On Mon, Feb 22, 2021 at 11:23 AM Andrea Corallo <akrl@sdf.org> wrote:
>> Pip Cet <pipcet@gmail.com> writes:
>> > On Mon, Feb 22, 2021 at 10:04 AM Pip Cet <pipcet@gmail.com> wrote:
>> > (Except for this one:
>> >
>> >       ((defun comp-tests-ret-type-spec-f (x)
>> >          (unless (symbolp x)
>> >            x))
>> >        (not symbol))
>> >
>> > If I'm reading that correctly, it tests that (unless (symbolp x) x)
>> > isn't a symbol, which it usually is)
>>
>> Yep, it verifies that this function has as inferred return type (not
>> symbol).
>
> Which means the return value shouldn't ever be a symbol, right?
> Because it's nil, which is a symbol, when (symbolp x). Am I missing
> something here?

Sorry I though the question was on the test mechanism and wasn't pay
attention to the specific testcase content :/

Right that's clearly a bug in `comp-cstr-union-1-no-mem' that was
missing to check that no negative type is shadowing any positive type
coming from values and giving-up returning t in case).

Good catch thanks! :) Should be fixed by d6227f6edc.

  Andrea

PS as I see you are interested into this part of the compiler, I find
typically handy to exercise this logic with like:

(let ((comp-ctxt (make-comp-cstr-ctxt)))
  (comp-cstr-to-type-spec
   (comp-type-spec-to-cstr '(or (not symbol) null))))

We'll probably see other bugs in this area cause is tricky, is important
we build the best coverage we can for this in the testsuite.

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

[Pip Cet]

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

On Mon, Feb 22, 2021 at 1:12 PM Andrea Corallo <akrl@sdf.org> wrote:
> Good catch thanks! :) Should be fixed by d6227f6edc.

I'm also confused by the use of NEGATED in comp-emit-assume: if RHS is
a constraint, it emits the complementary constraint.

However, the code in comp-add-cond-cstrs uses NEGATED to express a
much weaker constraint: that two mvars aren't strictly equal.

If x /= y and y in SET, we can't conclude that x not in SET (unless
SET is a singleton, an important special case).

So it all works right now because emit-assume NEGATED=t RHS=mvar means
"LHS isn't equal to RHS" but NEGATED=t RHS=cstr means "LHS can't
satisfy RHS".

My code changed the call to pass a constraint instead of the mvar, and
then things broke :-)

We should be consistent about what NEGATED means, I think.

But apart from such problems, my code appears to be working. I'm
attaching it for the sake of completeness, not because I expect you to
read it all before it's cleaned up.

[-- Attachment #2: emacs-bug46670-001.diff --]
[-- Type: text/x-patch, Size: 19487 bytes --]

diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
index 60c040926e54c..3cb7812b5a874 100644
--- a/lisp/emacs-lisp/comp.el
+++ b/lisp/emacs-lisp/comp.el
@@ -690,6 +690,16 @@ comp-args-base
              :documentation "This is a copy of the frame when leaving the block.
 Is in use to help the SSA rename pass."))
 
+(defun comp-block-insns-reverse (bb &optional start)
+  "Return the insns in BB in reverse order, starting with the one before
+START."
+  (let ((insns (comp-block-insns bb))
+        res)
+    (while (not (eq (car insns) start))
+      (push (car insns) res)
+      (setq insns (cdr insns)))
+    res))
+
 (cl-defstruct (comp-block-lap (:copier nil)
                               (:include comp-block)
                               (:constructor make--comp-block-lap
@@ -826,7 +836,7 @@ comp-mvar-value-vld-p
 (defun comp-mvar-value (mvar)
   "Return the constant value of MVAR.
 `comp-mvar-value-vld-p' *must* be satisfied before calling
-`comp-mvar-const'."
+`comp-mvar-value'."
   (declare (gv-setter
             (lambda (val)
               `(if (integerp ,val)
@@ -903,6 +913,10 @@ comp-assign-op-p
   "Assignment predicate for OP."
   (when (memq op comp-limple-assignments) t))
 
+(defun comp-clobbering-assign-op-p (op)
+  "Test if OP is a clobbering assignment."
+  (and (comp-assign-op-p op) (not (eq op 'assume))))
+
 (defun comp-call-op-p (op)
   "Call predicate for OP."
   (when (memq op comp-limple-calls) t))
@@ -2202,7 +2216,7 @@ comp-limplify
 
 
 (defsubst comp-mvar-used-p (mvar)
-  "Non-nil when MVAR is used as lhs in the current funciton."
+  "Non-nil when MVAR is used as rhs in the current function."
   (declare (gv-setter (lambda (val)
 			`(puthash ,mvar ,val comp-pass))))
   (gethash mvar comp-pass))
@@ -2217,7 +2231,7 @@ comp-collect-mvars
                do (setf (comp-mvar-used-p x) t)))
 
 (defun comp-collect-rhs ()
-  "Collect all lhs mvars into `comp-pass'."
+  "Collect all rhs mvars into `comp-pass'."
   (cl-loop
    for b being each hash-value of (comp-func-blocks comp-func)
    do (cl-loop
@@ -2245,7 +2259,14 @@ comp-reverse-cmp-fun
     (<= '>=)
     (t function)))
 
-(defun comp-emit-assume (kind lhs rhs bb negated)
+(defun comp-cstr-singleton-p (cstr)
+  (or (and (comp-cstr-valset cstr)
+           (length= (comp-cstr-valset cstr) 1))
+      (and (comp-cstr-range cstr)
+           (equal (car (comp-cstr-range cstr))
+                  (cdr (comp-cstr-range cstr))))))
+
+(defun comp-emit-assume (kind lhs rhs bb negated &optional strictly)
   "Emit an assume of kind KIND for mvar LHS being RHS.
 When NEGATED is non-nil the assumption is negated.
 The assume is emitted at the beginning of the block BB."
@@ -2253,6 +2274,7 @@ comp-emit-assume
     (cl-assert lhs-slot)
     (pcase kind
       ('and
+       (comp-log (format "assuming4 %S %S %S" lhs rhs negated))
        (if (comp-mvar-p rhs)
            (let ((tmp-mvar (if negated
                                (make-comp-mvar :slot (comp-mvar-slot rhs))
@@ -2263,29 +2285,47 @@ comp-emit-assume
              (if negated
                  (push `(assume ,tmp-mvar (not ,rhs))
 	               (comp-block-insns bb))))
-         ;; If is only a constraint we can negate it directly.
-         (push `(assume ,(make-comp-mvar :slot lhs-slot)
-                        (and ,lhs ,(if negated
-                                       (comp-cstr-negation-make rhs)
-                                     rhs)))
-	       (comp-block-insns bb))))
+         ;; If RHS is a constraint we can negate it directly.
+         (comp-log (format "assuming3 %S %S" lhs rhs))
+         (when (or strictly (not negated))
+           (push `(assume ,(make-comp-mvar :slot lhs-slot)
+                          (and ,lhs ,(if negated
+                                         (comp-cstr-negation-make rhs)
+                                       rhs)))
+	         (comp-block-insns bb)))))
       ((pred comp-range-cmp-fun-p)
-       (let ((kind (if negated
-                       (comp-negate-range-cmp-fun kind)
-                     kind)))
-         (push `(assume ,(make-comp-mvar :slot lhs-slot)
-                        (,kind ,lhs
-                               ,(if-let* ((vld (comp-mvar-value-vld-p rhs))
-                                          (val (comp-mvar-value rhs))
-                                          (ok (integerp val)))
-                                    val
-                                  (make-comp-mvar :slot (comp-mvar-slot rhs)))))
-	       (comp-block-insns bb))))
+       (when (or strictly (not negated) (comp-mvar-p rhs)
+                   (comp-cstr-singleton-p rhs))
+         (let ((kind (if negated
+                         (comp-negate-range-cmp-fun kind)
+                       kind)))
+           (comp-log (format "assuming2 %S %S" lhs rhs))
+           (push `(assume ,(make-comp-mvar :slot lhs-slot)
+                          (,kind ,lhs
+                                 ,(if (comp-mvar-p rhs)
+                                      (if-let* ((vld (comp-mvar-value-vld-p rhs))
+                                                (val (comp-mvar-value rhs))
+                                                (ok (integerp val)))
+                                          val
+                                        (make-comp-mvar :slot (comp-mvar-slot rhs)))
+                                    (comp-cstr-copy rhs))))
+	         (comp-block-insns bb)))))
       (_ (cl-assert nil)))
     (setf (comp-func-ssa-status comp-func) 'dirty)))
 
+(defun comp-emit-assumes (kind lhsl rhsl basic-block negated &optional strictly)
+  "Emit assume insns stating that all elements of LHSL relate to
+all elements of RHSL as KIND, which may be NEGATED.  The insns
+ara added to BASIC-BLOCK."
+  (comp-log (format "assumes %S %S" lhsl rhsl))
+  (dolist (lhs lhsl)
+    (and (comp-mvar-p lhs)
+         (comp-mvar-slot lhs)
+         (dolist (rhs rhsl)
+           (comp-emit-assume kind lhs rhs basic-block negated strictly)))))
+
 (defun comp-add-new-block-between (bb-symbol bb-a bb-b)
-  "Create a new basic-block named BB-SYMBOL and add it between BB-A and BB-B."
+  "Create a new basic block named BB-SYMBOL and add it between BB-A and BB-B."
   (cl-loop
    with new-bb = (make-comp-block-cstr :name bb-symbol
                                        :insns `((jump ,(comp-block-name bb-b))))
@@ -2305,24 +2345,84 @@ comp-add-new-block-between
    ;; Add `new-edge' to the current function and return it.
    (cl-return (puthash bb-symbol new-bb (comp-func-blocks comp-func)))
    finally (cl-assert nil)))
-
-;; Cheap substitute to a copy propagation pass...
-(defun comp-cond-cstrs-target-mvar (mvar exit-insn bb)
-  "Given MVAR search in BB the original mvar MVAR got assigned from.
-Keep on searching till EXIT-INSN is encountered."
-  (cl-flet ((targetp (x)
-              ;; Ret t if x is an mvar and target the correct slot number.
-              (and (comp-mvar-p x)
-                   (eql (comp-mvar-slot mvar) (comp-mvar-slot x)))))
-    (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)))
-     finally (cl-assert nil))))
+;; "Cheap" substitute for a copy propagation pass...
+(defun comp-cond-cstrs-identical-vars (mvars bb insn)
+  "Search BB for mvars known to be `eq' to all of the MVARS at the time INSN
+is executed."
+  (cl-assert (cl-every #'comp-mvar-p mvars))
+  (cl-loop
+   with slots = (delq nil (mapcar #'comp-mvar-slot mvars))
+   with res = (copy-sequence mvars)
+   with clobbered = nil
+   for insn in (comp-block-insns-reverse bb insn)
+   do (progn
+        (comp-log (format "insn %S slots %S res %S clobbered %S"
+                          insn slots res clobbered))
+        (pcase insn
+        (`(,(and op (pred comp-assign-op-p))
+           ,(and (pred comp-mvar-p) (pred comp-mvar-slot) lhs)
+           ,(and (pred comp-mvar-p) (pred comp-mvar-slot) rhs))
+         (let ((lhs-p (member (comp-mvar-slot lhs) slots))
+               (rhs-p (member (comp-mvar-slot rhs) slots)))
+           (and lhs-p (not rhs-p)
+                (push (comp-mvar-slot rhs) slots)
+                (unless (or (member (comp-mvar-slot lhs) clobbered)
+                            (memq rhs res))
+                  (push rhs res)))
+           (and rhs-p (not lhs-p)
+                (push (comp-mvar-slot lhs) slots)
+                (unless (or (member (comp-mvar-slot rhs) clobbered)
+                            (memq lhs res))
+                  (push lhs res)))
+           (and (comp-clobbering-assign-op-p op)
+                (not lhs-p)
+                (not rhs-p)
+                (setq slots (delete (comp-mvar-slot lhs) slots))
+                (unless (member (comp-mvar-slot lhs) clobbered)
+                  (push (comp-mvar-slot lhs) clobbered))))
+         (comp-log (format "post insn %S slots %S res %S clobbered %S"
+                           insn slots res clobbered)))
+        (`(,(pred comp-clobbering-assign-op-p)
+           ,(and (pred comp-mvar-slot) lhs)
+           _)
+         (unless (member (comp-mvar-slot lhs) clobbered)
+           (push (comp-mvar-slot lhs) clobbered))
+         (setq slots (delete (comp-mvar-slot lhs) slots)))))
+   finally (progn
+             (cl-return res))))
+
+;; "Cheap" substitute for a copy propagation pass...
+(defun comp-cond-cstrs-identical-vars-byvar (mvars bb insn)
+  "Search BB for mvars known to be `eq' to all of the MVARS at the time INSN
+is executed. Exclude the MVARS themselves from the result."
+  (cl-assert (cl-every #'comp-mvar-p mvars))
+  (cl-loop
+   with vars = (copy-sequence mvars)
+   with res = nil
+   with clobbered = nil
+   for insn in (comp-block-insns-reverse bb insn)
+   do (pcase insn
+        (`(,(and op (pred comp-assign-op-p))
+           ,lhs
+           ,(and (pred comp-mvar-p) rhs))
+         (let ((lhs-p (memq lhs vars))
+               (rhs-p (memq rhs vars)))
+           (cond
+            ((and (not lhs-p) rhs-p)
+             (push lhs vars)
+             (unless (or (memq lhs clobbered)
+                         (memq lhs res))
+               (push lhs res)))
+            ((or rhs-p (not (comp-clobbering-assign-op-p op))))
+            ((setq vars (delq lhs vars))
+             (unless (memq lhs clobbered) (push lhs clobbered))))))
+        (`(,(pred comp-clobbering-assign-op-p) ,lhs _)
+         (unless (memq lhs clobbered) (push lhs clobbered))
+         (setq vars (delq lhs vars))))
+   finally (progn
+             (comp-log (format "mvars %S res %S"
+                               mvars res))
+             (cl-return res))))
 
 (defun comp-add-cond-cstrs-target-block (curr-bb target-bb-sym)
   "Return the appropriate basic block to add constraint assumptions into.
@@ -2401,23 +2501,44 @@ comp-add-cond-cstrs
 	 ;; (comment ,_comment-str)
 	 (cond-jump ,cmp-res ,(pred comp-mvar-p) . ,blocks))
        (cl-loop
-        with target-mvar1 = (comp-cond-cstrs-target-mvar op1 (car insns-seq) b)
-        with target-mvar2 = (comp-cond-cstrs-target-mvar op2 (car insns-seq) b)
+        with target-mvars1 = (comp-cond-cstrs-identical-vars
+                               (list op1) b (car insns-seq))
+        with target-mvars2 = (comp-cond-cstrs-identical-vars
+                               (list op2) b (car insns-seq))
         with equality = (comp-equality-fun-p fun)
         for branch-target-cell on blocks
         for branch-target = (car branch-target-cell)
         for negated in '(t nil)
         for kind = (if equality 'and fun)
-        when (or (comp-mvar-used-p target-mvar1)
-                 (comp-mvar-used-p target-mvar2))
         do
+        (comp-log (format "target mvars %S %S"
+                          target-mvars1 target-mvars2))
+        (setq target-mvars1
+              (mapcar
+               (lambda (mvar)
+                 (if (and
+                      (comp-mvar-p mvar)
+                      (equal (comp-mvar-slot mvar)
+                             (comp-mvar-slot cmp-res)))
+                     (comp-cstr-copy mvar)
+                   mvar))
+               target-mvars1))
+        (setq target-mvars2
+              (mapcar
+               (lambda (mvar)
+                 (if (and
+                      (comp-mvar-p mvar)
+                      (equal (comp-mvar-slot mvar)
+                             (comp-mvar-slot cmp-res)))
+                     (comp-cstr-copy mvar)
+                   mvar))
+               target-mvars2))
         (let ((block-target (comp-add-cond-cstrs-target-block b branch-target)))
           (setf (car branch-target-cell) (comp-block-name block-target))
-          (when (comp-mvar-used-p target-mvar1)
-            (comp-emit-assume kind target-mvar1 op2 block-target negated))
-          (when (comp-mvar-used-p target-mvar2)
-            (comp-emit-assume (comp-reverse-cmp-fun kind)
-                              target-mvar2 op1 block-target negated)))
+          (comp-emit-assumes kind
+                             target-mvars1 target-mvars2 block-target negated)
+          (comp-emit-assumes (comp-reverse-cmp-fun kind)
+                             target-mvars2 target-mvars1 block-target negated))
         finally (cl-return-from in-the-basic-block)))
       (`((set ,(and (pred comp-mvar-p) cmp-res)
               (,(pred comp-call-op-p)
@@ -2426,16 +2547,26 @@ comp-add-cond-cstrs
 	 ;; (comment ,_comment-str)
 	 (cond-jump ,cmp-res ,(pred comp-mvar-p) . ,blocks))
        (cl-loop
-        with target-mvar = (comp-cond-cstrs-target-mvar op (car insns-seq) b)
+        with target-mvars = (comp-cond-cstrs-identical-vars
+                             (list op) b (car insns-seq))
         with cstr = (comp-pred-to-cstr fun)
         for branch-target-cell on blocks
         for branch-target = (car branch-target-cell)
         for negated in '(t nil)
-        when (comp-mvar-used-p target-mvar)
+        when target-mvars
         do
+        (setq target-mvars
+              (mapcar (lambda (mvar)
+                        (if (and
+                             (comp-mvar-p mvar)
+                             (equal (comp-mvar-slot mvar)
+                                    (comp-mvar-slot cmp-res)))
+                            (comp-cstr-copy mvar)
+                          mvar))
+                      target-mvars))
         (let ((block-target (comp-add-cond-cstrs-target-block b branch-target)))
           (setf (car branch-target-cell) (comp-block-name block-target))
-          (comp-emit-assume 'and target-mvar cstr block-target negated))
+          (comp-emit-assumes 'and target-mvars (list cstr) block-target negated t))
         finally (cl-return-from in-the-basic-block)))
       ;; Match predicate on the negated branch (unless).
       (`((set ,(and (pred comp-mvar-p) cmp-res)
@@ -2445,16 +2576,27 @@ comp-add-cond-cstrs
          (set ,neg-cmp-res (call eq ,cmp-res ,(pred comp-cstr-null-p)))
 	 (cond-jump ,neg-cmp-res ,(pred comp-mvar-p) . ,blocks))
        (cl-loop
-        with target-mvar = (comp-cond-cstrs-target-mvar op (car insns-seq) b)
+        with target-mvars = (comp-cond-cstrs-identical-vars
+                             (list op) b (car insns-seq))
         with cstr = (comp-pred-to-cstr fun)
         for branch-target-cell on blocks
         for branch-target = (car branch-target-cell)
         for negated in '(nil t)
-        when (comp-mvar-used-p target-mvar)
+        when target-mvars
         do
+        (setq target-mvars
+              (mapcar
+               (lambda (mvar)
+                 (if (and
+                      (comp-mvar-p mvar)
+                      (equal (comp-mvar-slot mvar)
+                             (comp-mvar-slot cmp-res)))
+                     (comp-cstr-copy mvar)
+                   mvar))
+               target-mvars))
         (let ((block-target (comp-add-cond-cstrs-target-block b branch-target)))
           (setf (car branch-target-cell) (comp-block-name block-target))
-          (comp-emit-assume 'and target-mvar cstr block-target negated))
+          (comp-emit-assumes 'and target-mvars (list cstr) block-target negated t))
         finally (cl-return-from in-the-basic-block)))))))
 
 (defsubst comp-insert-insn (insn insn-cell)
@@ -2465,13 +2607,14 @@ comp-insert-insn
           (cdr new-cell) next-cell
           (comp-func-ssa-status comp-func) 'dirty)))
 
-(defun comp-emit-call-cstr (mvar call-cell cstr)
+(defun comp-emit-call-cstrs (mvars call-cell cstr)
   "Emit a constraint CSTR for MVAR after CALL-CELL."
-  (let* ((new-mvar (make-comp-mvar :slot (comp-mvar-slot mvar)))
-         ;; Have new-mvar as LHS *and* RHS to ensure monotonicity and
-         ;; fwprop convergence!!
-         (insn `(assume ,new-mvar (and ,new-mvar ,mvar ,cstr))))
-    (comp-insert-insn insn call-cell)))
+  (dolist (mvar (cl-remove-if-not #'comp-mvar-p mvars))
+    (let* ((new-mvar (make-comp-mvar :slot (comp-mvar-slot mvar)))
+           ;; Have new-mvar as LHS *and* RHS to ensure monotonicity and
+           ;; fwprop convergence!!
+           (insn `(assume ,new-mvar (and ,new-mvar ,mvar ,cstr))))
+      (comp-insert-insn insn call-cell))))
 
 (defun comp-lambda-list-gen (lambda-list)
   "Return a generator to iterate over LAMBDA-LIST."
@@ -2508,18 +2651,24 @@ comp-add-call-cstr
           with gen = (comp-lambda-list-gen (comp-cstr-f-args cstr-f))
           for arg in args
           for cstr = (funcall gen)
-          for target = (comp-cond-cstrs-target-mvar arg insn bb)
+          for target-vars = (comp-cond-cstrs-identical-vars (list arg) bb insn)
           unless (comp-cstr-p cstr)
             do (signal 'native-ice
                        (list "Incoherent type specifier for function" f))
-          when (and target
+          do (setq target-vars (mapcar
+                                (lambda (mvar)
+                                  (if (and
+                                       (comp-mvar-p mvar)
+                                       (equal (comp-mvar-slot mvar)
+                                              (comp-mvar-slot lhs)))
+                                      (comp-cstr-copy mvar)
+                                    mvar))
+                                target-vars))
+          when (and target-vars
                     ;; No need to add call constraints if this is t
                     ;; (bug#45812 bug#45705 bug#45751).
-                    (not (equal comp-cstr-t cstr))
-                    (or (null lhs)
-                        (not (eql (comp-mvar-slot lhs)
-                                  (comp-mvar-slot target)))))
-            do (comp-emit-call-cstr target insn-cell cstr)))))))
+                    (not (equal comp-cstr-t cstr)))
+            do (comp-emit-call-cstrs target-vars insn-cell cstr)))))))
 
 (defun comp-add-cstrs (_)
   "Rewrite conditional branches adding appropriate 'assume' insns.
@@ -2529,7 +2678,7 @@ comp-add-cstrs
   (maphash (lambda (_ f)
              (when (and (>= (comp-func-speed f) 1)
                         ;; No point to run this on dynamic scope as
-                        ;; this pass is effecive only on local
+                        ;; this pass is effective only on local
                         ;; variables.
 			(comp-func-l-p f)
                         (not (comp-func-has-non-local f)))

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

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

Pip Cet <pipcet@gmail.com> writes:

> On Mon, Feb 22, 2021 at 1:12 PM Andrea Corallo <akrl@sdf.org> wrote:
>> Good catch thanks! :) Should be fixed by d6227f6edc.
>
> I'm also confused by the use of NEGATED in comp-emit-assume: if RHS is
> a constraint, it emits the complementary constraint.
>
> However, the code in comp-add-cond-cstrs uses NEGATED to express a
> much weaker constraint: that two mvars aren't strictly equal.
>
> If x /= y and y in SET, we can't conclude that x not in SET (unless
> SET is a singleton, an important special case).
>
> So it all works right now because emit-assume NEGATED=t RHS=mvar means
> "LHS isn't equal to RHS" but NEGATED=t RHS=cstr means "LHS can't
> satisfy RHS".
>
> My code changed the call to pass a constraint instead of the mvar, and
> then things broke :-)
>
> We should be consistent about what NEGATED means, I think.
>
> But apart from such problems, my code appears to be working. I'm
> attaching it for the sake of completeness, not because I expect you to
> read it all before it's cleaned up.

Hi Pip thanks for the patch,

the approach of adding a cstr directly in the assume works for this case
but is not generic as referencing there an mvar.

The reason is that a later run of fw-prop after add-cstrs might be able
to prove more precisely what the mvar is if the code was morphed in the
meanwhile by some other pass.  This in contrast with adding a cstr that
being "written into the stone" will stay as such no matter what.

Admittedly ATM the only pass rewriting some code after assumes are
placed and before the last fw-prop is run is 'tco' so this might be a
real case only for functions affected by this, but in the future we may
(and most likely want to) have more passes in that position of the
compiler pipeline.

So yeah I still prefer to general approach of keeping an mvar live till
there and referencing it in the assume so that any future propagation
within the SSA lattice can update this.

Yesterday evening I did some work in that direction, doesn't look too
invasive or complex.  I'll finish it this week as soon as I've some more
time to put into.

Thanks

  Andrea

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

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

Andrea Corallo via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs@gnu.org> writes:

[...]

> Hi Pip thanks for the patch,
>
> the approach of adding a cstr directly in the assume works for this case
> but is not generic as referencing there an mvar.
>
> The reason is that a later run of fw-prop after add-cstrs might be able
> to prove more precisely what the mvar is if the code was morphed in the
> meanwhile by some other pass.  This in contrast with adding a cstr that
> being "written into the stone" will stay as such no matter what.
>
> Admittedly ATM the only pass rewriting some code after assumes are
> placed and before the last fw-prop is run is 'tco' so this might be a
> real case only for functions affected by this, but in the future we may
> (and most likely want to) have more passes in that position of the
> compiler pipeline.
>
> So yeah I still prefer to general approach of keeping an mvar live till
> there and referencing it in the assume so that any future propagation
> within the SSA lattice can update this.
>
> Yesterday evening I did some work in that direction, doesn't look too
> invasive or complex.  I'll finish it this week as soon as I've some more
> time to put into.
>
> Thanks
>
>   Andrea

Right I've pushed bddd7a2d13 implementing the discussed solution as I'm
convinced is more general and future proof.

The patch is passing the tests and bootstrapping clean, also is adding a
test that should cover this specific bug.

Mauricio could you verify it actually solves the lsp-mode issue?

Thanks

  Andrea

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

[Mauricio Collares]

On February 23, 2021 8:26:25 PM GMT-03:00, Andrea Corallo <akrl@sdf.org> wrote:
>Andrea Corallo via "Bug reports for GNU Emacs, the Swiss army knife of
>text editors" <bug-gnu-emacs@gnu.org> writes:
>
>Right I've pushed bddd7a2d13 implementing the discussed solution as I'm
>convinced is more general and future proof.
>
>The patch is passing the tests and bootstrapping clean, also is adding a
>test that should cover this specific bug.
>
>Mauricio could you verify it actually solves the lsp-mode issue?

Hi Andrea,

I've verified that the lsp-mode completion issue is fixed. Many, many thanks! 

By the way, I just noticed that this is probably the same issue that was reported by X L and Óscar Fuentes in emacs-devel ('Completion doesn't start after "." Is pressed in go-mode with gopls').

Best,
Mauricio

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

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

Mauricio Collares <mauricio@collares.org> writes:

> On February 23, 2021 8:26:25 PM GMT-03:00, Andrea Corallo <akrl@sdf.org> wrote:
>>Andrea Corallo via "Bug reports for GNU Emacs, the Swiss army knife of
>>text editors" <bug-gnu-emacs@gnu.org> writes:
>>
>>Right I've pushed bddd7a2d13 implementing the discussed solution as I'm
>>convinced is more general and future proof.
>>
>>The patch is passing the tests and bootstrapping clean, also is adding a
>>test that should cover this specific bug.
>>
>>Mauricio could you verify it actually solves the lsp-mode issue?
>
> Hi Andrea,
>
> I've verified that the lsp-mode completion issue is fixed. Many, many thanks! 
>
> By the way, I just noticed that this is probably the same issue that
> was reported by X L and Óscar Fuentes in emacs-devel ('Completion
> doesn't start after "." Is pressed in go-mode with gopls').

Wonderful!  I'm closing this bug then.

Thank you for reporting it

  Andrea

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

[Pip Cet]

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

On Mon, Feb 22, 2021 at 1:12 PM Andrea Corallo <akrl@sdf.org> wrote:
> We'll probably see other bugs in this area cause is tricky, is important
> we build the best coverage we can for this in the testsuite.

Is this one of them, or am I confused?

[-- Attachment #2: 0001-Don-t-emit-incorrect-assumptions-for-non-equality-Bu.patch --]
[-- Type: text/x-patch, Size: 1102 bytes --]

From 9ab3a1e8196cce10eab50903302af1cc24580595 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Tue, 23 Feb 2021 19:05:53 +0000
Subject: [PATCH] Don't emit incorrect assumptions for non-equality (Bug#46770)

* lisp/emacs-lisp/comp.el (comp-add-cond-cstrs): Don't assume the
lhs and rhs coincide for a negated 'and constraint.
---
 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 60c040926e54c..847c5c424aaa8 100644
--- a/lisp/emacs-lisp/comp.el
+++ b/lisp/emacs-lisp/comp.el
@@ -2255,8 +2255,8 @@ comp-emit-assume
       ('and
        (if (comp-mvar-p rhs)
            (let ((tmp-mvar (if negated
-                               (make-comp-mvar :slot (comp-mvar-slot rhs))
-                             rhs)))
+                               (make-comp-mvar :slot (comp-mvar-slot lhs))
+                             lhs)))
              (push `(assume ,(make-comp-mvar :slot lhs-slot)
                             (and ,lhs ,tmp-mvar))
 	           (comp-block-insns bb))
-- 
2.30.0

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

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

Pip Cet <pipcet@gmail.com> writes:

> On Mon, Feb 22, 2021 at 1:12 PM Andrea Corallo <akrl@sdf.org> wrote:
>> We'll probably see other bugs in this area cause is tricky, is important
>> we build the best coverage we can for this in the testsuite.
>
> Is this one of them, or am I confused?

What's suspitions with that?  At present I'm admittedly quite done but
it looks okay to me.

BTW applying the patch and running comp-tests.el I get 39 regressions.

  Andrea

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

[Pip Cet]

On Tue, Feb 23, 2021 at 11:36 PM Andrea Corallo <akrl@sdf.org> wrote:
> Pip Cet <pipcet@gmail.com> writes:
>
> > On Mon, Feb 22, 2021 at 1:12 PM Andrea Corallo <akrl@sdf.org> wrote:
> >> We'll probably see other bugs in this area cause is tricky, is important
> >> we build the best coverage we can for this in the testsuite.
> >
> > Is this one of them, or am I confused?
>
> What's suspitions with that?  At present I'm admittedly quite done but
> it looks okay to me.

We're emitting

(assume ,lhs (and ,lhs ,rhs))

even when NEGATED is t.

> BTW applying the patch and running comp-tests.el I get 39 regressions.

That's expected, because we need to replace the incorrect assume by
correct ones to actually get to the same level of optimization.

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

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

Pip Cet <pipcet@gmail.com> writes:

> On Tue, Feb 23, 2021 at 11:36 PM Andrea Corallo <akrl@sdf.org> wrote:
>> Pip Cet <pipcet@gmail.com> writes:
>>
>> > On Mon, Feb 22, 2021 at 1:12 PM Andrea Corallo <akrl@sdf.org> wrote:
>> >> We'll probably see other bugs in this area cause is tricky, is important
>> >> we build the best coverage we can for this in the testsuite.
>> >
>> > Is this one of them, or am I confused?
>>
>> What's suspitions with that?  At present I'm admittedly quite done but
>> it looks okay to me.
>
> We're emitting
>
> (assume ,lhs (and ,lhs ,rhs))
>
> even when NEGATED is t.

Nope, when NEGATED is t the complete sequence we are emitting is (see
line just following your diff hunk):

(assume tmp-mvar (not rhs))
(assume lhs (and lhs tmp-mvar))

That's indeed the reason why it's working in the 39 testcases.

  Andrea

>> BTW applying the patch and running comp-tests.el I get 39 regressions.
>
> That's expected, because we need to replace the incorrect assume by
> correct ones to actually get to the same level of optimization.

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

[Pip Cet]

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

On Wed, Feb 24, 2021 at 9:04 AM Andrea Corallo <akrl@sdf.org> wrote:
> Pip Cet <pipcet@gmail.com> writes:
>
> > On Tue, Feb 23, 2021 at 11:36 PM Andrea Corallo <akrl@sdf.org> wrote:
> >> Pip Cet <pipcet@gmail.com> writes:
> >> > Is this one of them, or am I confused?
> >>
> >> What's suspitions with that?  At present I'm admittedly quite done but
> >> it looks okay to me.
> >
> > We're emitting
> >
> > (assume ,lhs (and ,lhs ,rhs))
> >
> > even when NEGATED is t.
>
> Nope, when NEGATED is t the complete sequence we are emitting is (see
> line just following your diff hunk):
>
> (assume tmp-mvar (not rhs))

But tmp-mvar is in the same slot as RHS.

> (assume lhs (and lhs tmp-mvar))

So this is equivalent (after the next SSA rename) to

(assume lhs (and lhs rhs))

> That's indeed the reason why it's working in the 39 testcases.

No, the reason it's "working" is that we never assert our assumes.
I've got a patch here that does just that :-)

[-- Attachment #2: 0001-Assert-at-runtime-that-assume-s-assumptions-actually.patch --]
[-- Type: text/x-patch, Size: 5583 bytes --]

From 6470b5ef25007ece8fea5754bee204e6b5ba0312 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Wed, 24 Feb 2021 09:25:44 +0000
Subject: [PATCH] Assert at runtime that assume's assumptions actually hold.

* src/comp.c (emit_limple_insn): Handle (assert) insns.
(Fcomp__assert): New function.
(syms_of_comp): New defsubr for above.

* lisp/emacs-lisp/comp.el (comp-passes): Add `comp-assert-assumes'
pass.
(comp-emit-assert, comp-assert-assumes-func, comp-assert-assumes):
New functions.
---
 lisp/emacs-lisp/comp.el | 40 ++++++++++++++++++++++++
 src/comp.c              | 69 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 109 insertions(+)

diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
index 60c040926e54c..d53b8876aa8ad 100644
--- a/lisp/emacs-lisp/comp.el
+++ b/lisp/emacs-lisp/comp.el
@@ -178,6 +178,7 @@ comp-passes
                         comp-tco
                         comp-fwprop
                         comp-remove-type-hints
+                        comp-assert-assumes
                         comp-final)
   "Passes to be executed in order.")
 
@@ -3385,6 +3386,45 @@ comp-remove-type-hints
            (comp-ctxt-funcs-h comp-ctxt)))
 
 \f
+;;; Assert-assumes pass specific code.
+(defun comp-emit-assert (lhs assertion)
+  (let (insns)
+    (pcase assertion
+      ((and (pred comp-mvar-p))
+       (let ((f `(lambda (x y) (equal x y))))
+         (comp-add-const-to-relocs f)
+         (push `(assert ,f ,lhs ,assertion) insns)))
+      ((and (pred comp-cstr-p)))
+      (`(and . ,operands)
+       (dolist (op operands)
+         (setq insns (nconc insns (comp-emit-assert lhs op)))))
+      (`(not ,(and (pred comp-mvar-p) operand))
+       (let ((f `(lambda (x y) (not (equal x y)))))
+         (comp-add-const-to-relocs f)
+         (push `(assert ,f ,lhs ,assertion) insns))))
+    insns))
+
+(defun comp-assert-assumes-func ()
+  (cl-loop
+   for b being each hash-value of (comp-func-blocks comp-func)
+   do (comp-loop-insn-in-block b
+        (pcase insn
+          (`(assume ,(and (pred comp-mvar-p) lhs)
+                    ,rhs)
+           (let ((f `(lambda (x y) (not (equal x y)))))
+             (comp-add-const-to-relocs f)
+             (setf (cdr insn-cell)
+                   (nconc (comp-emit-assert lhs rhs)
+                          (cdr insn-cell)))))))))
+
+(defun comp-assert-assumes (_)
+  "Turn (assume ...) insns into asserts."
+  (maphash (lambda (_ f)
+             (let ((comp-func f))
+               (comp-assert-assumes-func)
+               (comp-log-func comp-func 3)))
+           (comp-ctxt-funcs-h comp-ctxt)))
+\f
 ;;; Final pass specific code.
 
 (defun comp-args-to-lambda-list (args)
diff --git a/src/comp.c b/src/comp.c
index a8b8ef95fa14d..2a5eebee104ed 100644
--- a/src/comp.c
+++ b/src/comp.c
@@ -2037,6 +2037,58 @@ emit_limple_insn (Lisp_Object insn)
 			       n);
       emit_cond_jump (test, target1, target2);
     }
+  else if (EQ (op, Qassert))
+    {
+      Lisp_Object callee = Qcomp__assert;
+      emit_comment (SSDATA (Fprin1_to_string (arg[0], Qnil)));
+      imm_reloc_t reloc = obj_to_reloc (arg[0]);
+      gcc_jit_rvalue *assert_args[i];
+      assert_args[0] = gcc_jit_lvalue_as_rvalue (
+	gcc_jit_context_new_array_access (comp.ctxt,
+					  NULL,
+					  reloc.array.r_val,
+					  reloc.idx));
+      for (ptrdiff_t j = 1; j < i; j++)
+	assert_args[j] = emit_mvar_rval (arg[j]);
+
+      gcc_jit_lvalue *tmp_arr =
+	gcc_jit_function_new_local (
+				    comp.func,
+				    NULL,
+				    gcc_jit_context_new_array_type
+				    (comp.ctxt,
+				     NULL,
+				     comp.lisp_obj_type,
+				     i),
+				    "local_%d");
+
+      for (ptrdiff_t j = 0; j < i; j++)
+	{
+	  gcc_jit_block_add_assignment (
+					comp.block,
+					NULL,
+					gcc_jit_context_new_array_access (
+									  comp.ctxt,
+									  NULL,
+									  gcc_jit_lvalue_as_rvalue (tmp_arr),
+									  gcc_jit_context_new_rvalue_from_int (comp.ctxt,
+													       comp.int_type,
+													       j)),
+					assert_args[j]);
+	}
+
+      gcc_jit_rvalue *call = emit_call_ref (callee,
+		     i,
+		     gcc_jit_context_new_array_access (comp.ctxt,
+						       NULL,
+						       gcc_jit_lvalue_as_rvalue (tmp_arr),
+						       comp.zero),
+		     false);
+
+      gcc_jit_block_add_eval (comp.block,
+			      NULL,
+			      call);
+    }
   else if (EQ (op, Qphi) || EQ (op, Qassume))
     {
       /* Nothing to do for phis or assumes in the backend.  */
@@ -4980,6 +5032,20 @@ DEFUN ("comp--late-register-subr", Fcomp__late_register_subr,
   return Qnil;
 }
 
+DEFUN ("comp--assert", Fcomp__assert, Scomp__assert, 1, MANY, 0,
+       doc: /* */)
+  (ptrdiff_t nargs, Lisp_Object *args)
+{
+  if (NILP (Ffuncall (nargs, args)))
+    {
+      xsignal1 (Qnative_compiler_error,
+		Fcons (build_string ("assertion failed"),
+		       Flist (nargs, args)));
+    }
+
+  return Qnil;
+}
+
 static bool
 file_in_eln_sys_dir (Lisp_Object filename)
 {
@@ -5076,6 +5142,8 @@ syms_of_comp (void)
   DEFSYM (Qdirect_call, "direct-call");
   DEFSYM (Qdirect_callref, "direct-callref");
   DEFSYM (Qassume, "assume");
+  DEFSYM (Qassert, "assert");
+  DEFSYM (Qcomp__assert, "comp--assert");
   DEFSYM (Qsetimm, "setimm");
   DEFSYM (Qreturn, "return");
   DEFSYM (Qunreachable, "unreachable");
@@ -5179,6 +5247,7 @@ syms_of_comp (void)
   defsubr (&Scomp__register_lambda);
   defsubr (&Scomp__register_subr);
   defsubr (&Scomp__late_register_subr);
+  defsubr (&Scomp__assert);
   defsubr (&Snative_elisp_load);
 
   staticpro (&comp.exported_funcs_h);
-- 
2.30.0

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

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

Pip Cet <pipcet@gmail.com> writes:

> On Wed, Feb 24, 2021 at 9:04 AM Andrea Corallo <akrl@sdf.org> wrote:
>> Pip Cet <pipcet@gmail.com> writes:
>>
>> > On Tue, Feb 23, 2021 at 11:36 PM Andrea Corallo <akrl@sdf.org> wrote:
>> >> Pip Cet <pipcet@gmail.com> writes:
>> >> > Is this one of them, or am I confused?
>> >>
>> >> What's suspitions with that?  At present I'm admittedly quite done but
>> >> it looks okay to me.
>> >
>> > We're emitting
>> >
>> > (assume ,lhs (and ,lhs ,rhs))
>> >
>> > even when NEGATED is t.
>>
>> Nope, when NEGATED is t the complete sequence we are emitting is (see
>> line just following your diff hunk):
>>
>> (assume tmp-mvar (not rhs))
>
> But tmp-mvar is in the same slot as RHS.
>
>> (assume lhs (and lhs tmp-mvar))
>
> So this is equivalent (after the next SSA rename) to
>
> (assume lhs (and lhs rhs))

No sorry, after renaming this will be:

(assume rhs_2 (not rhs_1))
(assume lhs_2 (and lhs_1 rhs_2))

or if we prefer from an real dump:

(assume #(mvar 22593374 2 (not (integer 3 3))) (not #(mvar 22590962 2 (integer 3 3))))
(assume #(mvar 22593448 0 (or marker number)) (and #(mvar 22591258 0 (or marker number)) #(mvar 22593374 2 (not (integer 3 3)))))

Sorry my development time budget for this week has been almost entirely
consumed now, I'll try to come back on your many mails but I have other
duties in line.

  Andrea

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

[Pip Cet]

On Wed, Feb 24, 2021 at 9:42 AM Andrea Corallo <akrl@sdf.org> wrote:
> (assume #(mvar 22593374 2 (not (integer 3 3))) (not #(mvar 22590962 2 (integer 3 3))))

If that doesn't mean "the variable in slot 2 is not equal to the
variable in slot 2", we desperately need to work on the LIMPLE
syntax...

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

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

Pip Cet <pipcet@gmail.com> writes:

> On Wed, Feb 24, 2021 at 9:42 AM Andrea Corallo <akrl@sdf.org> wrote:
>> (assume #(mvar 22593374 2 (not (integer 3 3))) (not #(mvar 22590962 2 (integer 3 3))))
>
> If that doesn't mean "the variable in slot 2 is not equal to the
> variable in slot 2", we desperately need to work on the LIMPLE
> syntax...

Honestly I think would be fair from your side to first try to understand
how it works before criticizing it or submitting untested patches.

Indeed I'm happy to answer as I've explained what this means in my
previous mail.

And I've to say, not everything that's not working as you'd expect at
first glance is broken by definition, this is just not a very good
collaborative approach :/

  Andrea

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

[Pip Cet]

Hello Andrea,

On Wed, Feb 24, 2021 at 10:06 AM Andrea Corallo <akrl@sdf.org> wrote:
>
> Pip Cet <pipcet@gmail.com> writes:
>
> > On Wed, Feb 24, 2021 at 9:42 AM Andrea Corallo <akrl@sdf.org> wrote:
> >> (assume #(mvar 22593374 2 (not (integer 3 3))) (not #(mvar 22590962 2 (integer 3 3))))
> >
> > If that doesn't mean "the variable in slot 2 is not equal to the
> > variable in slot 2", we desperately need to work on the LIMPLE
> > syntax...
>
> Honestly I think would be fair from your side to first try to understand
> how it works before criticizing it or submitting untested patches.

I thought I'd give this some time to let tempers cool.

I appreciate your criticism (but not the ad hominem), and I will take
it into account when communicating with you.

I'm sorry you mistook the patches I sent as being submitted for
immediate inclusion: so far, that hasn't been my intention. They're
meant to demonstrate ideas and indicate which area I'm working on.
I'll try to make that clear in future.

As for the immediate issue: LIMPLE, of course, is yours to define as
you wish. If, however, you don't define it, either in documentation or
by providing code that handles it correctly, you can hardly blame me
for considering it a bug if the obvious interpretation causes subtle,
unnecessary problems.

To pick an example at random, after your recent changes, I assumed the
following would be valid LIMPLE (pseudocode):

(set (mvar :slot -1) (mvar :slot 0))

but it's not, because negatively-indexed "temporary variables" aren't
actually mapped to variables in the C backend (instead, they generate
out-of-bounds array accesses, usually a SIGSEGV).

If that is intentional, we need to document it (we also need to assert
rather than segfault when someone disregards this capricious rule) .
If it is unintentional, and I believe it is, do you really want me not
to point it out?

Lastly, on the subject of testing, I believe compiler correctness is
fundamentally more important than not missing any optimizations. Most
of your tests cover the latter aspect.  Maybe we could express that in
the tests somehow, by using another tag?

> Indeed I'm happy to answer as I've explained what this means in my
> previous mail.

I'm not sure I understand that sentence. I'll try looking through
previous messages from you for an explanation, I guess?

> And I've to say, not everything that's not working as you'd expect at
> first glance is broken by definition, this is just not a very good
> collaborative approach :/

My expectations are based, in part, on having read through your code,
including the comments. That's hardly "at first glance".

If I still misunderstand things fundamentally, it's possible, even
likely, that we need to add more documentation (and correct existing
documentation) explaining, for example, that meta-variables introduced
in an assume do not have a value and that their slot number is
meaningless. Once we've done that, we can discuss why I think that
would be a very bad design choice.

Regards
Pip

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

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Thu, 25 Feb 2021 12:41:42 +0000
> Cc: 46670@debbugs.gnu.org, Mauricio Collares <mauricio@collares.org>
> 
> My expectations are based, in part, on having read through your code,
> including the comments. That's hardly "at first glance".
> 
> If I still misunderstand things fundamentally, it's possible, even
> likely, that we need to add more documentation (and correct existing
> documentation) explaining, for example, that meta-variables introduced
> in an assume do not have a value and that their slot number is
> meaningless. Once we've done that, we can discuss why I think that
> would be a very bad design choice.

Please understand and keep in mind that the native-comp branch is
still WIP, and as such, it is not yet up to speed with all the
documentation and other necessary support info.  Andrea's work is
still mainly devoted to fixing issues reported against the native-comp
code.  Polishing the documentation will come later.  So please don't
expect the documentation to be anywhere near perfect, as we all are
used to expect from mainline Emacs code, and please don't blame Andrea
for not providing such quality of documentation at this point in time.

TIA

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

[Pip Cet]

On Thu, Feb 25, 2021 at 2:58 PM Eli Zaretskii <eliz@gnu.org> wrote:
> Please understand and keep in mind that the native-comp branch is
> still WIP, and as such, it is not yet up to speed with all the
> documentation and other necessary support info.

Thank you for that reminder.

You're absolutely correct, of course.

Andrea, I'm sorry I haven't made it clear that my standard is "is this
something that should be merged", not "is this very good code". It is
the latter, as I've said before, and I think the native-comp branch is
extremely interesting and should be merged VERY soon.

> Andrea's work is
> still mainly devoted to fixing issues reported against the native-comp
> code.  Polishing the documentation will come later.  So please don't
> expect the documentation to be anywhere near perfect, as we all are
> used to expect from mainline Emacs code, and please don't blame Andrea
> for not providing such quality of documentation at this point in time.

Again, you're absolutely correct. In the specific cases I mentioned,
my main concern is that other hackers will find it too frustrating to
encounter subtle restrictions that sometimes, incorrectly, feel like
pitfalls and look like bugs.

Thanks again,
Pip

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

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Thu, 25 Feb 2021 15:14:52 +0000
> Cc: Andrea Corallo <akrl@sdf.org>, 46670@debbugs.gnu.org, mauricio@collares.org
> 
> > Andrea's work is
> > still mainly devoted to fixing issues reported against the native-comp
> > code.  Polishing the documentation will come later.  So please don't
> > expect the documentation to be anywhere near perfect, as we all are
> > used to expect from mainline Emacs code, and please don't blame Andrea
> > for not providing such quality of documentation at this point in time.
> 
> Again, you're absolutely correct. In the specific cases I mentioned,
> my main concern is that other hackers will find it too frustrating to
> encounter subtle restrictions that sometimes, incorrectly, feel like
> pitfalls and look like bugs.

Any improvements to the documentation on the branch are of course very
welcome, regardless of the fact that it's still WIP.

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

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

Pip Cet <pipcet@gmail.com> writes:

> Hello Andrea,
>
> On Wed, Feb 24, 2021 at 10:06 AM Andrea Corallo <akrl@sdf.org> wrote:
>>
>> Pip Cet <pipcet@gmail.com> writes:
>>
>> > On Wed, Feb 24, 2021 at 9:42 AM Andrea Corallo <akrl@sdf.org> wrote:
>> >> (assume #(mvar 22593374 2 (not (integer 3 3))) (not #(mvar 22590962 2 (integer 3 3))))
>> >
>> > If that doesn't mean "the variable in slot 2 is not equal to the
>> > variable in slot 2", we desperately need to work on the LIMPLE
>> > syntax...
>>
>> Honestly I think would be fair from your side to first try to understand
>> how it works before criticizing it or submitting untested patches.
>
> I thought I'd give this some time to let tempers cool.
>
> I appreciate your criticism (but not the ad hominem), and I will take
> it into account when communicating with you.
>
> I'm sorry you mistook the patches I sent as being submitted for
> immediate inclusion: so far, that hasn't been my intention. They're
> meant to demonstrate ideas and indicate which area I'm working on.
> I'll try to make that clear in future.
>
> As for the immediate issue: LIMPLE, of course, is yours to define as
> you wish. If, however, you don't define it, either in documentation or
> by providing code that handles it correctly, you can hardly blame me
> for considering it a bug if the obvious interpretation causes subtle,
> unnecessary problems.
>
> To pick an example at random, after your recent changes, I assumed the
> following would be valid LIMPLE (pseudocode):
>
> (set (mvar :slot -1) (mvar :slot 0))
>
> but it's not, because negatively-indexed "temporary variables" aren't
> actually mapped to variables in the C backend (instead, they generate
> out-of-bounds array accesses, usually a SIGSEGV).
>
> If that is intentional, we need to document it (we also need to assert
> rather than segfault when someone disregards this capricious rule) .
> If it is unintentional, and I believe it is

Quoting myself this thread:

"The best option is to decide that negative slot numbers are not rendered
into libgccjit IR and we consider these virtual to solve these kind of
cases."

I agree this should be documented.  ATM LIMPLE is assumed to be correct
but an assertion in the back-end is a good idea.

As you know my time is constrained (well I guess this applies to most of
us here).  Tuesday night I wanted to fix this bug promising my-self to
come back on this for documenting what I've discussed here.  Indeed I've
to round-robin with all the other inputs I've (here and outside the
Emacs world) plus all the other things we have pending.

> Lastly, on the subject of testing, I believe compiler correctness is
> fundamentally more important than not missing any optimizations. Most
> of your tests cover the latter aspect.  Maybe we could express that in
> the tests somehow, by using another tag?

I'd like to state a concept that I think is very pertinent here and most
likely overlooked: The difference between propagation related bugs and
correctness bugs is very thin if *not* existent at all.  Many choices of
the compiler depends on what the propagation manage to proves.

Misscompilations like exactly this bug (46670) are a perfect example of.

I go further, these days this class of bugs is essentially the last we
see as misscompilations that gets reported.

That said, ATM in comp-tests.el we have ~150 tests of which ~60
verifying the value/type propagation through return type.

Indeed I'll be *very* happy to add any kind of test to the testsuite, as
I personally tried to do each time I fixed a misscompilation bug
reported on the list.

On this subject I highly recommend the following, let's adopt what we
essentially do for GCC development:

- each bug we want to call as such has to come with a reproducer that
  demostrates it.

- each patch has to come with a cover letter explaing why and what is
  doing.

- if the patch is to fix a bug the reproducer is added to the testsuite
  contextually as a testcase by the patch itself.

- patches submitted for inclusion must not cause regressions on the
  compiler test-suite and must bootstrap cleanly Emacs.

If this rules are not followed is just too difficult and, above all,
expensive from a time prespective to review.  This way of proceeding is
just the only sustainable.

We probably should document these points somewhere.

I myself follow all these rules with the exception of the cover letter
as I do not submit patches but just install them, tho I often try to
elaborate on the installed fix on this list as I actually did for this
specific bug.

>> Indeed I'm happy to answer as I've explained what this means in my
>> previous mail.
>
> I'm not sure I understand that sentence. I'll try looking through
> previous messages from you for an explanation, I guess?

In my previous message I've explained how that assume works and what's
his meaning.

>> And I've to say, not everything that's not working as you'd expect at
>> first glance is broken by definition, this is just not a very good
>> collaborative approach :/
>
> My expectations are based, in part, on having read through your code,
> including the comments. That's hardly "at first glance".

Unfortunatelly I understant that ATM reading the code and comments is
not sufficient to understand all mechanism and assumptions involved.

This is certainly in part because the compiler is just young and the
bring-up entered in final phase just now.

That said FWIW my experience with Emacs and other big projects like GCC
is that debugging and experimentation is typically needed to get into
the deepness of, the expectation that reading code and doc is sufficient
is IME at least often just a chimera.

Say I write a patch based on some assumption I'm convinced of and this
introduces a number of regressions.  I'd either pick the simplest non
passing testcase and debug it to see what I've been missing or I'd ask
if the assumption I used is correct.  I'd not claim the compiler is
broken in some of its parts to begin with.

Indeed as I've said will be my pleasure to answer any question if asked,
and even more to welcome any patch that improves the documentation as
outcome of this process.

> If I still misunderstand things fundamentally, it's possible, even
> likely, that we need to add more documentation (and correct existing
> documentation) explaining, for example, that meta-variables introduced
> in an assume do not have a value and that their slot number is
> meaningless.

Well it's not meaningless as is used in SSA renaming but again agreed
more documentation would be an improvement.

> Once we've done that, we can discuss why I think that
> would be a very bad design choice.

I hope you don't think something you state is likely not to be fully
unrestood for now by you is already considered a very bad design choice.

> Regards
> Pip
>

Please just reflect on the following:

Bringing up this compiler to the point of having a functional Emacs
based on it took an enormous quantity time divided in: coding, studying,
trial and error, debugging, thinking (not in this other).

As I know you are very used to work with compilers and low level
programming I'm sure you'll have no problem understanding that the
number of commits of this branch does not reflect the time it took to
bring it up and verifying it to the point is usable as it is today.

This was not to cry but just to say: if something is not as you expect,
starting from the assumption that the compiler is working, there are
most likely two reasons for that:

1- Some history, thinking, trial and error proved this to be a good
   solution.

2- Was deemed a non ideal solution but good enough at that moment.
   Bringing up such a large system implies sometimes to compromise on a
   single piece of the puzzle waiting for another to come in place.
   Variable slot naming is one of this cases.

   If there's something I've learned is that improvements in complex
   systems are by necessity just incremental, the trick is to find the
   best path through.

Indeed as I said, within time constraints, I'm happy to elaborate on
specific cases or design choices if asked.  But no solution falling in
cases 1 or 2 deserves to be called broken, bad or badly designed.
Please lets start from the assumtion that, to begin with, it works.

Thanks for your understanding, this will be very appreciated trust me.

  Andrea

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

[Pip Cet]

On Thu, Feb 25, 2021 at 4:56 PM Andrea Corallo <akrl@sdf.org> wrote:
> Pip Cet <pipcet@gmail.com> writes:
> > On Wed, Feb 24, 2021 at 10:06 AM Andrea Corallo <akrl@sdf.org> wrote:
> > I thought I'd give this some time to let tempers cool.
> >
> > I appreciate your criticism (but not the ad hominem), and I will take
> > it into account when communicating with you.

I'd like to repeat that point. I do agree with some of the points you raise.

> > As for the immediate issue: LIMPLE, of course, is yours to define as
> > you wish. If, however, you don't define it, either in documentation or
> > by providing code that handles it correctly, you can hardly blame me
> > for considering it a bug if the obvious interpretation causes subtle,
> > unnecessary problems.

Again, that's three conditions that need to happen simultaneously:
 - no documentation
 - no code
 - behavior that contradicts the obvious interpretation

In the case of `assume', all three are met: there's no documentation
describing it, there's (currently) no code that uses assumes in a
non-trivial fashion, and it doesn't mean "assume".

> > To pick an example at random, after your recent changes, I assumed the
> > following would be valid LIMPLE (pseudocode):
> >
> > (set (mvar :slot -1) (mvar :slot 0))
> >
> > but it's not, because negatively-indexed "temporary variables" aren't
> > actually mapped to variables in the C backend (instead, they generate
> > out-of-bounds array accesses, usually a SIGSEGV).
> >
> > If that is intentional, we need to document it (we also need to assert
> > rather than segfault when someone disregards this capricious rule) .
> > If it is unintentional, and I believe it is
>
> Quoting myself this thread:
>
> "The best option is to decide that negative slot numbers are not rendered
> into libgccjit IR and we consider these virtual to solve these kind of
> cases."

And then, much later, you were no longer talking about "virtual"
negative slot numbers, you were talking about temporary variables (not
metavariables) being kept live, and being created for any purpose.
Mixed signals, at best.

> I agree this should be documented.  ATM LIMPLE is assumed to be correct
> but an assertion in the back-end is a good idea.

(At some point in the future, the backend needs to reject (not
eassert, but error) invalid LIMPLE rather than crashing the Emacs
process. I understand we're not there yet.)

> > Lastly, on the subject of testing, I believe compiler correctness is
> > fundamentally more important than not missing any optimizations. Most
> > of your tests cover the latter aspect.  Maybe we could express that in
> > the tests somehow, by using another tag?
>
> I'd like to state a concept that I think is very pertinent here and most
> likely overlooked: The difference between propagation related bugs and
> correctness bugs is very thin if *not* existent at all.  Many choices of
> the compiler depends on what the propagation manage to proves.

I'm not sure what you're saying or how it relates to what I said.

> Misscompilations like exactly this bug (46670) are a perfect example of.

Of what? This is clearly a correctness bug, not a missed optimization.
If fixing it (and actually fixing the underlying issue, which I
believe we haven't done yet) involves temporarily disabling some
optimizations, then that's what we need to do. We can restore the
optimizations later.

> That said, ATM in comp-tests.el we have ~150 tests of which ~60
> verifying the value/type propagation through return type.

(Yes. That doesn't contradict anything I said.)

> On this subject I highly recommend the following, let's adopt what we
> essentially do for GCC development:

You might want to suggest that on emacs-devel, as it would be a very
drastic change. If you, as an individual, want to stop responding to
bug reports that don't meet your strict criteria, I don't think
there's anyone trying to stop you. Just define a nice keyboard macro
saying "I'm sorry, you need to jump through these hoops first".

> If this rules are not followed is just too difficult and, above all,
> expensive from a time prespective to review.  This way of proceeding is
> just the only sustainable.

I try to avoid speaking in absolutes like that. There's more than one
way to do it.

> We probably should document these points somewhere.

We should probably discuss and agree on them first.

> >> Indeed I'm happy to answer as I've explained what this means in my
> >> previous mail.
> >
> > I'm not sure I understand that sentence. I'll try looking through
> > previous messages from you for an explanation, I guess?
>
> In my previous message I've explained how that assume works and what's
> his meaning.

I don't think so. We're looking at (assume mvar1 (not mvar2)) where
the two mvars have overlapping lifetimes and share the same slot. That
specializes to a contradiction.

> >> And I've to say, not everything that's not working as you'd expect at
> >> first glance is broken by definition, this is just not a very good
> >> collaborative approach :/
> >
> > My expectations are based, in part, on having read through your code,
> > including the comments. That's hardly "at first glance".
>
> Unfortunatelly I understant that ATM reading the code and comments is
> not sufficient to understand all mechanism and assumptions involved.

Of course not. I didn't claim otherwise.

> This is certainly in part because the compiler is just young

It is, which is why assuming it's impeccably correct is not the
productive approach right now.

> That said FWIW my experience with Emacs and other big projects like GCC
> is that debugging and experimentation is typically needed to get into
> the deepness of, the expectation that reading code and doc is sufficient
> is IME at least often just a chimera.

And I didn't say it was sufficient.

> Say I write a patch based on some assumption I'm convinced of and this
> introduces a number of regressions.

I did: the assumption I'm convinced of is that an assume translates
into a test at runtime that must be true.

> I'd either pick the simplest non
> passing testcase and debug it to see what I've been missing

I did. The regressions were optimizations that (with an exception,
which you fixed) produced correct results from what I'm still
convinced are incorrect intermediate representations.

> or I'd ask
> if the assumption I used is correct.

I did. Your response can be summarized as "the tests are passing so
there's no bug".

> I'd not claim the compiler is
> broken in some of its parts to begin with.

After considering the alternatives, that still seems overwhelmingly
likely to me.

> Indeed as I've said will be my pleasure to answer any question if asked,
> and even more to welcome any patch that improves the documentation as
> outcome of this process.

What, if anything, does "(assume mvar1 (not mvar2))" mean?

> > Once we've done that, we can discuss why I think that
> > would be a very bad design choice.
>
> I hope you don't think something you state is likely not to be fully
> unrestood for now by you is already considered a very bad design choice.

Of course I don't.

> Please lets start from the assumtion that, to begin with, it works.

I find "there is no bug" to be an unhelpful axiom when looking for a bug.

Pip

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

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

Pip Cet <pipcet@gmail.com> writes:

>> On this subject I highly recommend the following, let's adopt what we
>> essentially do for GCC development:
>
> You might want to suggest that on emacs-devel, as it would be a very
> drastic change. If you, as an individual, want to stop responding to
> bug reports that don't meet your strict criteria, I don't think
> there's anyone trying to stop you. Just define a nice keyboard macro
> saying "I'm sorry, you need to jump through these hoops first".
>
>> If this rules are not followed is just too difficult and, above all,
>> expensive from a time prespective to review.  This way of proceeding is
>> just the only sustainable.
>
> I try to avoid speaking in absolutes like that. There's more than one
> way to do it.
>
>> We probably should document these points somewhere.
>
> We should probably discuss and agree on them first.

Sorry I realized I've been not clear, I was only talking about native
compiler misscompilation bugs.  There's no need to suggest or discuss
that, this is how I work and will as long as I maintain the native
compiler.

  Andrea

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

[Pip Cet]

On Fri, Feb 26, 2021 at 7:33 PM Andrea Corallo <akrl@sdf.org> wrote:
> Pip Cet <pipcet@gmail.com> writes:
> Sorry I realized I've been not clear, I was only talking about native
> compiler misscompilation bugs.

Ah, that makes sense. Thanks for the clarification!

> There's no need to suggest or discuss
> that, this is how I work and will as long as I maintain the native
> compiler.

Again, not trying to stop you from applying your own criteria. I
certainly agree that if it doesn't come with a reproducer, it cannot
be a miscompilation bug.

Pip

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

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

Pip Cet <pipcet@gmail.com> writes:

> On Fri, Feb 26, 2021 at 7:33 PM Andrea Corallo <akrl@sdf.org> wrote:
>> Pip Cet <pipcet@gmail.com> writes:
>> Sorry I realized I've been not clear, I was only talking about native
>> compiler misscompilation bugs.
>
> Ah, that makes sense. Thanks for the clarification!
>
>> There's no need to suggest or discuss
>> that, this is how I work and will as long as I maintain the native
>> compiler.
>
> Again, not trying to stop you from applying your own criteria. I
> certainly agree that if it doesn't come with a reproducer, it cannot
> be a miscompilation bug.

A compiler does essentially one thing, compiling.  IOW here I'm
referring to everything that is not infrastructure integration.

  Andrea

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

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Thu, 25 Feb 2021 20:59:41 +0000
> Cc: 46670@debbugs.gnu.org, Mauricio Collares <mauricio@collares.org>
> 
> > On this subject I highly recommend the following, let's adopt what we
> > essentially do for GCC development:
> 
> You might want to suggest that on emacs-devel, as it would be a very
> drastic change.

AFAICT, the principles proposed by Andrea are just common sense, and
definitely not a drastic change from our existing practices.

Granted, not everyone lives up to those goals, but everyone should
try.

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

[Pip Cet]

On Fri, Feb 26, 2021 at 8:12 PM Eli Zaretskii <eliz@gnu.org> wrote:

> AFAICT, the principles proposed by Andrea are just common sense, and
> definitely not a drastic change from our existing practices.

I misunderstood Andrea. For miscompilation bugs he's entirely correct,
but there are other bugs.

> Granted, not everyone lives up to those goals, but everyone should
> try.

Point taken, I believe.

Thanks
Pip

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

[Pip Cet]

On Fri, Feb 26, 2021 at 8:12 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Thu, 25 Feb 2021 20:59:41 +0000
> > Cc: 46670@debbugs.gnu.org, Mauricio Collares <mauricio@collares.org>
> >
> > > On this subject I highly recommend the following, let's adopt what we
> > > essentially do for GCC development:
> >
> > You might want to suggest that on emacs-devel, as it would be a very
> > drastic change.
>
> AFAICT, the principles proposed by Andrea are just common sense, and
> definitely not a drastic change from our existing practices.

Let me try to explain a situation in which I don't think they work
very well, and which may or may not be similar to the situation we're
actually in:

1. We're emitting strange "assume" insns.
2. These are pseudo-insns which are not rendered into functional code.
3. We do not have a facility for converting these "assume" insns into
functional code which asserts they hold at runtime.
4. We have test cases which ensure the "assume" insns are actually
generated as they currently are.

How, assuming for the moment that the "strange" in (1) actually means
"buggy", are we supposed to fix this?

A patch changing (1) will be rejected as invalid because there is no
reproducer. It will also be rejected as broken because the test cases
will fail.
A patch changing (2) (e.g. a new compiler feature which makes use of
the assumes) will be rejected as broken because it will generate
incorrect code.
A patch changing (3) will be rejected because the new assertions will
initially fail, because of (1).
A patch changing (4) will be rejected because it would mean dropping
tests which appear to work.

A patch changing (1), (3), and (4) at the same time will be rejected
because it wouldn't be incremental.

I think, but am willing to be convinced I'm wrong, that this is the
situation we're in. I can prepare patches changing any combination of
(1), (2), (3), and (4).

Pip

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

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Sat, 27 Feb 2021 05:06:43 +0000
> Cc: Andrea Corallo <akrl@sdf.org>, 46670@debbugs.gnu.org, mauricio@collares.org
> 
> > AFAICT, the principles proposed by Andrea are just common sense, and
> > definitely not a drastic change from our existing practices.
> 
> Let me try to explain a situation in which I don't think they work
> very well, and which may or may not be similar to the situation we're
> actually in:
> 
> 1. We're emitting strange "assume" insns.
> 2. These are pseudo-insns which are not rendered into functional code.
> 3. We do not have a facility for converting these "assume" insns into
> functional code which asserts they hold at runtime.
> 4. We have test cases which ensure the "assume" insns are actually
> generated as they currently are.
> 
> How, assuming for the moment that the "strange" in (1) actually means
> "buggy", are we supposed to fix this?

I don't see any evidence yet that this needs to be fixed.  Without
such evidence, the whole discussion is about a moot point.  Maybe I
don't understand the issue well enough?

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

[Pip Cet]

On Sat, Feb 27, 2021 at 7:49 AM Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Sat, 27 Feb 2021 05:06:43 +0000
> > Cc: Andrea Corallo <akrl@sdf.org>, 46670@debbugs.gnu.org, mauricio@collares.org
> >
> > > AFAICT, the principles proposed by Andrea are just common sense, and
> > > definitely not a drastic change from our existing practices.
> >
> > Let me try to explain a situation in which I don't think they work
> > very well, and which may or may not be similar to the situation we're
> > actually in:
[...]
> > How, assuming for the moment that the "strange" in (1) actually means
> > "buggy", are we supposed to fix this?
>
> I don't see any evidence yet that this needs to be fixed.
> Without
> such evidence, the whole discussion is about a moot point.

Quite the reverse: if we make rules saying such bug reports are to be
ignored, as Andrea suggested, actually reporting the bug is moot. It's
the rules I objected to in the previous mail, not the legitimate
requirement for further elaboration on my part before anyone else is
convinced there's a bug.

> Maybe I don't understand the issue well enough?

I'm certainly in no position to say I understand it perfectly and I
can explain it to you.

The problem is that "assume" insns do not have semantics yet: they
don't behave as you would expect "assume" to behave; they aren't
documented to behave differently; and there is no code (yet) which
uses them in ways that would make clear what they are supposed to
mean.

There is, I am convinced, no consistent way of _giving_ them semantics
without changing the assume insns we emit, first. For example, we're
emitting

<#(mvar Y :slot 1) is live>
(assume #(mvar X :slot 1) (not #(mvar Y :slot 1)))
<#(mvar X :slot 1) is live>

That's paradoxical on the face of it, as the two mvars refer to the
same variable. If there is a consistent nontrivial interpretation of
"assume" that would work in this case, I'm unaware of it.

Note that "assume" as we know and love it has very simple semantics:
it expresses a condition which, at runtime, is known to be satisfied.
You can convert an assume into a runtime test, and if it fails, the
assume was wrong in the first place.

That's not the case here, since the assume above would translate into

(assert (not (eq #(mvar X :slot 1) #(mvar Y :slot 1))))

which, obviously, never succeeds.

The fix is as trivial as not saying that the mvar X lives in slot 1.
There's actually a different slot that it does live in, and my patch
would have used that slot instead. It would have been equally valid
simply not to give it a slot at all, by whichever mechanism is
appropriate for that (I would have left the slot slot nil, but if
Andrea prefers assigning a negative "virtual" slot number to the mvar,
that's a valid choice as well).

There's a very similar issue with the other branch of the "if", as it turns out.

All of this was sufficient for me to write Andrea asking whether there
was an issue (leaving out what I thought would be, to him, the tedious
trivialities of the lines above), or whether I was confused. I could
certainly have handled the ensuing exchange better, and I'll try to do
so in the future. However, the categorical instalment of new rules
disallowing the presentation of patches or bug reports for all bugs
outside a very narrow class would prevent that entirely.

Pip

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

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Sat, 27 Feb 2021 09:39:50 +0000
> Cc: Andrea Corallo <akrl@sdf.org>, 46670@debbugs.gnu.org, mauricio@collares.org
> 
> > > How, assuming for the moment that the "strange" in (1) actually means
> > > "buggy", are we supposed to fix this?
> >
> > I don't see any evidence yet that this needs to be fixed.  Without
> > such evidence, the whole discussion is about a moot point.
> 
> Quite the reverse: if we make rules saying such bug reports are to be
> ignored, as Andrea suggested, actually reporting the bug is moot. It's
> the rules I objected to in the previous mail, not the legitimate
> requirement for further elaboration on my part before anyone else is
> convinced there's a bug.

Reports are never ignored, because someone needs to read them before
deciding whether they need any action.  But that's beside the point.

My point is that if those "assume" forms never generate any real code
in the produced .eln file, then why worry about them?  They are like
comments: if you don't like comments, just ignore them; they don't
actually affect any executable code.

> All of this was sufficient for me to write Andrea asking whether there
> was an issue (leaving out what I thought would be, to him, the tedious
> trivialities of the lines above), or whether I was confused. I could
> certainly have handled the ensuing exchange better, and I'll try to do
> so in the future. However, the categorical instalment of new rules
> disallowing the presentation of patches or bug reports for all bugs
> outside a very narrow class would prevent that entirely.

There are no rules that disallow presentation of patches.  But anyone
who presents patches must understand that people who review those
patches could decide the issue the patches try to fix is not worth
fixing, or isn't a problem in the first place.  If you want to avoid
such decisions, _then_ you need to satisfy those rules.  IOW, they
aren't rules for submitting stuff, they are rules for considering the
submitted stuff as significant and for attracting the attention of the
individuals responsible for the respective code.

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

[Pip Cet]

On Sat, Feb 27, 2021 at 10:24 AM Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Sat, 27 Feb 2021 09:39:50 +0000
> > Cc: Andrea Corallo <akrl@sdf.org>, 46670@debbugs.gnu.org, mauricio@collares.org
> >
> > > > How, assuming for the moment that the "strange" in (1) actually means
> > > > "buggy", are we supposed to fix this?
> > >
> > > I don't see any evidence yet that this needs to be fixed.  Without
> > > such evidence, the whole discussion is about a moot point.
> >
> > Quite the reverse: if we make rules saying such bug reports are to be
> > ignored, as Andrea suggested, actually reporting the bug is moot. It's
> > the rules I objected to in the previous mail, not the legitimate
> > requirement for further elaboration on my part before anyone else is
> > convinced there's a bug.
>
> Reports are never ignored, because someone needs to read them before
> deciding whether they need any action.  But that's beside the point.

"Deciding something doesn't need any (actual) attention" is pretty
close to the dictionary definition of "ignore".

After some playing around, I've now found a Lisp reproducer that is
mis-compiled because of the bug I reported.

However, I'd like to raise a general point:

The compiler consists of three stages. The first maps LAP code
programs to LIMPLE; the second stage transforms the LIMPLE tree into
another LIMPLE tree which is meant still to represent the same
program; the third translates LIMPLE to the libgccjit internal
representation.

Say we've found, as I have, an apparent bug in the second stage: a
LIMPLE tree representing one program is transformed into a LIMPLE tree
representing another, different program.

There are three possibilities:

1. it's easy to construct a preimage of such a LIMPLE tree under the
Lisp-to-LIMPLE transformation. Then we most likely have a reproducible
miscompilation bug, and we can fix it.
2. it's easy to prove that no erroneously-transformed LIMPLE tree is
arrived at by the first stage. In this case, we have no bug.
3. it's difficult to figure out whether the erroneously-transformed
LIMPLE tree can arise as a result of Lisp-to-LIMPLE transformation.

Case 3 is the difficult one. It's also the most common one, and the
one that we were talking about here until I found my example.

My strong opinion is that we must treat case #3 as a potential,
serious, miscompilation bug. Andrea's opinion, IIUC, is that we should
ignore case #3.

More importantly, when faced with a bug of case #1, Andrea's approach
might be to turn it into a case #3. Mine would be to fix it. That's
the situation we're in now.

> My point is that if those "assume" forms never generate any real code
> in the produced .eln file, then why worry about them?

They don't generate code themselves, but they affect later stages of
the code generation process and thus, ultimately, the real code that
results.

However, the task of proving that this actually results in a
Lisp-to-machine-code bug is, in general, too much to expect the
initial discoverer to perform.

> They are like
> comments: if you don't like comments, just ignore them; they don't
> actually affect any executable code.

They're not. We could redefine them to be, but we'd first have to
remove all code that uses them. And then that would leave us with
LIMPLE constructs that are quite literally meaningless.

> > All of this was sufficient for me to write Andrea asking whether there
> > was an issue (leaving out what I thought would be, to him, the tedious
> > trivialities of the lines above), or whether I was confused. I could
> > certainly have handled the ensuing exchange better, and I'll try to do
> > so in the future. However, the categorical instalment of new rules
> > disallowing the presentation of patches or bug reports for all bugs
> > outside a very narrow class would prevent that entirely.
>
> There are no rules that disallow presentation of patches.

Thanks for clarifying that.

Pip

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

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Sat, 27 Feb 2021 12:39:48 +0000
> Cc: Andrea Corallo <akrl@sdf.org>, 46670@debbugs.gnu.org, mauricio@collares.org
> 
> However, I'd like to raise a general point:

Is it really useful to present general points, and in a bug discussion
of all places?  I'd be happy if we had all the non-general points
figured out, and leave the general ones to people who have more free
time on their hands, you know?

> The compiler consists of three stages. The first maps LAP code
> programs to LIMPLE; the second stage transforms the LIMPLE tree into
> another LIMPLE tree which is meant still to represent the same
> program; the third translates LIMPLE to the libgccjit internal
> representation.
> 
> Say we've found, as I have, an apparent bug in the second stage: a
> LIMPLE tree representing one program is transformed into a LIMPLE tree
> representing another, different program.
> 
> There are three possibilities:
> 
> 1. it's easy to construct a preimage of such a LIMPLE tree under the
> Lisp-to-LIMPLE transformation. Then we most likely have a reproducible
> miscompilation bug, and we can fix it.
> 2. it's easy to prove that no erroneously-transformed LIMPLE tree is
> arrived at by the first stage. In this case, we have no bug.
> 3. it's difficult to figure out whether the erroneously-transformed
> LIMPLE tree can arise as a result of Lisp-to-LIMPLE transformation.
> 
> Case 3 is the difficult one. It's also the most common one, and the
> one that we were talking about here until I found my example.
> 
> My strong opinion is that we must treat case #3 as a potential,
> serious, miscompilation bug. Andrea's opinion, IIUC, is that we should
> ignore case #3.

My strong opinion is that in the Free Software world (and not only in
that, but let's forget about the rest for a moment), whoever does the
job gets to choose the methods and the methodology.  Andrea is doing
this job, he is our specialist on these issues, and he is developing
the code and maintaining it.  As long as he does that, his opinions
on the relevant parts of Emacs weigh more than anyone else's, mine
included.  So if we come up with case #3, and Andrea thinks it's a
non-issue, I tend to accept his judgment, especially after he
responded to your messages with sound reasons.

Please understand that any other way, we'd lose Andrea and any other
volunteers who come to us with significant new features.  We cannot
expect them to go to too great lengths doing the job on our terms.
The basic requirements of contributing to Emacs are already not easy
to satisfy; if we start challenging their technical judgment about
code they themselves designed and implemented, that'd become
unbearable.

> More importantly, when faced with a bug of case #1, Andrea's approach
> might be to turn it into a case #3. Mine would be to fix it. That's
> the situation we're in now.

What matters to me at this point is the end result.  Any issue that
causes mis-compilation of Lisp programs should be fixed, of course.
Issues that don't affect the natively-compiled code are much less
important, and as I explained, my tendency is to accept Andrea's
judgment on those.

> > My point is that if those "assume" forms never generate any real code
> > in the produced .eln file, then why worry about them?
> 
> They don't generate code themselves, but they affect later stages of
> the code generation process and thus, ultimately, the real code that
> results.
> 
> However, the task of proving that this actually results in a
> Lisp-to-machine-code bug is, in general, too much to expect the
> initial discoverer to perform.

The initial discoverer doesn't have to do that, it's enough to raise
the issue.  The issue has been raised, and it wasn't dismissed: Andrea
did consider it and did fix a couple of issues you brought up that he
thought were worth fixing.  What's left now is just the gray area,
where I trust Andrea more than anyone else.

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

[Pip Cet]

On Sat, Feb 27, 2021 at 1:30 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > However, I'd like to raise a general point:
>
> Is it really useful to present general points, and in a bug discussion
> of all places?  I'd be happy if we had all the non-general points
> figured out, and leave the general ones to people who have more free
> time on their hands, you know?

I agree, but I'll resort to the kindergarten defense. He started it :-)

> > The compiler consists of three stages. The first maps LAP code
> > programs to LIMPLE; the second stage transforms the LIMPLE tree into
> > another LIMPLE tree which is meant still to represent the same
> > program; the third translates LIMPLE to the libgccjit internal
> > representation.
> >
> > Say we've found, as I have, an apparent bug in the second stage: a
> > LIMPLE tree representing one program is transformed into a LIMPLE tree
> > representing another, different program.
> >
> > There are three possibilities:
> >
> > 1. it's easy to construct a preimage of such a LIMPLE tree under the
> > Lisp-to-LIMPLE transformation. Then we most likely have a reproducible
> > miscompilation bug, and we can fix it.
> > 2. it's easy to prove that no erroneously-transformed LIMPLE tree is
> > arrived at by the first stage. In this case, we have no bug.
> > 3. it's difficult to figure out whether the erroneously-transformed
> > LIMPLE tree can arise as a result of Lisp-to-LIMPLE transformation.
> >
> > Case 3 is the difficult one. It's also the most common one, and the
> > one that we were talking about here until I found my example.
> >
> > My strong opinion is that we must treat case #3 as a potential,
> > serious, miscompilation bug. Andrea's opinion, IIUC, is that we should
> > ignore case #3.
>
> My strong opinion is that in the Free Software world (and not only in
> that, but let's forget about the rest for a moment), whoever does the
> job gets to choose the methods and the methodology.  Andrea is doing
> this job, he is our specialist on these issues, and he is developing
> the code and maintaining it.

Absolutely.

> As long as he does that, his opinions
> on the relevant parts of Emacs weigh more than anyone else's, mine
> included.  So if we come up with case #3, and Andrea thinks it's a
> non-issue, I tend to accept his judgment,

That's okay.

If there is judgment. If it is an automated response to the fact that
there's no (Lisp) reproducer included with the bug report, we
shouldn't trust it; we should simply accept that it might still be a
bug.

> especially after he
> responded to your messages with sound reasons.

I take it you've read through the code, understood it all, and
concluded the reasons were "sound", then? (I haven't; I've tentatively
concluded they were unsound, in fact).

("Sound", as I'm sure Eli knows, doesn't mean "sounds good")

> Please understand that any other way, we'd lose Andrea and any other
> volunteers who come to us with significant new features.

I have my own opinions about why Emacs attracts so few volunteers and
drives away so many of those who might be.

> We cannot
> expect them to go to too great lengths doing the job on our terms.

I agree.

> The basic requirements of contributing to Emacs are already not easy
> to satisfy; if we start challenging their technical judgment about
> code they themselves designed and implemented, that'd become
> unbearable.

Again, I have my own opinions about basing recruitment strategies on
those we recruited successfully, rather than including in our sums
those we've driven away.

> > More importantly, when faced with a bug of case #1, Andrea's approach
> > might be to turn it into a case #3. Mine would be to fix it. That's
> > the situation we're in now.
>
> What matters to me at this point is the end result.
> Any issue that
> causes mis-compilation of Lisp programs should be fixed, of course.
> Issues that don't affect the natively-compiled code are much less
> important, and as I explained, my tendency is to accept Andrea's
> judgment on those.

There's a difference between "this issue doesn't affect
natively-compiled code" (which makes it a non-issue, case 2 above) and
"we don't know whether this issue affects natively-compiled code"
(which emphatically does not, case 3 above). Evidence of absence and
all that.

> > > My point is that if those "assume" forms never generate any real code
> > > in the produced .eln file, then why worry about them?
> >
> > They don't generate code themselves, but they affect later stages of
> > the code generation process and thus, ultimately, the real code that
> > results.
> >
> > However, the task of proving that this actually results in a
> > Lisp-to-machine-code bug is, in general, too much to expect the
> > initial discoverer to perform.
>
> The initial discoverer doesn't have to do that, it's enough to raise
> the issue.

Andrea has stated that if there's no reproducer, he doesn't consider
the issue a bug. So "raising the issue" would do, according to the
rules he proposed, precisely nothing.

> The issue has been raised, and it wasn't dismissed: Andrea

Note the "in general" in my statement.

IIUC, you want me to raise future issues and wait for them to be
dismissed rather than complaining, as I am, about the mere
announcement that they will be? That's certainly something I can do,
and resolving that I'll do that would actually be a good result of
this discussion.

> did consider it and did fix a couple of issues you brought up that he
> thought were worth fixing.

Yes. And he said he wouldn't do so in future, for issues of this category.


Pip

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

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Sat, 27 Feb 2021 17:15:20 +0000
> Cc: Andrea Corallo <akrl@sdf.org>, 46670@debbugs.gnu.org, mauricio@collares.org
> 
> > especially after he
> > responded to your messages with sound reasons.
> 
> I take it you've read through the code, understood it all, and
> concluded the reasons were "sound", then?

I have my own ways of judging what people say and deciding when they
are sound and when they aren't.  If you want to question my judgment
as well, you are talking to the wrong guy.

> > Please understand that any other way, we'd lose Andrea and any other
> > volunteers who come to us with significant new features.
> 
> I have my own opinions about why Emacs attracts so few volunteers and
> drives away so many of those who might be.

You are welcome to step up to be the Emacs maintainer, and then act
according to your opinions.

> > What matters to me at this point is the end result.
> > Any issue that
> > causes mis-compilation of Lisp programs should be fixed, of course.
> > Issues that don't affect the natively-compiled code are much less
> > important, and as I explained, my tendency is to accept Andrea's
> > judgment on those.
> 
> There's a difference between "this issue doesn't affect
> natively-compiled code" (which makes it a non-issue, case 2 above) and
> "we don't know whether this issue affects natively-compiled code"
> (which emphatically does not, case 3 above). Evidence of absence and
> all that.

When there's evidence, there's no doubt, and such issues should be and
are taken care of.  Where there's no evidence, we trust the judgment
of the best experts we have, when they show (as they usually do) they
carefully considered the issue before expressing their opinions.  The
rest of us, if we don't agree with the expert judgment, get to work
harder to find the evidence.  There's no way around this.

> > > However, the task of proving that this actually results in a
> > > Lisp-to-machine-code bug is, in general, too much to expect the
> > > initial discoverer to perform.
> >
> > The initial discoverer doesn't have to do that, it's enough to raise
> > the issue.
> 
> Andrea has stated that if there's no reproducer, he doesn't consider
> the issue a bug. So "raising the issue" would do, according to the
> rules he proposed, precisely nothing.

I suggest that you consider deeds before you consider words.  I
challenge you to find any response from Andrea where he dismissed any
report without first considering it seriously and responding to the
report with his reasoning.

> IIUC, you want me to raise future issues and wait for them to be
> dismissed rather than complaining, as I am, about the mere
> announcement that they will be? That's certainly something I can do,
> and resolving that I'll do that would actually be a good result of
> this discussion.
> 
> > did consider it and did fix a couple of issues you brought up that he
> > thought were worth fixing.
> 
> Yes. And he said he wouldn't do so in future, for issues of this category.

See above.

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

[Pip Cet]

On Sat, Feb 27, 2021 at 6:41 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > I take it you've read through the code, understood it all, and
> > concluded the reasons were "sound", then?
>
> I have my own ways of judging what people say and deciding when they
> are sound and when they aren't.  If you want to question my judgment
> as well, you are talking to the wrong guy.

You're using "sound" to mean "superficially correct". I understood it
to have its mathematical (and legal) meaning, "irrefutably correct".

> > I have my own opinions about why Emacs attracts so few volunteers and
> > drives away so many of those who might be.
>
> You are welcome to step up to be the Emacs maintainer, and then act
> according to your opinions.

There's a whole spectrum between "we shouldn't fly into that mountain"
and "let me take the controls".

> > > What matters to me at this point is the end result.
> > > Any issue that
> > > causes mis-compilation of Lisp programs should be fixed, of course.
> > > Issues that don't affect the natively-compiled code are much less
> > > important, and as I explained, my tendency is to accept Andrea's
> > > judgment on those.
> >
> > There's a difference between "this issue doesn't affect
> > natively-compiled code" (which makes it a non-issue, case 2 above) and
> > "we don't know whether this issue affects natively-compiled code"
> > (which emphatically does not, case 3 above). Evidence of absence and
> > all that.
>
> When there's evidence, there's no doubt, and such issues should be and
> are taken care of.  Where there's no evidence, we trust the judgment
> of the best experts we have, when they show (as they usually do) they
> carefully considered the issue before expressing their opinions.  The
> rest of us, if we don't agree with the expert judgment, get to work
> harder to find the evidence.  There's no way around this.

Let's see how this plays out.

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

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > You're using "sound" to mean "superficially correct". I understood it
  > to have its mathematical (and legal) meaning, "irrefutably correct".

I was not following this thread, but here

  > > > I take it you've read through the code, understood it all, and
  > > > concluded the reasons were "sound", then?

you seem to be talking about judging the reasons to make a change.

Generally, that is not a question of mathematics alone.  Sometimes a
bug is simple and a fix is cleary correct.  But usually what to change
and how is a matter of judgment, and the best answer is not
irrefutably right.

-- 
Dr Richard Stallman
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

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

[Pip Cet]

On Mon, Mar 1, 2021 at 5:24 AM Richard Stallman <rms@gnu.org> wrote:
>   > You're using "sound" to mean "superficially correct". I understood it
>   > to have its mathematical (and legal) meaning, "irrefutably correct".
>
> I was not following this thread, but here
>
>   > > > I take it you've read through the code, understood it all, and
>   > > > concluded the reasons were "sound", then?
>
> you seem to be talking about judging the reasons to make a change.

We're not, no. We're talking about the reasons given to justify the
claim that a pseudo-insn emitted by the compiler is not "suspicious".

To say those reasons are "sound" is to say that my initial objection
to the pseudo-insn was answered by the explanation proffered and is
now obviously invalid. I think it's a matter of further discussion,
not that it is, at this point, obviously valid.

(As it turns out, my objection was valid, at least in part. A small
number of the problematic assumptions are now, apparently, no longer
being emitted.)

Pip

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

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

Pip Cet <pipcet@gmail.com> writes:

> On Mon, Feb 22, 2021 at 9:38 AM Andrea Corallo <akrl@sdf.org> wrote:
>> Pip Cet <pipcet@gmail.com> writes:
>> > 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.
>>
>> Correct, ATM the assumption is that we keep LIMPLE always as
>> "conventional SSA form".  This for a number of reasons but mainly it
>> greatly helps in maintaining the compiler simple.
>
> "Conventional" meaning "not quite SSA"? I'm just trying to understand,
> the decision seems correct to me, since we already ran stack slot
> allocation in the byte compiler and we want to reuse those
> assignments.

The SSA book [1] and others discuss conventional and transformed SSA
forms.  IIRC you should find references of these in 2.5 and where copy
propagation is discussed.

>> > 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.
>>
>> Yes but in this case (and probably others) we *have* to emit this
>> assumption.
>
> Why? Are you saying the compiler requires (assume ...) insns for
> correctness? If it does, I'm afraid that's a serious issue.

It requires that assume if we want to infer precisely here.  If we
give-up emitting this assume it will just works perfectly but we'll miss
to predict the return value as we should.

>> The best option is to decide that negative slot numbers are not rendered
>> into libgccjit IR and we consider these virtual to solve these kind of
>> cases.  IIRC we already do something similar for the -1 index so this
>> concept has just to be generalized a bit.
>
> Again, that does seem very complicated, and if it improves
> optimization we could probably improve it much more by modifying the
> byte compiler to pop arguments in the caller rather than the callee.

To me this sounds considerably more invasive, probably is because I'm
much more used to work with the native compiler code that with the byte
compiler one :)

I like the idea of the native compiler patch being less invasive as
possible, this was the line I tried to follow and I think so far it
paid.  I guess a number of readers would'd agree with this kind of
approach to begin with.

I think I should be able to work it out as discussed in one two evenings
and it might be useful for other cases in the future too, so it does not
sound as a big deal to me.

>> > In fact, all we need to do is tell comp-cond-cstrs-target-mvar to
>> > return nil more often, isn't it?
>>
>> Nope, the target mvar identified is the correct one, we just have ATM no
>> way to reference it reliably into the assume.
>
> We don't have to, let's just not emit an assume about a variable that
> we just introduced and that's never read?

We disagree :)

We don't need that mvar to render functional code that's agreed, but
still we still need to reference it in the assume (assumes are not
functional code as they are not rendered in final).

As the byte compiler does not care about propagating types and values it
can just clobber the variable, here we aim for more so we need it to
keep it live till the assume.

>> BTW applying your patch
>> is breaking quite some of the comp-tests-ret-type-spec-* tests :)
>
> Where do you keep those?
>
> I see the same number of test failures with and without the patch,
> running "make check". The failures seem unrelated, too...

They are in "test/src/comp-tests.el", those are the first I suggest to
run after having modified the compiler.  Isn't "make check" picking-up
those?

Regards

  Andrea

[1] <http://ssabook.gforge.inria.fr/latest/book.pdf>

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

[Pip Cet]

On Mon, Feb 22, 2021 at 11:16 AM Andrea Corallo <akrl@sdf.org> wrote:
> Pip Cet <pipcet@gmail.com> writes:
> >> Yes but in this case (and probably others) we *have* to emit this
> >> assumption.
> >
> > Why? Are you saying the compiler requires (assume ...) insns for
> > correctness? If it does, I'm afraid that's a serious issue.
>
> It requires that assume if we want to infer precisely here.  If we
> give-up emitting this assume it will just works perfectly but we'll miss
> to predict the return value as we should.

Emitting an assumption about a dead variable only makes sense if the
dead variable matches a live one. And in that case, we can just emit
the assumption about the live variable to begin with.

> > Again, that does seem very complicated, and if it improves
> > optimization we could probably improve it much more by modifying the
> > byte compiler to pop arguments in the caller rather than the callee.
>
> To me this sounds considerably more invasive, probably is because I'm
> much more used to work with the native compiler code that with the byte
> compiler one :)

That is a little more invasive, but it's where you're going if you
make the major change of allocating your own stack slots rather than
letting the byte compiler do it.

> I like the idea of the native compiler patch being less invasive as
> possible, this was the line I tried to follow and I think so far it
> paid.  I guess a number of readers would'd agree with this kind of
> approach to begin with.

I agree with it! That's why "leave slot allocation to the byte
compiler" is something I don't want you to throw away unnecessarily,
because it's a great simplifying assumption.

> I think I should be able to work it out as discussed in one two evenings
> and it might be useful for other cases in the future too, so it does not
> sound as a big deal to me.

It does to me, I must say. There's nothing special about comparisons:
you're turning a = a OP b two-operand code into c = a OP b
three-operand code. Your code won't be as good as genuine
three-operand code, and it won't be as simple as two-operand code.

> >> > In fact, all we need to do is tell comp-cond-cstrs-target-mvar to
> >> > return nil more often, isn't it?
> >>
> >> Nope, the target mvar identified is the correct one, we just have ATM no
> >> way to reference it reliably into the assume.
> >
> > We don't have to, let's just not emit an assume about a variable that
> > we just introduced and that's never read?
>
> We disagree :)

We can disagree about the "let's" part (and should, of course), but we
should agree about the "we don't have to" part, because that's a
matter of fact, not opinion.

> As the byte compiler does not care about propagating types and values it
> can just clobber the variable, here we aim for more so we need it to
> keep it live till the assume.

If we decide the variable needs to be kept live, the byte compiler
should keep it live, not us.

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

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

Pip Cet <pipcet@gmail.com> writes:

> On Mon, Feb 22, 2021 at 11:16 AM Andrea Corallo <akrl@sdf.org> wrote:
>> Pip Cet <pipcet@gmail.com> writes:
>> >> Yes but in this case (and probably others) we *have* to emit this
>> >> assumption.
>> >
>> > Why? Are you saying the compiler requires (assume ...) insns for
>> > correctness? If it does, I'm afraid that's a serious issue.
>>
>> It requires that assume if we want to infer precisely here.  If we
>> give-up emitting this assume it will just works perfectly but we'll miss
>> to predict the return value as we should.
>
> Emitting an assumption about a dead variable only makes sense if the
> dead variable matches a live one. And in that case, we can just emit
> the assumption about the live variable to begin with.
>
>> > Again, that does seem very complicated, and if it improves
>> > optimization we could probably improve it much more by modifying the
>> > byte compiler to pop arguments in the caller rather than the callee.
>>
>> To me this sounds considerably more invasive, probably is because I'm
>> much more used to work with the native compiler code that with the byte
>> compiler one :)
>
> That is a little more invasive, but it's where you're going if you
> make the major change of allocating your own stack slots rather than
> letting the byte compiler do it.
>
>> I like the idea of the native compiler patch being less invasive as
>> possible, this was the line I tried to follow and I think so far it
>> paid.  I guess a number of readers would'd agree with this kind of
>> approach to begin with.
>
> I agree with it! That's why "leave slot allocation to the byte
> compiler" is something I don't want you to throw away unnecessarily,
> because it's a great simplifying assumption.
>
>> I think I should be able to work it out as discussed in one two evenings
>> and it might be useful for other cases in the future too, so it does not
>> sound as a big deal to me.
>
> It does to me, I must say. There's nothing special about comparisons:
> you're turning a = a OP b two-operand code into c = a OP b
> three-operand code. Your code won't be as good as genuine
> three-operand code, and it won't be as simple as two-operand code.
>
>> >> > In fact, all we need to do is tell comp-cond-cstrs-target-mvar to
>> >> > return nil more often, isn't it?
>> >>
>> >> Nope, the target mvar identified is the correct one, we just have ATM no
>> >> way to reference it reliably into the assume.
>> >
>> > We don't have to, let's just not emit an assume about a variable that
>> > we just introduced and that's never read?
>>
>> We disagree :)
>
> We can disagree about the "let's" part (and should, of course), but we
> should agree about the "we don't have to" part, because that's a
> matter of fact, not opinion.

This new mvar *is* used, actually by an assume.  And the assume in
discussion is targetting a live variable that will be used by functional
code so I really see no scandal here.

>> As the byte compiler does not care about propagating types and values it
>> can just clobber the variable, here we aim for more so we need it to
>> keep it live till the assume.
>
> If we decide the variable needs to be kept live, the byte compiler
> should keep it live, not us.

Again no, any pass in the compiler must have the right to create new
temporaries for whichever purpose it wants, and indeed we can't expect
the byte-compiler to know in advance all passes in the native compiler
and their scopes and goals.  `add-cstrs' is just the first pass that now
happen to have this need, is not a special case of any sort.

Not giving the possibilities to passes to create variables would be
totally equivalent to prevent GIMPLE passes in GCC to create new local
variables while performing transformations, it would be ludicrous and
this is just something we *want* to be able to do.

  Andrea

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

[Pip Cet]

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

Hello Andrea,

by the way, I'm mostly still pursuing it because it keeps flushing out
bugs in comp.el :-) I'm attaching another one, though I'm not sure the
bug in question can actually be triggered without modifying the byte
compiler.

On Tue, Feb 23, 2021 at 10:55 PM Andrea Corallo <akrl@sdf.org> wrote:
> Pip Cet <pipcet@gmail.com> writes:
> >> >> > We don't have to, let's just not emit an assume about a variable that
> >> > we just introduced and that's never read?
> >>
> >> We disagree :)
> >
> > We can disagree about the "let's" part (and should, of course), but we
> > should agree about the "we don't have to" part, because that's a
> > matter of fact, not opinion.
>
> This new mvar *is* used, actually by an assume.

I said it was never read, which I agree is more correct.

> And the assume in
> discussion is targetting a live variable that will be used by functional
> code so I really see no scandal here.

Only on one side of the assume. The other side holds a new "temporary"
variable which is assumed to be equal to something that it isn't
actually equal to, and that seems very scandalous to me.

> >> As the byte compiler does not care about propagating types and values it
> >> can just clobber the variable, here we aim for more so we need it to
> >> keep it live till the assume.
> >
> > If we decide the variable needs to be kept live, the byte compiler
> > should keep it live, not us.
>
> Again no, any pass in the compiler must have the right to create new
> temporaries for whichever purpose it wants, and indeed we can't expect

I agree with that part, by the way. We should, at some point in the
future when we need it, add the ability to introduce temporary
variables (and remove them), in a clean, well-considered fashion. At
present, we don't need it, we lack the ability to remove such
variables, and I'm afraid the last parts can be disputed, too.

[-- Attachment #2: 0001-Fix-ICE-when-compiling-an-explicit-call-to-single-ar.patch --]
[-- Type: text/x-patch, Size: 944 bytes --]

From f75e874d50a2e22a9ff9df2fcbfbf5e74d6bbff5 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Wed, 24 Feb 2021 06:58:24 +0000
Subject: [PATCH] Fix ICE when compiling an explicit call to single-argument
 `-'

* lisp/emacs-lisp/comp.el (comp-fwprop-call): Handle
negation as well as subtraction.
---
 lisp/emacs-lisp/comp.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
index 60c040926e54c..2980605bf8804 100644
--- a/lisp/emacs-lisp/comp.el
+++ b/lisp/emacs-lisp/comp.el
@@ -3040,7 +3040,7 @@ comp-fwprop-call
               (comp-mvar-neg lval) (comp-cstr-neg cstr))))
     (cl-case f
       (+ (comp-cstr-add lval args))
-      (- (comp-cstr-sub lval args))
+      (- (if (cdr args) (comp-cstr-sub lval args)))
       (1+ (comp-cstr-add lval `(,(car args) ,comp-cstr-one)))
       (1- (comp-cstr-sub lval `(,(car args) ,comp-cstr-one))))))
 
-- 
2.30.0