[foreign-memaccess+abi] RFR: 8301228: Add more ways to resize zero-length memory segments

[Jorn Vernee]

On Thu, 26 Jan 2023 23:54:44 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> When dealing with native libraries, it is not uncommon to obtain zero-length memory segments. Since the size of these segment is zero (for safety reasons), clients need to be able to resize these segments.
>> 
>> At the time of writing, there are two ways to resize zero-length memory segment:
>> 
>> * By calling `MemorySegment.ofAddress`, and obtaining a new segment with desired base address, size and lifetime.
>> * By using an "unbounded" layout (e.g. OfAddress.asUnbounded()) in order to perform address dereference.
>> 
>> Both approaches can be improved. First, while `MemorySegment.ofAddress` is a good primitive to create truly custom native segments, sometimes the only thing a client wants to do is to quickly be able to resize the segment to the desired size.
>> 
>> Secondly, while unbounded address layouts are useful, there are cases where the size of the dereferenced segment is known statically. It is a bit sad that the API does not let clients to specify a _bounded_ size to be specified for a given address layout (as that would lead to safer code). Of course, in some cases the size would still be unknonw statically, so there has to be a way to go unbounded, if needed. 
>> 
>> This patch fixes rectifies the situation in two ways:
>> 
>> * by adding a method, namely `MemorySegment::asUnbounded()` which allows to obtain a view of a native segment with maximal size (i.e. `Long.MAX_VALUE`). Note that clients can first use this method, and then use regular slicing methods to further restrict the size, as required.
>> * by replacing the `OfAddress::asUnbounded()` method with a more targeted `OfAddress::withTargetLayout(MemoryLayout)`. Note that the unbounded behavior can still be obtained by passing something like `MemoryLayout.sequenceLayout(JAVA_BYTE)`.
>> 
>> When working on the patch we have considered other options, such as that of adding unsafe slicing methods which can be thought of as the composition of `asUnbounded` with some other (safe) slicing method. And, whether or not to keep `OfAddress::asUnbounded` (as a shortcut).
>> 
>> I would like to start from the more principled approach in this patch. Since the methods we are talking about are all restricted, there is a certain appeal in trying to keep their number limited. And, duplicating all the slicing methods into new restricted variants can increase the footprint of the MemorySegment API for a relatively little gain. That said, other overloads can always be added as required, at a later point. 
>> 
>> Note that this patch also adds more safe slicing variants, namely:
>> 
>> * asSlice(long offset, MemoryLayout)
>> * asSlice(long offset, long newSize, long byteAlignment)
>> 
>> The former is very handy when resizing an unbounded segment to a given layout (e.g. JAVA_INT). Since it takes a layout parameter, it makes sense to check if the resulting slice conforms to the alignment constraint specified by the memory layout. The second method is basically the "real" slicing primitive - which does resizing, offset and alignment check. All other slicing methods can be explained in terms of this.
>> 
>> Interestingly, thanks to the new slicing methods, we can now more easily deal with alignment checks from `slicingAllocator` and `prefixAllocator`.
>> 
>> Another note: for the time being, `MemorySegment::asUnbounded` will only work on native segments. While the implementation could be easily made total, I'm a bit skeptical of allowing out-of-bound access from a Java array :-)
>
> test/jdk/java/foreign/TestAddressDereference.java line 115:
> 
>> 113:     public void testNativeUpcallArg(long alignment, ValueLayout layout) throws Throwable {
>> 114:         boolean badAlign = layout.byteAlignment() > alignment;
>> 115:         if (badAlign) return; // this will crash the JVM (exception occurs when going into the upcall stub)
> 
> Any idea on this @JornVernee ? When the alignment exception is thrown (this happens when we execute the box address bindings before entering the upcall stub), the JVM exits, reporting an exception that points to where the downcall is made, which is quite confusing. I still think the check is worthwhile having (after all a layout is more than just the size), it's a bit unfortunate that in the case of upcalls a failed check ends up terminating the JVM (but then again, if you are really expecting a pointer with certain characteristics, and you are given another - you might just cross fingers and hope it would be ok).

Right, an exception thrown outside of user code will go to the uncaught exception handler, and exit the VM after printing a stack trace. (Though, the stack trace should point at the wrapper class generated by the binding specializer, not the downcall call site. Does this change if you use `-XX:+ShowHiddenFrames`?)

I guess it could be problematic if an upcall has some 'optional' pointer argument, that is sometimes garbage (as opposed to `NULL`). But, a user can also create an address layout with an unaligned target to avoid the exception if needed.

If the pointer you receive is not supposed to be garbage, a mis-aligned pointer might mean you were given a corrupt pointer. So in that case the exception seems useful.

Either way, I think there is little reason why someone might want to try to catch such an exception, since the code would be in the middle of boxing arguments. There seems to be no way to sanely recover in that situation. (except maybe for doing some additional logging or something, but I think we could solve that by integrating our uncaught exception handler with the one installed on the thread. Or one installed through a linker option).

If you still want to test the negative case (which I think we probably should), I suggest adding a new class that extends `UpcallTestHelper` and use `runInNewProcess` to run the test code, then check stderr for the exception message. See e.g. `TestPassHeapSegment::testNoHeapReturns` which also tests an exception thrown when processing the bindings.

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

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