1berry/ruby
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

compile.c: stop wrong peephole optimization when covearge is enabled

Browse Source 
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`.
This commit is contained in:
Yusuke Endoh 2019-12-04 10:33:35 +09:00
parent 5a404efd29
commit f9e5c74cd2
2 changed files with 140 additions and 102 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

230
compile.c
View File

@ -2861,109 +2861,135 @@ iseq_peephole_optimize(rb_iseq_t *iseq, LINK_ELEMENT *list, const int do_tailcal
* if L2 * if L2
*/ */
INSN *nobj = (INSN *)get_destination_insn(iobj); 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 (;;) { /* This is super nasty hack!!!
if (IS_INSN_ID(nobj, jump)) { *
replace_destination(iobj, nobj); * This jump-jump optimization may ignore event flags of the jump
} * instruction being skipped. Actually, Line 2 TracePoint event
else if (prev_dup && IS_INSN_ID(nobj, dup) && * is never fired in the following code:
!!(nobj = (INSN *)nobj->link.next) && *
/* basic blocks, with no labels in the middle */ * 1: raise if 1 == 2
nobj->insn_id == iobj->insn_id) { * 2: while true
/* * 3: break
* dup * 4: end
* if L1 *
* ... * This is critical for coverage measurement. [Bug #15980]
* L1: *
* dup * This is a stopgap measure: stop the jump-jump optimization if
* if L2 * coverage measurement is enabled and if the skipped instruction
* => * has any event flag.
* dup *
* if L2 * Note that, still, TracePoint Line event does not occur on Line 2.
* ... * This should be fixed in future.
* L1: */
* dup int stop_optimization =
* if L2 ISEQ_COVERAGE(iseq) && ISEQ_LINE_COVERAGE(iseq) &&
*/ nobj->insn_info.events;
replace_destination(iobj, nobj); if (!stop_optimization) {
} INSN *pobj = (INSN *)iobj->link.prev;
else if (pobj) { int prev_dup = 0;
/* if (pobj) {
* putnil if (!IS_INSN(&pobj->link))
* if L1 pobj = 0;
* => else if (IS_INSN_ID(pobj, dup))
* # nothing prev_dup = 1;
* }
* putobject true
* if L1 for (;;) {
* => if (IS_INSN_ID(nobj, jump)) {
* jump L1 replace_destination(iobj, nobj);
* }
* putstring ".." else if (prev_dup && IS_INSN_ID(nobj, dup) &&
* if L1 !!(nobj = (INSN *)nobj->link.next) &&
* => /* basic blocks, with no labels in the middle */
* jump L1 nobj->insn_id == iobj->insn_id) {
* /*
* putstring ".." * dup
* dup * if L1
* if L1 * ...
* => * L1:
* putstring ".." * dup
* jump L1 * if L2
* * =>
*/ * dup
int cond; * if L2
if (prev_dup && IS_INSN(pobj->link.prev)) { * ...
pobj = (INSN *)pobj->link.prev; * L1:
} * dup
if (IS_INSN_ID(pobj, putobject)) { * if L2
cond = (IS_INSN_ID(iobj, branchif) ? */
OPERAND_AT(pobj, 0) != Qfalse : replace_destination(iobj, nobj);
IS_INSN_ID(iobj, branchunless) ? }
OPERAND_AT(pobj, 0) == Qfalse : else if (pobj) {
FALSE); /*
} * putnil
else if (IS_INSN_ID(pobj, putstring) || * if L1
IS_INSN_ID(pobj, duparray) || * =>
IS_INSN_ID(pobj, newarray)) { * # nothing
cond = IS_INSN_ID(iobj, branchif); *
} * putobject true
else if (IS_INSN_ID(pobj, putnil)) { * if L1
cond = !IS_INSN_ID(iobj, branchif); * =>
} * jump L1
else break; *
if (prev_dup || !IS_INSN_ID(pobj, newarray)) { * putstring ".."
ELEM_REMOVE(iobj->link.prev); * if L1
} * =>
else if (!iseq_pop_newarray(iseq, pobj)) { * jump L1
pobj = new_insn_core(iseq, pobj->insn_info.line_no, BIN(pop), 0, NULL); *
ELEM_INSERT_PREV(&iobj->link, &pobj->link); * putstring ".."
} * dup
if (cond) { * if L1
if (prev_dup) { * =>
pobj = new_insn_core(iseq, pobj->insn_info.line_no, BIN(putnil), 0, NULL); * putstring ".."
ELEM_INSERT_NEXT(&iobj->link, &pobj->link); * jump L1
} *
iobj->insn_id = BIN(jump); */
goto again; int cond;
} if (prev_dup && IS_INSN(pobj->link.prev)) {
else { pobj = (INSN *)pobj->link.prev;
unref_destination(iobj, 0); }
ELEM_REMOVE(&iobj->link); if (IS_INSN_ID(pobj, putobject)) {
} cond = (IS_INSN_ID(iobj, branchif) ?
break; OPERAND_AT(pobj, 0) != Qfalse :
} IS_INSN_ID(iobj, branchunless) ?
else break; OPERAND_AT(pobj, 0) == Qfalse :
nobj = (INSN *)get_destination_insn(nobj); 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)) { if (IS_INSN_ID(iobj, pop)) {

12
test/coverage/test_coverage.rb
View File

@ -728,4 +728,16 @@ class TestCoverage < Test::Unit::TestCase
} }
} }
end 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 end