[foreign-memaccess+abi] Integrated: 8264280: Foreign linker should be more friendly with implicit scopes

[Maurizio Cimadamore]

On Fri, 26 Mar 2021 15:09:54 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

> Following the recent introduction of ResourceScope, it is now "easier" to create segments backed by an implicit scope, which will deallocate memory when the scope becomes unreachable.
> 
> Because of this, some idioms, especially in the linker API, have become unsound, consider this:
> 
> MethodHandle STRLEN = CLinker.downcallHandle(...);
> STRLEN.invokeExact(CLinker.toCString("Hello!").address());
> 
> The above code contains a subtle issue: since the segment is converted into an address (and, by the linker, into a raw long), it is possible for the segment (and hence the scope associated with the segment) to become unreachable *before* the native call has returned.
> 
> When working with jextract code, this idiom is even sneakier, given that jextract makes abundant use of `Addressable` in places where an address or a segment can be provided. In those cases the conversion is not even explicit in the user code, it is instead made by jextract.
> 
> This seems like an inconsistency in the foreign API; while VarHandle keep segments and scope alive, native MethodHandle don't. Of course we can't solve _all_ possible issues - after all a native call could still hold onto an address for longer, and dereference it later on, when the original segment went unreachable. But there's not much we can do to protect against this (in fact, same issue is possible even with deterministic deallocation). But for simple (and very common!) cases we should try harder to keep the Addressable arguments passed to a native call alive for the duration of the call.
> 
> This includes, notably, the Addressable parameter passed to "virtual" method handle, which also can be collected prematurely (with pretty bad consequences).
> 
> This patch rectifies that. To do so, the linker machinery will keep alive any addressable argument coming its way (thanks Jorn for wrestling with the combinators). On the API side, I've refactored the memory address implementation into an abstract class, which contains no state (ht: Valhalla) and all the implementations we had before. Then we have two thin subclasses, one that is used for all unchecked addresses, which is just a wrapper around a long; another that is an offset into a segment.
> 
> For now, I didn't make any big claim in the direction that an address "has a scope" - the API doesn't say that. The only change API-wise is that MemorySegment::address now documents that the returned address will keep the segment alive.
> 
> I've added two tests in TestUpcall/Downcall to make sure that everything works when arguments and returns are created using the default scope; the test was also passing with the previous implementation, so I went a bit more drastic, and added explicit calls to System.gc() - which immediately revealed the problem in the old code. Since calling System.gc() on every test iteration slows down test execution too much, I added some sampling logic, where System.gc() is only called every 100 tests.

This pull request has now been integrated.

Changeset: 1dde0f22
Author:    Maurizio Cimadamore <mcimadamore at openjdk.org>
URL:       https://git.openjdk.java.net/panama-foreign/commit/1dde0f22
Stats:     135 lines in 10 files changed: 94 ins; 12 del; 29 mod

8264280: Foreign linker should be more friendly with implicit scopes

Reviewed-by: psandoz, jvernee

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

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