[foreign-memaccess+abi] RFR: 8291473: Unify MemorySegment and MemoryAddress [v4]

[Maurizio Cimadamore]

On Thu, 28 Jul 2022 14:24:37 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Update test/jdk/java/foreign/TestUpcallHighArity.java
>>   
>>   Co-authored-by: Jorn Vernee <JornVernee at users.noreply.github.com>
>
> src/java.base/share/classes/jdk/internal/foreign/abi/x64/windows/CallArranger.java line 278:
> 
>> 276:                     bindings.vmLoad(storage, long.class)
>> 277:                             .boxAddress(Long.MAX_VALUE)
>> 278:                             .toSegment(layout);
> 
> Same here. ToSegment should take a `long`.

Ok, so the problem, in the new world, is that we have a `boxAddress` but we don't really need it, and instead we just want `toSegment` ?

> test/jdk/java/foreign/TestUpcallBase.java line 149:
> 
>> 147:         for (int i = 0; i < o.length; i++) {
>> 148:             if (o[i] instanceof MemorySegment &&
>> 149:                     !((MemorySegment) o[i]).session().equals(MemorySession.global())) {
> 
> This is a bit mystical looking (though I understand what it does). I suggest passing/binding the function descriptor or list of `ParamType` to `passAndSave`, so that it has the right info to determine whether this argument should be copied, and then use that info.
> 
> Or, maybe just add a comment here explaining that this is discerning pointers from by-value struct arguments.

I had what you suggested, and then got back to this to minimize diffs (adding an extra param ended up being quire pervasive)  - I'll add a comment

> test/jdk/java/foreign/handles/invoker_module/handle/invoker/MethodHandleInvoker.java line 75:
> 
>> 73:         addDefaultMapping(MemoryLayout.class, ValueLayout.JAVA_INT);
>> 74:         addDefaultMapping(FunctionDescriptor.class, FunctionDescriptor.ofVoid());
>> 75:         addDefaultMapping(MemorySession.class, MemorySession.openShared());
> 
> I'm curious why this was needed? :)

Not needed - more a result of other shuffling - but then when coming back to it I realized that it was probably more correct to use implicit

> test/micro/org/openjdk/bench/java/lang/foreign/BulkMismatchAcquire.java line 52:
> 
>> 50: @OutputTimeUnit(TimeUnit.MILLISECONDS)
>> 51: @Fork(value = 3, jvmArgsAppend = "--enable-preview")
>> 52: public class BulkMismatchAcquire {
> 
> Was this removed intentionally?

no - thanks for the catch

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

PR: https://git.openjdk.org/panama-foreign/pull/694