I think this discussion should go either on core-libs or on panama-dev.
build-dev is not the place for it. I'm addding panama-dev - and then
I'll reply from there.
Maurizio
On 24/11/2021 16:15, Ty Young wrote:
Was this code actually reviewed or even used/tested at all? All "Java"
ValueLayouts have an 8-bit alignment, even if they are 2-byte, 4-byte,
or 8-byte in size:
https://github.com/openjdk/jdk/blob/cf7adae6333c7446048ef0364737927337631f63/src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java#L563
https://github.com/openjdk/jdk/blob/cf7adae6333c7446048ef0364737927337631f63/src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java#L573
https://github.com/openjdk/jdk/blob/cf7adae6333c7446048ef0364737927337631f63/src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java#L583
https://github.com/openjdk/jdk/blob/cf7adae6333c7446048ef0364737927337631f63/src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java#L594
https://github.com/openjdk/jdk/blob/cf7adae6333c7446048ef0364737927337631f63/src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java#L604
The doc for ValueLayout says:
"The layout constants in this class make implicit alignment and
byte-ordering assumption: all layout
constants in this class are byte-aligned"
Which is not true in the code itself. All of the docs for the
constants aren't correct, either. This results in a struct that
*should* have a byte alignment of 8 actually have 1:
GroupLayout layout = MemoryLayout.structLayout(
ValueLayout.JAVA_INT,
MemoryLayout.paddingLayout(32),
ValueLayout.JAVA_LONG);
System.out.println(layout.byteAlignment()); // prints 1!
Git blame shows that these issues were introduced a whole 2 months ago
as part of the "beautiful" and "perfect" API changes:
https://github.com/openjdk/panama-foreign/blame/c15cb9c11e03c7552d2143273959acac30969db7/src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java#L560
and no one noticed? There should probably be alignment validation
checks in place to catch this. The fact that constants are declared at
the bottom instead of the top probably didn't help in making this
issue more noticeable.
While I'm here, is passing ByteOrder.nativeOrder() to every
constructor needed?
On 11/24/21 5:55 AM, Maurizio Cimadamore wrote:
On Tue, 12 Oct 2021 11:16:51 GMT, Maurizio Cimadamore
<[email protected]> wrote:
This PR contains the API and implementation changes for JEP-419 [1].
A more detailed description of such changes, to avoid repetitions
during the review process, is included as a separate comment.
[1] - https://openjdk.java.net/jeps/419
This pull request has now been integrated.
Changeset: 96e36071
Author: Maurizio Cimadamore <[email protected]>
URL:
https://git.openjdk.java.net/jdk/commit/96e36071b63b624d56739b014b457ffc48147c4f
Stats: 14700 lines in 193 files changed: 6958 ins; 5126 del; 2616
mod
8275063: Implementation of Foreign Function & Memory API (Second
incubator)
Reviewed-by: erikj, psandoz, jvernee, darcy
-------------
PR: https://git.openjdk.java.net/jdk/pull/5907
