Skip to content

Commit 7b422f4

Browse files
liamappelbeCommit Queue
authored andcommitted
[vm] Fix branch coverage finally edge cases
StreamingFlowGraphBuilder::BuildTryFinally talks about 5 different cases where a finally block needs to be executed: 1) break/continue in a loop 2) continue in a switch 3) return statement 4) try body is an open fragment 5) exception is thrown inside try StreamingFlowGraphBuilder::BuildTryFinally itself only handles cases 4 and 5. So when we added branch coverage to the finally inside BuildTryFinally, we missed cases 1, 2, and 3, which go through TranslateFinallyFinalizers instead. Bug: dart-lang/tools#2241 Change-Id: I02b38a18452be709d5fabd9945d6570a2f1b5120 Fixes: dart-lang/tools#2241 TEST=SourceReport_BranchCoverage_finallyEdgeCases Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/463220 Commit-Queue: Liam Appelbe <[email protected]> Reviewed-by: Ryan Macnak <[email protected]>
1 parent 6062fe8 commit 7b422f4

File tree

3 files changed

+133
-1
lines changed

3 files changed

+133
-1
lines changed

runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1053,6 +1053,12 @@ Fragment StreamingFlowGraphBuilder::BuildStatementAt(intptr_t kernel_offset) {
10531053
return BuildStatement(); // read statement.
10541054
}
10551055

1056+
Fragment StreamingFlowGraphBuilder::BuildStatementAtWithBranchCoverage(
1057+
intptr_t kernel_offset) {
1058+
SetOffset(kernel_offset);
1059+
return BuildStatementWithBranchCoverage(); // read statement.
1060+
}
1061+
10561062
Fragment StreamingFlowGraphBuilder::BuildExpression(TokenPosition* position) {
10571063
++num_ast_nodes_;
10581064
uint8_t payload = 0;
@@ -1774,7 +1780,7 @@ Fragment StreamingFlowGraphBuilder::TranslateFinallyFinalizers(
17741780
intptr_t finalizer_kernel_offset =
17751781
B->try_finally_block_->finalizer_kernel_offset();
17761782
B->try_finally_block_ = B->try_finally_block_->outer();
1777-
instructions += BuildStatementAt(finalizer_kernel_offset);
1783+
instructions += BuildStatementAtWithBranchCoverage(finalizer_kernel_offset);
17781784

17791785
// We only need to make sure that if the finalizer ended normally, we
17801786
// continue towards the next outer try-finally.

runtime/vm/compiler/frontend/kernel_binary_flowgraph.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ class StreamingFlowGraphBuilder : public KernelReaderHelper {
5151
void ReportUnexpectedTag(const char* variant, Tag tag) override;
5252

5353
Fragment BuildStatementAt(intptr_t kernel_offset);
54+
Fragment BuildStatementAtWithBranchCoverage(intptr_t kernel_offset);
5455

5556
intptr_t num_ast_nodes() const { return num_ast_nodes_; }
5657

runtime/vm/source_report_test.cc

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1653,6 +1653,131 @@ main() {
16531653
buffer);
16541654
}
16551655

1656+
ISOLATE_UNIT_TEST_CASE(SourceReport_BranchCoverage_finallyEdgeCases) {
1657+
// Regression test for https://github.com/dart-lang/tools/issues/2241
1658+
// The cases covered in this test are the same as those listed in
1659+
// StreamingFlowGraphBuilder::BuildTryFinally.
1660+
const int kBufferSize = 1024;
1661+
char buffer[kBufferSize];
1662+
const char* kScript = R"(
1663+
int viaReturn() {
1664+
try {
1665+
return 6 * 7;
1666+
} finally {
1667+
}
1668+
}
1669+
1670+
void viaBreak() {
1671+
for (int i = 0; i < 10; ++i) {
1672+
try {
1673+
break;
1674+
} finally {
1675+
}
1676+
}
1677+
}
1678+
1679+
void viaContinue() {
1680+
for (int i = 0; i < 10; ++i) {
1681+
try {
1682+
continue;
1683+
} finally {
1684+
}
1685+
}
1686+
}
1687+
1688+
int viaSwitchContinue(int x) {
1689+
switch (x) {
1690+
case 123:
1691+
try {
1692+
continue otherCase;
1693+
} finally {
1694+
}
1695+
otherCase:
1696+
case 456:
1697+
return 789;
1698+
}
1699+
return 0;
1700+
}
1701+
1702+
void viaOpenBody() {
1703+
try {
1704+
} finally {
1705+
}
1706+
}
1707+
1708+
void viaThrow() {
1709+
try {
1710+
throw 123;
1711+
} catch (e) {
1712+
} finally {
1713+
}
1714+
}
1715+
1716+
main() {
1717+
viaReturn();
1718+
viaBreak();
1719+
viaContinue();
1720+
viaSwitchContinue(123);
1721+
viaOpenBody();
1722+
viaThrow();
1723+
}
1724+
)";
1725+
1726+
Library& lib = Library::Handle();
1727+
const bool old_branch_coverage = IsolateGroup::Current()->branch_coverage();
1728+
IsolateGroup::Current()->set_branch_coverage(true);
1729+
lib ^= ExecuteScript(kScript);
1730+
IsolateGroup::Current()->set_branch_coverage(old_branch_coverage);
1731+
ASSERT(!lib.IsNull());
1732+
const Script& script =
1733+
Script::Handle(lib.LookupScript(String::Handle(String::New("test-lib"))));
1734+
1735+
SourceReport report(SourceReport::kBranchCoverage,
1736+
SourceReport::kForceCompile);
1737+
JSONStream js;
1738+
report.PrintJSON(&js, script);
1739+
const char* json_str = js.ToCString();
1740+
ASSERT(strlen(json_str) < kBufferSize);
1741+
ElideJSONSubstring("classes", json_str, buffer);
1742+
ElideJSONSubstring("libraries", buffer, buffer);
1743+
EXPECT_STREQ(
1744+
"{\"type\":\"SourceReport\",\"ranges\":["
1745+
1746+
// viaReturn's finally is hit.
1747+
"{\"scriptIndex\":0,\"startPos\":1,\"endPos\":63,\"compiled\":true,"
1748+
"\"branchCoverage\":{\"hits\":[1,25,57],\"misses\":[]}},"
1749+
1750+
// viaBreak's finally is hit.
1751+
"{\"scriptIndex\":0,\"startPos\":66,\"endPos\":166,\"compiled\":true,"
1752+
"\"branchCoverage\":{\"hits\":[66,115,125,154],\"misses\":[]}},"
1753+
1754+
// viaContinue's finally is hit.
1755+
"{\"scriptIndex\":0,\"startPos\":169,\"endPos\":275,\"compiled\":true,"
1756+
"\"branchCoverage\":{\"hits\":[169,221,231,263],\"misses\":[]}},"
1757+
1758+
// viaSwitchContinue's finally is hit.
1759+
"{\"scriptIndex\":0,\"startPos\":278,\"endPos\":467,\"compiled\":true,"
1760+
"\"branchCoverage\":{\"hits\":[278,328,348,394,408],\"misses\":[]}},"
1761+
1762+
// viaOpenBody's finally is hit.
1763+
"{\"scriptIndex\":0,\"startPos\":470,\"endPos\":517,\"compiled\":true,"
1764+
"\"branchCoverage\":{\"hits\":[470,497,511],\"misses\":[]}},"
1765+
1766+
// viaThrow's finally is hit.
1767+
"{\"scriptIndex\":0,\"startPos\":520,\"endPos\":595,\"compiled\":true,"
1768+
"\"branchCoverage\":{\"hits\":[520,544,575,589],\"misses\":[]}},"
1769+
1770+
// Main is hit.
1771+
"{\"scriptIndex\":0,\"startPos\":598,\"endPos\":710,\"compiled\":true,"
1772+
"\"branchCoverage\":{\"hits\":[598],\"misses\":[]}}],"
1773+
1774+
// Only one script in the script table.
1775+
"\"scripts\":[{\"type\":\"@Script\",\"fixedId\":true,\"id\":\"\",\""
1776+
"uri\":\"file:\\/\\/\\/test-lib\",\"_kind\":\"kernel\"}]}",
1777+
1778+
buffer);
1779+
}
1780+
16561781
#endif // !PRODUCT
16571782

16581783
} // namespace dart

0 commit comments

Comments
 (0)