RFR: 8314949: linux PPC64 Big Endian: Implementation of Foreign Function & Memory API [v3]

[Jorn Vernee]

On Fri, 25 Aug 2023 10:59:45 GMT, Martin Doerr <mdoerr at openjdk.org> wrote:

>> I've found a way to solve the remaining FFI problem on linux PPC64 Big Endian. Large structs (>8 Bytes) which are passed in registers or on stack require shifting the Bytes in the last slot if the size is not a multiple of 8. This PR adds the required functionality to the Java code.
>> 
>> Please review and provide feedback. There may be better ways to implement it. I just found one which works and makes the tests pass:
>> 
>> Test summary
>> ==============================
>>    TEST                                              TOTAL  PASS  FAIL ERROR   
>>    jtreg:test/jdk/java/foreign                          88    88     0     0   
>> 
>> 
>> Note: This PR should be considered as preparation work for AIX which also uses ABIv1.
>
> Martin Doerr has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Remove unnecessary imports.

Sorry for the delay, I've been on vacation.

src/java.base/share/classes/jdk/internal/foreign/abi/Binding.java line 398:

> 396:                 bindings.add(Binding.cast(type, int.class));
> 397:                 type = int.class;
> 398:             }

Part of the casts are handled here with explicit cast bindings, but the widening from int -> long, and narrowing from long -> int are handled implicitly as part of the ShiftLeft implementation. I'd much prefer if all the type conversions are handled with explicit cast bindings. This would also semantically simplify the shift operator, since it would just handle the actual shifting.

src/java.base/share/classes/jdk/internal/foreign/abi/Binding.java line 695:

> 693:      *   Negative [shiftAmount] shifts right and converts to int if needed.
> 694:      */
> 695:     record ShiftLeft(int shiftAmount, Class<?> type) implements Binding {

Please split this into 2 binding operators: one for ShiftLeft and another for (arithmetic) ShiftRight, rather than mixing the 2 implementations into a single operator.

For the right shift you could then also use a positive shift amount, and avoid the double negation that's needed right now.

src/java.base/share/classes/jdk/internal/foreign/abi/Binding.java line 703:

> 701:                 if (last != ((type == long.class) ? long.class : int.class))
> 702:                     throw new IllegalArgumentException(
> 703:                         String.format("Invalid operand type: %s. integral type expected", last));

Why not use `SharedUtils.checkType` here?

Suggestion:

                SharedUtils.checkType(last, type);

src/java.base/share/classes/jdk/internal/foreign/abi/Binding.java line 711:

> 709:                 stack.push(type == long.class ? long.class : int.class);
> 710:             } else
> 711:                 throw new IllegalArgumentException("shiftAmount 0 not supported");

Please handle this validation in the static `shiftLeft` method. That's also where we do validation for other binding ops

src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java line 737:

> 735:                 cb.i2l();
> 736:                 typeStack.pop();
> 737:                 typeStack.push(long.class);

Please use `popType` and `pushType` instead of manipulating the type stack manually. The former also does some verification. This should happen along every control path. Even if the binding is 1-to-1 (like this one) and doesn't change the type of the value, this would validate that the input type is correct, and make sure that any bugs are caught early.

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

PR Review: https://git.openjdk.org/jdk/pull/15417#pullrequestreview-1595698959
PR Review Comment: https://git.openjdk.org/jdk/pull/15417#discussion_r1305681864
PR Review Comment: https://git.openjdk.org/jdk/pull/15417#discussion_r1314518216
PR Review Comment: https://git.openjdk.org/jdk/pull/15417#discussion_r1305642804
PR Review Comment: https://git.openjdk.org/jdk/pull/15417#discussion_r1305668315
PR Review Comment: https://git.openjdk.org/jdk/pull/15417#discussion_r1305650457