[vm/compiler] Fix uses of constant indexes in LoadIndexed/StoreIndexed.

Previously, CanBeImmediateIndex calculated the scaling factor for the
index using Instance::ElementSizeFor. However, the LoadIndexed and
StoreIndexed instructions which use this function have an index_scale_
field that is not necessarily the same as the element size for the
class id. This means the displacement calculated within
CanBeImmediateIndex can differ from the actual displacement calculated
within ElementAddressForIntIndex, causing the bug seen by the fuzzer.

This CL moves the check in CanBeImmediateIndex to a new static
method Assembler::AddressCanHoldConstantIndex. In addition to the
original arguments to CanBeImmediateIndex, the new static method takes
an index_scale argument, so the field value can be passed appropriately.

It also adds an is_external argument on X64 and IA32 to match the other
architectures, since assuming a non-external typed data object could
cause a register to be used in cases where it isn't necessary.

TEST=vm/dart/regress_54486

Fixes: https://github.com/dart-lang/sdk/issues/54486
Change-Id: Ia553fb7da0500113b35f8d9af91a52df55437a3c
Cq-Include-Trybots: luci.dart.try:vm-aot-linux-debug-x64-try,vm-linux-debug-ia32-try,vm-aot-linux-debug-simarm_x64-try,vm-aot-linux-debug-simriscv64-try,vm-aot-mac-release-arm64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/345002
Reviewed-by: Daco Harkes <dacoharkes@google.com>
This commit is contained in:
Tess Strickland
2024-01-08 15:46:01 +00:00
parent b52dc18ff8
commit 1827fcbf68
16 changed files with 219 additions and 152 deletions
@@ -0,0 +1,25 @@
// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
// Regression test for https://github.com/dart-lang/sdk/issues/54486.
// Verifies that VM doesn't crash with an assertion failure in debug mode when
// generating a LoadIndexed instruction with a too-large constant index.
// Trimmed down a program generated by the Dart Project Fuzz Tester (1.101):
// dart dartfuzz.dart --seed 1258834069 --no-fp --no-ffi --flat
import 'dart:io';
import 'dart:typed_data';
import 'package:expect/expect.dart';
Int32x4List array = Int32x4List(22);
void foo0() => print(array[-(-33 >>> 8)]);
void foo1() => array[49] ^= Int32x4.bool(false, false, false, true);
main() {
Expect.throws(foo0);
Expect.throws(foo1);
}
@@ -3732,6 +3732,44 @@ void Assembler::GenerateUnRelocatedPcRelativeTailCall(
pattern.set_distance(offset_into_target);
}
bool Assembler::AddressCanHoldConstantIndex(const Object& constant,
bool is_load,
bool is_external,
intptr_t cid,
intptr_t index_scale,
bool* needs_base) {
if ((cid == kTypedDataInt32x4ArrayCid) ||
(cid == kTypedDataFloat32x4ArrayCid) ||
(cid == kTypedDataFloat64x2ArrayCid)) {
// We are using vldmd/vstmd which do not support offset.
return false;
}
if (!IsSafeSmi(constant)) return false;
const int64_t index = target::SmiValue(constant);
const intptr_t offset_base =
(is_external ? 0
: (target::Instance::DataOffsetFor(cid) - kHeapObjectTag));
const int64_t offset = index * index_scale + offset_base;
if (!Utils::MagnitudeIsUint(12, offset)) {
return false;
}
if (Address::CanHoldImmediateOffset(is_load, cid, offset)) {
if (needs_base != nullptr) {
*needs_base = false;
}
return true;
}
if (needs_base != nullptr &&
Address::CanHoldImmediateOffset(is_load, cid, offset - offset_base)) {
*needs_base = true;
return true;
}
return false;
}
Address Assembler::ElementAddressForIntIndex(bool is_load,
bool is_external,
intptr_t cid,
@@ -1494,6 +1494,13 @@ class Assembler : public AssemblerBase {
// nearby the load of the table address.
void LoadAllocationTracingStateAddress(Register dest, intptr_t cid);
static bool AddressCanHoldConstantIndex(const Object& constant,
bool is_load,
bool is_external,
intptr_t cid,
intptr_t index_scale,
bool* needs_base = nullptr);
Address ElementAddressForIntIndex(bool is_load,
bool is_external,
intptr_t cid,
@@ -2174,6 +2174,20 @@ void Assembler::GenerateUnRelocatedPcRelativeTailCall(
pattern.set_distance(offset_into_target);
}
bool Assembler::AddressCanHoldConstantIndex(const Object& constant,
bool is_external,
intptr_t cid,
intptr_t index_scale) {
if (!IsSafeSmi(constant)) return false;
const int64_t index = target::SmiValue(constant);
const int64_t offset = index * index_scale + HeapDataOffset(is_external, cid);
if (!Utils::IsInt(32, offset)) {
return false;
}
return Address::CanHoldOffset(static_cast<int32_t>(offset), Address::Offset,
Address::OperandSizeFor(cid));
}
Address Assembler::ElementAddressForIntIndex(bool is_external,
intptr_t cid,
intptr_t index_scale,
@@ -2360,6 +2360,11 @@ class Assembler : public AssemblerBase {
// See also above for the pc-relative call.
void GenerateUnRelocatedPcRelativeTailCall(intptr_t offset_into_target = 0);
static bool AddressCanHoldConstantIndex(const Object& constant,
bool is_external,
intptr_t cid,
intptr_t index_scale);
Address ElementAddressForIntIndex(bool is_external,
intptr_t cid,
intptr_t index_scale,
@@ -3129,6 +3129,18 @@ void Assembler::EnsureHasClassIdInDEBUG(intptr_t cid,
#endif
}
bool Assembler::AddressCanHoldConstantIndex(const Object& constant,
bool is_external,
intptr_t cid,
intptr_t index_scale) {
if (!IsSafeSmi(constant)) return false;
const int64_t index = target::SmiValue(constant);
const int64_t offset =
is_external ? 0 : (target::Instance::DataOffsetFor(cid) - kHeapObjectTag);
const int64_t disp = index * index_scale + offset;
return Utils::IsInt(32, disp);
}
Address Assembler::ElementAddressForIntIndex(bool is_external,
intptr_t cid,
intptr_t index_scale,
@@ -1018,6 +1018,11 @@ class Assembler : public AssemblerBase {
Register scratch,
Label* is_smi);
static bool AddressCanHoldConstantIndex(const Object& constant,
bool is_external,
intptr_t cid,
intptr_t index_scale);
static Address ElementAddressForIntIndex(bool is_external,
intptr_t cid,
intptr_t index_scale,
@@ -4605,6 +4605,20 @@ static OperandSize OperandSizeFor(intptr_t cid) {
}
}
bool Assembler::AddressCanHoldConstantIndex(const Object& constant,
bool is_external,
intptr_t cid,
intptr_t index_scale) {
if (!IsSafeSmi(constant)) return false;
const int64_t index = target::SmiValue(constant);
const int64_t offset = index * index_scale + HeapDataOffset(is_external, cid);
if (IsITypeImm(offset)) {
ASSERT(IsSTypeImm(offset));
return true;
}
return false;
}
Address Assembler::ElementAddressForIntIndex(bool is_external,
intptr_t cid,
intptr_t index_scale,
@@ -1549,6 +1549,11 @@ class Assembler : public MicroAssembler {
// See also above for the pc-relative call.
void GenerateUnRelocatedPcRelativeTailCall(intptr_t offset_into_target = 0);
static bool AddressCanHoldConstantIndex(const Object& constant,
bool is_external,
intptr_t cid,
intptr_t index_scale);
Address ElementAddressForIntIndex(bool is_external,
intptr_t cid,
intptr_t index_scale,
@@ -3,6 +3,7 @@
// BSD-style license that can be found in the LICENSE file.
#include "vm/globals.h" // NOLINT
#include "vm/pointer_tagging.h"
#if defined(TARGET_ARCH_X64)
#define SHOULD_NOT_INCLUDE_RUNTIME
@@ -2761,6 +2762,18 @@ Address Assembler::VMTagAddress() {
return Address(THR, target::Thread::vm_tag_offset());
}
bool Assembler::AddressCanHoldConstantIndex(const Object& constant,
bool is_external,
intptr_t cid,
intptr_t index_scale) {
if (!IsSafeSmi(constant)) return false;
const int64_t index = target::SmiValue(constant);
const int64_t disp =
index * index_scale +
(is_external ? 0 : target::Instance::DataOffsetFor(cid) - kHeapObjectTag);
return Utils::IsInt(32, disp);
}
Address Assembler::ElementAddressForIntIndex(bool is_external,
intptr_t cid,
intptr_t index_scale,
@@ -1421,6 +1421,11 @@ class Assembler : public AssemblerBase {
// Debugging and bringup support.
void Breakpoint() override { int3(); }
static bool AddressCanHoldConstantIndex(const Object& constant,
bool is_external,
intptr_t cid,
intptr_t index_scale);
static Address ElementAddressForIntIndex(bool is_external,
intptr_t cid,
intptr_t index_scale,
+14 -53
View File
@@ -2102,45 +2102,6 @@ void Utf8ScanInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
__ StoreFieldToOffset(flags_temp_reg, decoder_reg, scan_flags_field_offset);
}
static bool CanBeImmediateIndex(Value* value,
intptr_t cid,
bool is_external,
bool is_load,
bool* needs_base) {
if ((cid == kTypedDataInt32x4ArrayCid) ||
(cid == kTypedDataFloat32x4ArrayCid) ||
(cid == kTypedDataFloat64x2ArrayCid)) {
// We are using vldmd/vstmd which do not support offset.
return false;
}
ConstantInstr* constant = value->definition()->AsConstant();
if ((constant == nullptr) ||
!compiler::Assembler::IsSafeSmi(constant->value())) {
return false;
}
const int64_t index = compiler::target::SmiValue(constant->value());
const intptr_t scale = compiler::target::Instance::ElementSizeFor(cid);
const intptr_t base_offset =
(is_external ? 0 : (Instance::DataOffsetFor(cid) - kHeapObjectTag));
const int64_t offset = index * scale + base_offset;
if (!Utils::MagnitudeIsUint(12, offset)) {
return false;
}
if (compiler::Address::CanHoldImmediateOffset(is_load, cid, offset)) {
*needs_base = false;
return true;
}
if (compiler::Address::CanHoldImmediateOffset(is_load, cid,
offset - base_offset)) {
*needs_base = true;
return true;
}
return false;
}
LocationSummary* LoadIndexedInstr::MakeLocationSummary(Zone* zone,
bool opt) const {
const bool directly_addressable =
@@ -2158,15 +2119,13 @@ LocationSummary* LoadIndexedInstr::MakeLocationSummary(Zone* zone,
LocationSummary* locs = new (zone)
LocationSummary(zone, kNumInputs, kNumTemps, LocationSummary::kNoCall);
locs->set_in(0, Location::RequiresRegister());
bool needs_base = false;
if (CanBeImmediateIndex(index(), class_id(), IsExternal(),
true, // Load.
&needs_base)) {
// CanBeImmediateIndex must return false for unsafe smis.
locs->set_in(1, Location::Constant(index()->definition()->AsConstant()));
} else {
locs->set_in(1, Location::RequiresRegister());
}
const bool can_be_constant = index()->BindsToConstant() &&
compiler::Assembler::AddressCanHoldConstantIndex(
index()->BoundConstant(), /*load=*/true,
IsExternal(), class_id(), index_scale());
locs->set_in(1, can_be_constant
? Location::Constant(index()->definition()->AsConstant())
: Location::RequiresRegister());
if ((representation() == kUnboxedFloat) ||
(representation() == kUnboxedDouble) ||
(representation() == kUnboxedFloat32x4) ||
@@ -2382,11 +2341,14 @@ LocationSummary* StoreIndexedInstr::MakeLocationSummary(Zone* zone,
const intptr_t kNumInputs = 3;
LocationSummary* locs;
bool needs_base = false;
intptr_t kNumTemps = 0;
if (CanBeImmediateIndex(index(), class_id(), IsExternal(),
false, // Store.
&needs_base)) {
bool needs_base = false;
const bool can_be_constant =
index()->BindsToConstant() &&
compiler::Assembler::AddressCanHoldConstantIndex(
index()->BoundConstant(), /*load=*/false, IsExternal(), class_id(),
index_scale(), &needs_base);
if (can_be_constant) {
if (!directly_addressable) {
kNumTemps += 2;
} else if (needs_base) {
@@ -2396,7 +2358,6 @@ LocationSummary* StoreIndexedInstr::MakeLocationSummary(Zone* zone,
locs = new (zone)
LocationSummary(zone, kNumInputs, kNumTemps, LocationSummary::kNoCall);
// CanBeImmediateIndex must return false for unsafe smis.
locs->set_in(1, Location::Constant(index()->definition()->AsConstant()));
} else {
if (!directly_addressable) {
+14 -28
View File
@@ -1887,24 +1887,6 @@ void Utf8ScanInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
}
}
static bool CanBeImmediateIndex(Value* value, intptr_t cid, bool is_external) {
ConstantInstr* constant = value->definition()->AsConstant();
if ((constant == nullptr) || !constant->value().IsSmi()) {
return false;
}
const int64_t index = Smi::Cast(constant->value()).AsInt64Value();
const intptr_t scale = Instance::ElementSizeFor(cid);
const int64_t offset =
index * scale +
(is_external ? 0 : (Instance::DataOffsetFor(cid) - kHeapObjectTag));
if (!Utils::IsInt(32, offset)) {
return false;
}
return compiler::Address::CanHoldOffset(
static_cast<int32_t>(offset), compiler::Address::Offset,
compiler::Address::OperandSizeFor(cid));
}
LocationSummary* LoadIndexedInstr::MakeLocationSummary(Zone* zone,
bool opt) const {
const intptr_t kNumInputs = 2;
@@ -1912,11 +1894,13 @@ LocationSummary* LoadIndexedInstr::MakeLocationSummary(Zone* zone,
LocationSummary* locs = new (zone)
LocationSummary(zone, kNumInputs, kNumTemps, LocationSummary::kNoCall);
locs->set_in(0, Location::RequiresRegister());
if (CanBeImmediateIndex(index(), class_id(), IsExternal())) {
locs->set_in(1, Location::Constant(index()->definition()->AsConstant()));
} else {
locs->set_in(1, Location::RequiresRegister());
}
const bool can_be_constant =
index()->BindsToConstant() &&
compiler::Assembler::AddressCanHoldConstantIndex(
index()->BoundConstant(), IsExternal(), class_id(), index_scale());
locs->set_in(1, can_be_constant
? Location::Constant(index()->definition()->AsConstant())
: Location::RequiresRegister());
if ((representation() == kUnboxedFloat) ||
(representation() == kUnboxedDouble) ||
(representation() == kUnboxedFloat32x4) ||
@@ -2087,11 +2071,13 @@ LocationSummary* StoreIndexedInstr::MakeLocationSummary(Zone* zone,
LocationSummary* locs = new (zone)
LocationSummary(zone, kNumInputs, kNumTemps, LocationSummary::kNoCall);
locs->set_in(0, Location::RequiresRegister());
if (CanBeImmediateIndex(index(), class_id(), IsExternal())) {
locs->set_in(1, Location::Constant(index()->definition()->AsConstant()));
} else {
locs->set_in(1, Location::RequiresRegister());
}
const bool can_be_constant =
index()->BindsToConstant() &&
compiler::Assembler::AddressCanHoldConstantIndex(
index()->BoundConstant(), IsExternal(), class_id(), index_scale());
locs->set_in(1, can_be_constant
? Location::Constant(index()->definition()->AsConstant())
: Location::RequiresRegister());
locs->set_temp(0, Location::RequiresRegister());
switch (class_id()) {
+24 -31
View File
@@ -1373,19 +1373,6 @@ void CCallInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
__ LeaveCFrame();
}
static bool CanBeImmediateIndex(Value* value, intptr_t cid) {
ConstantInstr* constant = value->definition()->AsConstant();
if ((constant == nullptr) ||
!compiler::Assembler::IsSafeSmi(constant->value())) {
return false;
}
const int64_t index = Smi::Cast(constant->value()).AsInt64Value();
const intptr_t scale = Instance::ElementSizeFor(cid);
const intptr_t offset = Instance::DataOffsetFor(cid);
const int64_t displacement = index * scale + offset;
return Utils::IsInt(32, displacement);
}
LocationSummary* OneByteStringFromCharCodeInstr::MakeLocationSummary(
Zone* zone,
bool opt) const {
@@ -1592,15 +1579,18 @@ LocationSummary* LoadIndexedInstr::MakeLocationSummary(Zone* zone,
LocationSummary* locs = new (zone)
LocationSummary(zone, kNumInputs, kNumTemps, LocationSummary::kNoCall);
locs->set_in(0, Location::RequiresRegister());
if (CanBeImmediateIndex(index(), class_id())) {
// CanBeImmediateIndex must return false for unsafe smis.
locs->set_in(1, Location::Constant(index()->definition()->AsConstant()));
} else {
// The index is either untagged (element size == 1) or a smi (for all
// element sizes > 1).
locs->set_in(1, (index_scale() == 1) ? Location::WritableRegister()
: Location::RequiresRegister());
}
// The index is either untagged (element size == 1) or a smi (for all
// element sizes > 1).
const bool need_writable_index_register = index_scale() == 1;
const bool can_be_constant =
index()->BindsToConstant() &&
compiler::Assembler::AddressCanHoldConstantIndex(
index()->BoundConstant(), IsExternal(), class_id(), index_scale());
locs->set_in(
1, can_be_constant
? Location::Constant(index()->definition()->AsConstant())
: (need_writable_index_register ? Location::WritableRegister()
: Location::RequiresRegister()));
if ((representation() == kUnboxedFloat) ||
(representation() == kUnboxedDouble) ||
(representation() == kUnboxedFloat32x4) ||
@@ -1751,15 +1741,18 @@ LocationSummary* StoreIndexedInstr::MakeLocationSummary(Zone* zone,
LocationSummary* locs = new (zone)
LocationSummary(zone, kNumInputs, kNumTemps, LocationSummary::kNoCall);
locs->set_in(0, Location::RequiresRegister());
if (CanBeImmediateIndex(index(), class_id())) {
// CanBeImmediateIndex must return false for unsafe smis.
locs->set_in(1, Location::Constant(index()->definition()->AsConstant()));
} else {
// The index is either untagged (element size == 1) or a smi (for all
// element sizes > 1).
locs->set_in(1, (index_scale() == 1) ? Location::WritableRegister()
: Location::RequiresRegister());
}
// The index is either untagged (element size == 1) or a smi (for all
// element sizes > 1).
const bool need_writable_index_register = index_scale() == 1;
const bool can_be_constant =
index()->BindsToConstant() &&
compiler::Assembler::AddressCanHoldConstantIndex(
index()->BoundConstant(), IsExternal(), class_id(), index_scale());
locs->set_in(
1, can_be_constant
? Location::Constant(index()->definition()->AsConstant())
: (need_writable_index_register ? Location::WritableRegister()
: Location::RequiresRegister()));
switch (class_id()) {
case kArrayCid:
locs->set_in(2, LocationRegisterOrConstant(value()));
+14 -27
View File
@@ -2038,23 +2038,6 @@ void Utf8ScanInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
}
}
static bool CanBeImmediateIndex(Value* value, intptr_t cid, bool is_external) {
ConstantInstr* constant = value->definition()->AsConstant();
if ((constant == nullptr) || !constant->value().IsSmi()) {
return false;
}
const int64_t index = Smi::Cast(constant->value()).AsInt64Value();
const intptr_t scale = Instance::ElementSizeFor(cid);
const int64_t offset =
index * scale +
(is_external ? 0 : (Instance::DataOffsetFor(cid) - kHeapObjectTag));
if (IsITypeImm(offset)) {
ASSERT(IsSTypeImm(offset));
return true;
}
return false;
}
LocationSummary* LoadIndexedInstr::MakeLocationSummary(Zone* zone,
bool opt) const {
const intptr_t kNumInputs = 2;
@@ -2062,11 +2045,13 @@ LocationSummary* LoadIndexedInstr::MakeLocationSummary(Zone* zone,
LocationSummary* locs = new (zone)
LocationSummary(zone, kNumInputs, kNumTemps, LocationSummary::kNoCall);
locs->set_in(0, Location::RequiresRegister());
if (CanBeImmediateIndex(index(), class_id(), IsExternal())) {
locs->set_in(1, Location::Constant(index()->definition()->AsConstant()));
} else {
locs->set_in(1, Location::RequiresRegister());
}
const bool can_be_constant =
index()->BindsToConstant() &&
compiler::Assembler::AddressCanHoldConstantIndex(
index()->BoundConstant(), IsExternal(), class_id(), index_scale());
locs->set_in(1, can_be_constant
? Location::Constant(index()->definition()->AsConstant())
: Location::RequiresRegister());
if ((representation() == kUnboxedFloat) ||
(representation() == kUnboxedDouble) ||
(representation() == kUnboxedFloat32x4) ||
@@ -2307,11 +2292,13 @@ LocationSummary* StoreIndexedInstr::MakeLocationSummary(Zone* zone,
LocationSummary* locs = new (zone)
LocationSummary(zone, kNumInputs, kNumTemps, LocationSummary::kNoCall);
locs->set_in(0, Location::RequiresRegister());
if (CanBeImmediateIndex(index(), class_id(), IsExternal())) {
locs->set_in(1, Location::Constant(index()->definition()->AsConstant()));
} else {
locs->set_in(1, Location::RequiresRegister());
}
const bool can_be_constant =
index()->BindsToConstant() &&
compiler::Assembler::AddressCanHoldConstantIndex(
index()->BoundConstant(), IsExternal(), class_id(), index_scale());
locs->set_in(1, can_be_constant
? Location::Constant(index()->definition()->AsConstant())
: Location::RequiresRegister());
locs->set_temp(0, Location::RequiresRegister());
switch (class_id()) {
+10 -13
View File
@@ -1597,17 +1597,6 @@ void CCallInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
__ LeaveCFrame();
}
static bool CanBeImmediateIndex(Value* index, intptr_t cid) {
if (!index->definition()->IsConstant()) return false;
const Object& constant = index->definition()->AsConstant()->value();
if (!constant.IsSmi()) return false;
const Smi& smi_const = Smi::Cast(constant);
const intptr_t scale = Instance::ElementSizeFor(cid);
const intptr_t data_offset = Instance::DataOffsetFor(cid);
const int64_t disp = smi_const.AsInt64Value() * scale + data_offset;
return Utils::IsInt(32, disp);
}
LocationSummary* OneByteStringFromCharCodeInstr::MakeLocationSummary(
Zone* zone,
bool opt) const {
@@ -1827,8 +1816,12 @@ LocationSummary* LoadIndexedInstr::MakeLocationSummary(Zone* zone,
const bool need_writable_index_register =
(index_scale() == 1 && !index_unboxed_) ||
(index_scale() == 16 && index_unboxed_);
const bool can_be_constant =
index()->BindsToConstant() &&
compiler::Assembler::AddressCanHoldConstantIndex(
index()->BoundConstant(), IsExternal(), class_id(), index_scale());
locs->set_in(
1, CanBeImmediateIndex(index(), class_id())
1, can_be_constant
? Location::Constant(index()->definition()->AsConstant())
: (need_writable_index_register ? Location::WritableRegister()
: Location::RequiresRegister()));
@@ -2029,8 +2022,12 @@ LocationSummary* StoreIndexedInstr::MakeLocationSummary(Zone* zone,
const bool need_writable_index_register =
(index_scale() == 1 && !index_unboxed_) ||
(index_scale() == 16 && index_unboxed_);
const bool can_be_constant =
index()->BindsToConstant() &&
compiler::Assembler::AddressCanHoldConstantIndex(
index()->BoundConstant(), IsExternal(), class_id(), index_scale());
locs->set_in(
1, CanBeImmediateIndex(index(), class_id())
1, can_be_constant
? Location::Constant(index()->definition()->AsConstant())
: (need_writable_index_register ? Location::WritableRegister()
: Location::RequiresRegister()));