[vm/ffi] Fix sign extension for small int arguments on x64 Linux/MacOS

Sign extension of small integers is not symmetric between register arguments and register return values.
On Linux/MacOS x64, the caller is responsible for both the arguments and return value.

Previous tests only tested equality, which tests the equality of the lowest byte. However, when printing or using the small ints arithmetic all 4 bytes are used. The new tests pass back the number in a different format.
The callback tests test the number in Dart, so they did not suffer from implicit conversions. (TestTakeMaxUint8x10 in runtime/bin/ffi_test/ffi_test_functions.cc and tests/ffi_2/function_callbacks_test.dart.)

Fixes: https://github.com/dart-lang/sdk/issues/40537

Change-Id: I6a57cb7cb43206926eb101a66e3b2abfacec72a0
Cq-Include-Trybots: luci.dart.try:vm-ffi-android-debug-arm-try,vm-ffi-android-debug-arm64-try,app-kernel-linux-debug-x64-try,vm-kernel-linux-debug-ia32-try,vm-kernel-win-debug-x64-try,vm-kernel-win-debug-ia32-try,vm-kernel-precomp-linux-debug-x64-try,vm-dartkb-linux-release-x64-abi-try,vm-kernel-precomp-android-release-arm64-try,vm-kernel-asan-linux-release-x64-try,vm-kernel-linux-release-simarm-try,vm-kernel-linux-release-simarm64-try,vm-kernel-precomp-android-release-arm_x64-try,vm-kernel-precomp-obfuscate-linux-release-x64-try,dart-sdk-linux-try,analyzer-analysis-server-linux-try,analyzer-linux-release-try,front-end-linux-release-x64-try,vm-kernel-precomp-win-release-x64-try,vm-kernel-mac-debug-x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/134825
Commit-Queue: Daco Harkes <dacoharkes@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>

runtime/bin/ffi_test/ffi_test_functions.cc

@@ -186,6 +186,21 @@ DART_EXPORT int64_t Regress39044(int64_t a, int8_t b) {
   return retval;
 }
 
+DART_EXPORT intptr_t Regress40537(uint8_t x) {
+  std::cout << "Regress40537(" << static_cast<int>(x) << ")\n";
+  return x == 249 ? 1 : 0;
+}
+
+DART_EXPORT intptr_t Regress40537Variant2(uint8_t x) {
+  std::cout << "Regress40537Variant2(" << static_cast<int>(x) << ")\n";
+  return x;
+}
+
+DART_EXPORT uint8_t Regress40537Variant3(intptr_t x) {
+  std::cout << "Regress40537Variant3(" << static_cast<int>(x) << ")\n";
+  return x;
+}
+
 // Performs some computation on various sized unsigned ints.
 // Used for testing value ranges for unsigned ints.
 DART_EXPORT int64_t UintComputation(uint8_t a,

runtime/vm/compiler/ffi/native_calling_convention.cc

@@ -244,7 +244,7 @@ static NativeLocation& ResultLocation(const NativeType& payload_type,
                                       Zone* zone) {
   const auto& payload_type_converted = ConvertIfSoftFp(payload_type, zone);
   const auto& container_type =
-      CallingConventions::kArgumentRegisterExtension == kExtendedTo4
+      CallingConventions::kReturnRegisterExtension == kExtendedTo4
           ? payload_type_converted.WidenTo4Bytes(zone)
           : payload_type_converted;
   if (container_type.IsFloat()) {

runtime/vm/constants.h

@@ -27,7 +27,7 @@ enum ExtensionStrategy {
   kNotExtended,
   // Values smaller than 4 bytes are passed around zero- or signextended to
   // 4 bytes.
-  kExtendedTo4
+  kExtendedTo4,
 };
 
 }  // namespace dart

runtime/vm/constants_arm.h

@@ -389,6 +389,7 @@ class CallingConventions {
 
   // Whether 1 or 2 byte-sized arguments or return values are passed extended
   // to 4 bytes.
+  static constexpr ExtensionStrategy kReturnRegisterExtension = kExtendedTo4;
   static constexpr ExtensionStrategy kArgumentRegisterExtension = kExtendedTo4;
   static constexpr ExtensionStrategy kArgumentStackExtension = kExtendedTo4;
 

runtime/vm/constants_arm64.h

@@ -233,8 +233,10 @@ class CallingConventions {
   // Whether 1 or 2 byte-sized arguments or return values are passed extended
   // to 4 bytes.
 #if defined(TARGET_OS_MACOS_IOS)
+  static constexpr ExtensionStrategy kReturnRegisterExtension = kExtendedTo4;
   static constexpr ExtensionStrategy kArgumentRegisterExtension = kExtendedTo4;
 #else
+  static constexpr ExtensionStrategy kReturnRegisterExtension = kNotExtended;
   static constexpr ExtensionStrategy kArgumentRegisterExtension = kNotExtended;
 #endif
   static constexpr ExtensionStrategy kArgumentStackExtension = kNotExtended;

runtime/vm/constants_ia32.h

@@ -182,6 +182,7 @@ class CallingConventions {
 
   // Whether 1 or 2 byte-sized arguments or return values are passed extended
   // to 4 bytes.
+  static constexpr ExtensionStrategy kReturnRegisterExtension = kNotExtended;
   static constexpr ExtensionStrategy kArgumentRegisterExtension = kNotExtended;
   static constexpr ExtensionStrategy kArgumentStackExtension = kExtendedTo4;
 };

runtime/vm/constants_x64.h

@@ -214,6 +214,7 @@ class CallingConventions {
 
   // Whether 1 or 2 byte-sized arguments or return values are passed extended
   // to 4 bytes.
+  static constexpr ExtensionStrategy kReturnRegisterExtension = kNotExtended;
   static constexpr ExtensionStrategy kArgumentRegisterExtension = kNotExtended;
   static constexpr ExtensionStrategy kArgumentStackExtension = kNotExtended;
 
@@ -275,7 +276,11 @@ class CallingConventions {
 
   // Whether 1 or 2 byte-sized arguments or return values are passed extended
   // to 4 bytes.
-  static constexpr ExtensionStrategy kArgumentRegisterExtension = kNotExtended;
+  // Note that `kReturnRegisterExtension != kArgumentRegisterExtension`, which
+  // effectively means that the caller is responsable for truncating and
+  // extending both arguments and return value.
+  static constexpr ExtensionStrategy kReturnRegisterExtension = kNotExtended;
+  static constexpr ExtensionStrategy kArgumentRegisterExtension = kExtendedTo4;
   static constexpr ExtensionStrategy kArgumentStackExtension = kExtendedTo4;
 
 #endif

tests/ffi/regress_40537_test.dart

@@ -0,0 +1,81 @@
+// Copyright (c) 2020, 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.
+//
+// Truncation and sign extension of small ints, tested with something else than
+// equality.
+//
+// SharedObjects=ffi_test_functions
+
+import 'dart:ffi';
+
+import "package:expect/expect.dart";
+
+import "dylib_utils.dart";
+
+main() {
+  variant1Negative();
+  variant1Positive();
+  variant2Negative();
+  variant2Positive();
+  variant3Negative();
+}
+
+/// `return x == 249 ? 1 : 0`.
+///
+/// This doesn't catch the error.
+final regress40537 = ffiTestFunctions
+    .lookupFunction<IntPtr Function(Uint8), int Function(int)>("Regress40537");
+
+variant1Negative() {
+  // 0xF9 =  -7 in 2s complement.
+  // 0xF9 = 249 in unsinged.
+  final result = regress40537(-7);
+  print(result);
+  Expect.equals(1, result);
+}
+
+variant1Positive() {
+  // 0xF9 = 249 in unsinged.
+  final result = regress40537(0xFFFFFFF9);
+  print(result);
+  Expect.equals(1, result);
+}
+
+/// `return x`.
+///
+/// This does.
+final regress40537Variant2 =
+    ffiTestFunctions.lookupFunction<IntPtr Function(Uint8), int Function(int)>(
+        "Regress40537Variant2");
+
+variant2Negative() {
+  // The 32 bit representation of -7 is 0xFFFFFFF9.
+  // Only the lowest byte, 0xF9, should be interpreted by calling convention,
+  // or it should be truncated and zero extended before calling.
+  final result = regress40537Variant2(-7);
+  print(result);
+  Expect.equals(249, result);
+}
+
+variant2Positive() {
+  // Only the lowest byte, 0xF9, should be interpreted by calling convention,
+  // or it should be truncated and zero extended before calling.
+  final result = regress40537Variant2(0xFFFFFFF9);
+  print(result);
+  Expect.equals(249, result);
+}
+
+/// `return x`.
+final regress40537Variant3 =
+    ffiTestFunctions.lookupFunction<Uint8 Function(IntPtr), int Function(int)>(
+        "Regress40537Variant3");
+
+variant3Negative() {
+  // This really passes -7 its intptr_t.
+  final result = regress40537Variant3(-7);
+  print(result);
+  Expect.equals(249, result);
+}
+
+final ffiTestFunctions = dlopenPlatformSpecific("ffi_test_functions");

tests/ffi_2/regress_40537_test.dart

@@ -0,0 +1,81 @@
+// Copyright (c) 2020, 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.
+//
+// Truncation and sign extension of small ints, tested with something else than
+// equality.
+//
+// SharedObjects=ffi_test_functions
+
+import 'dart:ffi';
+
+import "package:expect/expect.dart";
+
+import "dylib_utils.dart";
+
+main() {
+  variant1Negative();
+  variant1Positive();
+  variant2Negative();
+  variant2Positive();
+  variant3Negative();
+}
+
+/// `return x == 249 ? 1 : 0`.
+///
+/// This doesn't catch the error.
+final regress40537 = ffiTestFunctions
+    .lookupFunction<IntPtr Function(Uint8), int Function(int)>("Regress40537");
+
+variant1Negative() {
+  // 0xF9 =  -7 in 2s complement.
+  // 0xF9 = 249 in unsinged.
+  final result = regress40537(-7);
+  print(result);
+  Expect.equals(1, result);
+}
+
+variant1Positive() {
+  // 0xF9 = 249 in unsinged.
+  final result = regress40537(0xFFFFFFF9);
+  print(result);
+  Expect.equals(1, result);
+}
+
+/// `return x`.
+///
+/// This does.
+final regress40537Variant2 =
+    ffiTestFunctions.lookupFunction<IntPtr Function(Uint8), int Function(int)>(
+        "Regress40537Variant2");
+
+variant2Negative() {
+  // The 32 bit representation of -7 is 0xFFFFFFF9.
+  // Only the lowest byte, 0xF9, should be interpreted by calling convention,
+  // or it should be truncated and zero extended before calling.
+  final result = regress40537Variant2(-7);
+  print(result);
+  Expect.equals(249, result);
+}
+
+variant2Positive() {
+  // Only the lowest byte, 0xF9, should be interpreted by calling convention,
+  // or it should be truncated and zero extended before calling.
+  final result = regress40537Variant2(0xFFFFFFF9);
+  print(result);
+  Expect.equals(249, result);
+}
+
+/// `return x`.
+final regress40537Variant3 =
+    ffiTestFunctions.lookupFunction<Uint8 Function(IntPtr), int Function(int)>(
+        "Regress40537Variant3");
+
+variant3Negative() {
+  // This really passes -7 its intptr_t.
+  final result = regress40537Variant3(-7);
+  print(result);
+  Expect.equals(249, result);
+}
+
+final ffiTestFunctions = dlopenPlatformSpecific("ffi_test_functions");