[vm, ffi] Fix some confused register usage during callbacks.

EmitNativeMove is also used during NativeParamterInstr, not just FfiCallInstr, so the trick with WithIntermediateMarshalling doesn't work. Instead, move arguments to their final register in the prologue of FfiCallInstr and remove conflicting uses within FfiCallInstr.

TEST=ffi_2/function_callbacks_test
Bug: #48164
Change-Id: I652ab714dbfcb092dc5a23e4596f77136d91ffde
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/240324
Reviewed-by: Daco Harkes <dacoharkes@google.com>
Commit-Queue: Ryan Macnak <rmacnak@google.com>

Author: rmacnak-google

runtime/vm/compiler/backend/flow_graph_compiler.cc

@@ -3507,25 +3507,23 @@ void FlowGraphCompiler::EmitNativeMove(
     return;
   }
 
-#if !defined(TARGET_ARCH_RISCV32) && !defined(TARGET_ARCH_RISCV64)
   // Split moves from stack to stack, none of the architectures provides
-  // memory to memory move instructions. But RISC-V needs to avoid TMP.
+  // memory to memory move instructions.
   if (source.IsStack() && destination.IsStack()) {
-    Register scratch = TMP;
-    if (TMP == kNoRegister) {
-      scratch = temp->AllocateTemporary();
-    }
+    Register scratch = temp->AllocateTemporary();
+    ASSERT(scratch != kNoRegister);
+#if defined(TARGET_ARCH_RISCV32) || defined(TARGET_ARCH_RISCV64)
+    ASSERT(scratch != TMP);   // TMP is an argument register.
+    ASSERT(scratch != TMP2);  // TMP2 is an argument register.
+#endif
     const auto& intermediate =
         *new (zone_) compiler::ffi::NativeRegistersLocation(
             zone_, dst_payload_type, dst_container_type, scratch);
     EmitNativeMove(intermediate, source, temp);
     EmitNativeMove(destination, intermediate, temp);
-    if (TMP == kNoRegister) {
-      temp->ReleaseTemporary();
-    }
+    temp->ReleaseTemporary();
     return;
   }
-#endif
 
   const bool sign_or_zero_extend = dst_container_size > src_container_size;
 

runtime/vm/compiler/backend/flow_graph_compiler_riscv.cc

@@ -868,15 +868,6 @@ static compiler::OperandSize BytesToOperandSize(intptr_t bytes) {
   }
 }
 
-// See FfiCallInstr::MakeLocationSummary.
-static Register WithIntermediateMarshalling(Register r) {
-  if (r == A2) return T2;  // A2=CODE_REG
-  if (r == A3) return T3;  // A3=TMP
-  if (r == A4) return T4;  // A4=TMP2
-  if (r == A5) return T5;  // A5=PP
-  return r;
-}
-
 void FlowGraphCompiler::EmitNativeMoveArchitecture(
     const compiler::ffi::NativeLocation& destination,
     const compiler::ffi::NativeLocation& source) {
@@ -894,12 +885,12 @@ void FlowGraphCompiler::EmitNativeMoveArchitecture(
   if (source.IsRegisters()) {
     const auto& src = source.AsRegisters();
     ASSERT(src.num_regs() == 1);
-    const auto src_reg = WithIntermediateMarshalling(src.reg_at(0));
+    const auto src_reg = src.reg_at(0);
 
     if (destination.IsRegisters()) {
       const auto& dst = destination.AsRegisters();
       ASSERT(dst.num_regs() == 1);
-      const auto dst_reg = WithIntermediateMarshalling(dst.reg_at(0));
+      const auto dst_reg = dst.reg_at(0);
       if (!sign_or_zero_extend) {
 #if XLEN == 32
         __ MoveRegister(dst_reg, src_reg);
@@ -995,7 +986,7 @@ void FlowGraphCompiler::EmitNativeMoveArchitecture(
     if (destination.IsRegisters()) {
       const auto& dst = destination.AsRegisters();
       ASSERT(dst.num_regs() == 1);
-      const auto dst_reg = WithIntermediateMarshalling(dst.reg_at(0));
+      const auto dst_reg = dst.reg_at(0);
       ASSERT(!sign_or_zero_extend);
       __ LoadFromOffset(dst_reg, src.base_register(), src.offset_in_bytes(),
                         BytesToOperandSize(dst_size));
@@ -1015,14 +1006,8 @@ void FlowGraphCompiler::EmitNativeMoveArchitecture(
         default:
           UNIMPLEMENTED();
       }
-    } else if (destination.IsStack()) {
-      const auto& dst = destination.AsStack();
-      // TMP=A3, here not remapped to T3.
-      __ LoadFromOffset(TMP, src.base_register(), src.offset_in_bytes(),
-                        BytesToOperandSize(src_size));
-      __ StoreToOffset(TMP, dst.base_register(), dst.offset_in_bytes(),
-                       BytesToOperandSize(dst_size));
     } else {
+      ASSERT(destination.IsStack());
       UNREACHABLE();
     }
   }

runtime/vm/compiler/backend/il.cc

@@ -6532,7 +6532,8 @@ LocationSummary* FfiCallInstr::MakeLocationSummaryInternal(
 
 void FfiCallInstr::EmitParamMoves(FlowGraphCompiler* compiler,
                                   const Register saved_fp,
-                                  const Register temp) {
+                                  const Register temp0,
+                                  const Register temp1) {
   if (compiler::Assembler::EmittingComments()) {
     __ Comment("EmitParamMoves");
   }
@@ -6546,7 +6547,7 @@ void FfiCallInstr::EmitParamMoves(FlowGraphCompiler* compiler,
     const auto& pointer_register =
         pointer_location.IsRegisters()
             ? pointer_location.AsRegisters().reg_at(0)
-            : temp;
+            : temp0;
     __ MoveRegister(pointer_register, SPREG);
     __ AddImmediate(pointer_register, marshaller_.PassByPointerStackOffset(
                                           compiler::ffi::kResultIndex));
@@ -6586,7 +6587,7 @@ void FfiCallInstr::EmitParamMoves(FlowGraphCompiler* compiler,
               ? arg_target.AsPointerToMemory().pointer_location()
               : /*arg_target.IsStack()*/ arg_target.Split(zone_, num_defs, i);
 
-      ConstantTemporaryAllocator temp_alloc(temp);
+      ConstantTemporaryAllocator temp_alloc(temp0);
       if (origin.IsConstant()) {
         // Can't occur because we currently don't inline FFI trampolines (see
         // http://dartbug.com/45055), which means all incomming arguments
@@ -6610,11 +6611,11 @@ void FfiCallInstr::EmitParamMoves(FlowGraphCompiler* compiler,
       // TypedData/Pointer data pointed to in temp.
       const auto& dst = compiler::ffi::NativeRegistersLocation(
           zone_, pointer_loc.payload_type(), pointer_loc.container_type(),
-          temp);
+          temp0);
       compiler->EmitNativeMove(dst, pointer_loc, &temp_alloc);
-      __ LoadField(temp,
+      __ LoadField(temp0,
                    compiler::FieldAddress(
-                       temp, compiler::target::PointerBase::data_offset()));
+                       temp0, compiler::target::PointerBase::data_offset()));
 
       // Copy chuncks.
       const intptr_t sp_offset =
@@ -6624,16 +6625,16 @@ void FfiCallInstr::EmitParamMoves(FlowGraphCompiler* compiler,
       // space on the stack.
       for (intptr_t i = 0; i < arg_target.payload_type().SizeInBytes();
            i += compiler::target::kWordSize) {
-        __ LoadMemoryValue(TMP, temp, i);
-        __ StoreMemoryValue(TMP, SPREG, i + sp_offset);
+        __ LoadMemoryValue(temp1, temp0, i);
+        __ StoreMemoryValue(temp1, SPREG, i + sp_offset);
       }
 
       // Store the stack address in the argument location.
-      __ MoveRegister(temp, SPREG);
-      __ AddImmediate(temp, sp_offset);
+      __ MoveRegister(temp0, SPREG);
+      __ AddImmediate(temp0, sp_offset);
       const auto& src = compiler::ffi::NativeRegistersLocation(
           zone_, pointer_loc.payload_type(), pointer_loc.container_type(),
-          temp);
+          temp0);
       compiler->EmitNativeMove(pointer_loc, src, &temp_alloc);
     }
   }

runtime/vm/compiler/backend/il.h

@@ -5307,18 +5307,18 @@ class FfiCallInstr : public Definition {
                                                bool is_optimizing,
                                                const RegList temps) const;
 
-  // Clobbers both given registers.
+  // Clobbers the first two given registers.
   // `saved_fp` is used as the frame base to rebase off of.
+  // `temp1` is only used in case of PointerToMemoryLocation.
   void EmitParamMoves(FlowGraphCompiler* compiler,
                       const Register saved_fp,
-                      const Register temp);
+                      const Register temp0,
+                      const Register temp1);
   // Clobbers both given temp registers.
   void EmitReturnMoves(FlowGraphCompiler* compiler,
                        const Register temp0,
                        const Register temp1);
 
-  void EmitCall(FlowGraphCompiler* compiler, Register target);
-
   Zone* const zone_;
   const compiler::ffi::CallMarshaller& marshaller_;
 

runtime/vm/compiler/backend/il_arm.cc

@@ -1446,7 +1446,7 @@ void FfiCallInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
   // Reserve space for the arguments that go on the stack (if any), then align.
   __ ReserveAlignedFrameSpace(marshaller_.RequiredStackSpaceInBytes());
 
-  EmitParamMoves(compiler, is_leaf_ ? FPREG : saved_fp_or_sp, temp1);
+  EmitParamMoves(compiler, is_leaf_ ? FPREG : saved_fp_or_sp, temp1, temp2);
 
   if (compiler::Assembler::EmittingComments()) {
     __ Comment(is_leaf_ ? "Leaf Call" : "Call");

runtime/vm/compiler/backend/il_arm64.cc

@@ -1270,7 +1270,7 @@ void FfiCallInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
   // Reserve space for the arguments that go on the stack (if any), then align.
   __ ReserveAlignedFrameSpace(marshaller_.RequiredStackSpaceInBytes());
 
-  EmitParamMoves(compiler, is_leaf_ ? FPREG : saved_fp_or_sp, temp1);
+  EmitParamMoves(compiler, is_leaf_ ? FPREG : saved_fp_or_sp, temp1, temp2);
 
   if (compiler::Assembler::EmittingComments()) {
     __ Comment(is_leaf_ ? "Leaf Call" : "Call");

runtime/vm/compiler/backend/il_ia32.cc

@@ -1044,7 +1044,9 @@ void FfiCallInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
   // Reserve space for the arguments that go on the stack (if any), then align.
   __ ReserveAlignedFrameSpace(stack_required);
 
-  EmitParamMoves(compiler, is_leaf_ ? FPREG : saved_fp_or_sp, temp);
+  // No second temp: PointerToMemoryLocation is not used for arguments in ia32.
+  EmitParamMoves(compiler, is_leaf_ ? FPREG : saved_fp_or_sp, temp,
+                 kNoRegister);
 
   if (is_leaf_) {
     // We store the pre-align SP at a fixed offset from the final SP.

runtime/vm/compiler/backend/il_riscv.cc

@@ -1382,18 +1382,17 @@ LocationSummary* FfiCallInstr::MakeLocationSummary(Zone* zone,
       zone, is_optimizing,
       (R(CallingConventions::kSecondNonArgumentRegister) |
        R(CallingConventions::kFfiAnyNonAbiRegister) | R(CALLEE_SAVED_TEMP2)));
-  // A3/A4/A5 are blocked during Dart register allocation because they are
+
+  // A3/A4/A5 are unavailable in normal register allocation because they are
   // assigned to TMP/TMP2/PP. This assignment is important for reducing code
-  // size. To work around this for FFI calls, the FFI argument definitions are
-  // allocated to other registers and moved to the correct register at the last
-  // moment (so there are no conflicting uses of TMP/TMP2/PP).
-  // FfiCallInstr itself sometimes also clobbers A2/CODE_REG.
-  // See also FfiCallInstr::EmitCall.
+  // size. We can't just override the normal blockage of these registers because
+  // they may be used by other instructions between the argument's definition
+  // and its use in FfiCallInstr.
+  // Note that A3/A4/A5 might be not be the 3rd/4th/5th input because of mixed
+  // integer and floating-point arguments.
   for (intptr_t i = 0; i < summary->input_count(); i++) {
     if (!summary->in(i).IsRegister()) continue;
-    if (summary->in(i).reg() == A2) {
-      summary->set_in(i, Location::RegisterLocation(T2));
-    } else if (summary->in(i).reg() == A3) {
+    if (summary->in(i).reg() == A3) {
       summary->set_in(i, Location::RegisterLocation(T3));
     } else if (summary->in(i).reg() == A4) {
       summary->set_in(i, Location::RegisterLocation(T4));
@@ -1407,6 +1406,20 @@ LocationSummary* FfiCallInstr::MakeLocationSummary(Zone* zone,
 #undef R
 
 void FfiCallInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
+  // Beware! Do not use CODE_REG/TMP/TMP2/PP within FfiCallInstr as they are
+  // assigned to A2/A3/A4/A5, which may be in use as argument registers.
+  __ set_constant_pool_allowed(false);
+  for (intptr_t i = 0; i < locs()->input_count(); i++) {
+    if (!locs()->in(i).IsRegister()) continue;
+    if (locs()->in(i).reg() == T3) {
+      __ mv(A3, T3);
+    } else if (locs()->in(i).reg() == T4) {
+      __ mv(A4, T4);
+    } else if (locs()->in(i).reg() == T5) {
+      __ mv(A5, T5);
+    }
+  }
+
   const Register target = locs()->in(TargetAddressIndex()).reg();
 
   // The temps are indexed according to their register number.
@@ -1431,17 +1444,27 @@ void FfiCallInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
   __ mv(saved_fp_or_sp, is_leaf_ ? SPREG : FPREG);
 
   if (!is_leaf_) {
-    // We need to create a dummy "exit frame". It will share the same pool
-    // pointer but have a null code object.
-    __ LoadObject(CODE_REG, Object::null_object());
-    __ set_constant_pool_allowed(false);
-    __ EnterDartFrame(0, PP);
+    // We need to create a dummy "exit frame".
+    // This is EnterDartFrame without accessing A2=CODE_REG or A5=PP.
+    if (FLAG_precompiled_mode) {
+      __ subi(SP, SP, 2 * compiler::target::kWordSize);
+      __ sx(RA, compiler::Address(SP, 1 * compiler::target::kWordSize));
+      __ sx(FP, compiler::Address(SP, 0 * compiler::target::kWordSize));
+      __ addi(FP, SP, 2 * compiler::target::kWordSize);
+    } else {
+      __ subi(SP, SP, 4 * compiler::target::kWordSize);
+      __ sx(RA, compiler::Address(SP, 3 * compiler::target::kWordSize));
+      __ sx(FP, compiler::Address(SP, 2 * compiler::target::kWordSize));
+      __ sx(NULL_REG, compiler::Address(SP, 1 * compiler::target::kWordSize));
+      __ sx(NULL_REG, compiler::Address(SP, 0 * compiler::target::kWordSize));
+      __ addi(FP, SP, 4 * compiler::target::kWordSize);
+    }
   }
 
   // Reserve space for the arguments that go on the stack (if any), then align.
   __ ReserveAlignedFrameSpace(marshaller_.RequiredStackSpaceInBytes());
 
-  EmitParamMoves(compiler, is_leaf_ ? FPREG : saved_fp_or_sp, temp1);
+  EmitParamMoves(compiler, is_leaf_ ? FPREG : saved_fp_or_sp, temp1, temp2);
 
   if (compiler::Assembler::EmittingComments()) {
     __ Comment(is_leaf_ ? "Leaf Call" : "Call");
@@ -1456,10 +1479,7 @@ void FfiCallInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
     __ StoreToOffset(target, THR, compiler::target::Thread::vm_tag_offset());
 #endif
 
-    EmitCall(compiler, target);
-
-    ASSERT(!IsCalleeSavedRegister(PP));
-    __ RestorePoolPointer();
+    __ jalr(target);
 
 #if !defined(PRODUCT)
     __ LoadImmediate(temp1, compiler::target::Thread::vm_tag_dart_id());
@@ -1484,7 +1504,7 @@ void FfiCallInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
       __ TransitionGeneratedToNative(target, FPREG, temp1,
                                      /*enter_safepoint=*/true);
 
-      EmitCall(compiler, target);
+      __ jalr(target);
 
       // Update information in the thread object and leave the safepoint.
       __ TransitionNativeToGenerated(temp1, /*leave_safepoint=*/true);
@@ -1500,7 +1520,7 @@ void FfiCallInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
 
       // Calls T0 and clobbers R19 (along with volatile registers).
       ASSERT(target == T0);
-      EmitCall(compiler, temp1);
+      __ jalr(temp1);
     }
 
     // Refresh pinned registers values (inc. write barrier mask and null
@@ -1514,27 +1534,18 @@ void FfiCallInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
     // Restore the pre-aligned SP.
     __ mv(SPREG, saved_fp_or_sp);
   } else {
-    // Although PP is a callee-saved register, it may have been moved by the GC.
-    __ LeaveDartFrame(compiler::kRestoreCallerPP);
+    __ LeaveDartFrame();
 
     // Restore the global object pool after returning from runtime (old space is
     // moving, so the GOP could have been relocated).
     if (FLAG_precompiled_mode) {
       __ SetupGlobalPoolAndDispatchTable();
     }
-
-    __ set_constant_pool_allowed(true);
   }
-}
 
-void FfiCallInstr::EmitCall(FlowGraphCompiler* compiler, Register target) {
-  // Marshall certain argument registers at the last possible moment.
-  // See FfiCallInstr::MakeLocationSummary for the details.
-  if (InputCount() > 2) __ mv(A2, T2);  // A2=CODE_REG
-  if (InputCount() > 3) __ mv(A3, T3);  // A3=TMP
-  if (InputCount() > 4) __ mv(A4, T4);  // A4=TMP2
-  if (InputCount() > 5) __ mv(A5, T5);  // A5=PP
-  __ jalr(target);
+  // PP is a volatile register, so it must be restored even for leaf FFI calls.
+  __ RestorePoolPointer();
+  __ set_constant_pool_allowed(true);
 }
 
 // Keep in sync with NativeEntryInstr::EmitNativeCode.

runtime/vm/compiler/backend/il_x64.cc

@@ -1226,9 +1226,9 @@ void FfiCallInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
   __ ReserveAlignedFrameSpace(marshaller_.RequiredStackSpaceInBytes());
 
   if (is_leaf_) {
-    EmitParamMoves(compiler, FPREG, saved_fp);
+    EmitParamMoves(compiler, FPREG, saved_fp, TMP);
   } else {
-    EmitParamMoves(compiler, saved_fp, saved_sp);
+    EmitParamMoves(compiler, saved_fp, saved_sp, TMP);
   }
 
   if (compiler::Assembler::EmittingComments()) {

runtime/vm/instructions_riscv.cc

@@ -227,7 +227,9 @@ bool DecodeLoadObjectFromPoolOrThread(uword pc, const Code& code, Object* obj) {
 #endif
       if (instr.rs1p() == PP) {
         // PP is untagged on RISC-V.
-        ASSERT(Utils::IsAligned(offset, kWordSize));
+        if (!Utils::IsAligned(offset, kWordSize)) {
+          return false;  // Being used as argument register A5.
+        }
         intptr_t index = ObjectPool::IndexFromOffset(offset - kHeapObjectTag);
         const ObjectPool& pool = ObjectPool::Handle(code.GetObjectPool());
         if (!pool.IsNull() && (index < pool.Length()) &&
@@ -249,7 +251,9 @@ bool DecodeLoadObjectFromPoolOrThread(uword pc, const Code& code, Object* obj) {
       intptr_t offset = instr.itype_imm();
       if (instr.rs1() == PP) {
         // PP is untagged on RISC-V.
-        ASSERT(Utils::IsAligned(offset, kWordSize));
+        if (!Utils::IsAligned(offset, kWordSize)) {
+          return false;  // Being used as argument register A5.
+        }
         intptr_t index = ObjectPool::IndexFromOffset(offset - kHeapObjectTag);
         const ObjectPool& pool = ObjectPool::Handle(code.GetObjectPool());
         if (!pool.IsNull() && (index < pool.Length()) &&