8317545: AIX PPC64: Implementation of Foreign Function & Memory API #16179
Conversation
|
👋 Welcome back mdoerr! A progress list of the required criteria for merging this PR into |
|
@TheRealMDoerr The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
openjdk
bot
added
hotspot-compiler
Webrevs
|
| @@ -63,7 +64,7 @@ public ValueLayout.OfInt layout() { | |||
| public static final OfDouble<Double> C_DOUBLE = new OfDouble<>() { | |||
| @Override | |||
| public ValueLayout.OfDouble layout() { | |||
| return ValueLayout.JAVA_DOUBLE; | |||
| return ValueLayout.JAVA_DOUBLE.withByteAlignment(IS_AIX ? 4 : 8); | |||
| @@ -64,7 +65,8 @@ public class CLayouts { | |||
| /** | |||
| * The layout for the {@code double} C type | |||
| */ | |||
| public static final ValueLayout.OfDouble C_DOUBLE = ValueLayout.JAVA_DOUBLE; | |||
| public static final ValueLayout.OfDouble C_DOUBLE = ValueLayout.JAVA_DOUBLE.withByteAlignment(IS_AIX ? 4 : 8); | |||
There was a problem hiding this comment.
Would prefer if this used Linker.canonicalLayouts
| public static final ValueLayout.OfDouble C_DOUBLE = ValueLayout.JAVA_DOUBLE.withByteAlignment(IS_AIX ? 4 : 8); | |
| public static final ValueLayout.OfDouble C_DOUBLE = (ValueLayout.OfDouble) LINKER.canonicalLayouts().get("double"); |
There was a problem hiding this comment.
I've improved the micro benchmarks with 3rd commit.
|
|
||
| static { | ||
| HashMap<String, MemoryLayout> layouts = new HashMap<>(); | ||
| layouts.putAll(SharedUtils.canonicalLayouts(ValueLayout.JAVA_LONG, ValueLayout.JAVA_LONG, ValueLayout.JAVA_INT)); |
There was a problem hiding this comment.
You may also add an extra parameter for the double layout to SharedUtils::canonicalLayouts.
Also, what about jdouble? It seems to be defined to a native double on AIX as well? (see src/java.base/share/native/include/jni.h)
There was a problem hiding this comment.
Not sure how jdouble would be used. JNI doesn't support C structures and the double alignment is only an issue in structures. Do we support embedding jdouble in structures? I guess changing it would probably be better?
Note that we will need something which maps to the 8-Byte aligned double. Otherwise we get an Exception when passing a JAVA_DOUBLE as normal argument:
IllegalArgumentException: Unsupported layout: D8
at java.base/jdk.internal.foreign.abi.AbstractLinker.checkSupported(AbstractLinker.java:244)
at java.base/jdk.internal.foreign.abi.AbstractLinker.checkLayoutRecursive(AbstractLinker.java:185)
at java.base/jdk.internal.foreign.abi.AbstractLinker.checkLayout(AbstractLinker.java:179)
at java.base/java.lang.Iterable.forEach(Iterable.java:75)
at java.base/jdk.internal.foreign.abi.AbstractLinker.checkLayouts(AbstractLinker.java:171)
at java.base/jdk.internal.foreign.abi.AbstractLinker.downcallHandle0(AbstractLinker.java:98)
at java.base/jdk.internal.foreign.abi.AbstractLinker.downcallHandle(AbstractLinker.java:85)
at TestDowncall.<clinit>(TestDowncall.java:127)
There was a problem hiding this comment.
I guess changing it would probably be better?
Yeah, I think so.
Note that we will need something which maps to the 8-Byte aligned double. Otherwise we get an Exception when passing a JAVA_DOUBLE as normal argument.
Okay, so it sounds like Java double on AIX is still 8-byte aligned, but the native double is 4-byte aligned?
In that case, I'd say that passing JAVA_DOUBLE as an argument resulting in an exception, is expected behavior. Using JAVA_DOUBLE to link native functions works more or less by coincidence, since it has the same layout as the native double, on all other platforms. The AIX issue with double is not dissimilar to using JAVA_LONG to link against a function taking a native long on Windows, in which case the size of the type doesn't match.
The philosophy here is: a client is responsible for passing the right FunctionDescriptor/layouts, that matches the native function declaration. On AIX, JAVA_DOUBLE is just never the right layout, and the linker detects this and throws an exception.
I understand that some of the existing tests might be lazy and use JAVA_DOUBLE to link functions. It is fine to update those tests, e.g. to use C_DOUBLE defined in NativeTestHelper.
There was a problem hiding this comment.
Just to clarify, is the ABI equal to what is described in this table: https://www.ibm.com/docs/en/xl-c-and-cpp-aix/16.1?topic=data-using-alignment-modes ?
This shows double having 8-byte alignment, but being 4-byte aligned when not the first member of an aggregate.
There was a problem hiding this comment.
Okay, so it sounds like Java double on AIX is still 8-byte aligned, but the native double is 4-byte aligned?
Correct within structures. I believe they are always 8-byte aligned except in structures which are packed more densely.
In that case, I'd say that passing
JAVA_DOUBLEas an argument resulting in an exception, is expected behavior.
Hmm... Should we disallow to pass 8-byte aligned double values? Fixing the tests is a good thing. But, I have a different concern: I guess many developers may use them without testing on AIX. This would cause problems which we could avoid by supporting 8-byte aligned doubles in addition.
What about adding
layouts.put("jdouble", ValueLayout.JAVA_DOUBLE.withByteAlignment(4));
layouts.put("aligneddouble", ValueLayout.JAVA_DOUBLE); // allowed for usage outside of structures
?
There was a problem hiding this comment.
I was thinking something like this:
diff --git a/src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java b/src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java
index dbd9a3f67a4..10b371457b3 100644
--- a/src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java
+++ b/src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java
@@ -180,6 +180,11 @@ private void checkLayout(MemoryLayout layout) {
}
}
+ // some ABIs have special handling for struct members
+ protected void checkStructMember(MemoryLayout member, long offset) {
+ checkLayoutRecursive(member);
+ }
+
private void checkLayoutRecursive(MemoryLayout layout) {
if (layout instanceof ValueLayout vl) {
checkSupported(vl);
@@ -191,7 +196,7 @@ private void checkLayoutRecursive(MemoryLayout layout) {
// check element offset before recursing so that an error points at the
// outermost layout first
checkMemberOffset(sl, member, lastUnpaddedOffset, offset);
- checkLayoutRecursive(member);
+ checkStructMember(member, offset);
offset += member.byteSize();
if (!(member instanceof PaddingLayout)) {
diff --git a/src/java.base/share/classes/jdk/internal/foreign/abi/ppc64/aix/AixPPC64Linker.java b/src/java.base/share/classes/jdk/internal/foreign/abi/ppc64/aix/AixPPC64Linker.java
index c24d2553ec0..f70af2bc025 100644
--- a/src/java.base/share/classes/jdk/internal/foreign/abi/ppc64/aix/AixPPC64Linker.java
+++ b/src/java.base/share/classes/jdk/internal/foreign/abi/ppc64/aix/AixPPC64Linker.java
@@ -36,19 +36,14 @@
import java.lang.invoke.MethodHandle;
import java.lang.invoke.MethodType;
import java.nio.ByteOrder;
-import java.util.HashMap;
import java.util.Map;
public final class AixPPC64Linker extends AbstractLinker {
- static final Map<String, MemoryLayout> CANONICAL_LAYOUTS;
+ static final Map<String, MemoryLayout> CANONICAL_LAYOUTS
+ = SharedUtils.canonicalLayouts(ValueLayout.JAVA_LONG, ValueLayout.JAVA_LONG, ValueLayout.JAVA_INT);
- static {
- HashMap<String, MemoryLayout> layouts = new HashMap<>();
- layouts.putAll(SharedUtils.canonicalLayouts(ValueLayout.JAVA_LONG, ValueLayout.JAVA_LONG, ValueLayout.JAVA_INT));
- layouts.put("double", ValueLayout.JAVA_DOUBLE.withByteAlignment(4));
- CANONICAL_LAYOUTS = Map.copyOf(layouts);
- }
+ private static final MemoryLayout C_DOUBLE = CANONICAL_LAYOUTS.get("double");
public static AixPPC64Linker getInstance() {
final class Holder {
@@ -62,6 +57,19 @@ private AixPPC64Linker() {
// Ensure there is only one instance
}
+ @Override
+ protected void checkStructMember(MemoryLayout member, long offset) {
+ // special case double members that are not the first member
+ // see: https://www.ibm.com/docs/en/xl-c-and-cpp-aix/16.1?topic=data-using-alignment-modes
+ if ((offset > 0) && member.equals(C_DOUBLE)) {
+ if (member.byteAlignment() != 4) {
+ throw new IllegalArgumentException("double struct member following the first member should be 4-byte aligned");
+ }
+ } else {
+ super.checkStructMember(member, offset);
+ }
+ }
+
@Override
protected MethodHandle arrangeDowncall(MethodType inferredMethodType, FunctionDescriptor function, LinkerOptions options) {
return CallArranger.AIX.arrangeDowncall(inferredMethodType, function, options);
I.e. there should not have to be a special 4-byte aligned canonical layout. Canonical layouts are for mapping native/C type names to memory layouts, and double4bytealigned is not such a name.
Then, with the above changes, you'd also need to change the tests to use the right double layouts when creating struct layouts on AIX. Note that that will also require changes to jdk.internal.foreign.Utils::computePaddedStructLayout which computes struct layouts for some of the tests (including all the TestDowncall* and TestUpcall* tests). In that method, there are a couple of calls to l.byteAlignment() that should be replaced by something that returns 4 for non-first double layouts on AIX.
There was a problem hiding this comment.
Err, actually using member.equals(C_DOUBLE) in the above doesn't work since it still checks alignment. What you have with checking for ValueLayout and carrier() == double.class is better, but the byte order should also be checked at some point.
There was a problem hiding this comment.
Please take a look at commit number 4. I think we need to support both, 4-byte and 8-byte aligned doubles in structures. IBM recommends to use "#pragma align (natural)": "The power suboption is the default to ensure compatibility with existing objects. If compatibility with earlier versions is not necessary, you should consider using natural alignment to improve potential application performance." https://www.ibm.com/docs/en/openxl-c-and-cpp-aix/17.1.1?topic=pragmas-pragma-align
We could also recommend to use "#pragma align (natural)" for the FFI (and possibly for tests). That would reduce incompatibility with other platforms. I can file a subtask for test adaptation if needed.
There was a problem hiding this comment.
Hi @TheRealMDoerr - yes, last weekend I read some bits from the AIX ABI, and it does seem that the more optimized variant is the "natural" variant, as that respects data alignment. As you write "power" seems to be the default, out of some compatibility constraints. But I think the main point that Jorn and I are trying to make is that the fact that "power" behaves in the way it does (e.g. it aligns all double fields after the first one at 4 bytes, rather than 8) has nothing to do with the intrinsic alignment of a double in AIX. E.g. the double type in AIX has alignment 8 (and you can check that using alignof). Then, it seems like all structs in AIX are compiled as if by using a pack pragma - and that is what causes some double fields to feature a more packed alignment constraints.
Where does this leave us? There are few options:
- As you say, one option would be to just say that on AIX we only support the "natural" variant. This is similar to what we do on other platforms, where we do not support passing packed structs to functions (and what the javadoc of Linker says).
- Another option is to relax the Linker javadoc so that struct layouts are completely Linker specific. This would allow the AIX linker to support whatever alignment is requested.
I have very little experience with AIX, so I'm not in a position to say how much the first option would be perceived as a restriction or not. The nice thing about the first option is that clients of FFM do not have to worry about structs in AIX having special rules. But, perhaps that's an unrealistic goal anyway, and I can imagine other ABIs also going down the path of specifying various packed representations which are not supported by the current javadoc text.
Perhaps, from a logistical point of view, starting off with "natural" for now, and then relaxing the documentation later, and add support for packed structs, would be better, as the changes would be more localized.
There was a problem hiding this comment.
I don't think we can afford rejecting the default variant. But we can state that we only use and test the "natural" variant internally. That will keep most of the trouble away from us, but still require the javadoc change.
|
I have reverted the changes in PlatformLayouts.java. We use 8-byte aligned double by default, now. I have filed a subtask to handle the tests: https://bugs.openjdk.org/browse/JDK-8318175 |
Supporting both seems like the best solution to me.
This seems fine to me. For linking in general, the developer is already expected to know what the right descriptor is. We try to do as much checking as possible to look for descriptors that don't make sense, or which the CallArrangers do not support (hence packed structs are currently rejected), but we can not check everything.
Ok. I'm assuming there are currently some failing tests due to the mismatch in ABI? (assuming the test libraries are compiled for the packed ABI) |
We should be careful with the names:
Almost all of the jdk/java/foreign are currently failing because of https://bugs.openjdk.org/browse/JDK-8317799. Suchi has a preliminary fix and with that most tests have passed with the 4-byte aligned double members. Almost all tests should work after both subtasks are resolved. (There are still a couple of things to investigate which are not related to Up-/Downcalls.) |
| @@ -56,14 +58,14 @@ public non-sealed static abstract class OfStruct<X> extends NativeType<X> { | |||
| public static final OfInt<Integer> C_INT = new OfInt<>() { | |||
| @Override | |||
| public ValueLayout.OfInt layout() { | |||
| return ValueLayout.JAVA_INT; | |||
| return (ValueLayout.OfInt) LINKER.canonicalLayouts().get("int"); | |||
There was a problem hiding this comment.
For this benchmark, we rely on constant folding through these layouts. So, we shouldn't go through canonicalLayouts here. We can store the layout in a static final field, and then return it here instead.
There was a problem hiding this comment.
Done. Thanks!
| } | ||
| }; | ||
|
|
||
| public static final OfDouble<Double> C_DOUBLE = new OfDouble<>() { | ||
| @Override | ||
| public ValueLayout.OfDouble layout() { | ||
| return ValueLayout.JAVA_DOUBLE; | ||
| return (ValueLayout.OfDouble) LINKER.canonicalLayouts().get("double"); |
| // see: https://www.ibm.com/docs/en/xl-c-and-cpp-aix/16.1?topic=data-using-alignment-modes | ||
| // Note: It is possible to enforce 8-byte alignment by #pragma align (natural) | ||
| // Therefore, we use normal checks if we are already 8-byte aligned. | ||
| if ((offset % 8 != 0) && (member instanceof ValueLayout vl && vl.carrier() == double.class)) { |
There was a problem hiding this comment.
I think you should check for the correct byte order as well at some point, for this special case
There was a problem hiding this comment.
Done. Thanks!
|
@TheRealMDoerr This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 285 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
Tested Martins changes using symbol resolution fix. ==============================
|
|
Thanks for testing! This sounds good. We need to handle the failing tests separately as they are not immediately related to the up-/downcalls. TestUpcallAsync is failing because of problems with AttachCurrentThreadAsDaemon on AIX. The other ones because libLoaderLookupInvoker.so seems to be broken on AIX. |
|
@TheRealMDoerr This is good to go. We will clarify the javadoc separately (https://bugs.openjdk.org/browse/JDK-8319316) |
|
Thanks for doing that! |
|
Going to push as commit 99efcde.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@TheRealMDoerr Pushed as commit 99efcde. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
The AIX linker has a few minor diffs to the linux ABIv1 linker. In addition, double values have only 4 Byte alignment within structures. This PR is based on JDK22 version of the FFI.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16179/head:pull/16179$ git checkout pull/16179Update a local copy of the PR:
$ git checkout pull/16179$ git pull https://git.openjdk.org/jdk.git pull/16179/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 16179View PR using the GUI difftool:
$ git pr show -t 16179Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16179.diff
Webrev
Link to Webrev Comment