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" <bug-gnu-emacs@gnu.org>]

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