[foreign-memaccess+abi] RFR: 8294970: Add linker option for saving thread-locals that the VM can overwrite

[Maurizio Cimadamore]

On Fri, 14 Oct 2022 15:40:48 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

> This patch adds a new linker option, called `preserveValue`, that can be used to save thread-local values such as `errno` and `GetLastError` that the VM might overwrite. It takes a list of names of the values to save. Values are saved into a MemorySegment passed by the user, that is allocated according to the layout associated with the returned `PreserveValue` instance.
> 
> The API for this is somewhat convoluted, as we have to account for:
>  1. a different JVM implementation might not overwrite the same thread-locals, or non at all. Such JVM implementations should be free to support a different set of names to save, or none at all.
>  2. it should be possible to save multiple values during a single call.
> 
> But, this API is also flexible enough for an implementation to choose any layout it wants for the MemorySegment into which the values are saved. In the current implementation it's just a set of ints, but this could be changed as well.
> 
> ---------
> ### Progress
> - [x] Change must not contain extraneous whitespace
> - [ ] Change must be properly reviewed (1 review required, with at least 1 [Committer](https://openjdk.org/bylaws#committer))
> 
> 
> 
> ### Reviewing
> <details><summary>Using <code>git</code></summary>
> 
> Checkout this PR locally: \
> `$ git fetch https://git.openjdk.org/panama-foreign pull/742/head:pull/742` \
> `$ git checkout pull/742`
> 
> Update a local copy of the PR: \
> `$ git checkout pull/742` \
> `$ git pull https://git.openjdk.org/panama-foreign pull/742/head`
> 
> </details>
> <details><summary>Using Skara CLI tools</summary>
> 
> Checkout this PR locally: \
> `$ git pr checkout 742`
> 
> View PR using the GUI difftool: \
> `$ git pr show -t 742`
> 
> </details>
> <details><summary>Using diff file</summary>
> 
> Download this PR as a diff file: \
> <a href="https://git.openjdk.org/panama-foreign/pull/742.diff">https://git.openjdk.org/panama-foreign/pull/742.diff</a>
> 
> </details>

A very good start - I left some comments/questions, especially around the API

src/java.base/share/classes/java/lang/foreign/Linker.java line 319:

> 317:          *          before they can be read through conventional means}
> 318:          * <p>
> 319:          * A downcall method handle linked with this option will feature an additional {@link MemorySegment}

We should say somewhere that the additional segment should be a native segment (right?)

src/java.base/share/classes/java/lang/foreign/Linker.java line 326:

> 324:          * @see PreserveValue#supported()
> 325:          */
> 326:         static PreserveValue preserveValue(String... preservedValues) {

I think I prefer `saveValue` - it's simpler, but means more or less the same thing.
Also, I think we should use the plural "saveValues" both in the factory and in the option interface.

src/java.base/share/classes/java/lang/foreign/Linker.java line 362:

> 360:                                        permits LinkerOptions.PreserveValueImpl {
> 361:             /**
> 362:              * {@return A struct layout that represents the layout of the memory region passed

maybe "layout of the native segment" instead of region? If you stick with region, I suggest "off-heap region of memory" which is what we use elsewhere,

src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java line 57:

> 55:         checkHasNaturalAlignment(function);
> 56:         LinkerOptions optionSet = LinkerOptions.of(options);
> 57:         optionSet.validateForDowncall(function);

Does it make sense to have a validation step that is disjoint from the creation of the LinkerOptions? E.g. should LinkerOptions factory just take the descriptor and perform all the required validation in one go?

src/java.base/share/classes/jdk/internal/foreign/abi/PreservableValues.java line 43:

> 41:     PreservableValues(String valueName, ValueLayout layout, int mask) {
> 42:         this.valueName = valueName;
> 43:         this.layout = layout.withName(valueName);

Ok, layouts get the name attached - which can be used by developers to generate the correct getter. Another choice, I realize, would have been for the PreserveValue linker option to expose a static getter which takes a name. Then you don't need to expose a layout. Not sure which one is better. The current approach is probably cleaner in that if the layout doesn't have a certain named layout, then you cannot obtain a getter for it.

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

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