Skip to content

Commit b7bbd87

Browse files
bcoebcoe
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. [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} 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 <[email protected]> Reviewed-by: Yang Guo <[email protected]> Commit-Queue: Jakob Gruber <[email protected]> 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>[email protected] > > Bug: v8:8237 > Change-Id: Ibc9412b31efe900b6d8bff0d8fa8c52ddfbf460a > Reviewed-on: https://chromium-review.googlesource.com/1254127 > Reviewed-by: Georg Neis <[email protected]> > Commit-Queue: Yang Guo <[email protected]> > Cr-Commit-Position: refs/heads/master@{#56347} [email protected],[email protected],[email protected] # 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 <[email protected]> Reviewed-by: Toon Verwaest <[email protected]> Reviewed-by: Yang Guo <[email protected]> Commit-Queue: Jakob Gruber <[email protected]> Cr-Commit-Position: refs/heads/master@{#56513} Refs: v8/v8@47d34a3 PR-URL: #25429 Reviewed-By: Yang Guo <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
1 parent 8528c21 commit b7bbd87

17 files changed

+467
-228
lines changed

‎common.gypi

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@
3838

3939
# Reset this number to 0 on major V8 upgrades.
4040
# Increment by one for each non-official patch applied to deps/v8.
41-
'v8_embedder_string': '-node.9',
41+
'v8_embedder_string': '-node.10',
4242

4343
##### V8 defaults for Node.js #####
4444

‎deps/v8/BUILD.gn

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1589,6 +1589,8 @@ v8_source_set("v8_base") {
15891589
"src/ast/scopes-inl.h",
15901590
"src/ast/scopes.cc",
15911591
"src/ast/scopes.h",
1592+
"src/ast/source-range-ast-visitor.cc",
1593+
"src/ast/source-range-ast-visitor.h",
15921594
"src/ast/variables.cc",
15931595
"src/ast/variables.h",
15941596
"src/bailout-reason.cc",

‎deps/v8/gypfiles/v8.gyp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -611,6 +611,8 @@
611611
'../src/ast/scopes-inl.h',
612612
'../src/ast/scopes.cc',
613613
'../src/ast/scopes.h',
614+
'../src/ast/source-range-ast-visitor.cc',
615+
'../src/ast/source-range-ast-visitor.h',
614616
'../src/ast/variables.cc',
615617
'../src/ast/variables.h',
616618
'../src/bailout-reason.cc',

‎deps/v8/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

‎deps/v8/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 == 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

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_

0 commit comments

Comments
 (0)