On Wed, 10 May 2023 14:19:43 GMT, Martin Doerr <mdo...@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
