[foreign-abi] RFR: 8248420: Add a variant of VaList::make which takes a NativeScope [v3]

Paul Sandoz psandoz at openjdk.java.net
Wed Jul 15 01:03:51 UTC 2020


On Tue, 14 Jul 2020 09:59:56 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> Hi,
>> 
>> This patch adds overloads to several VaList related methods that might need to do allocations, namely, VaList::make,
>> VaList::vargAsSegment, and VaList::copy.
>> In order to share the existing code, I've added an `Allocator` helper interface that can wrap either a NativeScope, or
>> be used as a functional interface to bind to MemorySegment::allocateNative.
>> While testing this patch, I've also discovered a bug in the SysV implementation; The VaList::Builder was allocating a
>> MemorySegment for the VaList, then creating the SysVVaList object wrapping that segment, and then filling in some of
>> the contents of the segment. But, the SysVVaList constructor was also trying to read from the segment as well, and as a
>> result was reading uninitialized fields. This is a problem when creating a VaList in Java, and then trying to read from
>> it also in Java.  I've restructured the code a bit to fix this problem, moving the reading logic from the constructor
>> to a static factory called readFromSegment, so that it's clear that only a fully initialized segment should be passed.
>> The builder still calls the constructor directly, but it now requires all field values to be passed in explicitly.
>> There was a similar problem in the AArch64 implementation, which I've tried to fix. Unfortunately I was not able to
>> fully test this change since there was a failure elsewhere in the testing infrastructure, and I currently don't have
>> direct access to an aarch64 machine to be able to debug that. I did end up fixing some build failures on aarch64 in the
>> process though, which I will submit another PR for.  Thanks, Jorn
>
> Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update CSupport.java
>   
>   Typo

Docs are much better. Recommend where you space out paragraphs to also include <p> otherwise the JavaDoc will render
them as one paragraph (using the doc view in IntelliJ should help you get a sense of that).

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CSupport.java line 161:

> 160:          * of a va list does not need any native memory, so nothing has to be released. After calling {@code
> close()} on 161:          * such an instance {@link #isAlive()} will still return {@code true}.
> 162:          *

Is it possible hide the implementation detail so close behaviour is consistent across all implementations, even if on
some implementation no allocation is required? Especially since close also may close any native memory that is attached.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CSupport.java line 72:

> 71:         /**
> 72:          * Reads the next value into an {@code int} and advances this va list's position.
> 73:          *

For primitives values i recommend saying "Reads the next value as an {@code int), and ...".

What happens if the layout is not compatible with the carrier type `int.class`?

-------------

PR: https://git.openjdk.java.net/panama-foreign/pull/237


More information about the panama-dev mailing list