[foreign-memaccess+abi] RFR: 8265751: MemoryAddress should have a scope accessor [v3]

[Paul Sandoz]

On Thu, 22 Apr 2021 13:20:59 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> Following the recent API changes, I think `MemoryAddress` has been left in an inconsistent state. On the one hand, we clearly want `MemoryAddress` to have a link to a `ResourceScope`; we clearly state so in the javadoc for:
>> 
>> * `CLinker::upcallStub`
>> * `MemorySegment::address`
>> 
>> (and we don't say, but we probably should in `VaList::address`).
>> 
>> This link is crucial, as it prevents scope closure, if an address is passed to a native function and the scope is implicit.
>> 
>> However, this dependency is, currently, ad-hoc, and not cleanly stated in the API.
>> 
>> This patch rectifies that; now all memory address have a scope (which can be the "global scope" for addresses obtained from native, or from `long` values).
>> 
>> There are not many changes in terms of implementation, but by putting the association between addresses and scopes front and center we can make the javadoc clearer.
>> 
>> Also, this allows us to move the scope check from `MemorySegment::address` (that has always been a bit odd) to `MemoryAddress::toRawLongAddress` - that is, closer to where the address value is actually used.
>> 
>> This allows us to, effectively, make calls to downcall method handles *safer*, since now a scope check will take place before the raw long value is extracted.
>> 
>> Note that this doesn't negatively affect performance: the check was occurring even before, albeit at a different level (closer to the user code). I've run all the downcall benchmarks we have and found no regressions.
>> 
>> I've added a test which makes sure that passing "closed" segments/addresses to native function is no longer possible.
>
> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Javadoc of MemoryAddress::equals is still segment-centric

Marked as reviewed by psandoz (Committer).

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryAddress.java line 106:

> 104:     /**
> 105:      Returns a new native memory segment with given size and resource scope (possibly overriding the scope already associated
> 106:      * with this address), and whose base address is this address. This method can be useful when interacting with custom

Suggestion:

     Returns a new native memory segment with given size and resource scope (replacing the scope already associated
     * with this address), and whose base address is this address. This method can be useful when interacting with custom

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryAddress.java line 132:

> 130:      * @throws UnsupportedOperationException if this address is an heap address.
> 131:      */
> 132:     MemorySegment asSegment(long bytesSize, ResourceScope scope);

Do we need to throw ISE if the existing scope is closed etc.? I think the implementation will do so, since it eventually calls `toRawLongValue()` (in `NativeMemorySegmentImpl.makeNativeSegmentUnchecked`) i.e. the method is predicated on the scope that is being replaced.

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

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