Skip to content

Commit 673e434

Browse files
bcoeaddaleax
bcoe
authored and
addaleax
committed
deps: v8, cherry-pick 9365d09, aac2f8c, 47d34a3
 Original commit message 9365d09: [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. R=bmeurer@chromium.org, jgruber@chromium.org, yangguo@chromium.org Bug: v8:8381, v8:8539 Change-Id: I6bcaea5060d8c481d7bae099f6db9f993cc30ee3 Reviewed-on: https://chromium-review.googlesource.com/c/1339119 Reviewed-by: Yang Guo <yangguo@chromium.org> Reviewed-by: Leszek Swirski <leszeks@chromium.org> Reviewed-by: Georg Neis <neis@chromium.org> Commit-Queue: Jakob Gruber <jgruber@chromium.org> Cr-Commit-Position: refs/heads/master@{#58443} Refs: v8/v8@9365d09 Original commit message aac2f8c: [coverage] Filter out singleton ranges that alias full ranges Block coverage is based on a system of ranges that can either have both a start and end position, or only a start position (so-called singleton ranges). When formatting coverage information, singletons are expanded until the end of the immediate full parent range. E.g. in: {0, 10} // Full range. {5, -1} // Singleton range. the singleton range is expanded to {5, 10}. Singletons are produced mostly for continuation counters that track whether we execute past a specific language construct. Unfortunately, continuation counters can turn up in spots that confuse our post-processing. For example: if (true) { ... block1 ... } else { ... block2 ... } If block1 produces a continuation counter, it could end up with the same start position as the else-branch counter. Since we merge identical blocks, the else-branch could incorrectly end up with an execution count of one. We need to avoid merging such cases. A full range should always take precedence over a singleton range; a singleton range should never expand to completely fill a full range. An additional post-processing pass ensures this. Bug: v8:8237 Change-Id: Idb3ec7b2feddc0585313810b9c8be1e9f4ec64bf Reviewed-on: https://chromium-review.googlesource.com/c/1273095 Reviewed-by: Georg Neis <neis@chromium.org> Reviewed-by: Yang Guo <yangguo@chromium.org> Commit-Queue: Jakob Gruber <jgruber@chromium.org> Cr-Commit-Position: refs/heads/master@{#56531} Refs: v8/v8@aac2f8c deps: V8: backport 47d34a3 Original commit message: Revert "[coverage] change block range to avoid ambiguity." This reverts commit 471fef0469d04d7c487f3a08e81f3d77566a2f50. Reason for revert: A more general fix incoming at https://crrev.com/c/1273095. Original change's description: > [coverage] change block range to avoid ambiguity. > > By moving the block range end to left of closing bracket, > we can avoid ambiguity where an open-ended singleton range > could be both interpreted as inside the parent range, or > next to it. > > R=<U+200B>verwaest@chromium.org > > Bug: v8:8237 > Change-Id: Ibc9412b31efe900b6d8bff0d8fa8c52ddfbf460a > Reviewed-on: https://chromium-review.googlesource.com/1254127 > Reviewed-by: Georg Neis <neis@chromium.org> > Commit-Queue: Yang Guo <yangguo@chromium.org> > Cr-Commit-Position: refs/heads/master@{#56347} TBR=yangguo@chromium.org,neis@chromium.org,verwaest@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: v8:8237 Change-Id: I39310cf3c2f06a0d98ff314740aaeefbfffc0834 Reviewed-on: https://chromium-review.googlesource.com/c/1273096 Reviewed-by: Jakob Gruber <jgruber@chromium.org> Reviewed-by: Toon Verwaest <verwaest@chromium.org> Reviewed-by: Yang Guo <yangguo@chromium.org> Commit-Queue: Jakob Gruber <jgruber@chromium.org> Cr-Commit-Position: refs/heads/master@{#56513} Refs: v8/v8@47d34a3 PR-URL: #25429 Backport-PR-URL: #25728 Reviewed-By: Yang Guo <yangguo@chromium.org> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
1 parent 411f6fe commit 673e434

13 files changed

+392
-166
lines changed

‎deps/v8/BUILD.gn

+2
Original file line numberDiff line numberDiff line change
@@ -1570,6 +1570,8 @@ v8_source_set("v8_base") {
15701570
"src/ast/prettyprinter.h",
15711571
"src/ast/scopes.cc",
15721572
"src/ast/scopes.h",
1573+
"src/ast/source-range-ast-visitor.cc",
1574+
"src/ast/source-range-ast-visitor.h",
15731575
"src/ast/variables.cc",
15741576
"src/ast/variables.h",
15751577
"src/bailout-reason.cc",

‎deps/v8/gypfiles/v8.gyp

+2
Original file line numberDiff line numberDiff line change
@@ -548,6 +548,8 @@
548548
'../src/ast/prettyprinter.h',
549549
'../src/ast/scopes.cc',
550550
'../src/ast/scopes.h',
551+
'../src/ast/source-range-ast-visitor.cc',
552+
'../src/ast/source-range-ast-visitor.h',
551553
'../src/ast/variables.cc',
552554
'../src/ast/variables.h',
553555
'../src/bailout-reason.cc',

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

+80-8
Original file line numberDiff line numberDiff line change
@@ -58,18 +58,24 @@ class AstNodeSourceRanges : public ZoneObject {
5858
public:
5959
virtual ~AstNodeSourceRanges() {}
6060
virtual SourceRange GetRange(SourceRangeKind kind) = 0;
61+
virtual bool HasRange(SourceRangeKind kind) = 0;
62+
virtual void RemoveContinuationRange() { UNREACHABLE(); }
6163
};
6264

6365
class BinaryOperationSourceRanges final : public AstNodeSourceRanges {
6466
public:
6567
explicit BinaryOperationSourceRanges(const SourceRange& right_range)
6668
: right_range_(right_range) {}
6769

68-
SourceRange GetRange(SourceRangeKind kind) {
70+
SourceRange GetRange(SourceRangeKind kind) override {
6971
DCHECK_EQ(kind, SourceRangeKind::kRight);
7072
return right_range_;
7173
}
7274

75+
bool HasRange(SourceRangeKind kind) override {
76+
return kind == SourceRangeKind::kRight;
77+
}
78+
7379
private:
7480
SourceRange right_range_;
7581
};
@@ -79,11 +85,20 @@ class ContinuationSourceRanges : public AstNodeSourceRanges {
7985
explicit ContinuationSourceRanges(int32_t continuation_position)
8086
: continuation_position_(continuation_position) {}
8187

82-
SourceRange GetRange(SourceRangeKind kind) {
88+
SourceRange GetRange(SourceRangeKind kind) override {
8389
DCHECK_EQ(kind, SourceRangeKind::kContinuation);
8490
return SourceRange::OpenEnded(continuation_position_);
8591
}
8692

93+
bool HasRange(SourceRangeKind kind) override {
94+
return kind == SourceRangeKind::kContinuation;
95+
}
96+
97+
void RemoveContinuationRange() override {
98+
DCHECK(HasRange(SourceRangeKind::kContinuation));
99+
continuation_position_ = kNoSourcePosition;
100+
}
101+
87102
private:
88103
int32_t continuation_position_;
89104
};
@@ -99,11 +114,15 @@ class CaseClauseSourceRanges final : public AstNodeSourceRanges {
99114
explicit CaseClauseSourceRanges(const SourceRange& body_range)
100115
: body_range_(body_range) {}
101116

102-
SourceRange GetRange(SourceRangeKind kind) {
117+
SourceRange GetRange(SourceRangeKind kind) override {
103118
DCHECK_EQ(kind, SourceRangeKind::kBody);
104119
return body_range_;
105120
}
106121

122+
bool HasRange(SourceRangeKind kind) override {
123+
return kind == SourceRangeKind::kBody;
124+
}
125+
107126
private:
108127
SourceRange body_range_;
109128
};
@@ -114,7 +133,7 @@ class ConditionalSourceRanges final : public AstNodeSourceRanges {
114133
const SourceRange& else_range)
115134
: then_range_(then_range), else_range_(else_range) {}
116135

117-
SourceRange GetRange(SourceRangeKind kind) {
136+
SourceRange GetRange(SourceRangeKind kind) override {
118137
switch (kind) {
119138
case SourceRangeKind::kThen:
120139
return then_range_;
@@ -125,6 +144,10 @@ class ConditionalSourceRanges final : public AstNodeSourceRanges {
125144
}
126145
}
127146

147+
bool HasRange(SourceRangeKind kind) override {
148+
return kind == SourceRangeKind::kThen || kind == SourceRangeKind::kElse;
149+
}
150+
128151
private:
129152
SourceRange then_range_;
130153
SourceRange else_range_;
@@ -136,13 +159,14 @@ class IfStatementSourceRanges final : public AstNodeSourceRanges {
136159
const SourceRange& else_range)
137160
: then_range_(then_range), else_range_(else_range) {}
138161

139-
SourceRange GetRange(SourceRangeKind kind) {
162+
SourceRange GetRange(SourceRangeKind kind) override {
140163
switch (kind) {
141164
case SourceRangeKind::kElse:
142165
return else_range_;
143166
case SourceRangeKind::kThen:
144167
return then_range_;
145168
case SourceRangeKind::kContinuation: {
169+
if (!has_continuation_) return SourceRange::Empty();
146170
const SourceRange& trailing_range =
147171
else_range_.IsEmpty() ? then_range_ : else_range_;
148172
return SourceRange::ContinuationOf(trailing_range);
@@ -152,29 +176,52 @@ class IfStatementSourceRanges final : public AstNodeSourceRanges {
152176
}
153177
}
154178

179+
bool HasRange(SourceRangeKind kind) override {
180+
return kind == SourceRangeKind::kThen || kind == SourceRangeKind::kElse ||
181+
kind == SourceRangeKind::kContinuation;
182+
}
183+
184+
void RemoveContinuationRange() override {
185+
DCHECK(HasRange(SourceRangeKind::kContinuation));
186+
has_continuation_ = false;
187+
}
188+
155189
private:
156190
SourceRange then_range_;
157191
SourceRange else_range_;
192+
bool has_continuation_ = true;
158193
};
159194

160195
class IterationStatementSourceRanges final : public AstNodeSourceRanges {
161196
public:
162197
explicit IterationStatementSourceRanges(const SourceRange& body_range)
163198
: body_range_(body_range) {}
164199

165-
SourceRange GetRange(SourceRangeKind kind) {
200+
SourceRange GetRange(SourceRangeKind kind) override {
166201
switch (kind) {
167202
case SourceRangeKind::kBody:
168203
return body_range_;
169204
case SourceRangeKind::kContinuation:
205+
if (!has_continuation_) return SourceRange::Empty();
170206
return SourceRange::ContinuationOf(body_range_);
171207
default:
172208
UNREACHABLE();
173209
}
174210
}
175211

212+
bool HasRange(SourceRangeKind kind) override {
213+
return kind == SourceRangeKind::kBody ||
214+
kind == SourceRangeKind::kContinuation;
215+
}
216+
217+
void RemoveContinuationRange() override {
218+
DCHECK(HasRange(SourceRangeKind::kContinuation));
219+
has_continuation_ = false;
220+
}
221+
176222
private:
177223
SourceRange body_range_;
224+
bool has_continuation_ = true;
178225
};
179226

180227
class JumpStatementSourceRanges final : public ContinuationSourceRanges {
@@ -199,6 +246,7 @@ class NaryOperationSourceRanges final : public AstNodeSourceRanges {
199246
size_t RangeCount() const { return ranges_.size(); }
200247

201248
SourceRange GetRange(SourceRangeKind kind) { UNREACHABLE(); }
249+
bool HasRange(SourceRangeKind kind) { return false; }
202250

203251
private:
204252
ZoneVector<SourceRange> ranges_;
@@ -227,39 +275,63 @@ class TryCatchStatementSourceRanges final : public AstNodeSourceRanges {
227275
explicit TryCatchStatementSourceRanges(const SourceRange& catch_range)
228276
: catch_range_(catch_range) {}
229277

230-
SourceRange GetRange(SourceRangeKind kind) {
278+
SourceRange GetRange(SourceRangeKind kind) override {
231279
switch (kind) {
232280
case SourceRangeKind::kCatch:
233281
return catch_range_;
234282
case SourceRangeKind::kContinuation:
283+
if (!has_continuation_) return SourceRange::Empty();
235284
return SourceRange::ContinuationOf(catch_range_);
236285
default:
237286
UNREACHABLE();
238287
}
239288
}
240289

290+
bool HasRange(SourceRangeKind kind) override {
291+
return kind == SourceRangeKind::kCatch ||
292+
kind == SourceRangeKind::kContinuation;
293+
}
294+
295+
void RemoveContinuationRange() override {
296+
DCHECK(HasRange(SourceRangeKind::kContinuation));
297+
has_continuation_ = false;
298+
}
299+
241300
private:
242301
SourceRange catch_range_;
302+
bool has_continuation_ = true;
243303
};
244304

245305
class TryFinallyStatementSourceRanges final : public AstNodeSourceRanges {
246306
public:
247307
explicit TryFinallyStatementSourceRanges(const SourceRange& finally_range)
248308
: finally_range_(finally_range) {}
249309

250-
SourceRange GetRange(SourceRangeKind kind) {
310+
SourceRange GetRange(SourceRangeKind kind) override {
251311
switch (kind) {
252312
case SourceRangeKind::kFinally:
253313
return finally_range_;
254314
case SourceRangeKind::kContinuation:
315+
if (!has_continuation_) return SourceRange::Empty();
255316
return SourceRange::ContinuationOf(finally_range_);
256317
default:
257318
UNREACHABLE();
258319
}
259320
}
260321

322+
bool HasRange(SourceRangeKind kind) override {
323+
return kind == SourceRangeKind::kFinally ||
324+
kind == SourceRangeKind::kContinuation;
325+
}
326+
327+
void RemoveContinuationRange() override {
328+
DCHECK(HasRange(SourceRangeKind::kContinuation));
329+
has_continuation_ = false;
330+
}
331+
261332
private:
262333
SourceRange finally_range_;
334+
bool has_continuation_ = true;
263335
};
264336

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

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

+68
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 == nullptr || 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

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

+49
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_

0 commit comments

Comments
 (0)