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

[Andrea Corallo]

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