On Wed, 9 Nov 2022 18:16:59 GMT, Jorn Vernee <jver...@openjdk.org> wrote:
>> Pull in linker implementation changes, that include non-trivial changes to >> VM code, from the panama-foreign repo into the main JDK. >> >> This is split off from the main JEP integration to make reviewing easier. >> >> This includes the following patches: >> >> 1. https://github.com/openjdk/panama-foreign/pull/698 >> 2. https://github.com/openjdk/panama-foreign/pull/699 >> 3. (part of) https://github.com/openjdk/panama-foreign/pull/731 >> 4. https://github.com/openjdk/panama-foreign/pull/740 >> 5. https://github.com/openjdk/panama-foreign/pull/746 >> 6. https://github.com/openjdk/panama-foreign/pull/742 >> 7. https://github.com/openjdk/panama-foreign/pull/743 >> >> Probably the biggest change to the code comes from replacing `VMReg` - which >> can not represent offsets into the stack that are not a multiple of the VM's >> stack slot size (32-bits) - with the new `VMStorage` class, which can >> describe byte offsets into the stack, as well as having a register mask to >> indicate only certain register segments. >> >> The only part of 3. that is in this PR is the part that turns the >> `VMStorage` class in Java into a record. >> >> Please refer to the PR of each individual patch for a more detailed >> description. > > Jorn Vernee has updated the pull request incrementally with two additional > commits since the last revision: > > - Work around x86 failures > - Review comments Java changes look good - added few nits src/java.base/share/classes/java/lang/foreign/Linker.java line 319: > 317: * {@return A linker option used to save portions of the > execution state immediately after > 318: * calling a foreign function associated with a > downcall method handle, > 319: * before it can be overwritten by the runtime, or read > through conventional means} Suggestion: * before it can be overwritten by the Java runtime, or read through conventional means} src/java.base/share/classes/java/lang/foreign/Linker.java line 340: > 338: * before it can be overwritten by the runtime, or read through > conventional means. > 339: * <p> > 340: * State is captured by a downcall method handle on invocation, > by writing it Suggestion: * Execution state is captured by a downcall method handle on invocation, by writing it src/java.base/share/classes/jdk/internal/foreign/abi/CallingSequence.java line 188: > 186: } > 187: > 188: public int capturedStateMask() { Isn't this a final static during execution? src/java.base/share/classes/jdk/internal/foreign/abi/NativeEntryPoint.java line 78: > 76: private static void checkType(MethodType methodType, boolean > needsReturnBuffer, int savedValueMask) { > 77: if (methodType.parameterType(0) != long.class) { > 78: throw new IllegalArgumentException("Address expected as first > param: " + methodType); Is throwing IAE correct here? E.g. can the user do anything about it, or does the exception describe more of an internal error? (In that case AssertionError might be better?) src/java.base/share/classes/jdk/internal/foreign/abi/NativeEntryPoint.java line 83: > 81: if ((needsReturnBuffer && methodType.parameterType(checkIdx++) != > long.class) > 82: || (savedValueMask != 0 && methodType.parameterType(checkIdx) > != long.class)) { > 83: throw new IllegalArgumentException("return buffer and/or > preserved value address expected: " + methodType); Same here src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/CallArranger.java line 3: > 1: /* > 2: * Copyright (c) 2020, 2022, Oracle and/or its affiliates. All rights > reserved. > 3: * Copyright (c) 2019, 2022, Arm Limited. All rights reserved. Not sure if all copyrights of all changed classes have been tweaked? This might be a more general problem with FFM API. test/jdk/ProblemList.txt line 484: > 482: # jdk_foreign > 483: > 484: java/foreign/callarranger/TestAarch64CallArranger.java generic-x86 Should we exclude these tests on 32 bits in the jtreg header (as I think we do for other tests) ? ------------- PR: https://git.openjdk.org/jdk/pull/11019
