RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v30]

[Richard Reingruber]

On Wed, 10 May 2023 14:19:43 GMT, Martin Doerr <mdoerr at openjdk.org> wrote:

>> Implementation of "Foreign Function & Memory API" for linux on Power (Little Endian) according to "Power Architecture 64-Bit ELF V2 ABI Specification".
>> 
>> This PR does not include code for VaList support because it's supposed to get removed by [JDK-8299736](https://bugs.openjdk.org/browse/JDK-8299736). I've kept the related tests disabled for this platform and throw an exception instead. Note that the ABI doesn't precisely specify variable argument lists. Instead, it refers to `<stdarg.h>` (2.2.4 Variable Argument Lists).
>> 
>> Big Endian support is implemented to some extend, but not complete. E.g. structs with size not divisible by 8 are not passed correctly (see `useABIv2` in CallArranger.java). Big Endian is excluded by selecting `ARCH.equals("ppc64le")` (CABI.java) only.
>> 
>> There's another limitation: This PR only accepts structures with size divisible by 4. (An `IllegalArgumentException` gets thrown otherwise.) I think arbitrary sizes are not usable on other platforms, either, because `SharedUtils.primitiveCarrierForSize` only accepts powers of 2. Update: Resolved after merging of [JDK-8303017](https://bugs.openjdk.org/browse/JDK-8303017)
>> 
>> The ABI has some tricky corner cases related to HFA (Homogeneous Float Aggregate). The same argument may need to get passed in both, a FP reg and a GP reg or stack slot (see "no partial DW rule"). This cases are not covered by the existing tests.
>> 
>> I had to make changes to shared code and code for other platforms:
>> 1. Pass type information when creating `VMStorage` objects from `VMReg`. This is needed for the following reasons:
>> - PPC64 ABI requires integer types to get extended to 64 bit (also see CCallingConventionRequiresIntsAsLongs in existing hotspot code). We need to know the type or at least the bit width for that.
>> - Floating point load / store instructions need the correct width to select between the correct IEEE 754 formats. The register representation in single FP registers is always IEEE 754 double precision on PPC64.
>> - Big Endian also needs usage of the precise size. Storing 8 Bytes and loading 4 Bytes yields different values than on Little Endian!
>> 2. It happens that a `NativeMemorySegmentImpl` is used as a raw pointer (with byteSize() == 0) while running TestUpcallScope. Hence, existing size checks don't work (see MemorySegment.java). As a workaround, I'm just skipping the check in this particular case. Please check if this makes sense or if there's a better fix (possibly as separat...
>
> Martin Doerr has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add NONZERO check for downcall_stub_address_offset_in_bytes().

Hi Martin,
I've made a pass over the Java part (except HFA). I found the specs hard to understand but most specs are like this.
I'll finish the rest beginning of next week.
Cheers, Richard.

src/java.base/share/classes/jdk/internal/foreign/abi/ppc64/CallArranger.java line 28:

> 26: package jdk.internal.foreign.abi.ppc64;
> 27: 
> 28: import java.lang.foreign.AddressLayout;

Imports are not grouped and ordered alphabetically.
(Very much as the aarch64 version)

src/java.base/share/classes/jdk/internal/foreign/abi/ppc64/CallArranger.java line 73:

> 71:     public static final int MAX_FLOAT_REGISTER_ARGUMENTS = 13;
> 72: 
> 73:     // This is derived from the 64-Bit ELF V2 ABI spec, restricted to what's

The comment says ABI V2 but the code seems to handle V1 too.

src/java.base/share/classes/jdk/internal/foreign/abi/ppc64/CallArranger.java line 158:

> 156:     class StorageCalculator {
> 157:         private final boolean forArguments;
> 158:         private boolean forVarArgs = false;

Seems to be not used.

src/java.base/share/classes/jdk/internal/foreign/abi/ppc64/CallArranger.java line 221:

> 219:             // !useABIv2() && layout.byteSize() > 8 && layout.byteSize() % 8 != 0
> 220: 
> 221:             // Allocate individual fields as gp slots (regs and stack).

You explained to me, it's not individual (struct) fields that are handled here. Looks like registers and 8 byte stack slots are allocated to completely cover the struct. Would be good if you could change the comment and names in the code to better reflect this.

src/java.base/share/classes/jdk/internal/foreign/abi/ppc64/linux/LinuxPPC64leLinker.java line 41:

> 39: 
> 40:     public static LinuxPPC64leLinker getInstance() {
> 41:         if (instance == null) {

Other platforms optimized this to return a constant (probably after you forked off the port).

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

PR Review: https://git.openjdk.org/jdk/pull/12708#pullrequestreview-1428763014
PR Review Comment: https://git.openjdk.org/jdk/pull/12708#discussion_r1195280060
PR Review Comment: https://git.openjdk.org/jdk/pull/12708#discussion_r1195344452
PR Review Comment: https://git.openjdk.org/jdk/pull/12708#discussion_r1195363380
PR Review Comment: https://git.openjdk.org/jdk/pull/12708#discussion_r1198915617
PR Review Comment: https://git.openjdk.org/jdk/pull/12708#discussion_r1199011136