summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorusa <usa@b2dd03c8-39d4-4d8f-98ff-823fe69b080e>2021-02-28 14:16:37 +0000
committerusa <usa@b2dd03c8-39d4-4d8f-98ff-823fe69b080e>2021-02-28 14:16:37 +0000
commit3f4f5fdf0b0ef9df7a8a32a73b4af2c46a2373a5 (patch)
treed0ddfdb98f59852e7d9a8117dfcbb9e4e2746e5a
parentc64fbfae6f67a70b76fd9048ee9b764577cdef49 (diff)
downloadruby-3f4f5fdf0b0ef9df7a8a32a73b4af2c46a2373a5.tar.gz
merge revision(s) f9e5c74c: [Backport #15980]
compile.c: stop wrong peephole optimization when covearge is enabled jump-jump optimization ignores the event flags of the jump instruction being skipped, which leads to overlook of line events. This changeset stops the wrong optimization when coverage measurement is neabled and when the jump instruction has any event flag. Note that this issue is not only for coverage but also for TracePoint, and this change does not fix TracePoint. However, fixing it fundamentally is tough (which requires revamp of the compiler). This issue is critical in terms of coverage measurement, but minor for TracePoint (ko1 said), so we here choose a stopgap measurement. [Bug #15980] [Bug #16397] Note for backporters: this changeset can be viewed by `git diff -w`. --- compile.c | 232 +++++++++++++++++++++++------------------ test/coverage/test_coverage.rb | 12 +++ 2 files changed, 141 insertions(+), 103 deletions(-) git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/branches/ruby_2_6@67898 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
-rw-r--r--compile.c232
-rw-r--r--test/coverage/test_coverage.rb12
-rw-r--r--version.h2
3 files changed, 142 insertions, 104 deletions
diff --git a/compile.c b/compile.c
index 4ceb8951b7..bc4d475923 100644
--- a/compile.c
+++ b/compile.c
@@ -2867,109 +2867,135 @@ iseq_peephole_optimize(rb_iseq_t *iseq, LINK_ELEMENT *list, const int do_tailcal
* if L2
*/
INSN *nobj = (INSN *)get_destination_insn(iobj);
- INSN *pobj = (INSN *)iobj->link.prev;
- int prev_dup = 0;
- if (pobj) {
- if (!IS_INSN(&pobj->link))
- pobj = 0;
- else if (IS_INSN_ID(pobj, dup))
- prev_dup = 1;
- }
-
- for (;;) {
- if (IS_INSN_ID(nobj, jump)) {
- replace_destination(iobj, nobj);
- }
- else if (prev_dup && IS_INSN_ID(nobj, dup) &&
- !!(nobj = (INSN *)nobj->link.next) &&
- /* basic blocks, with no labels in the middle */
- nobj->insn_id == iobj->insn_id) {
- /*
- * dup
- * if L1
- * ...
- * L1:
- * dup
- * if L2
- * =>
- * dup
- * if L2
- * ...
- * L1:
- * dup
- * if L2
- */
- replace_destination(iobj, nobj);
- }
- else if (pobj) {
- /*
- * putnil
- * if L1
- * =>
- * # nothing
- *
- * putobject true
- * if L1
- * =>
- * jump L1
- *
- * putstring ".."
- * if L1
- * =>
- * jump L1
- *
- * putstring ".."
- * dup
- * if L1
- * =>
- * putstring ".."
- * jump L1
- *
- */
- int cond;
- if (prev_dup && IS_INSN(pobj->link.prev)) {
- pobj = (INSN *)pobj->link.prev;
- }
- if (IS_INSN_ID(pobj, putobject)) {
- cond = (IS_INSN_ID(iobj, branchif) ?
- OPERAND_AT(pobj, 0) != Qfalse :
- IS_INSN_ID(iobj, branchunless) ?
- OPERAND_AT(pobj, 0) == Qfalse :
- FALSE);
- }
- else if (IS_INSN_ID(pobj, putstring) ||
- IS_INSN_ID(pobj, duparray) ||
- IS_INSN_ID(pobj, newarray)) {
- cond = IS_INSN_ID(iobj, branchif);
- }
- else if (IS_INSN_ID(pobj, putnil)) {
- cond = !IS_INSN_ID(iobj, branchif);
- }
- else break;
- if (prev_dup || !IS_INSN_ID(pobj, newarray)) {
- ELEM_REMOVE(iobj->link.prev);
- }
- else if (!iseq_pop_newarray(iseq, pobj)) {
- pobj = new_insn_core(iseq, pobj->insn_info.line_no, BIN(pop), 0, NULL);
- ELEM_INSERT_PREV(&iobj->link, &pobj->link);
- }
- if (cond) {
- if (prev_dup) {
- pobj = new_insn_core(iseq, pobj->insn_info.line_no, BIN(putnil), 0, NULL);
- ELEM_INSERT_NEXT(&iobj->link, &pobj->link);
- }
- iobj->insn_id = BIN(jump);
- goto again;
- }
- else {
- unref_destination(iobj, 0);
- ELEM_REMOVE(&iobj->link);
- }
- break;
- }
- else break;
- nobj = (INSN *)get_destination_insn(nobj);
- }
+
+ /* This is super nasty hack!!!
+ *
+ * This jump-jump optimization may ignore event flags of the jump
+ * instruction being skipped. Actually, Line 2 TracePoint event
+ * is never fired in the following code:
+ *
+ * 1: raise if 1 == 2
+ * 2: while true
+ * 3: break
+ * 4: end
+ *
+ * This is critical for coverage measurement. [Bug #15980]
+ *
+ * This is a stopgap measure: stop the jump-jump optimization if
+ * coverage measurement is enabled and if the skipped instruction
+ * has any event flag.
+ *
+ * Note that, still, TracePoint Line event does not occur on Line 2.
+ * This should be fixed in future.
+ */
+ int stop_optimization =
+ ISEQ_COVERAGE(iseq) && ISEQ_LINE_COVERAGE(iseq) &&
+ nobj->insn_info.events;
+ if (!stop_optimization) {
+ INSN *pobj = (INSN *)iobj->link.prev;
+ int prev_dup = 0;
+ if (pobj) {
+ if (!IS_INSN(&pobj->link))
+ pobj = 0;
+ else if (IS_INSN_ID(pobj, dup))
+ prev_dup = 1;
+ }
+
+ for (;;) {
+ if (IS_INSN_ID(nobj, jump)) {
+ replace_destination(iobj, nobj);
+ }
+ else if (prev_dup && IS_INSN_ID(nobj, dup) &&
+ !!(nobj = (INSN *)nobj->link.next) &&
+ /* basic blocks, with no labels in the middle */
+ nobj->insn_id == iobj->insn_id) {
+ /*
+ * dup
+ * if L1
+ * ...
+ * L1:
+ * dup
+ * if L2
+ * =>
+ * dup
+ * if L2
+ * ...
+ * L1:
+ * dup
+ * if L2
+ */
+ replace_destination(iobj, nobj);
+ }
+ else if (pobj) {
+ /*
+ * putnil
+ * if L1
+ * =>
+ * # nothing
+ *
+ * putobject true
+ * if L1
+ * =>
+ * jump L1
+ *
+ * putstring ".."
+ * if L1
+ * =>
+ * jump L1
+ *
+ * putstring ".."
+ * dup
+ * if L1
+ * =>
+ * putstring ".."
+ * jump L1
+ *
+ */
+ int cond;
+ if (prev_dup && IS_INSN(pobj->link.prev)) {
+ pobj = (INSN *)pobj->link.prev;
+ }
+ if (IS_INSN_ID(pobj, putobject)) {
+ cond = (IS_INSN_ID(iobj, branchif) ?
+ OPERAND_AT(pobj, 0) != Qfalse :
+ IS_INSN_ID(iobj, branchunless) ?
+ OPERAND_AT(pobj, 0) == Qfalse :
+ FALSE);
+ }
+ else if (IS_INSN_ID(pobj, putstring) ||
+ IS_INSN_ID(pobj, duparray) ||
+ IS_INSN_ID(pobj, newarray)) {
+ cond = IS_INSN_ID(iobj, branchif);
+ }
+ else if (IS_INSN_ID(pobj, putnil)) {
+ cond = !IS_INSN_ID(iobj, branchif);
+ }
+ else break;
+ if (prev_dup || !IS_INSN_ID(pobj, newarray)) {
+ ELEM_REMOVE(iobj->link.prev);
+ }
+ else if (!iseq_pop_newarray(iseq, pobj)) {
+ pobj = new_insn_core(iseq, pobj->insn_info.line_no, BIN(pop), 0, NULL);
+ ELEM_INSERT_PREV(&iobj->link, &pobj->link);
+ }
+ if (cond) {
+ if (prev_dup) {
+ pobj = new_insn_core(iseq, pobj->insn_info.line_no, BIN(putnil), 0, NULL);
+ ELEM_INSERT_NEXT(&iobj->link, &pobj->link);
+ }
+ iobj->insn_id = BIN(jump);
+ goto again;
+ }
+ else {
+ unref_destination(iobj, 0);
+ ELEM_REMOVE(&iobj->link);
+ }
+ break;
+ }
+ else break;
+ nobj = (INSN *)get_destination_insn(nobj);
+ }
+ }
}
if (IS_INSN_ID(iobj, pop)) {
diff --git a/test/coverage/test_coverage.rb b/test/coverage/test_coverage.rb
index 30523c341a..1625d48845 100644
--- a/test/coverage/test_coverage.rb
+++ b/test/coverage/test_coverage.rb
@@ -696,4 +696,16 @@ class TestCoverage < Test::Unit::TestCase
}
}
end
+
+ def test_stop_wrong_peephole_optimization
+ result = {
+ :lines => [1, 1, 1, nil]
+ }
+ assert_coverage(<<~"end;", { lines: true }, result)
+ raise if 1 == 2
+ while true
+ break
+ end
+ end;
+ end
end
diff --git a/version.h b/version.h
index bc10bb4f12..bcfe99aff6 100644
--- a/version.h
+++ b/version.h
@@ -1,6 +1,6 @@
#define RUBY_VERSION "2.6.7"
#define RUBY_RELEASE_DATE "2021-02-28"
-#define RUBY_PATCHLEVEL 157
+#define RUBY_PATCHLEVEL 158
#define RUBY_RELEASE_YEAR 2021
#define RUBY_RELEASE_MONTH 2