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

[Pip Cet <pipcet@gmail.com>]

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