From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Andrea Corallo via "Bug reports for GNU Emacs, the Swiss army knife of text editors" Newsgroups: gmane.emacs.bugs Subject: bug#46670: 28.0.50; [feature/native-comp] possible miscompilation affecting lsp-mode Date: Thu, 25 Feb 2021 16:56:03 +0000 Message-ID: References: <87a6ry46uc.fsf@collares.org> Reply-To: Andrea Corallo Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="26190"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux) Cc: 46670@debbugs.gnu.org, Mauricio Collares To: Pip Cet Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Thu Feb 25 17:58:23 2021 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1lFJy2-0006hn-MI for geb-bug-gnu-emacs@m.gmane-mx.org; Thu, 25 Feb 2021 17:58:22 +0100 Original-Received: from localhost ([::1]:51926 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lFJy1-0002Bq-PH for geb-bug-gnu-emacs@m.gmane-mx.org; Thu, 25 Feb 2021 11:58:21 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:43316) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lFJwk-0001MK-Ol for bug-gnu-emacs@gnu.org; Thu, 25 Feb 2021 11:57:02 -0500 Original-Received: from debbugs.gnu.org ([209.51.188.43]:55785) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1lFJwk-00057P-FK for bug-gnu-emacs@gnu.org; Thu, 25 Feb 2021 11:57:02 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1lFJwk-0000mm-CV for bug-gnu-emacs@gnu.org; Thu, 25 Feb 2021 11:57:02 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Andrea Corallo Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Thu, 25 Feb 2021 16:57:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 46670 X-GNU-PR-Package: emacs Original-Received: via spool by 46670-submit@debbugs.gnu.org id=B46670.16142721742956 (code B ref 46670); Thu, 25 Feb 2021 16:57:02 +0000 Original-Received: (at 46670) by debbugs.gnu.org; 25 Feb 2021 16:56:14 +0000 Original-Received: from localhost ([127.0.0.1]:39098 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1lFJvx-0000lc-VD for submit@debbugs.gnu.org; Thu, 25 Feb 2021 11:56:14 -0500 Original-Received: from mx.sdf.org ([205.166.94.24]:54109) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1lFJvt-0000lR-MP for 46670@debbugs.gnu.org; Thu, 25 Feb 2021 11:56:12 -0500 Original-Received: from mab (ma.sdf.org [205.166.94.33]) by mx.sdf.org (8.15.2/8.14.5) with ESMTPS id 11PGu3pj006873 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256 bits) verified NO); Thu, 25 Feb 2021 16:56:04 GMT In-Reply-To: (Pip Cet's message of "Thu, 25 Feb 2021 12:41:42 +0000") X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.io gmane.emacs.bugs:200796 Archived-At: Pip Cet writes: > Hello Andrea, > > On Wed, Feb 24, 2021 at 10:06 AM Andrea Corallo wrote: >> >> Pip Cet writes: >> >> > On Wed, Feb 24, 2021 at 9:42 AM Andrea Corallo 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