Skip to content

Commit 9365d09

Browse files
schuayCommit Bot
schuay
authored and
Commit Bot
committed
[coverage] Rework continuation counter handling
This changes a few bits about how continuation counters are handled. It introduces a new mechanism that allows removal of a continuation range after it has been created. If coverage is enabled, we run a first post-processing pass on the AST immediately after parsing, which removes problematic continuation ranges in two situations: 1. nested continuation counters - only the outermost stays alive. 2. trailing continuation counters within a block-like structure are removed if the containing structure itself has a continuation. [email protected], [email protected], [email protected] Bug: v8:8381, v8:8539 Change-Id: I6bcaea5060d8c481d7bae099f6db9f993cc30ee3 Reviewed-on: https://chromium-review.googlesource.com/c/1339119 Reviewed-by: Yang Guo <[email protected]> Reviewed-by: Leszek Swirski <[email protected]> Reviewed-by: Georg Neis <[email protected]> Commit-Queue: Jakob Gruber <[email protected]> Cr-Commit-Position: refs/heads/master@{#58443}
1 parent 491eff8 commit 9365d09

File tree

12 files changed

+382
-162
lines changed

12 files changed

+382
-162
lines changed

‎BUILD.gn‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1668,6 +1668,8 @@ v8_source_set("v8_base") {
16681668
"src/ast/prettyprinter.h",
16691669
"src/ast/scopes.cc",
16701670
"src/ast/scopes.h",
1671+
"src/ast/source-range-ast-visitor.cc",
1672+
"src/ast/source-range-ast-visitor.h",
16711673
"src/ast/variables.cc",
16721674
"src/ast/variables.h",
16731675
"src/bailout-reason.cc",

‎src/ast/ast-source-ranges.h‎

Lines changed: 80 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,8 @@ class AstNodeSourceRanges : public ZoneObject {
5959
public:
6060
virtual ~AstNodeSourceRanges() = default;
6161
virtual SourceRange GetRange(SourceRangeKind kind) = 0;
62+
virtual bool HasRange(SourceRangeKind kind) = 0;
63+
virtual void RemoveContinuationRange() { UNREACHABLE(); }
6264
};
6365

6466
class BinaryOperationSourceRanges final : public AstNodeSourceRanges {
@@ -67,10 +69,14 @@ class BinaryOperationSourceRanges final : public AstNodeSourceRanges {
6769
: right_range_(right_range) {}
6870

6971
SourceRange GetRange(SourceRangeKind kind) override {
70-
DCHECK_EQ(kind, SourceRangeKind::kRight);
72+
DCHECK(HasRange(kind));
7173
return right_range_;
7274
}
7375

76+
bool HasRange(SourceRangeKind kind) override {
77+
return kind == SourceRangeKind::kRight;
78+
}
79+
7480
private:
7581
SourceRange right_range_;
7682
};
@@ -81,10 +87,19 @@ class ContinuationSourceRanges : public AstNodeSourceRanges {
8187
: continuation_position_(continuation_position) {}
8288

8389
SourceRange GetRange(SourceRangeKind kind) override {
84-
DCHECK_EQ(kind, SourceRangeKind::kContinuation);
90+
DCHECK(HasRange(kind));
8591
return SourceRange::OpenEnded(continuation_position_);
8692
}
8793

94+
bool HasRange(SourceRangeKind kind) override {
95+
return kind == SourceRangeKind::kContinuation;
96+
}
97+
98+
void RemoveContinuationRange() override {
99+
DCHECK(HasRange(SourceRangeKind::kContinuation));
100+
continuation_position_ = kNoSourcePosition;
101+
}
102+
88103
private:
89104
int32_t continuation_position_;
90105
};
@@ -101,10 +116,14 @@ class CaseClauseSourceRanges final : public AstNodeSourceRanges {
101116
: body_range_(body_range) {}
102117

103118
SourceRange GetRange(SourceRangeKind kind) override {
104-
DCHECK_EQ(kind, SourceRangeKind::kBody);
119+
DCHECK(HasRange(kind));
105120
return body_range_;
106121
}
107122

123+
bool HasRange(SourceRangeKind kind) override {
124+
return kind == SourceRangeKind::kBody;
125+
}
126+
108127
private:
109128
SourceRange body_range_;
110129
};
@@ -116,6 +135,7 @@ class ConditionalSourceRanges final : public AstNodeSourceRanges {
116135
: then_range_(then_range), else_range_(else_range) {}
117136

118137
SourceRange GetRange(SourceRangeKind kind) override {
138+
DCHECK(HasRange(kind));
119139
switch (kind) {
120140
case SourceRangeKind::kThen:
121141
return then_range_;
@@ -126,6 +146,10 @@ class ConditionalSourceRanges final : public AstNodeSourceRanges {
126146
}
127147
}
128148

149+
bool HasRange(SourceRangeKind kind) override {
150+
return kind == SourceRangeKind::kThen || kind == SourceRangeKind::kElse;
151+
}
152+
129153
private:
130154
SourceRange then_range_;
131155
SourceRange else_range_;
@@ -138,12 +162,14 @@ class IfStatementSourceRanges final : public AstNodeSourceRanges {
138162
: then_range_(then_range), else_range_(else_range) {}
139163

140164
SourceRange GetRange(SourceRangeKind kind) override {
165+
DCHECK(HasRange(kind));
141166
switch (kind) {
142167
case SourceRangeKind::kElse:
143168
return else_range_;
144169
case SourceRangeKind::kThen:
145170
return then_range_;
146171
case SourceRangeKind::kContinuation: {
172+
if (!has_continuation_) return SourceRange::Empty();
147173
const SourceRange& trailing_range =
148174
else_range_.IsEmpty() ? then_range_ : else_range_;
149175
return SourceRange::ContinuationOf(trailing_range);
@@ -153,9 +179,20 @@ class IfStatementSourceRanges final : public AstNodeSourceRanges {
153179
}
154180
}
155181

182+
bool HasRange(SourceRangeKind kind) override {
183+
return kind == SourceRangeKind::kThen || kind == SourceRangeKind::kElse ||
184+
kind == SourceRangeKind::kContinuation;
185+
}
186+
187+
void RemoveContinuationRange() override {
188+
DCHECK(HasRange(SourceRangeKind::kContinuation));
189+
has_continuation_ = false;
190+
}
191+
156192
private:
157193
SourceRange then_range_;
158194
SourceRange else_range_;
195+
bool has_continuation_ = true;
159196
};
160197

161198
class IterationStatementSourceRanges final : public AstNodeSourceRanges {
@@ -164,18 +201,31 @@ class IterationStatementSourceRanges final : public AstNodeSourceRanges {
164201
: body_range_(body_range) {}
165202

166203
SourceRange GetRange(SourceRangeKind kind) override {
204+
DCHECK(HasRange(kind));
167205
switch (kind) {
168206
case SourceRangeKind::kBody:
169207
return body_range_;
170208
case SourceRangeKind::kContinuation:
209+
if (!has_continuation_) return SourceRange::Empty();
171210
return SourceRange::ContinuationOf(body_range_);
172211
default:
173212
UNREACHABLE();
174213
}
175214
}
176215

216+
bool HasRange(SourceRangeKind kind) override {
217+
return kind == SourceRangeKind::kBody ||
218+
kind == SourceRangeKind::kContinuation;
219+
}
220+
221+
void RemoveContinuationRange() override {
222+
DCHECK(HasRange(SourceRangeKind::kContinuation));
223+
has_continuation_ = false;
224+
}
225+
177226
private:
178227
SourceRange body_range_;
228+
bool has_continuation_ = true;
179229
};
180230

181231
class JumpStatementSourceRanges final : public ContinuationSourceRanges {
@@ -200,6 +250,7 @@ class NaryOperationSourceRanges final : public AstNodeSourceRanges {
200250
size_t RangeCount() const { return ranges_.size(); }
201251

202252
SourceRange GetRange(SourceRangeKind kind) override { UNREACHABLE(); }
253+
bool HasRange(SourceRangeKind kind) override { return false; }
203254

204255
private:
205256
ZoneVector<SourceRange> ranges_;
@@ -229,18 +280,31 @@ class TryCatchStatementSourceRanges final : public AstNodeSourceRanges {
229280
: catch_range_(catch_range) {}
230281

231282
SourceRange GetRange(SourceRangeKind kind) override {
283+
DCHECK(HasRange(kind));
232284
switch (kind) {
233285
case SourceRangeKind::kCatch:
234286
return catch_range_;
235287
case SourceRangeKind::kContinuation:
288+
if (!has_continuation_) return SourceRange::Empty();
236289
return SourceRange::ContinuationOf(catch_range_);
237290
default:
238291
UNREACHABLE();
239292
}
240293
}
241294

295+
bool HasRange(SourceRangeKind kind) override {
296+
return kind == SourceRangeKind::kCatch ||
297+
kind == SourceRangeKind::kContinuation;
298+
}
299+
300+
void RemoveContinuationRange() override {
301+
DCHECK(HasRange(SourceRangeKind::kContinuation));
302+
has_continuation_ = false;
303+
}
304+
242305
private:
243306
SourceRange catch_range_;
307+
bool has_continuation_ = true;
244308
};
245309

246310
class TryFinallyStatementSourceRanges final : public AstNodeSourceRanges {
@@ -249,18 +313,31 @@ class TryFinallyStatementSourceRanges final : public AstNodeSourceRanges {
249313
: finally_range_(finally_range) {}
250314

251315
SourceRange GetRange(SourceRangeKind kind) override {
316+
DCHECK(HasRange(kind));
252317
switch (kind) {
253318
case SourceRangeKind::kFinally:
254319
return finally_range_;
255320
case SourceRangeKind::kContinuation:
321+
if (!has_continuation_) return SourceRange::Empty();
256322
return SourceRange::ContinuationOf(finally_range_);
257323
default:
258324
UNREACHABLE();
259325
}
260326
}
261327

328+
bool HasRange(SourceRangeKind kind) override {
329+
return kind == SourceRangeKind::kFinally ||
330+
kind == SourceRangeKind::kContinuation;
331+
}
332+
333+
void RemoveContinuationRange() override {
334+
DCHECK(HasRange(SourceRangeKind::kContinuation));
335+
has_continuation_ = false;
336+
}
337+
262338
private:
263339
SourceRange finally_range_;
340+
bool has_continuation_ = true;
264341
};
265342

266343
// Maps ast node pointers to associated source ranges. The parser creates these

‎src/ast/source-range-ast-visitor.cc‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
// Copyright 2018 the V8 project authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
#include "src/ast/source-range-ast-visitor.h"
6+
7+
#include "src/ast/ast-source-ranges.h"
8+
9+
namespace v8 {
10+
namespace internal {
11+
12+
SourceRangeAstVisitor::SourceRangeAstVisitor(uintptr_t stack_limit,
13+
Expression* root,
14+
SourceRangeMap* source_range_map)
15+
: AstTraversalVisitor(stack_limit, root),
16+
source_range_map_(source_range_map) {}
17+
18+
void SourceRangeAstVisitor::VisitBlock(Block* stmt) {
19+
AstTraversalVisitor::VisitBlock(stmt);
20+
ZonePtrList<Statement>* stmts = stmt->statements();
21+
AstNodeSourceRanges* enclosingSourceRanges = source_range_map_->Find(stmt);
22+
if (enclosingSourceRanges != nullptr) {
23+
CHECK(enclosingSourceRanges->HasRange(SourceRangeKind::kContinuation));
24+
MaybeRemoveLastContinuationRange(stmts);
25+
}
26+
}
27+
28+
void SourceRangeAstVisitor::VisitFunctionLiteral(FunctionLiteral* expr) {
29+
AstTraversalVisitor::VisitFunctionLiteral(expr);
30+
ZonePtrList<Statement>* stmts = expr->body();
31+
MaybeRemoveLastContinuationRange(stmts);
32+
}
33+
34+
bool SourceRangeAstVisitor::VisitNode(AstNode* node) {
35+
AstNodeSourceRanges* range = source_range_map_->Find(node);
36+
37+
if (range == nullptr) return true;
38+
if (!range->HasRange(SourceRangeKind::kContinuation)) return true;
39+
40+
// Called in pre-order. In case of conflicting continuation ranges, only the
41+
// outermost range may survive.
42+
43+
SourceRange continuation = range->GetRange(SourceRangeKind::kContinuation);
44+
if (continuation_positions_.find(continuation.start) !=
45+
continuation_positions_.end()) {
46+
range->RemoveContinuationRange();
47+
} else {
48+
continuation_positions_.emplace(continuation.start);
49+
}
50+
51+
return true;
52+
}
53+
54+
void SourceRangeAstVisitor::MaybeRemoveLastContinuationRange(
55+
ZonePtrList<Statement>* statements) {
56+
if (statements->is_empty()) return;
57+
58+
Statement* last_statement = statements->last();
59+
AstNodeSourceRanges* last_range = source_range_map_->Find(last_statement);
60+
if (last_range == nullptr) return;
61+
62+
if (last_range->HasRange(SourceRangeKind::kContinuation)) {
63+
last_range->RemoveContinuationRange();
64+
}
65+
}
66+
67+
} // namespace internal
68+
} // namespace v8

‎src/ast/source-range-ast-visitor.h‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
// Copyright 2018 the V8 project authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
#ifndef V8_AST_SOURCE_RANGE_AST_VISITOR_H_
6+
#define V8_AST_SOURCE_RANGE_AST_VISITOR_H_
7+
8+
#include <unordered_set>
9+
10+
#include "src/ast/ast-traversal-visitor.h"
11+
12+
namespace v8 {
13+
namespace internal {
14+
15+
class SourceRangeMap;
16+
17+
// Post-processes generated source ranges while the AST structure still exists.
18+
//
19+
// In particular, SourceRangeAstVisitor
20+
//
21+
// 1. deduplicates continuation source ranges, only keeping the outermost one.
22+
// See also: https://crbug.com/v8/8539.
23+
//
24+
// 2. removes the source range associated with the final statement in a block
25+
// or function body if the parent itself has a source range associated with it.
26+
// See also: https://crbug.com/v8/8381.
27+
class SourceRangeAstVisitor final
28+
: public AstTraversalVisitor<SourceRangeAstVisitor> {
29+
public:
30+
SourceRangeAstVisitor(uintptr_t stack_limit, Expression* root,
31+
SourceRangeMap* source_range_map);
32+
33+
private:
34+
friend class AstTraversalVisitor<SourceRangeAstVisitor>;
35+
36+
void VisitBlock(Block* stmt);
37+
void VisitFunctionLiteral(FunctionLiteral* expr);
38+
bool VisitNode(AstNode* node);
39+
40+
void MaybeRemoveLastContinuationRange(ZonePtrList<Statement>* stmts);
41+
42+
SourceRangeMap* source_range_map_ = nullptr;
43+
std::unordered_set<int> continuation_positions_;
44+
};
45+
46+
} // namespace internal
47+
} // namespace v8
48+
49+
#endif // V8_AST_SOURCE_RANGE_AST_VISITOR_H_

‎src/debug/debug-coverage.cc‎

Lines changed: 0 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -233,25 +233,6 @@ bool HaveSameSourceRange(const CoverageBlock& lhs, const CoverageBlock& rhs) {
233233
return lhs.start == rhs.start && lhs.end == rhs.end;
234234
}
235235

236-
void MergeDuplicateSingletons(CoverageFunction* function) {
237-
CoverageBlockIterator iter(function);
238-
239-
while (iter.Next() && iter.HasNext()) {
240-
CoverageBlock& block = iter.GetBlock();
241-
CoverageBlock& next_block = iter.GetNextBlock();
242-
243-
// Identical ranges should only occur through singleton ranges. Consider the
244-
// ranges for `for (.) break;`: continuation ranges for both the `break` and
245-
// `for` statements begin after the trailing semicolon.
246-
// Such ranges are merged and keep the maximal execution count.
247-
if (!HaveSameSourceRange(block, next_block)) continue;
248-
249-
DCHECK_EQ(kNoSourcePosition, block.end); // Singleton range.
250-
next_block.count = std::max(block.count, next_block.count);
251-
iter.DeleteBlock();
252-
}
253-
}
254-
255236
void MergeDuplicateRanges(CoverageFunction* function) {
256237
CoverageBlockIterator iter(function);
257238

@@ -424,9 +405,6 @@ void CollectBlockCoverage(CoverageFunction* function, SharedFunctionInfo info,
424405
// If in binary mode, only report counts of 0/1.
425406
if (mode == debug::Coverage::kBlockBinary) ClampToBinary(function);
426407

427-
// Remove duplicate singleton ranges, keeping the max count.
428-
MergeDuplicateSingletons(function);
429-
430408
// Remove singleton ranges with the same start position as a full range and
431409
// throw away their counts.
432410
// Singleton ranges are only intended to split existing full ranges and should

‎src/interpreter/bytecode-generator.cc‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -368,7 +368,6 @@ class BytecodeGenerator::ControlScopeForBreakable final
368368
protected:
369369
bool Execute(Command command, Statement* statement,
370370
int source_position) override {
371-
control_builder_->set_needs_continuation_counter();
372371
if (statement != statement_) return false;
373372
switch (command) {
374373
case CMD_BREAK:

0 commit comments

Comments
 (0)