8301228: Add more ways to resize zero-length memory segments

[mcimadamore]

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 :-)

---

Progress

• Change must not contain extraneous whitespace
• Change must be properly reviewed (1 review required, with at least 1 Committer)

Issue

• JDK-8301228: Add more ways to resize zero-length memory segments

Reviewers

• Jorn Vernee (@JornVernee - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/panama-foreign pull/775/head:pull/775
$ git checkout pull/775

Update a local copy of the PR:
$ git checkout pull/775
$ git pull https://git.openjdk.org/panama-foreign pull/775/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 775

View PR using the GUI difftool:
$ git pr show -t 775

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/panama-foreign/pull/775.diff

[bridgekeeper]

👋 Welcome back mcimadamore! A progress list of the required criteria for merging this PR into foreign-memaccess+abi will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[JornVernee]

Some initial comments

[mcimadamore]

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

[JornVernee]

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.

[mlbridge]

Webrevs

• 01: Full - Incremental (fa9f1be1)
• 00: Full (07babca3)

[JornVernee]

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.

[JornVernee]

AFAIK this requires also needs a check for sun.arch.data.model == "64" as the linker doesn't work on 32-bit VMs. (could still be running a 32-bit VM on an amd64 machine)

[openjdk]

@mcimadamore This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8301228: Add more ways to resize zero-length memory segments

Reviewed-by: jvernee

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 117 new commits pushed to the foreign-memaccess+abi branch:

• a2822b6: Automatic merge of master into foreign-memaccess+abi
• 4e71fa3: Automatic merge of jdk:master into master
• d7aa87f: 8300176: URLEncoder/URLDecoder static fields should be private static final
• 107e184: 8301179: Replace NULL with nullptr in share/gc/serial/
• b77abc6: 8301178: Replace NULL with nullptr in share/gc/epsilon/
• f7da09c: 8301164: Remove unused ResourceStack class
• 6e4710b: 8300253: Introduce AArch64 nzcv accessors
• c6b3f2d: 8301129: Link to debuginfo files should only be made after stripping
• 938b409: 8301133: IGV: NPE occurs when creating a diff graph with a graph in a different folder
• 0eb1f66: 8298038: [s390] Configure script detects num_cores +1
• ... and 107 more: https://git.openjdk.org/panama-foreign/compare/219d0ab43019b9efd4b109c8a441b86321ac2c80...foreign-memaccess+abi

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the foreign-memaccess+abi branch, type /integrate in a new comment.

[mcimadamore]

/integrate

[openjdk]

Going to push as commit add0cf9.
Since your change was applied there have been 117 commits pushed to the foreign-memaccess+abi branch:

• a2822b6: Automatic merge of master into foreign-memaccess+abi
• 4e71fa3: Automatic merge of jdk:master into master
• d7aa87f: 8300176: URLEncoder/URLDecoder static fields should be private static final
• 107e184: 8301179: Replace NULL with nullptr in share/gc/serial/
• b77abc6: 8301178: Replace NULL with nullptr in share/gc/epsilon/
• f7da09c: 8301164: Remove unused ResourceStack class
• 6e4710b: 8300253: Introduce AArch64 nzcv accessors
• c6b3f2d: 8301129: Link to debuginfo files should only be made after stripping
• 938b409: 8301133: IGV: NPE occurs when creating a diff graph with a graph in a different folder
• 0eb1f66: 8298038: [s390] Configure script detects num_cores +1
• ... and 107 more: https://git.openjdk.org/panama-foreign/compare/219d0ab43019b9efd4b109c8a441b86321ac2c80...foreign-memaccess+abi

Your commit was automatically rebased without conflicts.

[openjdk]

@mcimadamore Pushed as commit add0cf9.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.