From a2acd59d739fb79dbfe7634bd85cf44a841599e0 Mon Sep 17 00:00:00 2001 From: Vyacheslav Egorov Date: Fri, 27 Nov 2020 09:11:49 +0000 Subject: [PATCH] [vm/ffi] Fix LR clobber in JumpToFrame stub on ARM Both ARM and ARM64 variants did not account for the fact that TransitionNativeToGenerated will clobber LR on slow path. TEST=tests/ffi/regress_jump_to_frame_test.dart Cq-Include-Trybots: luci.dart.try:vm-ffi-android-release-arm-try,vm-ffi-android-release-arm64-try,vm-kernel-linux-release-simarm64-try,vm-kernel-linux-release-simarm-try,vm-kernel-precomp-linux-release-simarm-try,vm-kernel-precomp-linux-release-simarm64-try Change-Id: I40d48b720b2a7f31ff20903a887295ab2034662c Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/173962 Commit-Queue: Vyacheslav Egorov Reviewed-by: Martin Kustermann --- runtime/vm/compiler/asm_intrinsifier_arm.cc | 10 +-- runtime/vm/compiler/asm_intrinsifier_arm64.cc | 20 +++--- runtime/vm/compiler/asm_intrinsifier_x64.cc | 14 ++-- runtime/vm/compiler/graph_intrinsifier_arm.cc | 10 +-- .../vm/compiler/graph_intrinsifier_arm64.cc | 20 +++--- runtime/vm/compiler/graph_intrinsifier_x64.cc | 14 ++-- runtime/vm/compiler/stub_code_compiler_arm.cc | 13 ++-- .../vm/compiler/stub_code_compiler_arm64.cc | 7 +- runtime/vm/constants.h | 6 ++ runtime/vm/constants_x64.h | 3 + runtime/vm/runtime_entry_arm.cc | 4 +- runtime/vm/runtime_entry_arm64.cc | 4 +- tests/ffi/regress_jump_to_frame_test.dart | 64 +++++++++++++++++++ tests/ffi_2/regress_jump_to_frame_test.dart | 64 +++++++++++++++++++ 14 files changed, 185 insertions(+), 68 deletions(-) create mode 100644 tests/ffi/regress_jump_to_frame_test.dart create mode 100644 tests/ffi_2/regress_jump_to_frame_test.dart diff --git a/runtime/vm/compiler/asm_intrinsifier_arm.cc b/runtime/vm/compiler/asm_intrinsifier_arm.cc index 26a7f91cedd..fdfb79d0053 100644 --- a/runtime/vm/compiler/asm_intrinsifier_arm.cc +++ b/runtime/vm/compiler/asm_intrinsifier_arm.cc @@ -28,14 +28,10 @@ intptr_t AsmIntrinsifier::ParameterSlotFromSp() { return -1; } -static bool IsABIPreservedRegister(Register reg) { - return ((1 << reg) & kAbiPreservedCpuRegs) != 0; -} - void AsmIntrinsifier::IntrinsicCallPrologue(Assembler* assembler) { - ASSERT(IsABIPreservedRegister(CODE_REG)); - ASSERT(IsABIPreservedRegister(ARGS_DESC_REG)); - ASSERT(IsABIPreservedRegister(CALLEE_SAVED_TEMP)); + COMPILE_ASSERT(IsAbiPreservedRegister(CODE_REG)); + COMPILE_ASSERT(IsAbiPreservedRegister(ARGS_DESC_REG)); + COMPILE_ASSERT(IsAbiPreservedRegister(CALLEE_SAVED_TEMP)); // Save LR by moving it to a callee saved temporary register. assembler->Comment("IntrinsicCallPrologue"); diff --git a/runtime/vm/compiler/asm_intrinsifier_arm64.cc b/runtime/vm/compiler/asm_intrinsifier_arm64.cc index 14857e9b1a4..e218d712290 100644 --- a/runtime/vm/compiler/asm_intrinsifier_arm64.cc +++ b/runtime/vm/compiler/asm_intrinsifier_arm64.cc @@ -28,19 +28,15 @@ intptr_t AsmIntrinsifier::ParameterSlotFromSp() { return -1; } -static bool IsABIPreservedRegister(Register reg) { - return ((1 << reg) & kAbiPreservedCpuRegs) != 0; -} - void AsmIntrinsifier::IntrinsicCallPrologue(Assembler* assembler) { - ASSERT(IsABIPreservedRegister(CODE_REG)); - ASSERT(!IsABIPreservedRegister(ARGS_DESC_REG)); - ASSERT(IsABIPreservedRegister(CALLEE_SAVED_TEMP)); - ASSERT(IsABIPreservedRegister(CALLEE_SAVED_TEMP2)); - ASSERT(CALLEE_SAVED_TEMP != CODE_REG); - ASSERT(CALLEE_SAVED_TEMP != ARGS_DESC_REG); - ASSERT(CALLEE_SAVED_TEMP2 != CODE_REG); - ASSERT(CALLEE_SAVED_TEMP2 != ARGS_DESC_REG); + COMPILE_ASSERT(IsAbiPreservedRegister(CODE_REG)); + COMPILE_ASSERT(!IsAbiPreservedRegister(ARGS_DESC_REG)); + COMPILE_ASSERT(IsAbiPreservedRegister(CALLEE_SAVED_TEMP)); + COMPILE_ASSERT(IsAbiPreservedRegister(CALLEE_SAVED_TEMP2)); + COMPILE_ASSERT(CALLEE_SAVED_TEMP != CODE_REG); + COMPILE_ASSERT(CALLEE_SAVED_TEMP != ARGS_DESC_REG); + COMPILE_ASSERT(CALLEE_SAVED_TEMP2 != CODE_REG); + COMPILE_ASSERT(CALLEE_SAVED_TEMP2 != ARGS_DESC_REG); assembler->Comment("IntrinsicCallPrologue"); assembler->mov(CALLEE_SAVED_TEMP, LR); diff --git a/runtime/vm/compiler/asm_intrinsifier_x64.cc b/runtime/vm/compiler/asm_intrinsifier_x64.cc index ac7112c3218..f13d41f4e99 100644 --- a/runtime/vm/compiler/asm_intrinsifier_x64.cc +++ b/runtime/vm/compiler/asm_intrinsifier_x64.cc @@ -28,16 +28,12 @@ intptr_t AsmIntrinsifier::ParameterSlotFromSp() { return 0; } -static bool IsABIPreservedRegister(Register reg) { - return ((1 << reg) & CallingConventions::kCalleeSaveCpuRegisters) != 0; -} - void AsmIntrinsifier::IntrinsicCallPrologue(Assembler* assembler) { - ASSERT(IsABIPreservedRegister(CODE_REG)); - ASSERT(!IsABIPreservedRegister(ARGS_DESC_REG)); - ASSERT(IsABIPreservedRegister(CALLEE_SAVED_TEMP)); - ASSERT(CALLEE_SAVED_TEMP != CODE_REG); - ASSERT(CALLEE_SAVED_TEMP != ARGS_DESC_REG); + COMPILE_ASSERT(IsAbiPreservedRegister(CODE_REG)); + COMPILE_ASSERT(!IsAbiPreservedRegister(ARGS_DESC_REG)); + COMPILE_ASSERT(IsAbiPreservedRegister(CALLEE_SAVED_TEMP)); + COMPILE_ASSERT(CALLEE_SAVED_TEMP != CODE_REG); + COMPILE_ASSERT(CALLEE_SAVED_TEMP != ARGS_DESC_REG); assembler->Comment("IntrinsicCallPrologue"); assembler->movq(CALLEE_SAVED_TEMP, ARGS_DESC_REG); diff --git a/runtime/vm/compiler/graph_intrinsifier_arm.cc b/runtime/vm/compiler/graph_intrinsifier_arm.cc index cc8dc4c1994..1cebb33735f 100644 --- a/runtime/vm/compiler/graph_intrinsifier_arm.cc +++ b/runtime/vm/compiler/graph_intrinsifier_arm.cc @@ -17,14 +17,10 @@ intptr_t GraphIntrinsifier::ParameterSlotFromSp() { return -1; } -static bool IsABIPreservedRegister(Register reg) { - return ((1 << reg) & kAbiPreservedCpuRegs) != 0; -} - void GraphIntrinsifier::IntrinsicCallPrologue(Assembler* assembler) { - ASSERT(IsABIPreservedRegister(CODE_REG)); - ASSERT(IsABIPreservedRegister(ARGS_DESC_REG)); - ASSERT(IsABIPreservedRegister(CALLEE_SAVED_TEMP)); + COMPILE_ASSERT(IsAbiPreservedRegister(CODE_REG)); + COMPILE_ASSERT(IsAbiPreservedRegister(ARGS_DESC_REG)); + COMPILE_ASSERT(IsAbiPreservedRegister(CALLEE_SAVED_TEMP)); // Save LR by moving it to a callee saved temporary register. assembler->Comment("IntrinsicCallPrologue"); diff --git a/runtime/vm/compiler/graph_intrinsifier_arm64.cc b/runtime/vm/compiler/graph_intrinsifier_arm64.cc index 9bd2ae58062..99d33b45eb6 100644 --- a/runtime/vm/compiler/graph_intrinsifier_arm64.cc +++ b/runtime/vm/compiler/graph_intrinsifier_arm64.cc @@ -17,19 +17,15 @@ intptr_t GraphIntrinsifier::ParameterSlotFromSp() { return -1; } -static bool IsABIPreservedRegister(Register reg) { - return ((1 << reg) & kAbiPreservedCpuRegs) != 0; -} - void GraphIntrinsifier::IntrinsicCallPrologue(Assembler* assembler) { - ASSERT(IsABIPreservedRegister(CODE_REG)); - ASSERT(!IsABIPreservedRegister(ARGS_DESC_REG)); - ASSERT(IsABIPreservedRegister(CALLEE_SAVED_TEMP)); - ASSERT(IsABIPreservedRegister(CALLEE_SAVED_TEMP2)); - ASSERT(CALLEE_SAVED_TEMP != CODE_REG); - ASSERT(CALLEE_SAVED_TEMP != ARGS_DESC_REG); - ASSERT(CALLEE_SAVED_TEMP2 != CODE_REG); - ASSERT(CALLEE_SAVED_TEMP2 != ARGS_DESC_REG); + COMPILE_ASSERT(IsAbiPreservedRegister(CODE_REG)); + COMPILE_ASSERT(!IsAbiPreservedRegister(ARGS_DESC_REG)); + COMPILE_ASSERT(IsAbiPreservedRegister(CALLEE_SAVED_TEMP)); + COMPILE_ASSERT(IsAbiPreservedRegister(CALLEE_SAVED_TEMP2)); + COMPILE_ASSERT(CALLEE_SAVED_TEMP != CODE_REG); + COMPILE_ASSERT(CALLEE_SAVED_TEMP != ARGS_DESC_REG); + COMPILE_ASSERT(CALLEE_SAVED_TEMP2 != CODE_REG); + COMPILE_ASSERT(CALLEE_SAVED_TEMP2 != ARGS_DESC_REG); assembler->Comment("IntrinsicCallPrologue"); assembler->mov(CALLEE_SAVED_TEMP, LR); diff --git a/runtime/vm/compiler/graph_intrinsifier_x64.cc b/runtime/vm/compiler/graph_intrinsifier_x64.cc index 58c02b2de4a..e1479a8f8aa 100644 --- a/runtime/vm/compiler/graph_intrinsifier_x64.cc +++ b/runtime/vm/compiler/graph_intrinsifier_x64.cc @@ -17,16 +17,12 @@ intptr_t GraphIntrinsifier::ParameterSlotFromSp() { return 0; } -static bool IsABIPreservedRegister(Register reg) { - return ((1 << reg) & CallingConventions::kCalleeSaveCpuRegisters) != 0; -} - void GraphIntrinsifier::IntrinsicCallPrologue(Assembler* assembler) { - ASSERT(IsABIPreservedRegister(CODE_REG)); - ASSERT(!IsABIPreservedRegister(ARGS_DESC_REG)); - ASSERT(IsABIPreservedRegister(CALLEE_SAVED_TEMP)); - ASSERT(CALLEE_SAVED_TEMP != CODE_REG); - ASSERT(CALLEE_SAVED_TEMP != ARGS_DESC_REG); + COMPILE_ASSERT(IsAbiPreservedRegister(CODE_REG)); + COMPILE_ASSERT(!IsAbiPreservedRegister(ARGS_DESC_REG)); + COMPILE_ASSERT(IsAbiPreservedRegister(CALLEE_SAVED_TEMP)); + COMPILE_ASSERT(CALLEE_SAVED_TEMP != CODE_REG); + COMPILE_ASSERT(CALLEE_SAVED_TEMP != ARGS_DESC_REG); assembler->Comment("IntrinsicCallPrologue"); assembler->movq(CALLEE_SAVED_TEMP, ARGS_DESC_REG); diff --git a/runtime/vm/compiler/stub_code_compiler_arm.cc b/runtime/vm/compiler/stub_code_compiler_arm.cc index 4bc6aee1099..5bf87cb5d2c 100644 --- a/runtime/vm/compiler/stub_code_compiler_arm.cc +++ b/runtime/vm/compiler/stub_code_compiler_arm.cc @@ -350,7 +350,7 @@ void StubCodeCompiler::GenerateExitSafepointStub(Assembler* assembler) { // NOTFP, R4: clobbered, although normally callee-saved void StubCodeCompiler::GenerateCallNativeThroughSafepointStub( Assembler* assembler) { - COMPILE_ASSERT((kAbiPreservedCpuRegs & (1 << R4)) != 0); + COMPILE_ASSERT(IsAbiPreservedRegister(R4)); // TransitionGeneratedToNative might clobber LR if it takes the slow path. __ mov(R4, Operand(LR)); @@ -2867,10 +2867,13 @@ void StubCodeCompiler::GenerateGetCStackPointerStub(Assembler* assembler) { // // Notice: We need to keep this in sync with `Simulator::JumpToFrame()`. void StubCodeCompiler::GenerateJumpToFrameStub(Assembler* assembler) { - ASSERT(kExceptionObjectReg == R0); - ASSERT(kStackTraceObjectReg == R1); + COMPILE_ASSERT(kExceptionObjectReg == R0); + COMPILE_ASSERT(kStackTraceObjectReg == R1); + COMPILE_ASSERT(IsAbiPreservedRegister(R4)); + COMPILE_ASSERT(IsAbiPreservedRegister(THR)); __ mov(IP, Operand(R1)); // Copy Stack pointer into IP. - __ mov(LR, Operand(R0)); // Program counter. + // TransitionGeneratedToNative might clobber LR if it takes the slow path. + __ mov(R4, Operand(R0)); // Program counter. __ mov(THR, Operand(R3)); // Thread. __ mov(FP, Operand(R2)); // Frame_pointer. __ mov(SP, Operand(IP)); // Set Stack pointer. @@ -2903,7 +2906,7 @@ void StubCodeCompiler::GenerateJumpToFrameStub(Assembler* assembler) { } else { __ LoadPoolPointer(); } - __ bx(LR); // Jump to continuation point. + __ bx(R4); // Jump to continuation point. } // Run an exception handler. Execution comes from JumpToFrame diff --git a/runtime/vm/compiler/stub_code_compiler_arm64.cc b/runtime/vm/compiler/stub_code_compiler_arm64.cc index 1ef8b6abe8b..f98d773ed25 100644 --- a/runtime/vm/compiler/stub_code_compiler_arm64.cc +++ b/runtime/vm/compiler/stub_code_compiler_arm64.cc @@ -300,7 +300,7 @@ void StubCodeCompiler::GenerateExitSafepointStub(Assembler* assembler) { // Stack: preserved, CSP == SP void StubCodeCompiler::GenerateCallNativeThroughSafepointStub( Assembler* assembler) { - COMPILE_ASSERT((1 << R19) & kAbiPreservedCpuRegs); + COMPILE_ASSERT(IsAbiPreservedRegister(R19)); __ mov(R19, LR); __ LoadImmediate(R10, target::Thread::exit_through_ffi()); @@ -3007,7 +3007,8 @@ void StubCodeCompiler::GenerateGetCStackPointerStub(Assembler* assembler) { void StubCodeCompiler::GenerateJumpToFrameStub(Assembler* assembler) { ASSERT(kExceptionObjectReg == R0); ASSERT(kStackTraceObjectReg == R1); - __ mov(LR, R0); // Program counter. + // TransitionGeneratedToNative might clobber LR if it takes the slow path. + __ mov(CALLEE_SAVED_TEMP, R0); // Program counter. __ mov(SP, R1); // Stack pointer. __ mov(FP, R2); // Frame_pointer. __ mov(THR, R3); @@ -3042,7 +3043,7 @@ void StubCodeCompiler::GenerateJumpToFrameStub(Assembler* assembler) { } else { __ LoadPoolPointer(); } - __ ret(); // Jump to continuation point. + __ ret(CALLEE_SAVED_TEMP); // Jump to continuation point. } // Run an exception handler. Execution comes from JumpToFrame diff --git a/runtime/vm/constants.h b/runtime/vm/constants.h index f89abc69a99..01d9d08b5da 100644 --- a/runtime/vm/constants.h +++ b/runtime/vm/constants.h @@ -53,6 +53,12 @@ static constexpr bool IsCalleeSavedRegister(Register reg) { return ((1 << reg) & CallingConventions::kCalleeSaveCpuRegisters) != 0; } +#if !defined(TARGET_ARCH_IA32) +constexpr bool IsAbiPreservedRegister(Register reg) { + return (kAbiPreservedCpuRegs & (1 << reg)) != 0; +} +#endif + } // namespace dart #endif // RUNTIME_VM_CONSTANTS_H_ diff --git a/runtime/vm/constants_x64.h b/runtime/vm/constants_x64.h index 824a871d7dd..480f565c720 100644 --- a/runtime/vm/constants_x64.h +++ b/runtime/vm/constants_x64.h @@ -457,6 +457,9 @@ class CallingConventions { (kArgumentRegisters | R(kPointerToReturnStructRegisterCall))) == 0); }; +constexpr intptr_t kAbiPreservedCpuRegs = + CallingConventions::kCalleeSaveCpuRegisters; + #undef R class Instr { diff --git a/runtime/vm/runtime_entry_arm.cc b/runtime/vm/runtime_entry_arm.cc index b2cd9daaab5..0870d4541a3 100644 --- a/runtime/vm/runtime_entry_arm.cc +++ b/runtime/vm/runtime_entry_arm.cc @@ -58,8 +58,8 @@ void RuntimeEntry::CallInternal(const RuntimeEntry* runtime_entry, __ LoadImmediate(TMP, VMTag::kDartTagId); __ str(TMP, compiler::Address(THR, compiler::target::Thread::vm_tag_offset())); - ASSERT((kAbiPreservedCpuRegs & (1 << THR)) != 0); - ASSERT((kAbiPreservedCpuRegs & (1 << PP)) != 0); + COMPILE_ASSERT(IsAbiPreservedRegister(THR)); + COMPILE_ASSERT(IsAbiPreservedRegister(PP)); } else { // Argument count is not checked here, but in the runtime entry for a more // informative error message. diff --git a/runtime/vm/runtime_entry_arm64.cc b/runtime/vm/runtime_entry_arm64.cc index 1580e1aa41e..93a80fd91b7 100644 --- a/runtime/vm/runtime_entry_arm64.cc +++ b/runtime/vm/runtime_entry_arm64.cc @@ -73,8 +73,8 @@ void RuntimeEntry::CallInternal(const RuntimeEntry* runtime_entry, __ str(TMP, compiler::Address(THR, Thread::vm_tag_offset())); __ mov(SP, kCallLeafRuntimeCalleeSaveScratch2); __ mov(CSP, kCallLeafRuntimeCalleeSaveScratch1); - ASSERT((kAbiPreservedCpuRegs & (1 << THR)) != 0); - ASSERT((kAbiPreservedCpuRegs & (1 << PP)) != 0); + COMPILE_ASSERT(IsAbiPreservedRegister(THR)); + COMPILE_ASSERT(IsAbiPreservedRegister(PP)); } else { // Argument count is not checked here, but in the runtime entry for a more // informative error message. diff --git a/tests/ffi/regress_jump_to_frame_test.dart b/tests/ffi/regress_jump_to_frame_test.dart new file mode 100644 index 00000000000..a3d90545515 --- /dev/null +++ b/tests/ffi/regress_jump_to_frame_test.dart @@ -0,0 +1,64 @@ +// 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. + +// Check that JumpToFrame does not use LR clobbered by slow path of +// TransitionNativeToGenerated. +// VMOptions=--use-slow-path --enable-testing-pragmas + +import 'dart:ffi'; + +import 'package:ffi/ffi.dart'; +import 'package:expect/expect.dart'; + +typedef CallbackDartType = Object Function(); +typedef CallbackNativeType = Handle Function(); + +Object alwaysThrows() { + throw 'exception'; +} + +void alwaysCatches(CallbackDartType f) { + try { + propagateError(f()); + } catch (e) { + Expect.equals('exception', e); + return; + } + Expect.isTrue(false); +} + +void main() { + final ptr = Pointer.fromFunction(alwaysThrows); + final f = ptr.asFunction(); + alwaysCatches(f); +} + +typedef Dart_PropagateError_NativeType = Void Function(Handle); +typedef Dart_PropagateError_DartType = void Function(Object); + +final Dart_PropagateError_DartType propagateError = () { + final Pointer<_DartApi> dlapi = NativeApi.initializeApiDLData.cast(); + for (int i = 0; dlapi.ref.functions[i].name != nullptr; i++) { + final name = Utf8.fromUtf8(dlapi.ref.functions[i].name.cast()); + if (name == 'Dart_PropagateError') { + return dlapi.ref.functions[i].function + .cast>() + .asFunction(); + } + } + throw 'Not found'; +}(); + +class _DartEntry extends Struct { + external Pointer name; + external Pointer function; +} + +class _DartApi extends Struct { + @Int32() + external int major; + @Int32() + external int minor; + external Pointer<_DartEntry> functions; +} diff --git a/tests/ffi_2/regress_jump_to_frame_test.dart b/tests/ffi_2/regress_jump_to_frame_test.dart new file mode 100644 index 00000000000..9838443aa05 --- /dev/null +++ b/tests/ffi_2/regress_jump_to_frame_test.dart @@ -0,0 +1,64 @@ +// 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. + +// Check that JumpToFrame does not use LR clobbered by slow path of +// TransitionNativeToGenerated. +// VMOptions=--use-slow-path --enable-testing-pragmas + +import 'dart:ffi'; + +import 'package:ffi/ffi.dart'; +import 'package:expect/expect.dart'; + +typedef CallbackDartType = Object Function(); +typedef CallbackNativeType = Handle Function(); + +Object alwaysThrows() { + throw 'exception'; +} + +void alwaysCatches(CallbackDartType f) { + try { + propagateError(f()); + } catch (e) { + Expect.equals('exception', e); + return; + } + Expect.isTrue(false); +} + +void main() { + final ptr = Pointer.fromFunction(alwaysThrows); + final f = ptr.asFunction(); + alwaysCatches(f); +} + +typedef Dart_PropagateError_NativeType = Void Function(Handle); +typedef Dart_PropagateError_DartType = void Function(Object); + +final Dart_PropagateError_DartType propagateError = () { + final Pointer<_DartApi> dlapi = NativeApi.initializeApiDLData.cast(); + for (int i = 0; dlapi.ref.functions[i].name != nullptr; i++) { + final name = Utf8.fromUtf8(dlapi.ref.functions[i].name.cast()); + if (name == 'Dart_PropagateError') { + return dlapi.ref.functions[i].function + .cast>() + .asFunction(); + } + } + throw 'Not found'; +}(); + +class _DartEntry extends Struct { + Pointer name; + Pointer function; +} + +class _DartApi extends Struct { + @Int32() + int major; + @Int32() + int minor; + Pointer<_DartEntry> functions; +}