On Fri, 25 Aug 2023 10:59:45 GMT, Martin Doerr <mdo...@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
