Skip to content

Commit

Permalink
8326172: Dubious claim on long[]/double[] alignment in MemorySegment …
Browse files Browse the repository at this point in the history
…javadoc

Reviewed-by: mcimadamore
  • Loading branch information
JornVernee committed Mar 5, 2024
1 parent c653e67 commit 2372aba
Show file tree
Hide file tree
Showing 10 changed files with 44 additions and 49 deletions.
35 changes: 11 additions & 24 deletions src/java.base/share/classes/java/lang/foreign/MemorySegment.java
Original file line number Diff line number Diff line change
Expand Up @@ -305,8 +305,7 @@
* and/or garbage collection behavior).
* <p>
* In practice, the Java runtime lays out arrays in memory so that each n-byte element
* occurs at an n-byte aligned physical address (except for {@code long[]} and
* {@code double[]}, where alignment is platform-dependent, as explained below). The
* occurs at an n-byte aligned physical address. The
* runtime preserves this invariant even if the array is relocated during garbage
* collection. Access operations rely on this invariant to determine if the specified
* offset in a heap segment refers to an aligned address in physical memory.
Expand All @@ -325,26 +324,17 @@
* would correspond to physical address 1008 but offset 4 would correspond to
* physical address 1010.</li>
* <li>The starting physical address of a {@code long[]} array will be 8-byte aligned
* (e.g. 1000) on 64-bit platforms, so that successive long elements occur at
* 8-byte aligned addresses (e.g., 1000, 1008, 1016, 1024, etc.) On 64-bit platforms,
* a heap segment backed by a {@code long[]} array can be accessed at offsets
* 0, 8, 16, 24, etc under an 8-byte alignment constraint. In addition, the segment
* can be accessed at offsets 0, 4, 8, 12, etc under a 4-byte alignment constraint,
* because the target addresses (1000, 1004, 1008, 1012) are 4-byte aligned. And,
* the segment can be accessed at offsets 0, 2, 4, 6, etc under a 2-byte alignment
* constraint, because the target addresses (e.g. 1000, 1002, 1004, 1006) are
* 2-byte aligned.</li>
* <li>The starting physical address of a {@code long[]} array will be 4-byte aligned
* (e.g. 1004) on 32-bit platforms, so that successive long elements occur at 4-byte
* aligned addresses (e.g., 1004, 1008, 1012, 1016, etc.) On 32-bit platforms, a heap
* segment backed by a {@code long[]} array can be accessed at offsets
* 0, 4, 8, 12, etc under a 4-byte alignment constraint, because the target addresses
* (1004, 1008, 1012, 1016) are 4-byte aligned. And, the segment can be accessed at
* offsets 0, 2, 4, 6, etc under a 2-byte alignment constraint, because the target
* addresses (e.g. 1000, 1002, 1004, 1006) are 2-byte aligned.</li>
* (e.g. 1000), so that successive long elements occur at 8-byte aligned addresses
* (e.g., 1000, 1008, 1016, 1024, etc.) A heap segment backed by a {@code long[]}
* array can be accessed at offsets 0, 8, 16, 24, etc under an 8-byte alignment
* constraint. In addition, the segment can be accessed at offsets 0, 4, 8, 12,
* etc under a 4-byte alignment constraint, because the target addresses (1000, 1004,
* 1008, 1012) are 4-byte aligned. And, the segment can be accessed at offsets 0, 2,
* 4, 6, etc under a 2-byte alignment constraint, because the target addresses (e.g.
* 1000, 1002, 1004, 1006) are 2-byte aligned.</li>
* </ul>
* <p>
* In other words, heap segments feature a (platform-dependent) <em>maximum</em>
* In other words, heap segments feature a <em>maximum</em>
* alignment which is derived from the size of the elements of the Java array backing the
* segment, as shown in the following table:
*
Expand Down Expand Up @@ -389,10 +379,7 @@
* In such circumstances, clients have two options. They can use a heap segment backed
* by a different array type (e.g. {@code long[]}), capable of supporting greater maximum
* alignment. More specifically, the maximum alignment associated with {@code long[]} is
* set to {@code ValueLayout.JAVA_LONG.byteAlignment()} which is a platform-dependent
* value (set to {@code ValueLayout.ADDRESS.byteSize()}). That is, {@code long[]}) is
* guaranteed to provide at least 8-byte alignment in 64-bit platforms, but only 4-byte
* alignment in 32-bit platforms:
* set to {@code ValueLayout.JAVA_LONG.byteAlignment()}, which is 8 bytes:
*
* {@snippet lang=java :
* MemorySegment longSegment = MemorySegment.ofArray(new long[10]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,6 @@
* For instance, the byte order of these constants is set to the
* {@linkplain ByteOrder#nativeOrder() native byte order}, thus making it easy
* to work with other APIs, such as arrays and {@link java.nio.ByteBuffer}.
* Moreover, the alignment constraint of {@link ValueLayout#JAVA_LONG} and
* {@link ValueLayout#JAVA_DOUBLE} is set to 8 bytes on 64-bit platforms,
* but only to 4 bytes on 32-bit platforms.
*
* @implSpec implementing classes and subclasses are immutable, thread-safe and
* <a href="{@docRoot}/java.base/java/lang/doc-files/ValueBased.html">value-based</a>.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,6 @@
abstract sealed class HeapMemorySegmentImpl extends AbstractMemorySegmentImpl {

// Constants defining the maximum alignment supported by various kinds of heap arrays.
// While for most arrays, the maximum alignment is constant (the size, in bytes, of the array elements),
// note that the alignment of a long[]/double[] depends on the platform: it's 4-byte on x86, but 8 bytes on x64
// (as specified by the JAVA_LONG layout constant).

private static final long MAX_ALIGN_BYTE_ARRAY = ValueLayout.JAVA_BYTE.byteAlignment();
private static final long MAX_ALIGN_SHORT_ARRAY = ValueLayout.JAVA_SHORT.byteAlignment();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -307,8 +307,8 @@ class Holder {
Map.entry("bool", JAVA_BOOLEAN),
Map.entry("char", JAVA_BYTE),
Map.entry("float", JAVA_FLOAT),
Map.entry("long long", JAVA_LONG),
Map.entry("double", JAVA_DOUBLE),
Map.entry("long long", JAVA_LONG.withByteAlignment(LibFallback.longLongAlign())),
Map.entry("double", JAVA_DOUBLE.withByteAlignment(LibFallback.doubleAlign())),
Map.entry("void*", ADDRESS),
// platform-dependent sizes
Map.entry("size_t", FFIType.SIZE_T),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ public Boolean run() {
static int intSize() { return NativeConstants.SIZEOF_INT; }
static int longSize() {return NativeConstants.SIZEOF_LONG; }
static int wcharSize() {return NativeConstants.SIZEOF_WCHAR; }
static int longLongAlign() { return NativeConstants.ALIGNOF_LONG_LONG; }
static int doubleAlign() { return NativeConstants.ALIGNOF_DOUBLE; }

static short structTag() { return NativeConstants.STRUCT_TAG; }

Expand Down Expand Up @@ -242,6 +244,9 @@ private static native void doDowncall(long cif, long fn, long rvalue, long avalu
private static native int ffi_sizeof_long();
private static native int ffi_sizeof_wchar();

private static native int alignof_long_long();
private static native int alignof_double();

// put these in a separate class to avoid an UnsatisfiedLinkError
// when LibFallback is initialized but the library is not present
private static final class NativeConstants {
Expand All @@ -263,6 +268,8 @@ private NativeConstants() {}
static final int SIZEOF_LONG = ffi_sizeof_long();
static final int SIZEOF_WCHAR = ffi_sizeof_wchar();

static final int ALIGNOF_LONG_LONG = alignof_long_long();
static final int ALIGNOF_DOUBLE = alignof_double();

static final MemorySegment VOID_TYPE = MemorySegment.ofAddress(ffi_type_void());
static final short STRUCT_TAG = ffi_type_struct();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,7 @@ OfLongImpl dup(ByteOrder order, long byteAlignment, Optional<String> name) {
}

public static OfLong of(ByteOrder order) {
return new OfLongImpl(order, ADDRESS_SIZE_BYTES, Optional.empty());
return new OfLongImpl(order, Long.BYTES, Optional.empty());
}
}

Expand All @@ -294,7 +294,7 @@ OfDoubleImpl dup(ByteOrder order, long byteAlignment, Optional<String> name) {
}

public static OfDouble of(ByteOrder order) {
return new OfDoubleImpl(order, ADDRESS_SIZE_BYTES, Optional.empty());
return new OfDoubleImpl(order, Double.BYTES, Optional.empty());
}

}
Expand Down
11 changes: 11 additions & 0 deletions src/java.base/share/native/libfallbackLinker/fallbackLinker.c
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#include <ffi.h>

#include <errno.h>
#include <stdalign.h>
#include <stdint.h>
#include <stdlib.h>
#include <wchar.h>
Expand Down Expand Up @@ -275,3 +276,13 @@ JNIEXPORT jint JNICALL
Java_jdk_internal_foreign_abi_fallback_LibFallback_ffi_1sizeof_1wchar(JNIEnv* env, jclass cls) {
return sizeof(wchar_t);
}

JNIEXPORT jint JNICALL
Java_jdk_internal_foreign_abi_fallback_LibFallback_alignof_1long_1long(JNIEnv* env, jclass cls) {
return alignof(long long);
}

JNIEXPORT jint JNICALL
Java_jdk_internal_foreign_abi_fallback_LibFallback_alignof_1double(JNIEnv* env, jclass cls) {
return alignof(double);
}
20 changes: 8 additions & 12 deletions test/jdk/java/foreign/TestLayouts.java
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ public void testStructSizeAndAlign() {
ValueLayout.JAVA_LONG
);
assertEquals(struct.byteSize(), 1 + 1 + 2 + 4 + 8);
assertEquals(struct.byteAlignment(), ADDRESS.byteSize());
assertEquals(struct.byteAlignment(), 8);
}

@Test(dataProvider="basicLayouts")
Expand Down Expand Up @@ -192,7 +192,7 @@ public void testUnionSizeAndAlign() {
ValueLayout.JAVA_LONG
);
assertEquals(struct.byteSize(), 8);
assertEquals(struct.byteAlignment(), ADDRESS.byteSize());
assertEquals(struct.byteAlignment(), 8);
}

@Test
Expand Down Expand Up @@ -477,24 +477,24 @@ public Object[][] layoutsAndAlignments() {
List<Object[]> layoutsAndAlignments = new ArrayList<>();
int i = 0;
//add basic layouts
for (MemoryLayout l : basicLayoutsNoLongDouble) {
for (MemoryLayout l : basicLayouts) {
layoutsAndAlignments.add(new Object[] { l, l.byteAlignment() });
}
//add basic layouts wrapped in a sequence with given size
for (MemoryLayout l : basicLayoutsNoLongDouble) {
for (MemoryLayout l : basicLayouts) {
layoutsAndAlignments.add(new Object[] { MemoryLayout.sequenceLayout(4, l), l.byteAlignment() });
}
//add basic layouts wrapped in a struct
for (MemoryLayout l1 : basicLayoutsNoLongDouble) {
for (MemoryLayout l2 : basicLayoutsNoLongDouble) {
for (MemoryLayout l1 : basicLayouts) {
for (MemoryLayout l2 : basicLayouts) {
if (l1.byteSize() % l2.byteAlignment() != 0) continue; // second element is not aligned, skip
long align = Math.max(l1.byteAlignment(), l2.byteAlignment());
layoutsAndAlignments.add(new Object[]{MemoryLayout.structLayout(l1, l2), align});
}
}
//add basic layouts wrapped in a union
for (MemoryLayout l1 : basicLayoutsNoLongDouble) {
for (MemoryLayout l2 : basicLayoutsNoLongDouble) {
for (MemoryLayout l1 : basicLayouts) {
for (MemoryLayout l2 : basicLayouts) {
long align = Math.max(l1.byteAlignment(), l2.byteAlignment());
layoutsAndAlignments.add(new Object[]{MemoryLayout.unionLayout(l1, l2), align});
}
Expand Down Expand Up @@ -543,8 +543,4 @@ static Stream<MemoryLayout> groupLayoutStream() {
ValueLayout.JAVA_LONG,
ValueLayout.JAVA_DOUBLE,
};

static MemoryLayout[] basicLayoutsNoLongDouble = Stream.of(basicLayouts)
.filter(l -> l.carrier() != long.class && l.carrier() != double.class)
.toArray(MemoryLayout[]::new);
}
4 changes: 2 additions & 2 deletions test/jdk/java/foreign/TestValueLayouts.java
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ public void testIntUnaligned() {

@Test
public void testLong() {
testAligned(JAVA_LONG, long.class, Long.BYTES, ADDRESS.byteSize());
testAligned(JAVA_LONG, long.class, Long.BYTES, Long.BYTES);
}

@Test
Expand All @@ -90,7 +90,7 @@ public void testFloatUnaligned() {

@Test
public void testDouble() {
testAligned(JAVA_DOUBLE, double.class, Double.BYTES, ADDRESS.byteSize());
testAligned(JAVA_DOUBLE, double.class, Double.BYTES, Double.BYTES);
}

@Test
Expand Down
2 changes: 1 addition & 1 deletion test/jdk/java/foreign/stackwalk/TestReentrantUpcalls.java
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ public static void main(String[] args) throws Throwable {
}

static void m(int depth, MemorySegment thisStub, MethodHandle downcallHandle) throws Throwable {
if (depth < 100) {
if (depth < 50) {
downcallHandle.invokeExact(depth + 1, thisStub);
} else {
WB.verifyFrames(/*log=*/true, /*updateRegisterMap=*/true);
Expand Down

3 comments on commit 2372aba

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vova7878
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/** * A value layout constant whose size is the same as that of a Java {@code long}, * (platform-dependent) byte alignment set to {@code ADDRESS.byteSize()}, * and byte order set to {@link ByteOrder#nativeOrder()}. */ OfLong JAVA_LONG = ValueLayouts.OfLongImpl.of(ByteOrder.nativeOrder());

And

/** * A value layout constant whose size is the same as that of a Java {@code double}, * (platform-dependent) byte alignment set to {@code ADDRESS.byteSize()}, * and byte order set to {@link ByteOrder#nativeOrder()}. */ OfDouble JAVA_DOUBLE = ValueLayouts.OfDoubleImpl.of(ByteOrder.nativeOrder());

Has wrong javadoc

@JornVernee
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vova7878 Thanks for the report. I've filed: https://bugs.openjdk.org/browse/JDK-8330272

Please sign in to comment.