GNU bug report logs -
#35770
[PATCH] Broken duplicate case elimination in switch byte-compilation
Previous Next
Reported by: Mattias Engdegård <mattiase <at> acm.org>
Date: Fri, 17 May 2019 09:34:02 UTC
Severity: normal
Tags: fixed, patch
Fixed in version 27.1
Done: Noam Postavsky <npostavs <at> gmail.com>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 35770 in the body.
You can then email your comments to 35770 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35770; Package
emacs.
(Fri, 17 May 2019 09:34:02 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
Mattias Engdegård <mattiase <at> acm.org>:
New bug report received and forwarded. Copy sent to
bug-gnu-emacs <at> gnu.org.
(Fri, 17 May 2019 09:34:02 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
The byte-code compiler attempts to eliminate duplicated cases when turning a `cond' into a switch table, as in
(cond ((eq x 'a) 1)
(eq x 'b) 2)
(eq x 'a) 3)) ; remove
but it doesn't work properly, because of a confusion between expressions and their values, and a logic error that would have discarded the entire table instead of just skipped the duplicate.
This patch attempts to rectify that. I also removed a seemingly redundant condition, the (consp condition), which should always be true at that point.
[0001-Correctly-eliminate-duplicate-cases-in-switch-compil.patch (application/octet-stream, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35770; Package
emacs.
(Fri, 17 May 2019 12:17:02 GMT)
Full text and
rfc822 format available.
Message #8 received at submit <at> debbugs.gnu.org (full text, mbox):
> * lisp/emacs-lisp/bytecomp.el (byte-compile-cond-vars): Return obj2 eval'ed.
> (byte-compile-cond-jump-table-info):
> Discard redundant condition. Use `obj2' as evaluated.
> Discard duplicated cases instead of failing the table generation.
The patch looks fine, but could you add corresponding regression tests?
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35770; Package
emacs.
(Fri, 17 May 2019 14:49:02 GMT)
Full text and
rfc822 format available.
Message #11 received at submit <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
fre 2019-05-17 klockan 08:16 -0400 skrev Stefan Monnier:
> The patch looks fine, but could you add corresponding regression
> tests?
Added. I wasn't sure how you test these things, but I both added some
functional tests and some kind of check of the bytecode. If you find
the latter superfluous, I can drop it; I think the functional tests are
reliable enough. In other words, we have a case of miscompilation.
There was another bug in the original code which survived into the
first patch: the check for duplicate keys was done with `assq', which
of course doesn't work for stuff compared with `eql' or `equal'. Now
fixed.
[0001-Correctly-eliminate-duplicate-cases-in-switch-compil.patch (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35770; Package
emacs.
(Wed, 22 May 2019 10:53:02 GMT)
Full text and
rfc822 format available.
Message #14 received at submit <at> debbugs.gnu.org (full text, mbox):
No hurry in reviewing the patch, but since this is a wrong-code bug, I suppose it should be back-ported to emacs-26. Does that mean that it should be written for that branch and then merged to master, or the other way around?
The test uses bignums for numbers that aren't eq. In Emacs 26, those would be floats instead, but bignums seemed more robust since it's not unreasonable to have unboxed floats in the future.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35770; Package
emacs.
(Wed, 22 May 2019 10:59:02 GMT)
Full text and
rfc822 format available.
Message #17 received at 35770 <at> debbugs.gnu.org (full text, mbox):
> From: Mattias Engdegård <mattiase <at> acm.org>
> Date: Wed, 22 May 2019 12:52:08 +0200
> Cc: 35770 <at> debbugs.gnu.org, vibhavp <at> gmail.com
>
> No hurry in reviewing the patch, but since this is a wrong-code bug, I suppose it should be back-ported to emacs-26. Does that mean that it should be written for that branch and then merged to master, or the other way around?
The former.
However, in this case I really wonder why we should be so eager to
install the change on the emacs-26 branch. How long this wrong code
has been with us?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35770; Package
emacs.
(Wed, 22 May 2019 11:12:02 GMT)
Full text and
rfc822 format available.
Message #20 received at 35770 <at> debbugs.gnu.org (full text, mbox):
22 maj 2019 kl. 12.58 skrev Eli Zaretskii <eliz <at> gnu.org>:
>
> However, in this case I really wonder why we should be so eager to
> install the change on the emacs-26 branch. How long this wrong code
> has been with us?
About two years, and it only matters for (arguably) buggy code, so you are probably right.
On the other hand, different behaviour in compiled and interpreted code does not make it easier to find such bugs.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35770; Package
emacs.
(Wed, 22 May 2019 11:22:01 GMT)
Full text and
rfc822 format available.
Message #23 received at 35770 <at> debbugs.gnu.org (full text, mbox):
Mattias Engdegård <mattiase <at> acm.org> writes:
> 22 maj 2019 kl. 12.58 skrev Eli Zaretskii <eliz <at> gnu.org>:
>>
>> However, in this case I really wonder why we should be so eager to
>> install the change on the emacs-26 branch. How long this wrong code
>> has been with us?
>
> About two years, and it only matters for (arguably) buggy code, so you are probably right.
I think the significant milestone is that it's been with us since 26.1,
i.e., this is a regression since Emacs 25.
> On the other hand, different behaviour in compiled and interpreted
> code does not make it easier to find such bugs.
Another option is turning off byte-compile-cond-use-jump-table by
default.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35770; Package
emacs.
(Wed, 22 May 2019 11:24:02 GMT)
Full text and
rfc822 format available.
Message #26 received at 35770 <at> debbugs.gnu.org (full text, mbox):
> From: Noam Postavsky <npostavs <at> gmail.com>
> Cc: Eli Zaretskii <eliz <at> gnu.org>, Stefan Monnier <monnier <at> iro.umontreal.ca>, vibhavp <at> gmail.com, 35770 <at> debbugs.gnu.org
> Date: Wed, 22 May 2019 07:21:12 -0400
>
> Mattias Engdegård <mattiase <at> acm.org> writes:
>
> > 22 maj 2019 kl. 12.58 skrev Eli Zaretskii <eliz <at> gnu.org>:
> >>
> >> However, in this case I really wonder why we should be so eager to
> >> install the change on the emacs-26 branch. How long this wrong code
> >> has been with us?
> >
> > About two years, and it only matters for (arguably) buggy code, so you are probably right.
>
> I think the significant milestone is that it's been with us since 26.1,
> i.e., this is a regression since Emacs 25.
Then maybe we should again consider reverting on emacs-26 the change
which caused the regression.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35770; Package
emacs.
(Wed, 22 May 2019 14:20:02 GMT)
Full text and
rfc822 format available.
Message #29 received at 35770 <at> debbugs.gnu.org (full text, mbox):
> Then maybe we should again consider reverting on emacs-26 the change
> which caused the regression.
turning off byte-compile-cond-use-jump-table by default is the safer way
to do that, I think.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35770; Package
emacs.
(Sun, 26 May 2019 17:07:02 GMT)
Full text and
rfc822 format available.
Message #32 received at 35770 <at> debbugs.gnu.org (full text, mbox):
22 maj 2019 kl. 16.19 skrev Stefan Monnier <monnier <at> iro.umontreal.ca>:
>
> turning off byte-compile-cond-use-jump-table by default is the safer way
> to do that, I think.
Is the consensus then that the patch is fine for master, and that the switch op generation should be disabled by default in emacs-26? Maybe with an explanation in the doc string:
-(defcustom byte-compile-cond-use-jump-table t
- "Compile `cond' clauses to a jump table implementation (using a hash-table)."
+(defcustom byte-compile-cond-use-jump-table nil
+ "Compile `cond' clauses to a jump table implementation (using a hash-table).
+
+In Emacs 26, this feature is disabled by default because of a bug
+in the code generation of `cond' forms with duplicated test clauses."
:version "26.1"
:group 'bytecomp
:type 'boolean)
If so, I'll push. No NEWS change needed in either branch, right?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35770; Package
emacs.
(Sun, 26 May 2019 18:44:01 GMT)
Full text and
rfc822 format available.
Message #35 received at 35770 <at> debbugs.gnu.org (full text, mbox):
> From: Mattias Engdegård <mattiase <at> acm.org>
> Date: Sun, 26 May 2019 19:05:55 +0200
> Cc: Eli Zaretskii <eliz <at> gnu.org>, Noam Postavsky <npostavs <at> gmail.com>,
> vibhavp <at> gmail.com, 35770 <at> debbugs.gnu.org
>
> 22 maj 2019 kl. 16.19 skrev Stefan Monnier <monnier <at> iro.umontreal.ca>:
> >
> > turning off byte-compile-cond-use-jump-table by default is the safer way
> > to do that, I think.
>
> Is the consensus then that the patch is fine for master, and that the switch op generation should be disabled by default in emacs-26? Maybe with an explanation in the doc string:
>
> -(defcustom byte-compile-cond-use-jump-table t
> - "Compile `cond' clauses to a jump table implementation (using a hash-table)."
> +(defcustom byte-compile-cond-use-jump-table nil
> + "Compile `cond' clauses to a jump table implementation (using a hash-table).
> +
> +In Emacs 26, this feature is disabled by default because of a bug
> +in the code generation of `cond' forms with duplicated test clauses."
> :version "26.1"
> :group 'bytecomp
> :type 'boolean)
I don't think we should explain why we did this in a doc string. And
:version should change as well.
More generally, I can't say I like this, and don't understand why this
would be better than reverting the offending change on emacs-26. Why
is it a problem to defer the features which caused the regression to
Emacs 27?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35770; Package
emacs.
(Sun, 26 May 2019 22:08:02 GMT)
Full text and
rfc822 format available.
Message #38 received at 35770 <at> debbugs.gnu.org (full text, mbox):
> More generally, I can't say I like this, and don't understand why this
> would be better than reverting the offending change on emacs-26. Why
> is it a problem to defer the features which caused the regression to
> Emacs 27?
I personally don't have a preference. I find reverting the change to be
a more intrusive change with more risks of errors, but it's likely
a wash either way.
FWIW I also think it's generally good to introduce new byte-codes in one
version and only enable the compiler to use them in the next version
(that's what I did for the condition-case byte codes, for example).
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35770; Package
emacs.
(Mon, 27 May 2019 11:28:02 GMT)
Full text and
rfc822 format available.
Message #41 received at 35770 <at> debbugs.gnu.org (full text, mbox):
27 maj 2019 kl. 00.06 skrev Stefan Monnier <monnier <at> iro.umontreal.ca>:
>
> I personally don't have a preference. I find reverting the change to be
> a more intrusive change with more risks of errors, but it's likely
> a wash either way.
>
> FWIW I also think it's generally good to introduce new byte-codes in one
> version and only enable the compiler to use them in the next version
> (that's what I did for the condition-case byte codes, for example).
Trying to revert 'the rest' (the switch code generation) while keeping the switch op itself seems indeed to be risky and a lot of work compared to just turning off the generation. It is not a matter of finding a few commits to revert; there are fixes upon fixes, and changes that need to be untangled. It does not sound very practical.
Since nobody complained about the actual bug fix, I'm boldly pushing it to master.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35770; Package
emacs.
(Sat, 01 Jun 2019 13:59:02 GMT)
Full text and
rfc822 format available.
Message #44 received at 35770 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Mattias Engdegård <mattiase <at> acm.org> writes:
> Trying to revert 'the rest' (the switch code generation) while keeping
> the switch op itself seems indeed to be risky and a lot of work
> compared to just turning off the generation. It is not a matter of
> finding a few commits to revert; there are fixes upon fixes, and
> changes that need to be untangled. It does not sound very practical.
So, is this patch okay for emacs-26?
[0001-Disable-byte-compile-cond-use-jump-table-Bug-35770.patch (text/x-diff, inline)]
From 257a750a10ebb77277a5d7d771c01d66a69a66e4 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs <at> gmail.com>
Date: Sat, 1 Jun 2019 09:53:35 -0400
Subject: [PATCH] Disable byte-compile-cond-use-jump-table (Bug#35770)
* lisp/emacs-lisp/bytecomp.el (byte-compile-cond-use-jump-table): Set
to nil by default.
Don't merge to master, the bug is already fixed there.
---
lisp/emacs-lisp/bytecomp.el | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index e3b34c189f..9273626c80 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -234,9 +234,9 @@ (defcustom byte-compile-delete-errors nil
:group 'bytecomp
:type 'boolean)
-(defcustom byte-compile-cond-use-jump-table t
+(defcustom byte-compile-cond-use-jump-table nil
"Compile `cond' clauses to a jump table implementation (using a hash-table)."
- :version "26.1"
+ :version "26.3" ;; Disabled due to Bug#35770.
:group 'bytecomp
:type 'boolean)
--
2.11.0
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35770; Package
emacs.
(Sat, 01 Jun 2019 15:48:01 GMT)
Full text and
rfc822 format available.
Message #47 received at 35770 <at> debbugs.gnu.org (full text, mbox):
> From: Noam Postavsky <npostavs <at> gmail.com>
> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, Eli Zaretskii <eliz <at> gnu.org>, vibhavp <at> gmail.com, 35770 <at> debbugs.gnu.org
> Date: Sat, 01 Jun 2019 09:58:42 -0400
>
> So, is this patch okay for emacs-26?
Yes, thanks.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#35770; Package
emacs.
(Sat, 01 Jun 2019 21:54:02 GMT)
Full text and
rfc822 format available.
Message #50 received at 35770 <at> debbugs.gnu.org (full text, mbox):
tags 35770 fixed
close 35770 26.3
quit
Eli Zaretskii <eliz <at> gnu.org> writes:
>> From: Noam Postavsky <npostavs <at> gmail.com>
>> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, Eli Zaretskii <eliz <at> gnu.org>, vibhavp <at> gmail.com, 35770 <at> debbugs.gnu.org
>> Date: Sat, 01 Jun 2019 09:58:42 -0400
>>
>> So, is this patch okay for emacs-26?
>
> Yes, thanks.
Pushed.
04f13a5d9b 2019-06-01T17:48:43-04:00 "Disable byte-compile-cond-use-jump-table (Bug#35770)"
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=04f13a5d9bc19cfe0382e4257f1a1d856aa354ed
Added tag(s) fixed.
Request was from
Noam Postavsky <npostavs <at> gmail.com>
to
control <at> debbugs.gnu.org.
(Sat, 01 Jun 2019 21:54:03 GMT)
Full text and
rfc822 format available.
bug marked as fixed in version 26.3, send any further explanations to
35770 <at> debbugs.gnu.org and Mattias Engdegård <mattiase <at> acm.org>
Request was from
Noam Postavsky <npostavs <at> gmail.com>
to
control <at> debbugs.gnu.org.
(Sat, 01 Jun 2019 21:54:03 GMT)
Full text and
rfc822 format available.
bug Marked as fixed in versions 27.1.
Request was from
Noam Postavsky <npostavs <at> gmail.com>
to
control <at> debbugs.gnu.org.
(Sat, 01 Jun 2019 21:56:01 GMT)
Full text and
rfc822 format available.
bug No longer marked as fixed in versions 26.3.
Request was from
Noam Postavsky <npostavs <at> gmail.com>
to
control <at> debbugs.gnu.org.
(Sat, 01 Jun 2019 21:56:01 GMT)
Full text and
rfc822 format available.
bug archived.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org.
(Sun, 30 Jun 2019 11:24:09 GMT)
Full text and
rfc822 format available.
This bug report was last modified 4 years and 302 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.