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

[Jorn Vernee]

On Thu, 9 Jul 2020 21:42:27 GMT, Paul Sandoz <psandoz 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
>
> The API changes look good. I think some follow on work is required in the JavaDoc.
> 
> It's tempting to consider a `CNativeScope` extending `NativeScope`, perhaps making it slightly easier to allocate c
> strings, va_lists etc. Something to consider later maybe.
> It's been a while since i have used `va_list` and needed to refamiliarize myself with it :-)
> 
> I think the documentation could make clearer the side-effecting nature of obtaining arguments, and that the copy is a
> copying the va_list at its current position.
> `vargAsInt` reads the *next* argument as an int.
> 
> Referring to the copy as a view is likely misleading to Java developers. va_list is more like a cursor or iterator, and
> its current state can be copied. A copy in this respect is useful for subsequent traversal at the current position, and
> may be called at any point while traversing a va_list to traverse from the same position without affecting the position
> of the source va_list and vice versa.

Thanks for the comments, I'm working on updating the javadoc, but found another bug after adding extra tests that I'm
also resolving.

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

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