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

[Henry Jen]

On Thu, 9 Jul 2020 14:24:55 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

Overall looks good, I only have one doubt, what would happen if we close a VaList obtained via NativeScope?

The Allocator concept had surfaced a couple times, not sure if we have discussed whether it worth to be public so that
we don't need to have double APIs everywhere for MS allocation as default and another to have NativeScope.

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

> 161:          * That means that if this va list was created with the {@link #make(Consumer)} method, closing
> 162:          * this va list will also free the argument space, and make the copy unusable.
> 163:          *

I don't fully have a grasp on this note. Not sure I understand the 'view' and 'argument space'. My guess is that this
means for the content of a MS argument. Do we have a test case for this note?

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/x64/windows/WinVaList.java line 174:

> 173:     public VaList copy(NativeScope scope) {
> 174:         return copy();
> 175:     }

This seems strange for someone not knowing about Windows implementation, worth to have a comment to state the reason?

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

> 174:          * That means that if this va list was created with the {@link #make(Consumer)} method, closing
> 175:          * this va list will also free the argument space, and make the copy unusable.
> 176:          *

Seems to me, this method is only use NativeScope to allocate the VaList itself, the actual copy operation itself is no
difference. In that case, should we clarify that copy() is using MemorySegment.allocateNative if allocation is needed?

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/SharedUtils.java line 372:

> 371:         static Allocator ofScope(NativeScope scope) {
> 372:             return (size, align) -> scope.allocate(size, align).segment();
> 373:         }

IIUC, this segment won't have CLOSE mode, so when VaList.close() called, wouldn't that cause exception?

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

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