8275584: Incorrect stack spilling in CallArranger on MacOS/AArch64

Co-authored-by: Jorn Vernee <jvernee@openjdk.org>
Reviewed-by: jvernee

Author: nick-arm

src/hotspot/cpu/aarch64/foreignGlobals_aarch64.cpp

@@ -109,15 +109,16 @@ static void move_reg64(MacroAssembler* masm, int out_stk_bias,
       break;
     case StorageType::STACK:
       out_bias = out_stk_bias;
-    case StorageType::FRAME_DATA:
+    case StorageType::FRAME_DATA: {
+      Address dest(sp, to_reg.offset() + out_bias);
       switch (to_reg.stack_size()) {
-        // FIXME use correctly sized stores
-        case 8: case 4: case 2: case 1:
-          masm->str(from_reg, Address(sp, to_reg.offset() + out_stk_bias));
-        break;
+        case 8: masm->str (from_reg, dest); break;
+        case 4: masm->strw(from_reg, dest); break;
+        case 2: masm->strh(from_reg, dest); break;
+        case 1: masm->strb(from_reg, dest); break;
         default: ShouldNotReachHere();
       }
-      break;
+    } break;
     default: ShouldNotReachHere();
   }
 }
@@ -130,10 +131,10 @@ static void move_stack(MacroAssembler* masm, Register tmp_reg, int in_stk_bias,
     case StorageType::INTEGER:
       assert(to_reg.segment_mask() == REG64_MASK, "only moves to 64-bit registers supported");
       switch (from_reg.stack_size()) {
-        // FIXME use correctly sized loads
-        case 8: case 4: case 2: case 1:
-          masm->ldr(as_Register(to_reg), from_addr);
-        break;
+        case 8: masm->ldr (as_Register(to_reg), from_addr); break;
+        case 4: masm->ldrw(as_Register(to_reg), from_addr); break;
+        case 2: masm->ldrh(as_Register(to_reg), from_addr); break;
+        case 1: masm->ldrb(as_Register(to_reg), from_addr); break;
         default: ShouldNotReachHere();
       }
       break;
@@ -151,18 +152,23 @@ static void move_stack(MacroAssembler* masm, Register tmp_reg, int in_stk_bias,
       break;
     case StorageType::STACK:
       out_bias = out_stk_bias;
-    case StorageType::FRAME_DATA:
-      // We assume 8 bytes stack size when converting from VMReg (Java CC)
-      //assert(from_reg.stack_size() == to_reg.stack_size(), "must be same");
+    case StorageType::FRAME_DATA: {
       switch (from_reg.stack_size()) {
-        // FIXME use correctly sized loads & stores
-        case 8: case 4: case 2: case 1:
-          masm->ldr(tmp_reg, from_addr);
-          masm->str(tmp_reg, Address(sp, to_reg.offset() + out_bias));
-        break;
+        case 8: masm->ldr (tmp_reg, from_addr); break;
+        case 4: masm->ldrw(tmp_reg, from_addr); break;
+        case 2: masm->ldrh(tmp_reg, from_addr); break;
+        case 1: masm->ldrb(tmp_reg, from_addr); break;
         default: ShouldNotReachHere();
       }
-      break;
+      Address dest(sp, to_reg.offset() + out_bias);
+      switch (to_reg.stack_size()) {
+        case 8: masm->str (tmp_reg, dest); break;
+        case 4: masm->strw(tmp_reg, dest); break;
+        case 2: masm->strh(tmp_reg, dest); break;
+        case 1: masm->strb(tmp_reg, dest); break;
+        default: ShouldNotReachHere();
+      }
+    } break;
     default: ShouldNotReachHere();
   }
 }
@@ -174,17 +180,14 @@ static void move_v128(MacroAssembler* masm, int out_stk_bias,
       assert(to_reg.segment_mask() == V128_MASK, "only moves to v128 registers supported");
       masm->fmovd(as_FloatRegister(to_reg), from_reg);
       break;
-    case StorageType::STACK:
-      switch(to_reg.stack_size()) {
-        case 8:
-          masm->strd(from_reg, Address(sp, to_reg.offset() + out_stk_bias));
-        break;
-        case 4:
-          masm->strs(from_reg, Address(sp, to_reg.offset() + out_stk_bias));
-        break;
+    case StorageType::STACK: {
+      Address dest(sp, to_reg.offset() + out_stk_bias);
+      switch (to_reg.stack_size()) {
+        case 8: masm->strd(from_reg, dest); break;
+        case 4: masm->strs(from_reg, dest); break;
         default: ShouldNotReachHere();
       }
-      break;
+    } break;
     default: ShouldNotReachHere();
   }
 }

src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/CallArranger.java

@@ -1,6 +1,6 @@
 /*
  * Copyright (c) 2020, 2022, Oracle and/or its affiliates. All rights reserved.
- * Copyright (c) 2019, 2021, Arm Limited. All rights reserved.
+ * Copyright (c) 2019, 2022, Arm Limited. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -186,21 +186,13 @@ public StorageCalculator(boolean forArguments) {
             this.forArguments = forArguments;
         }
 
+        void alignStack(long alignment) {
+            stackOffset = Utils.alignUp(stackOffset, alignment);
+        }
+
         VMStorage stackAlloc(long size, long alignment) {
             assert forArguments : "no stack returns";
-            // Implementation limit: each arg must take up at least an 8 byte stack slot (on the Java side)
-            // There is currently no way to address stack offsets that are not multiples of 8 bytes
-            // The VM can only address multiple-of-4-bytes offsets, which is also not good enough for some ABIs
-            // see JDK-8283462 and related issues
-            long stackSlotAlignment = Math.max(alignment, STACK_SLOT_SIZE);
-            long alignedStackOffset = Utils.alignUp(stackOffset, stackSlotAlignment);
-            // macos-aarch64 ABI potentially requires addressing stack offsets that are not multiples of 8 bytes
-            // Reject such call types here, to prevent undefined behavior down the line
-            // Reject if the above stack-slot-aligned offset does not match the offset the ABI really wants
-            // Except for variadic arguments, which _are_ passed at 8-byte-aligned offsets
-            if (requiresSubSlotStackPacking() && alignedStackOffset != Utils.alignUp(stackOffset, alignment)
-                    && !forVarArgs) // varargs are given a pass on all aarch64 ABIs
-                throw new UnsupportedOperationException("Call type not supported on this platform");
+            long alignedStackOffset = Utils.alignUp(stackOffset, alignment);
 
             short encodedSize = (short) size;
             assert (encodedSize & 0xFFFF) == size;
@@ -212,7 +204,10 @@ VMStorage stackAlloc(long size, long alignment) {
         }
 
         VMStorage stackAlloc(MemoryLayout layout) {
-            return stackAlloc(layout.byteSize(), layout.byteAlignment());
+            long stackSlotAlignment = requiresSubSlotStackPacking() && !forVarArgs
+                    ? layout.byteAlignment()
+                    : Math.max(layout.byteAlignment(), STACK_SLOT_SIZE);
+            return stackAlloc(layout.byteSize(), stackSlotAlignment);
         }
 
         VMStorage[] regAlloc(int type, int count) {
@@ -245,6 +240,26 @@ VMStorage nextStorage(int type, MemoryLayout layout) {
             return storage[0];
         }
 
+        VMStorage[] nextStorageForHFA(GroupLayout group) {
+            final int nFields = group.memberLayouts().size();
+            VMStorage[] regs = regAlloc(StorageType.VECTOR, nFields);
+            if (regs == null && requiresSubSlotStackPacking() && !forVarArgs) {
+                // For the ABI variants that pack arguments spilled to the
+                // stack, HFA arguments are spilled as if their individual
+                // fields had been allocated separately rather than as if the
+                // struct had been spilled as a whole.
+
+                VMStorage[] slots = new VMStorage[nFields];
+                for (int i = 0; i < nFields; i++) {
+                    slots[i] = stackAlloc(group.memberLayouts().get(i));
+                }
+
+                return slots;
+            } else {
+                return regs;
+            }
+        }
+
         void adjustForVarArgs() {
             // This system passes all variadic parameters on the stack. Ensure
             // no further arguments are allocated to registers.
@@ -280,6 +295,12 @@ protected void spillStructUnbox(Binding.Builder bindings, MemoryLayout layout) {
                         .vmStore(storage, type);
                 offset += STACK_SLOT_SIZE;
             }
+
+            if (requiresSubSlotStackPacking()) {
+                // Pad to the next stack slot boundary instead of packing
+                // additional arguments into the unused space.
+                storageCalculator.alignStack(STACK_SLOT_SIZE);
+            }
         }
 
         protected void spillStructBox(Binding.Builder bindings, MemoryLayout layout) {
@@ -299,6 +320,12 @@ protected void spillStructBox(Binding.Builder bindings, MemoryLayout layout) {
                         .bufferStore(offset, type);
                 offset += STACK_SLOT_SIZE;
             }
+
+            if (requiresSubSlotStackPacking()) {
+                // Pad to the next stack slot boundary instead of packing
+                // additional arguments into the unused space.
+                storageCalculator.alignStack(STACK_SLOT_SIZE);
+            }
         }
 
         abstract List<Binding> getBindings(Class<?> carrier, MemoryLayout layout);
@@ -360,8 +387,7 @@ List<Binding> getBindings(Class<?> carrier, MemoryLayout layout) {
                 case STRUCT_HFA: {
                     assert carrier == MemorySegment.class;
                     GroupLayout group = (GroupLayout)layout;
-                    VMStorage[] regs = storageCalculator.regAlloc(
-                        StorageType.VECTOR, group.memberLayouts().size());
+                    VMStorage[] regs = storageCalculator.nextStorageForHFA(group);
                     if (regs != null) {
                         long offset = 0;
                         for (int i = 0; i < group.memberLayouts().size(); i++) {
@@ -458,8 +484,7 @@ List<Binding> getBindings(Class<?> carrier, MemoryLayout layout) {
                     assert carrier == MemorySegment.class;
                     bindings.allocate(layout);
                     GroupLayout group = (GroupLayout) layout;
-                    VMStorage[] regs = storageCalculator.regAlloc(
-                            StorageType.VECTOR, group.memberLayouts().size());
+                    VMStorage[] regs = storageCalculator.nextStorageForHFA(group);
                     if (regs != null) {
                         long offset = 0;
                         for (int i = 0; i < group.memberLayouts().size(); i++) {

test/jdk/ProblemList.txt

@@ -479,13 +479,6 @@ java/beans/XMLEncoder/Test6570354.java 8015593 macosx-all
 
 ############################################################################
 
-# jdk_foreign
-
-java/foreign/TestUpcallStack.java 8275584 macosx-aarch64
-java/foreign/TestDowncallStack.java 8275584 macosx-aarch64
-
-############################################################################
-
 # jdk_lang
 
 java/lang/ProcessHandle/InfoTest.java                           8211847 aix-ppc64

test/jdk/java/foreign/callarranger/CallArrangerTestBase.java

@@ -32,16 +32,17 @@
 public class CallArrangerTestBase {
 
     public static void checkArgumentBindings(CallingSequence callingSequence, Binding[][] argumentBindings) {
-        assertEquals(callingSequence.argumentBindingsCount(), argumentBindings.length);
+        assertEquals(callingSequence.argumentBindingsCount(), argumentBindings.length,
+          callingSequence.asString() + " != " + Arrays.deepToString(argumentBindings));
 
         for (int i = 0; i < callingSequence.argumentBindingsCount(); i++) {
             List<Binding> actual = callingSequence.argumentBindings(i);
             Binding[] expected = argumentBindings[i];
-            assertEquals(actual, Arrays.asList(expected));
+            assertEquals(actual, Arrays.asList(expected), "bindings at: " + i + ": " + actual + " != " + Arrays.toString(expected));
         }
     }
 
     public static void checkReturnBindings(CallingSequence callingSequence, Binding[] returnBindings) {
-        assertEquals(callingSequence.returnBindings(), Arrays.asList(returnBindings));
+        assertEquals(callingSequence.returnBindings(), Arrays.asList(returnBindings), callingSequence.returnBindings() + " != " + Arrays.toString(returnBindings));
     }
 }