Integrated: 8275063: Implementation of Foreign Function & Memory API (Second incubator)

[Ty Young]

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 <mcimadamore at openjdk.org> 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 <mcimadamore at openjdk.org>
> 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