Skip to content

Commit a40fd6a

Browse files
aartbikcommit-bot@chromium.org
authored andcommitted
[vm/compiler] Fix bug in unboxed constant spilling
Rationale: Subtle but disastrous, spilling unboxed integers in arm64 and x64 would store the value as smi, detected as a nasty off by one error in flutter code. flutter/flutter#23879 #35091 Change-Id: I07d71727de8574ebd7d3ec3610d517d7903972f0 Reviewed-on: https://dart-review.googlesource.com/c/83565 Commit-Queue: Aart Bik <[email protected]> Reviewed-by: Siva Annamalai <[email protected]> Reviewed-by: Martin Kustermann <[email protected]> Reviewed-by: Vyacheslav Egorov <[email protected]> Reviewed-by: Alexander Markov <[email protected]>
1 parent c52a38c commit a40fd6a

File tree

3 files changed

+84
-9
lines changed

3 files changed

+84
-9
lines changed

runtime/vm/compiler/backend/il_arm64.cc

+6-4
Original file line numberDiff line numberDiff line change
@@ -320,8 +320,7 @@ void ConstantInstr::EmitMoveToLocation(FlowGraphCompiler* compiler,
320320
if (destination.IsRegister()) {
321321
if (representation() == kUnboxedInt32 ||
322322
representation() == kUnboxedInt64) {
323-
const int64_t value = value_.IsSmi() ? Smi::Cast(value_).Value()
324-
: Mint::Cast(value_).value();
323+
const int64_t value = Integer::Cast(value_).AsInt64Value();
325324
__ LoadImmediate(destination.reg(), value);
326325
} else {
327326
ASSERT(representation() == kTagged);
@@ -346,9 +345,12 @@ void ConstantInstr::EmitMoveToLocation(FlowGraphCompiler* compiler,
346345
ASSERT(destination.IsStackSlot());
347346
ASSERT(tmp != kNoRegister);
348347
const intptr_t dest_offset = destination.ToStackSlotOffset();
349-
if (value_.IsSmi() && representation() == kUnboxedInt32) {
350-
__ LoadImmediate(tmp, static_cast<int32_t>(Smi::Cast(value_).Value()));
348+
if (representation() == kUnboxedInt32 ||
349+
representation() == kUnboxedInt64) {
350+
const int64_t value = Integer::Cast(value_).AsInt64Value();
351+
__ LoadImmediate(tmp, value);
351352
} else {
353+
ASSERT(representation() == kTagged);
352354
__ LoadObject(tmp, value_);
353355
}
354356
__ StoreToOffset(tmp, destination.base_reg(), dest_offset);

runtime/vm/compiler/backend/il_x64.cc

+6-5
Original file line numberDiff line numberDiff line change
@@ -291,8 +291,7 @@ void ConstantInstr::EmitMoveToLocation(FlowGraphCompiler* compiler,
291291
if (destination.IsRegister()) {
292292
if (representation() == kUnboxedInt32 ||
293293
representation() == kUnboxedInt64) {
294-
const int64_t value = value_.IsSmi() ? Smi::Cast(value_).Value()
295-
: Mint::Cast(value_).value();
294+
const int64_t value = Integer::Cast(value_).AsInt64Value();
296295
if (value == 0) {
297296
__ xorl(destination.reg(), destination.reg());
298297
} else {
@@ -322,10 +321,12 @@ void ConstantInstr::EmitMoveToLocation(FlowGraphCompiler* compiler,
322321
__ movsd(destination.ToStackSlotAddress(), XMM0);
323322
} else {
324323
ASSERT(destination.IsStackSlot());
325-
if (value_.IsSmi() && representation() == kUnboxedInt32) {
326-
__ movl(destination.ToStackSlotAddress(),
327-
Immediate(Smi::Cast(value_).Value()));
324+
if (representation() == kUnboxedInt32 ||
325+
representation() == kUnboxedInt64) {
326+
const int64_t value = Integer::Cast(value_).AsInt64Value();
327+
__ movq(destination.ToStackSlotAddress(), Immediate(value));
328328
} else {
329+
ASSERT(representation() == kTagged);
329330
__ StoreObject(destination.ToStackSlotAddress(), value_);
330331
}
331332
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
// Copyright (c) 2018, 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+
// Bug in unboxed int spilling (https://github.com/flutter/flutter/issues/23879).
6+
//
7+
// VMOptions=--deterministic
8+
9+
import "package:expect/expect.dart";
10+
11+
List<int> list = new List<int>(10);
12+
13+
List<int> expected_values = [null, 152, 168, 184, 200, 216, 232, 248, 264, 280];
14+
15+
int count = 0;
16+
17+
bool getNext() {
18+
return ++count <= 9;
19+
}
20+
21+
int foo() {
22+
int a = 1;
23+
int b = 2;
24+
int c = 3;
25+
int d = 4;
26+
int e = 5;
27+
int f = 6;
28+
int g = 7;
29+
int h = 8;
30+
int i = 9;
31+
int j = 10;
32+
int k = 11;
33+
int l = 12;
34+
int m = 13;
35+
int n = 14;
36+
int o = 15;
37+
int p = 16;
38+
count = 0;
39+
int componentIndex = 1;
40+
while (getNext()) {
41+
// Make spilling likely.
42+
a++;
43+
b++;
44+
c++;
45+
d++;
46+
e++;
47+
f++;
48+
g++;
49+
h++;
50+
i++;
51+
j++;
52+
k++;
53+
l++;
54+
m++;
55+
n++;
56+
o++;
57+
p++;
58+
list[componentIndex] =
59+
a + b + c + d + e + f + g + h + i + j + k + l + m + n + o + p;
60+
componentIndex++;
61+
}
62+
return componentIndex;
63+
}
64+
65+
void main() {
66+
int x = foo();
67+
Expect.equals(10, x);
68+
Expect.equals(10, count);
69+
for (int i = 0; i < 10; i++) {
70+
Expect.equals(expected_values[i], list[i]);
71+
}
72+
}

0 commit comments

Comments
 (0)