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

[Maurizio Cimadamore]

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 
>> <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