[foreign-memaccess+abi] RFR: 8275332: Variadic functions don't work on Linux/AArch64

[Jorn Vernee]

On Wed, 20 Oct 2021 13:09:43 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> Well I did think about that, but at the moment CallArranger is just a bag of static methods: we'd need to refactor it quite a bit to fit it into that pattern. 
>> 
>> Perhaps (an AArch64 extension of) AbiDescriptor is the right place to store this? That already stores information like stack alignment, shadow space, etc. We'd then have a `MACOS_C` and `LINUX_C` instead of the single `C` we have now, and then pass that into `getBindings()`, etc.
>> 
>>> If I'm right this is the only place where something different needs to happen for the two ABIs?
>> 
>> Actually I think there's one more case we don't handle correctly at the moment but isn't covered by the existing tests:
>> 
>> 
>> void f(int, int, int, int, int, int, int, int, char x, short y);
>> 
>> 
>> In the standard ABI `x` and `y` get spilled to the stack as eight byte quantities (as if they were assigned to a register and then spilled) but in the Mac ABI they are spilled to the stack with their original size and natural alignment. So we need another flag somewhere to handle that difference. I'll make a JBS ticket for it.
>
> Maybe I'm wrong, but it doesn't seem like the refactoring would be that big. After all, the public API of CallArranger is the two static methods arrangeUpcall/Downcall. So, from a client perspective that is the only change. CallArranger could provide two already-made constants, which will give you a call arranger for linux or mac, so that clients won't need to instantiate a new call arranger all the time. This should keep the number of changes in clients under control?

> in the Mac ABI they are spilled to the stack with their original size and natural alignment

Argh, this is exactly the thing I was afraid of some ABIs might do. I was thinking about our shuffling code and noticed that it only supports shuffling to stack slots, which are 64-bit of size (at least on the Java side). Supporting this correctly would require quite a bit of tinkering I think, and probably adding support for byte-addressed stack offsets, rather than stack-slot-addressed stack offsets. The problem is that the current HotSpot code thinks in 32-bit stack slots as well, and it is not possible to represent a sub-32-bit offset as a VMReg.

So, in short this would require re-writing the shuffling code. I think it would be better to wait for that until after  the buffered invocation strategy is removed though, so it doesn't have to be implemented twice. The only big blocker I see for that currently is implementing optimized upcall stubs on AArch64 [JDK-8275646](https://bugs.openjdk.java.net/browse/JDK-8275646). There's also [JDK-8255903](https://bugs.openjdk.java.net/browse/JDK-8255903), [JDK-8275647](https://bugs.openjdk.java.net/browse/JDK-8275647), and [JDK-8255902](https://bugs.openjdk.java.net/browse/JDK-8255902), but for those last three I already have patches for 2 of them, and the last one (JDK-8275647) doesn't seem like that much work based on what was needed for downcalls.

---

I was thinking we need to replace VMReg with our own representation eventually any ways, since VMReg can not be used to distinguish between vector registers of different sizes, or e.g. between RAX, EAX, AX, AL, and AH. I think something more fine-grained is needed.

My current idea was to have a class like this:


class VMStorage {
  char _type;
  char _reserved = 0;
  short _segment_mask;
  int _index;
}


the `_type` and `_index` fields are just like the current `type` and `index` components in VMStorage (but for the stack, the `index` would be a byte-offset, not a (64-bit/32-bit) stack slot offset). But, the `_segment_mask` field can be used to distinguish between different overlapping registers. For instance, RAX would be `_type = 0, _segment_mask = 0b0000_0000_0000_1111, _index = 0`. while EAX would be `_type = 0, _segment_mask = 0b0000_0000_0000_0111, _index = 0`, i.e. same type and index, just a different segment mask. (filed [JDK-8275644](https://bugs.openjdk.java.net/browse/JDK-8275644))

-------------

PR: https://git.openjdk.java.net/panama-foreign/pull/601