Skip to content

Commit 62354c1

Browse files
mralephcommit-bot@chromium.org
mraleph
authored and
[email protected]
committed
[vm/arm64] Refactor leaf runtime call sequence.
Leaf runtime calls a silently clobbering R23 and R25 registers, which causes bugs when these calls happen from write barriers (which are not expected to clobber anything outside of R25). We introduce a special helper class to handle saving and restoring necessary registers. The bug originally reported by Joe Lin <[email protected]> who proposed a fix[1]. This CL builds on top of that fix and tries to make it more safe by forcing users to use a more strict API. [1] https://dart-review.googlesource.com/c/sdk/+/162300 TEST=vm/dart/write_barrier_register_clobber_test Change-Id: I93e0a13a3c4c38ad28210b35750a66e615e3e44a Cq-Include-Trybots: luci.dart.try:vm-kernel-linux-release-simarm64-try,vm-kernel-precomp-linux-release-simarm64-try Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/162191 Reviewed-by: Martin Kustermann <[email protected]> Commit-Queue: Vyacheslav Egorov <[email protected]>
1 parent f4d3f8e commit 62354c1

File tree

9 files changed

+325
-70
lines changed

9 files changed

+325
-70
lines changed

‎runtime/tests/vm/dart/write_barrier_register_clobber_test.dart‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
// Copyright (c) 2020, the Dart project authors. Please see the AUTHORS file
2+
// for details. All rights reserved. Use of this source code is governed by a
3+
// BSD-style license that can be found in the LICENSE file.
4+
5+
// This test attempts to verify that write barrier slow path does not
6+
// clobber any live values.
7+
8+
import 'dart:_internal' show VMInternalsForTesting;
9+
10+
import 'package:expect/expect.dart';
11+
12+
class Old {
13+
var f;
14+
Old(this.f);
15+
}
16+
17+
@pragma('vm:never-inline')
18+
int crashy(int v, List<Old> oldies) {
19+
// This test attempts to create a lot of live values which would live across
20+
// write barrier invocation so that when write-barrier calls runtime and
21+
// clobbers a register this is detected.
22+
var young = Object();
23+
var len = oldies.length;
24+
var i = 0;
25+
var v00 = v + 0;
26+
var v01 = v + 1;
27+
var v02 = v + 2;
28+
var v03 = v + 3;
29+
var v04 = v + 4;
30+
var v05 = v + 5;
31+
var v06 = v + 6;
32+
var v07 = v + 7;
33+
var v08 = v + 8;
34+
var v09 = v + 9;
35+
var v10 = v + 10;
36+
var v11 = v + 11;
37+
var v12 = v + 12;
38+
var v13 = v + 13;
39+
var v14 = v + 14;
40+
var v15 = v + 15;
41+
var v16 = v + 16;
42+
var v17 = v + 17;
43+
var v18 = v + 18;
44+
var v19 = v + 19;
45+
while (i < len) {
46+
// Eventually this will overflow store buffer and call runtime to acquire
47+
// a new block.
48+
oldies[i++].f = young;
49+
}
50+
return v00 +
51+
v01 +
52+
v02 +
53+
v03 +
54+
v04 +
55+
v05 +
56+
v06 +
57+
v07 +
58+
v08 +
59+
v09 +
60+
v10 +
61+
v11 +
62+
v12 +
63+
v13 +
64+
v14 +
65+
v15 +
66+
v16 +
67+
v17 +
68+
v18 +
69+
v19;
70+
}
71+
72+
void main(List<String> args) {
73+
final init = args.contains('impossible') ? 1 : 0;
74+
final oldies = List<Old>.generate(100000, (i) => Old(""));
75+
VMInternalsForTesting.collectAllGarbage();
76+
VMInternalsForTesting.collectAllGarbage();
77+
Expect.equals(crashy(init, oldies), 190);
78+
for (var o in oldies) {
79+
Expect.isTrue(o.f is! String);
80+
}
81+
}

‎runtime/tests/vm/dart_2/write_barrier_register_clobber_test.dart‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
// Copyright (c) 2020, the Dart project authors. Please see the AUTHORS file
2+
// for details. All rights reserved. Use of this source code is governed by a
3+
// BSD-style license that can be found in the LICENSE file.
4+
5+
// This test attempts to verify that write barrier slow path does not
6+
// clobber any live values.
7+
8+
import 'dart:_internal' show VMInternalsForTesting;
9+
10+
import 'package:expect/expect.dart';
11+
12+
class Old {
13+
var f;
14+
Old(this.f);
15+
}
16+
17+
@pragma('vm:never-inline')
18+
int crashy(int v, List<Old> oldies) {
19+
// This test attempts to create a lot of live values which would live across
20+
// write barrier invocation so that when write-barrier calls runtime and
21+
// clobbers a register this is detected.
22+
var young = Object();
23+
var len = oldies.length;
24+
var i = 0;
25+
var v00 = v + 0;
26+
var v01 = v + 1;
27+
var v02 = v + 2;
28+
var v03 = v + 3;
29+
var v04 = v + 4;
30+
var v05 = v + 5;
31+
var v06 = v + 6;
32+
var v07 = v + 7;
33+
var v08 = v + 8;
34+
var v09 = v + 9;
35+
var v10 = v + 10;
36+
var v11 = v + 11;
37+
var v12 = v + 12;
38+
var v13 = v + 13;
39+
var v14 = v + 14;
40+
var v15 = v + 15;
41+
var v16 = v + 16;
42+
var v17 = v + 17;
43+
var v18 = v + 18;
44+
var v19 = v + 19;
45+
while (i < len) {
46+
// Eventually this will overflow store buffer and call runtime to acquire
47+
// a new block.
48+
oldies[i++].f = young;
49+
}
50+
return v00 +
51+
v01 +
52+
v02 +
53+
v03 +
54+
v04 +
55+
v05 +
56+
v06 +
57+
v07 +
58+
v08 +
59+
v09 +
60+
v10 +
61+
v11 +
62+
v12 +
63+
v13 +
64+
v14 +
65+
v15 +
66+
v16 +
67+
v17 +
68+
v18 +
69+
v19;
70+
}
71+
72+
void main(List<String> args) {
73+
final init = args.contains('impossible') ? 1 : 0;
74+
final oldies = List<Old>.generate(100000, (i) => Old(""));
75+
VMInternalsForTesting.collectAllGarbage();
76+
VMInternalsForTesting.collectAllGarbage();
77+
Expect.equals(crashy(init, oldies), 190);
78+
for (var o in oldies) {
79+
Expect.isTrue(o.f is! String);
80+
}
81+
}

‎runtime/vm/compiler/assembler/assembler_arm64.cc‎

Lines changed: 49 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1485,7 +1485,7 @@ void Assembler::TransitionNativeToGenerated(Register state,
14851485
StoreToOffset(state, THR, target::Thread::exit_through_ffi_offset());
14861486
}
14871487

1488-
void Assembler::EnterCallRuntimeFrame(intptr_t frame_size) {
1488+
void Assembler::EnterCallRuntimeFrame(intptr_t frame_size, bool is_leaf) {
14891489
Comment("EnterCallRuntimeFrame");
14901490
EnterFrame(0);
14911491
if (!(FLAG_precompiled_mode && FLAG_use_bare_instructions)) {
@@ -1510,19 +1510,30 @@ void Assembler::EnterCallRuntimeFrame(intptr_t frame_size) {
15101510
Push(reg);
15111511
}
15121512

1513-
ReserveAlignedFrameSpace(frame_size);
1513+
if (!is_leaf) { // Leaf calling sequence aligns the stack itself.
1514+
ReserveAlignedFrameSpace(frame_size);
1515+
} else {
1516+
PushPair(kCallLeafRuntimeCalleeSaveScratch1,
1517+
kCallLeafRuntimeCalleeSaveScratch2);
1518+
}
15141519
}
15151520

1516-
void Assembler::LeaveCallRuntimeFrame() {
1521+
void Assembler::LeaveCallRuntimeFrame(bool is_leaf) {
15171522
// SP might have been modified to reserve space for arguments
15181523
// and ensure proper alignment of the stack frame.
15191524
// We need to restore it before restoring registers.
1525+
const intptr_t fixed_frame_words_without_pc_and_fp =
1526+
target::frame_layout.dart_fixed_frame_size - 2;
15201527
const intptr_t kPushedRegistersSize =
1521-
kDartVolatileCpuRegCount * target::kWordSize +
1522-
kDartVolatileFpuRegCount * target::kWordSize +
1523-
(target::frame_layout.dart_fixed_frame_size - 2) *
1524-
target::kWordSize; // From EnterStubFrame (excluding PC / FP)
1528+
kDartVolatileFpuRegCount * sizeof(double) +
1529+
(kDartVolatileCpuRegCount + (is_leaf ? 2 : 0) +
1530+
fixed_frame_words_without_pc_and_fp) *
1531+
target::kWordSize;
15251532
AddImmediate(SP, FP, -kPushedRegistersSize);
1533+
if (is_leaf) {
1534+
PopPair(kCallLeafRuntimeCalleeSaveScratch1,
1535+
kCallLeafRuntimeCalleeSaveScratch2);
1536+
}
15261537
for (int i = kDartLastVolatileCpuReg; i >= kDartFirstVolatileCpuReg; i--) {
15271538
const Register reg = static_cast<Register>(i);
15281539
Pop(reg);
@@ -1547,6 +1558,37 @@ void Assembler::CallRuntime(const RuntimeEntry& entry,
15471558
entry.Call(this, argument_count);
15481559
}
15491560

1561+
void Assembler::CallRuntimeScope::Call(intptr_t argument_count) {
1562+
assembler_->CallRuntime(entry_, argument_count);
1563+
}
1564+
1565+
Assembler::CallRuntimeScope::~CallRuntimeScope() {
1566+
if (preserve_registers_) {
1567+
assembler_->LeaveCallRuntimeFrame(entry_.is_leaf());
1568+
if (restore_code_reg_) {
1569+
assembler_->Pop(CODE_REG);
1570+
}
1571+
}
1572+
}
1573+
1574+
Assembler::CallRuntimeScope::CallRuntimeScope(Assembler* assembler,
1575+
const RuntimeEntry& entry,
1576+
intptr_t frame_size,
1577+
bool preserve_registers,
1578+
const Address* caller)
1579+
: assembler_(assembler),
1580+
entry_(entry),
1581+
preserve_registers_(preserve_registers),
1582+
restore_code_reg_(caller != nullptr) {
1583+
if (preserve_registers_) {
1584+
if (caller != nullptr) {
1585+
assembler_->Push(CODE_REG);
1586+
assembler_->ldr(CODE_REG, *caller);
1587+
}
1588+
assembler_->EnterCallRuntimeFrame(frame_size, entry.is_leaf());
1589+
}
1590+
}
1591+
15501592
void Assembler::EnterStubFrame() {
15511593
EnterDartFrame(0);
15521594
}

‎runtime/vm/compiler/assembler/assembler_arm64.h‎

Lines changed: 48 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1651,10 +1651,51 @@ class Assembler : public AssemblerBase {
16511651
void EnterOsrFrame(intptr_t extra_size, Register new_pp = kNoRegister);
16521652
void LeaveDartFrame(RestorePP restore_pp = kRestoreCallerPP);
16531653

1654-
void EnterCallRuntimeFrame(intptr_t frame_size);
1655-
void LeaveCallRuntimeFrame();
16561654
void CallRuntime(const RuntimeEntry& entry, intptr_t argument_count);
16571655

1656+
// Helper method for performing runtime calls from callers requiring manual
1657+
// register preservation is required (e.g. outside IL instructions marked
1658+
// as calling).
1659+
class CallRuntimeScope : public ValueObject {
1660+
public:
1661+
CallRuntimeScope(Assembler* assembler,
1662+
const RuntimeEntry& entry,
1663+
intptr_t frame_size,
1664+
bool preserve_registers = true)
1665+
: CallRuntimeScope(assembler,
1666+
entry,
1667+
frame_size,
1668+
preserve_registers,
1669+
/*caller=*/nullptr) {}
1670+
1671+
CallRuntimeScope(Assembler* assembler,
1672+
const RuntimeEntry& entry,
1673+
intptr_t frame_size,
1674+
Address caller,
1675+
bool preserve_registers = true)
1676+
: CallRuntimeScope(assembler,
1677+
entry,
1678+
frame_size,
1679+
preserve_registers,
1680+
&caller) {}
1681+
1682+
void Call(intptr_t argument_count);
1683+
1684+
~CallRuntimeScope();
1685+
1686+
private:
1687+
CallRuntimeScope(Assembler* assembler,
1688+
const RuntimeEntry& entry,
1689+
intptr_t frame_size,
1690+
bool preserve_registers,
1691+
const Address* caller);
1692+
1693+
Assembler* const assembler_;
1694+
const RuntimeEntry& entry_;
1695+
const bool preserve_registers_;
1696+
const bool restore_code_reg_;
1697+
};
1698+
16581699
// Set up a stub frame so that the stack traversal code can easily identify
16591700
// a stub frame.
16601701
void EnterStubFrame();
@@ -2402,6 +2443,11 @@ class Assembler : public AssemblerBase {
24022443
CanBeSmi can_be_smi,
24032444
BarrierFilterMode barrier_filter_mode);
24042445

2446+
// Note: leaf call sequence uses some abi callee save registers as scratch
2447+
// so they should be manually preserved.
2448+
void EnterCallRuntimeFrame(intptr_t frame_size, bool is_leaf);
2449+
void LeaveCallRuntimeFrame(bool is_leaf);
2450+
24052451
friend class dart::FlowGraphCompiler;
24062452
std::function<void(Register reg)> generate_invoke_write_barrier_wrapper_;
24072453
std::function<void()> generate_invoke_array_write_barrier_;

‎runtime/vm/compiler/runtime_api.cc‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -278,6 +278,10 @@ word RuntimeEntry::OffsetFromThread() const {
278278
return target::Thread::OffsetFromThread(runtime_entry_);
279279
}
280280

281+
bool RuntimeEntry::is_leaf() const {
282+
return runtime_entry_->is_leaf();
283+
}
284+
281285
namespace target {
282286

283287
const word kOldPageSize = dart::kOldPageSize;

‎runtime/vm/compiler/runtime_api.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -233,6 +233,8 @@ class RuntimeEntry : public ValueObject {
233233

234234
word OffsetFromThread() const;
235235

236+
bool is_leaf() const;
237+
236238
protected:
237239
RuntimeEntry(const dart::RuntimeEntry* runtime_entry,
238240
RuntimeEntryCallInternal call)

0 commit comments

Comments
 (0)