Skip to content

Commit

Permalink
[vm/ffi] Fix representation of value for 8-bit and 16-bit FFI loads a…
Browse files Browse the repository at this point in the history
…nd stores

Previously, FFI store could use kUnboxedUint32 for value being stored
via 8-bit or 16-bit StoreIndexed instruction. However, such
StoreIndexed instructions require kUnboxedIntPtr representation.
Due to the mismatch in the representations, SelectRepresentations
pass inserts a speculative (deoptimizing) IntConverter instruction,
which cases crash in AOT mode. Similar problem exists for FFI loads.

This change corrects representation when unboxing value in the body
of FFI store intrinsics and when boxing the value in FFI loads,
so representation of the value matches representation required by
StoreIndexed / returned by LoadIndexed.

TEST=ffi/regress_flutter79441_test
Fixes flutter/flutter#79441

Change-Id: Ida144e8d2e7a69d6767c9d4447bb20e79d847d48
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/193824
Commit-Queue: Alexander Markov <alexmarkov@google.com>
Reviewed-by: Daco Harkes <dacoharkes@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
  • Loading branch information
alexmarkov authored and commit-bot@chromium.org committed Apr 6, 2021
1 parent 1bf4b0c commit 707cd92
Show file tree
Hide file tree
Showing 3 changed files with 96 additions and 0 deletions.
20 changes: 20 additions & 0 deletions runtime/vm/compiler/frontend/kernel_to_il.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1332,6 +1332,16 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfRecognizedMethod(
body += FloatToDouble();
}
body += Box(kUnboxedDouble);
} else if (kind == MethodRecognizer::kFfiLoadInt8 ||
kind == MethodRecognizer::kFfiLoadInt16 ||
kind == MethodRecognizer::kFfiLoadUint8 ||
kind == MethodRecognizer::kFfiLoadUint16) {
// LoadIndexed instruction with 8-bit and 16-bit elements
// results in value with kUnboxedIntPtr representation
// (see LoadIndexedInstr::representation).
// Avoid any unnecessary (and potentially deoptimizing) int
// conversions by using the correct representation at the first place.
body += Box(kUnboxedIntPtr);
} else {
body += Box(native_rep.AsRepresentationOverApprox(zone_));
if (kind == MethodRecognizer::kFfiLoadPointer) {
Expand Down Expand Up @@ -1462,6 +1472,16 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfRecognizedMethod(
kind == MethodRecognizer::kFfiStoreFloatUnaligned) {
body += DoubleToFloat();
}
} else if (kind == MethodRecognizer::kFfiStoreInt8 ||
kind == MethodRecognizer::kFfiStoreInt16 ||
kind == MethodRecognizer::kFfiStoreUint8 ||
kind == MethodRecognizer::kFfiStoreUint16) {
// StoreIndexed instruction with 8-bit and 16-bit elements
// takes value with kUnboxedIntPtr representation
// (see StoreIndexedInstr::RequiredInputRepresentation).
// Avoid any unnecessary (and potentially deoptimizing) int
// conversions by using the correct representation at the first place.
body += UnboxTruncate(kUnboxedIntPtr);
} else {
body += UnboxTruncate(native_rep.AsRepresentationOverApprox(zone_));
}
Expand Down
38 changes: 38 additions & 0 deletions tests/ffi/regress_flutter79441_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// Copyright (c) 2021, 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/flutter/flutter/issues/79441.

import 'dart:ffi';

// FFI signature
typedef _dart_memset = void Function(Pointer<Uint8>, int, int);
typedef _c_memset = Void Function(Pointer<Uint8>, Int32, IntPtr);

_dart_memset? fbMemset;

void _fallbackMemset(Pointer<Uint8> ptr, int byte, int size) {
final bytes = ptr.cast<Uint8>();
for (var i = 0; i < size; i++) {
bytes[i] = byte;
}
}

void main() {
try {
fbMemset = DynamicLibrary.process()
.lookupFunction<_c_memset, _dart_memset>('memset');
} catch (_) {
// This works:
// fbMemset = _fallbackMemset;

// This doesn't: /aot/precompiler.cc: 2761: error: unreachable code
fbMemset = (Pointer<Uint8> ptr, int byte, int size) {
final bytes = ptr.cast<Uint8>();
for (var i = 0; i < size; i++) {
bytes[i] = byte;
}
};
}
}
38 changes: 38 additions & 0 deletions tests/ffi_2/regress_flutter79441_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// Copyright (c) 2021, 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/flutter/flutter/issues/79441.

import 'dart:ffi';

// FFI signature
typedef _dart_memset = void Function(Pointer<Uint8>, int, int);
typedef _c_memset = Void Function(Pointer<Uint8>, Int32, IntPtr);

_dart_memset fbMemset;

void _fallbackMemset(Pointer<Uint8> ptr, int byte, int size) {
final bytes = ptr.cast<Uint8>();
for (var i = 0; i < size; i++) {
bytes[i] = byte;
}
}

void main() {
try {
fbMemset = DynamicLibrary.process()
.lookupFunction<_c_memset, _dart_memset>('memset');
} catch (_) {
// This works:
// fbMemset = _fallbackMemset;

// This doesn't: /aot/precompiler.cc: 2761: error: unreachable code
fbMemset = (Pointer<Uint8> ptr, int byte, int size) {
final bytes = ptr.cast<Uint8>();
for (var i = 0; i < size; i++) {
bytes[i] = byte;
}
};
}
}

0 comments on commit 707cd92

Please sign in to comment.