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

[Jorn Vernee]

On Thu, 9 Jul 2020 20:10:41 GMT, Henry Jen <henryjen at openjdk.org> wrote:

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

I talked to Maurizio a bit about this last week, and we ended up with the verdict that it might be interesting to have
a public Allocator API, but that we should revisit later and for now go with an internal-only utility.

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

Yes, it would cause an exception. It's the same when allocating from a NativeScope like normal. You don't have to call
close() in that case.

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

Yeah I'll add that.

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

Ok. FWIW, the Windows implementation of copy() does not need to do any allocation, because a VaList on Windows is just
a pointer. But, on SysV a VaList is a struct, so we need to allocate some memory for that.

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

A VaList is really only a 'view' of a set of arguments stored elsewhere. In native code the place where the arguments
are stored (i.e. the 'argument space') is on the stack, so the memory will get cleaned up automatically. But, if we
create a VaList on the Java side we need to do an explicit off-heap allocation and later call free to clean up the
memory that stores the arguments. This argument space is freed when calling `close()` on the VaList.

So, if you `copy()` a VaList, only the 'view' part is copied, but not the memory that holds the arguments.

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

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